refactor(protocol): improve contracts based on suggestions from OpenZ…

…eppelin (#16949) Co-authored-by: Keszey Dániel <[email protected]> Co-authored-by: Daniel Wang <[email protected]> Co-authored-by: David <[email protected]>
taikoxyz · May 2, 2024 · ad92a14 · ad92a14
1 parent e7a34dd
commit ad92a14
Show file tree

Hide file tree

Showing 13 changed files with 41 additions and 324 deletions.
diff --git a/packages/protocol/contracts/L1/TaikoL1.sol b/packages/protocol/contracts/L1/TaikoL1.sol
@@ -36,7 +36,7 @@ contract TaikoL1 is EssentialContract, ITaikoL1, TaikoEvents, TaikoErrors {
 
     /// @dev Allows for receiving Ether from Hooks
     receive() external payable {
-        if (!_inNonReentrant()) revert L1_RECEIVE_DISABLED();
+        if (!inNonReentrant()) revert L1_RECEIVE_DISABLED();
     }
 
     /// @notice Initializes the contract.

diff --git a/packages/protocol/contracts/common/EssentialContract.sol b/packages/protocol/contracts/common/EssentialContract.sol
@@ -94,13 +94,13 @@ abstract contract EssentialContract is UUPSUpgradeable, Ownable2StepUpgradeable,
     /// @notice Initializes the contract.
     /// @param _owner The owner of this contract. msg.sender will be used if this value is zero.
     /// @param _addressManager The address of the {AddressManager} contract.
-    function __Essential_init(address _owner, address _addressManager) internal onlyInitializing {
+    function __Essential_init(address _owner, address _addressManager) internal {
         if (_addressManager == address(0)) revert ZERO_ADDR_MANAGER();
         __Essential_init(_owner);
         __AddressResolver_init(_addressManager);
     }
 
-    function __Essential_init(address _owner) internal virtual {
+    function __Essential_init(address _owner) internal virtual onlyInitializing {
         _transferOwnership(_owner == address(0) ? msg.sender : _owner);
         __paused = _FALSE;
     }
@@ -142,7 +142,7 @@ abstract contract EssentialContract is UUPSUpgradeable, Ownable2StepUpgradeable,
         }
     }
 
-    function _inNonReentrant() internal view returns (bool) {
+    function inNonReentrant() public view returns (bool) {
         return _loadReentryLock() == _TRUE;
     }
 }
diff --git a/packages/protocol/contracts/signal/ISignalService.sol b/packages/protocol/contracts/signal/ISignalService.sol
@@ -67,8 +67,8 @@ interface ISignalService {
 
     /// @notice Emitted when an address is authorized or deauthorized.
     /// @param addr The address to be authorized or deauthorized.
-    /// @param authrized True if authorized, false otherwise.
-    event Authorized(address indexed addr, bool authrized);
+    /// @param authorized True if authorized, false otherwise.
+    event Authorized(address indexed addr, bool authorized);
 
     /// @notice Send a signal (message) by setting the storage slot to a value of 1.
     /// @param _signal The signal (message) to send.

diff --git a/packages/protocol/contracts/signal/SignalService.sol b/packages/protocol/contracts/signal/SignalService.sol
@@ -42,7 +42,7 @@ contract SignalService is EssentialContract, ISignalService {
     error SS_UNAUTHORIZED();
     error SS_UNSUPPORTED();
 
-    modifier validSender(address _app) {
+    modifier nonZeroApp(address _app) {
         if (_app == address(0)) revert SS_INVALID_SENDER();
         _;
     }
@@ -205,7 +205,7 @@ contract SignalService is EssentialContract, ISignalService {
         internal
         view
         virtual
-        validSender(_app)
+        nonZeroApp(_app)
         nonZeroValue(_signal)
         nonZeroValue(_value)
         returns (bytes32)
@@ -248,7 +248,7 @@ contract SignalService is EssentialContract, ISignalService {
         bytes32 _value
     )
         private
-        validSender(_app)
+        nonZeroApp(_app)
         nonZeroValue(_signal)
         nonZeroValue(_value)
         returns (bytes32 slot_)
@@ -290,7 +290,7 @@ contract SignalService is EssentialContract, ISignalService {
     )
         private
         view
-        validSender(_app)
+        nonZeroApp(_app)
         nonZeroValue(_signal)
         returns (bytes32 value_)
     {
@@ -309,7 +309,7 @@ contract SignalService is EssentialContract, ISignalService {
     )
         private
         view
-        validSender(_app)
+        nonZeroApp(_app)
         nonZeroValue(_signal)
         returns (CacheAction[] memory actions)
     {

diff --git a/packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol b/packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol
diff --git a/packages/protocol/contracts/thirdparty/optimism/rlp/RLPReader.sol b/packages/protocol/contracts/thirdparty/optimism/rlp/RLPReader.sol
@@ -5,7 +5,7 @@ pragma solidity 0.8.24;
 /// @title RLPReader
 /// @notice RLPReader is a library for parsing RLP-encoded byte arrays into Solidity types. Adapted
 ///         from Solidity-RLP (https://github.com/hamdiallam/Solidity-RLP) by Hamdi Allam with
-///         various tweaks to improve readability.
+///         various tweaks to improve readability. (A shout-out to Optimism !)
 library RLPReader {
     /// @notice Custom pointer type to avoid confusion between pointers and uint256s.
     type MemoryPointer is uint256;

diff --git a/packages/protocol/contracts/thirdparty/optimism/rlp/RLPWriter.sol b/packages/protocol/contracts/thirdparty/optimism/rlp/RLPWriter.sol
@@ -5,7 +5,7 @@ pragma solidity 0.8.24;
 /// @title RLPWriter
 /// @author RLPWriter is a library for encoding Solidity types to RLP bytes. Adapted from Bakaoh's
 ///         RLPEncode library (https://github.com/bakaoh/solidity-rlp-encode) with minor
-///         modifications to improve legibility.
+///         modifications to improve legibility. (A shout-out to Optimism !)
 library RLPWriter {
     /// @notice RLP encodes a byte string.
     /// @param _in The byte string to encode.

diff --git a/packages/protocol/contracts/tokenvault/BridgedERC1155.sol b/packages/protocol/contracts/tokenvault/BridgedERC1155.sol
@@ -94,21 +94,21 @@ contract BridgedERC1155 is EssentialContract, ERC1155Upgradeable {
         _mintBatch(_to, _tokenIds, _amounts, "");
     }
 
-    /// @dev Burns tokens.
+    /// @dev Batch burns tokens.
     /// @param _account Address from which tokens are burned.
-    /// @param _tokenId ID of the token to burn.
-    /// @param _amount Amount of tokens to burn.
-    function burn(
+    /// @param _ids Array of IDs of the tokens to burn.
+    /// @param _amounts Amount of tokens to burn respectively.
+    function burnBatch(
         address _account,
-        uint256 _tokenId,
-        uint256 _amount
+        uint256[] calldata _ids,
+        uint256[] calldata _amounts
     )
-        public
+        external
         whenNotPaused
         onlyFromNamed(LibStrings.B_ERC1155_VAULT)
         nonReentrant
     {
-        _burn(_account, _tokenId, _amount);
+        _burnBatch(_account, _ids, _amounts);
     }
 
     /// @notice Gets the name of the bridged token.

diff --git a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol
@@ -21,11 +21,17 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 {
     /// @param inbound If false then signals migrating 'from', true if migrating 'into'.
     event MigrationStatusChanged(address addr, bool inbound);
 
-    /// @notice Emitted when tokens are migrated to or from the bridged token.
-    /// @param fromToken The address of the bridged token.
+    /// @notice Emitted when tokens are migrated to the new bridged token.
+    /// @param migratedTo The address of the bridged token.
     /// @param account The address of the account.
     /// @param amount The amount of tokens migrated.
-    event MigratedTo(address indexed fromToken, address indexed account, uint256 amount);
+    event MigratedTo(address indexed migratedTo, address indexed account, uint256 amount);
+
+    /// @notice Emitted when tokens are migrated from the old bridged token.
+    /// @param migratedFrom The address of the bridged token.
+    /// @param account The address of the account.
+    /// @param amount The amount of tokens migrated.
+    event MigratedFrom(address indexed migratedFrom, address indexed account, uint256 amount);
 
     error BB_PERMISSION_DENIED();
     error BB_INVALID_PARAMS();
@@ -62,7 +68,7 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 {
         address _migratingAddress = migratingAddress;
         if (msg.sender == _migratingAddress) {
             // Inbound migration
-            emit MigratedTo(_migratingAddress, _account, _amount);
+            emit MigratedFrom(_migratingAddress, _account, _amount);
         } else if (msg.sender != resolve(LibStrings.B_ERC20_VAULT, true)) {
             // Bridging from vault
             revert BB_PERMISSION_DENIED();

diff --git a/packages/protocol/contracts/tokenvault/ERC1155Vault.sol b/packages/protocol/contracts/tokenvault/ERC1155Vault.sol
@@ -249,9 +249,7 @@ contract ERC1155Vault is BaseNFTVault, ERC1155ReceiverUpgradeable {
             CanonicalNFT storage _ctoken = bridgedToCanonical[_op.token];
             if (_ctoken.addr != address(0)) {
                 ctoken_ = _ctoken;
-                for (uint256 i; i < _op.tokenIds.length; ++i) {
-                    BridgedERC1155(_op.token).burn(msg.sender, _op.tokenIds[i], _op.amounts[i]);
-                }
+                BridgedERC1155(_op.token).burnBatch(msg.sender, _op.tokenIds, _op.amounts);
             } else {
                 // is a ctoken token, meaning, it lives on this chain
                 ctoken_ = CanonicalNFT({
@@ -267,15 +265,14 @@ contract ERC1155Vault is BaseNFTVault, ERC1155ReceiverUpgradeable {
                 try t.symbol() returns (string memory _symbol) {
                     ctoken_.symbol = _symbol;
                 } catch { }
-                for (uint256 i; i < _op.tokenIds.length; ++i) {
-                    IERC1155(_op.token).safeTransferFrom({
-                        from: msg.sender,
-                        to: address(this),
-                        id: _op.tokenIds[i],
-                        amount: _op.amounts[i],
-                        data: ""
-                    });
-                }
+
+                IERC1155(_op.token).safeBatchTransferFrom({
+                    from: msg.sender,
+                    to: address(this),
+                    ids: _op.tokenIds,
+                    amounts: _op.amounts,
+                    data: ""
+                });
             }
         }
         msgData_ = abi.encodeCall(

diff --git a/packages/protocol/contracts/verifiers/SgxVerifier.sol b/packages/protocol/contracts/verifiers/SgxVerifier.sol
@@ -64,7 +64,7 @@ contract SgxVerifier is EssentialContract, IVerifier {
     /// instance, this value is zero address.
     /// @param validSince The time since the instance is valid.
     event InstanceAdded(
-        uint256 indexed id, address indexed instance, address replaced, uint256 validSince
+        uint256 indexed id, address indexed instance, address indexed replaced, uint256 validSince
     );
 
     /// @notice Emitted when an SGX instance is deleted from the registry.

diff --git a/packages/protocol/test/TaikoTest.sol b/packages/protocol/test/TaikoTest.sol
@@ -21,7 +21,6 @@ import "../contracts/L1/provers/GuardianProver.sol";
 import "../contracts/L2/DelegateOwner.sol";
 
 import "../contracts/team/airdrop/ERC20Airdrop.sol";
-import "../contracts/team/airdrop/ERC20Airdrop2.sol";
 
 import "../test/common/erc20/FreeMintERC20.sol";
 import "../test/L2/TaikoL2EIP1559Configurable.sol";