fix(contracts): bug fixing based on openzeppelin's audit

contracts/docs/apis/L1ScrollMessenger.md

@@ -63,28 +63,6 @@ Initialize the storage of L1ScrollMessenger.
 | _rollup | address | The address of ScrollChain contract. |
 | _messageQueue | address | The address of L1MessageQueue contract. |
 
-### isL1MessageRelayed
-
-```solidity
-function isL1MessageRelayed(bytes32) external view returns (bool)
-```
-
-Mapping from relay id to relay status.
-
-
-
-#### Parameters
-
-| Name | Type | Description |
-|---|---|---|
-| _0 | bytes32 | undefined |
-
-#### Returns
-
-| Name | Type | Description |
-|---|---|---|
-| _0 | bool | undefined |
-
 ### isL1MessageSent
 
 ```solidity

contracts/integration-test/L1MessageQueue.spec.ts

@@ -2,7 +2,16 @@
 /* eslint-disable node/no-missing-import */
 import { expect } from "chai";
 import { BigNumber, constants } from "ethers";
-import { concat, getAddress, hexlify, keccak256, randomBytes, RLP } from "ethers/lib/utils";
+import {
+  concat,
+  getAddress,
+  hexlify,
+  keccak256,
+  randomBytes,
+  RLP,
+  stripZeros,
+  TransactionTypes,
+} from "ethers/lib/utils";
 import { ethers } from "hardhat";
 import { L1MessageQueue, L2GasPriceOracle } from "../typechain";
 import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
@@ -94,8 +103,8 @@ describe("L1MessageQueue", async () => {
 
   context("#computeTransactionHash", async () => {
     it("should succeed", async () => {
-      const sender = hexlify(randomBytes(20));
-      const target = hexlify(randomBytes(20));
+      const sender = "0xb2a70fab1a45b1b9be443b6567849a1702bc1232";
+      const target = "0xcb18150e4efefb6786130e289a5f61a82a5b86d7";
       const transactionType = "0x7E";
 
       for (const nonce of [
@@ -123,19 +132,30 @@ describe("L1MessageQueue", async () => {
             constants.MaxUint256,
           ]) {
             for (const dataLen of [0, 1, 2, 3, 4, 55, 56, 100]) {
-              const data = randomBytes(dataLen);
-              const transactionPayload = RLP.encode([
-                nonce.toHexString(),
-                gasLimit.toHexString(),
-                target,
-                value.toHexString(),
-                data,
-                sender,
-              ]);
-              const payload = concat([transactionType, transactionPayload]);
-              const expectedHash = keccak256(payload);
-              const computedHash = await queue.computeTransactionHash(sender, nonce, value, target, gasLimit, data);
-              expect(expectedHash).to.eq(computedHash);
+              const tests = [randomBytes(dataLen)];
+              if (dataLen === 1) {
+                for (const byte of [0, 1, 127, 128]) {
+                  tests.push(Uint8Array.from([byte]));
+                }
+              }
+              for (const data of tests) {
+                const transactionPayload = RLP.encode([
+                  stripZeros(nonce.toHexString()),
+                  stripZeros(gasLimit.toHexString()),
+                  target,
+                  stripZeros(value.toHexString()),
+                  data,
+                  sender,
+                ]);
+                const payload = concat([transactionType, transactionPayload]);
+                const expectedHash = keccak256(payload);
+                const computedHash = await queue.computeTransactionHash(sender, nonce, value, target, gasLimit, data);
+                if (computedHash !== expectedHash) {
+                  console.log(hexlify(transactionPayload));
+                  console.log(nonce, gasLimit, target, value, data, sender);
+                }
+                expect(expectedHash).to.eq(computedHash);
+              }
             }
           }
         }

contracts/src/L1/L1ScrollMessenger.sol

@@ -30,9 +30,6 @@ contract L1ScrollMessenger is PausableUpgradeable, ScrollMessengerBase, IL1Scrol
      * Variables *
      *************/
 
-    /// @notice Mapping from relay id to relay status.
-    mapping(bytes32 => bool) public isL1MessageRelayed;
-
     /// @notice Mapping from L1 message hash to sent status.
     mapping(bytes32 => bool) public isL1MessageSent;
 
@@ -45,28 +42,6 @@ contract L1ScrollMessenger is PausableUpgradeable, ScrollMessengerBase, IL1Scrol
     /// @notice The address of L1MessageQueue contract.
     address public messageQueue;
 
-    // @note move to ScrollMessengerBase in next big refactor
-    /// @dev The status of for non-reentrant check.
-    uint256 private _lock_status;
-
-    /**********************
-     * Function Modifiers *
-     **********************/
-
-    modifier nonReentrant() {
-        // On the first call to nonReentrant, _notEntered will be true
-        require(_lock_status != _ENTERED, "ReentrancyGuard: reentrant call");
-
-        // Any calls to nonReentrant after this point will fail
-        _lock_status = _ENTERED;
-
-        _;
-
-        // By storing the original value once again, a refund is triggered (see
-        // https://eips.ethereum.org/EIPS/eip-2200)
-        _lock_status = _NOT_ENTERED;
-    }
-
     /***************
      * Constructor *
      ***************/
@@ -162,9 +137,6 @@ contract L1ScrollMessenger is PausableUpgradeable, ScrollMessengerBase, IL1Scrol
         } else {
             emit FailedRelayedMessage(_xDomainCalldataHash);
         }
-
-        bytes32 _relayId = keccak256(abi.encodePacked(_xDomainCalldataHash, msg.sender, block.number));
-        isL1MessageRelayed[_relayId] = true;
     }
 
     /// @inheritdoc IL1ScrollMessenger

contracts/src/L1/rollup/L1MessageQueue.sol

@@ -139,18 +139,27 @@ contract L1MessageQueue is OwnableUpgradeable, IL1MessageQueue {
                 }
             }
 
-            function store_uint(_ptr, v) -> ptr {
+            // This is used for both store uint and single byte.
+            // Integer zero is special handled by geth to encode as `0x80`
+            function store_uint_or_byte(_ptr, v, is_uint) -> ptr {
                 ptr := _ptr
                 switch lt(v, 128)
                 case 1 {
-                    // single byte in the [0x00, 0x7f]
-                    mstore(ptr, shl(248, v))
+                    switch and(iszero(v), is_uint)
+                    case 1 {
+                        // integer 0
+                        mstore8(ptr, 0x80)
+                    }
+                    default {
+                        // single byte in the [0x00, 0x7f]
+                        mstore8(ptr, v)
+                    }
                     ptr := add(ptr, 1)
                 }
                 default {
                     // 1-32 bytes long
                     let len := get_uint_bytes(v)
-                    mstore(ptr, shl(248, add(len, 0x80)))
+                    mstore8(ptr, add(len, 0x80))
                     ptr := add(ptr, 1)
                     mstore(ptr, shl(mul(8, sub(32, len)), v))
                     ptr := add(ptr, len)
@@ -160,7 +169,7 @@ contract L1MessageQueue is OwnableUpgradeable, IL1MessageQueue {
             function store_address(_ptr, v) -> ptr {
                 ptr := _ptr
                 // 20 bytes long
-                mstore(ptr, shl(248, 0x94)) // 0x80 + 0x14
+                mstore8(ptr, 0x94) // 0x80 + 0x14
                 ptr := add(ptr, 1)
                 mstore(ptr, shl(96, v))
                 ptr := add(ptr, 0x14)
@@ -170,29 +179,29 @@ contract L1MessageQueue is OwnableUpgradeable, IL1MessageQueue {
             // 4 byte for list payload length
             let start_ptr := add(mload(0x40), 5)
             let ptr := start_ptr
-            ptr := store_uint(ptr, _queueIndex)
-            ptr := store_uint(ptr, _gasLimit)
+            ptr := store_uint_or_byte(ptr, _queueIndex, 1)
+            ptr := store_uint_or_byte(ptr, _gasLimit, 1)
             ptr := store_address(ptr, _target)
-            ptr := store_uint(ptr, _value)
+            ptr := store_uint_or_byte(ptr, _value, 1)
 
             switch eq(_data.length, 1)
             case 1 {
                 // single byte
-                ptr := store_uint(ptr, shr(248, calldataload(_data.offset)))
+                ptr := store_uint_or_byte(ptr, byte(0, calldataload(_data.offset)), 0)
             }
             default {
                 switch lt(_data.length, 56)
                 case 1 {
                     // a string is 0-55 bytes long
-                    mstore(ptr, shl(248, add(0x80, _data.length)))
+                    mstore8(ptr, add(0x80, _data.length))
                     ptr := add(ptr, 1)
                     calldatacopy(ptr, _data.offset, _data.length)
                     ptr := add(ptr, _data.length)
                 }
                 default {
                     // a string is more than 55 bytes long
                     let len_bytes := get_uint_bytes(_data.length)
-                    mstore(ptr, shl(248, add(0xb7, len_bytes)))
+                    mstore8(ptr, add(0xb7, len_bytes))
                     ptr := add(ptr, 1)
                     mstore(ptr, shl(mul(8, sub(32, len_bytes)), _data.length))
                     ptr := add(ptr, len_bytes)

contracts/src/L1/rollup/ScrollChain.sol

@@ -130,12 +130,7 @@ contract ScrollChain is OwnableUpgradeable, IScrollChain {
      *****************************/
 
     /// @notice Import layer 2 genesis block
-    /// @dev Although `_withdrawRoot` is always zero, we add this parameter for the convenience of unit testing.
-    function importGenesisBatch(
-        bytes calldata _batchHeader,
-        bytes32 _stateRoot,
-        bytes32 _withdrawRoot
-    ) external {
+    function importGenesisBatch(bytes calldata _batchHeader, bytes32 _stateRoot) external {
         // check genesis batch header length
         require(_stateRoot != bytes32(0), "zero state root");
 
@@ -157,10 +152,9 @@ contract ScrollChain is OwnableUpgradeable, IScrollChain {
 
         committedBatches[0] = _batchHash;
         finalizedStateRoots[0] = _stateRoot;
-        withdrawRoots[0] = _withdrawRoot;
 
         emit CommitBatch(_batchHash);
-        emit FinalizeBatch(_batchHash, _stateRoot, _withdrawRoot);
+        emit FinalizeBatch(_batchHash, _stateRoot, bytes32(0));
     }
 
     /// @inheritdoc IScrollChain

contracts/src/L2/L2ScrollMessenger.sol

@@ -62,28 +62,6 @@ contract L2ScrollMessenger is ScrollMessengerBase, PausableUpgradeable, IL2Scrol
     /// @notice The maximum number of times each L1 message can fail on L2.
     uint256 public maxFailedExecutionTimes;
 
-    // @note move to ScrollMessengerBase in next big refactor
-    /// @dev The status of for non-reentrant check.
-    uint256 private _lock_status;
-
-    /**********************
-     * Function Modifiers *
-     **********************/
-
-    modifier nonReentrant() {
-        // On the first call to nonReentrant, _notEntered will be true
-        require(_lock_status != _ENTERED, "ReentrancyGuard: reentrant call");
-
-        // Any calls to nonReentrant after this point will fail
-        _lock_status = _ENTERED;
-
-        _;
-
-        // By storing the original value once again, a refund is triggered (see
-        // https://eips.ethereum.org/EIPS/eip-2200)
-        _lock_status = _NOT_ENTERED;
-    }
-
     /***************
      * Constructor *
      ***************/
@@ -126,14 +104,16 @@ contract L2ScrollMessenger is ScrollMessengerBase, PausableUpgradeable, IL2Scrol
         require(_expectedStateRoot != bytes32(0), "Block is not imported");
 
         bytes32 _storageKey;
-        // `mapping(bytes32 => bool) public isL1MessageSent` is the 105-nd slot of contract `L1ScrollMessenger`.
+        // `mapping(bytes32 => bool) public isL1MessageSent` is the 155-th slot of contract `L1ScrollMessenger`.
+        // + 1 from `Initializable`
         // + 50 from `OwnableUpgradeable`
+        // + 50 from `ContextUpgradeable`
         // + 4 from `ScrollMessengerBase`
         // + 50 from `PausableUpgradeable`
-        // + 2-nd in `L1ScrollMessenger`
+        // + 1-st in `L1ScrollMessenger`
         assembly {
             mstore(0x00, _msgHash)
-            mstore(0x20, 105)
+            mstore(0x20, 155)
             _storageKey := keccak256(0x00, 0x40)
         }
 
@@ -161,14 +141,16 @@ contract L2ScrollMessenger is ScrollMessengerBase, PausableUpgradeable, IL2Scrol
         require(_expectedStateRoot != bytes32(0), "Block not imported");
 
         bytes32 _storageKey;
-        // `mapping(bytes32 => bool) public isL2MessageExecuted` is the 106-th slot of contract `L1ScrollMessenger`.
+        // `mapping(bytes32 => bool) public isL2MessageExecuted` is the 156-th slot of contract `L1ScrollMessenger`.
+        // + 1 from `Initializable`
         // + 50 from `OwnableUpgradeable`
+        // + 50 from `ContextUpgradeable`
         // + 4 from `ScrollMessengerBase`
         // + 50 from `PausableUpgradeable`
-        // + 3-rd in `L1ScrollMessenger`
+        // + 2-nd in `L1ScrollMessenger`
         assembly {
             mstore(0x00, _msgHash)
-            mstore(0x20, 106)
+            mstore(0x20, 156)
             _storageKey := keccak256(0x00, 0x40)
         }
 

contracts/src/L2/predeploys/WETH9.sol

@@ -51,7 +51,8 @@ contract WETH9 {
             balanceOf[msg.sender] -= wad;
         }
 
-        payable(msg.sender).transfer(wad);
+        (bool success, ) = msg.sender.call{value:wad}("");
+        require(success, "withdraw ETH failed");
 
         emit Withdrawal(msg.sender, wad);
     }

contracts/src/libraries/ScrollMessengerBase.sol

@@ -38,6 +38,28 @@ abstract contract ScrollMessengerBase is OwnableUpgradeable, IScrollMessenger {
     /// @notice The address of fee vault, collecting cross domain messaging fee.
     address public feeVault;
 
+    // @note move to ScrollMessengerBase in next big refactor
+    /// @dev The status of for non-reentrant check.
+    uint256 private _lock_status;
+
+    /**********************
+     * Function Modifiers *
+     **********************/
+
+    modifier nonReentrant() {
+        // On the first call to nonReentrant, _notEntered will be true
+        require(_lock_status != _ENTERED, "ReentrancyGuard: reentrant call");
+
+        // Any calls to nonReentrant after this point will fail
+        _lock_status = _ENTERED;
+
+        _;
+
+        // By storing the original value once again, a refund is triggered (see
+        // https://eips.ethereum.org/EIPS/eip-2200)
+        _lock_status = _NOT_ENTERED;
+    }
+
     /***************
      * Constructor *
      ***************/