fix(contracts): OZ-L1-M01 Enforced Transactions Signed Off-Chain Are Likely to Fail

contracts/integration-test/EnforcedTxGateway.spec.ts

@@ -1,10 +1,11 @@
 /* eslint-disable node/no-unpublished-import */
 /* eslint-disable node/no-missing-import */
 import { expect } from "chai";
-import { BigNumberish, BytesLike, constants, utils } from "ethers";
+import { BigNumberish, BytesLike, constants } from "ethers";
 import { ethers } from "hardhat";
 import { EnforcedTxGateway, L1MessageQueue, L2GasPriceOracle, MockCaller } from "../typechain";
 import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
+import { arrayify } from "ethers/lib/utils";
 
 describe("EnforcedTxGateway.spec", async () => {
   let deployer: SignerWithAddress;
@@ -172,40 +173,109 @@ describe("EnforcedTxGateway.spec", async () => {
       value: BigNumberish,
       gasLimit: BigNumberish,
       data: BytesLike
-    ): Promise<string> => {
-      const queueIndex = await queue.nextCrossDomainMessageIndex();
-      const txHash = await queue.computeTransactionHash(signer.address, queueIndex, value, target, gasLimit, data);
-      return await signer.signMessage(utils.arrayify(txHash));
+    ) => {
+      const enforcedTx = {
+        sender: signer.address,
+        target: target,
+        value: value,
+        gasLimit: gasLimit,
+        data: arrayify(data),
+        nonce: await gateway.nonces(signer.address),
+        deadline: constants.MaxUint256,
+      };
+
+      const domain = {
+        name: "EnforcedTxGateway",
+        version: "1",
+        chainId: (await ethers.provider.getNetwork()).chainId,
+        verifyingContract: gateway.address,
+      };
+
+      const types = {
+        EnforcedTransaction: [
+          {
+            name: "sender",
+            type: "address",
+          },
+          {
+            name: "target",
+            type: "address",
+          },
+          {
+            name: "value",
+            type: "uint256",
+          },
+          {
+            name: "gasLimit",
+            type: "uint256",
+          },
+          {
+            name: "data",
+            type: "bytes",
+          },
+          {
+            name: "nonce",
+            type: "uint256",
+          },
+          {
+            name: "deadline",
+            type: "uint256",
+          },
+        ],
+      };
+
+      const signature = await signer._signTypedData(domain, types, enforcedTx);
+      return signature;
     };
 
     it("should revert, when contract is paused", async () => {
       await gateway.setPaused(true);
       await expect(
         gateway
           .connect(deployer)
-          ["sendTransaction(address,address,uint256,uint256,bytes,bytes,address)"](
+          ["sendTransaction(address,address,uint256,uint256,bytes,uint256,bytes,address)"](
             signer.address,
             signer.address,
             0,
             0,
             "0x",
+            constants.MaxUint256,
             "0x",
             constants.AddressZero
           )
       ).to.revertedWith("Pausable: paused");
     });
 
+    it("should revert, when signature expired", async () => {
+      const timestamp = (await ethers.provider.getBlock("latest")).timestamp;
+      await expect(
+        gateway
+          .connect(deployer)
+          ["sendTransaction(address,address,uint256,uint256,bytes,uint256,bytes,address)"](
+            signer.address,
+            signer.address,
+            0,
+            0,
+            "0x",
+            timestamp - 1,
+            "0x",
+            constants.AddressZero
+          )
+      ).to.revertedWith("signature expired");
+    });
+
     it("should revert, when signature is wrong", async () => {
       const signature = await signer.signMessage("0x00");
       await expect(
         gateway
           .connect(deployer)
-          ["sendTransaction(address,address,uint256,uint256,bytes,bytes,address)"](
+          ["sendTransaction(address,address,uint256,uint256,bytes,uint256,bytes,address)"](
             signer.address,
             signer.address,
             0,
             0,
             "0x",
+            constants.MaxUint256,
             signature,
             constants.AddressZero
           )
@@ -218,12 +288,13 @@ describe("EnforcedTxGateway.spec", async () => {
       await expect(
         gateway
           .connect(deployer)
-          ["sendTransaction(address,address,uint256,uint256,bytes,bytes,address)"](
+          ["sendTransaction(address,address,uint256,uint256,bytes,uint256,bytes,address)"](
             signer.address,
             signer.address,
             0,
             1000000,
             "0x",
+            constants.MaxUint256,
             signature,
             signer.address,
             { value: fee.sub(1) }
@@ -238,12 +309,13 @@ describe("EnforcedTxGateway.spec", async () => {
       await expect(
         gateway
           .connect(deployer)
-          ["sendTransaction(address,address,uint256,uint256,bytes,bytes,address)"](
+          ["sendTransaction(address,address,uint256,uint256,bytes,uint256,bytes,address)"](
             signer.address,
             signer.address,
             0,
             1000000,
             "0x",
+            constants.MaxUint256,
             signature,
             signer.address,
             { value: fee }
@@ -255,65 +327,106 @@ describe("EnforcedTxGateway.spec", async () => {
       const signature = await getSignature(signer, deployer.address, 0, 1000000, "0x");
       const fee = await queue.estimateCrossDomainMessageFee(1000000);
       const feeVaultBalanceBefore = await ethers.provider.getBalance(feeVault.address);
+      expect(await gateway.nonces(signer.address)).to.eq(0);
       await expect(
         gateway
           .connect(deployer)
-          ["sendTransaction(address,address,uint256,uint256,bytes,bytes,address)"](
+          ["sendTransaction(address,address,uint256,uint256,bytes,uint256,bytes,address)"](
             signer.address,
             deployer.address,
             0,
             1000000,
             "0x",
+            constants.MaxUint256,
             signature,
             signer.address,
             { value: fee }
           )
       )
         .to.emit(queue, "QueueTransaction")
         .withArgs(signer.address, deployer.address, 0, 0, 1000000, "0x");
+      expect(await gateway.nonces(signer.address)).to.eq(1);
       const feeVaultBalanceAfter = await ethers.provider.getBalance(feeVault.address);
       expect(feeVaultBalanceAfter.sub(feeVaultBalanceBefore)).to.eq(fee);
+
+      // use the same nonce to sign should fail
+      await expect(
+        gateway
+          .connect(deployer)
+          ["sendTransaction(address,address,uint256,uint256,bytes,uint256,bytes,address)"](
+            signer.address,
+            deployer.address,
+            0,
+            1000000,
+            "0x",
+            constants.MaxUint256,
+            signature,
+            signer.address,
+            { value: fee }
+          )
+      ).to.revertedWith("Incorrect signature");
     });
 
     it("should succeed, with refund", async () => {
       const signature = await getSignature(signer, deployer.address, 0, 1000000, "0x");
       const fee = await queue.estimateCrossDomainMessageFee(1000000);
       const feeVaultBalanceBefore = await ethers.provider.getBalance(feeVault.address);
       const signerBalanceBefore = await ethers.provider.getBalance(signer.address);
+      expect(await gateway.nonces(signer.address)).to.eq(0);
       await expect(
         gateway
           .connect(deployer)
-          ["sendTransaction(address,address,uint256,uint256,bytes,bytes,address)"](
+          ["sendTransaction(address,address,uint256,uint256,bytes,uint256,bytes,address)"](
             signer.address,
             deployer.address,
             0,
             1000000,
             "0x",
+            constants.MaxUint256,
             signature,
             signer.address,
             { value: fee.add(100) }
           )
       )
         .to.emit(queue, "QueueTransaction")
         .withArgs(signer.address, deployer.address, 0, 0, 1000000, "0x");
+      expect(await gateway.nonces(signer.address)).to.eq(1);
       const feeVaultBalanceAfter = await ethers.provider.getBalance(feeVault.address);
       const signerBalanceAfter = await ethers.provider.getBalance(signer.address);
       expect(feeVaultBalanceAfter.sub(feeVaultBalanceBefore)).to.eq(fee);
       expect(signerBalanceAfter.sub(signerBalanceBefore)).to.eq(100);
+
+      // use the same nonce to sign should fail
+      await expect(
+        gateway
+          .connect(deployer)
+          ["sendTransaction(address,address,uint256,uint256,bytes,uint256,bytes,address)"](
+            signer.address,
+            deployer.address,
+            0,
+            1000000,
+            "0x",
+            constants.MaxUint256,
+            signature,
+            signer.address,
+            { value: fee.add(100) }
+          )
+      ).to.revertedWith("Incorrect signature");
     });
 
     it("should revert, when refund failed", async () => {
-      const signature = await getSignature(signer, signer.address, 0, 1000000, "0x");
+      const signature = await getSignature(signer, signer.address, 0, 1000000, "0x1234");
       const fee = await queue.estimateCrossDomainMessageFee(1000000);
       await expect(
         gateway
           .connect(deployer)
-          ["sendTransaction(address,address,uint256,uint256,bytes,bytes,address)"](
+          ["sendTransaction(address,address,uint256,uint256,bytes,uint256,bytes,address)"](
             signer.address,
             signer.address,
             0,
             1000000,
-            "0x",
+            "0x1234",
+            constants.MaxUint256,
             signature,
             gateway.address,
             { value: fee.add(100) }

contracts/src/L1/gateways/EnforcedTxGateway.sol

@@ -4,12 +4,15 @@ pragma solidity =0.8.16;
 
 import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 import {ECDSAUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/cryptography/ECDSAUpgradeable.sol";
+import {EIP712Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/cryptography/EIP712Upgradeable.sol";
 import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
 import {PausableUpgradeable} from "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
 
 import {IL1MessageQueue} from "../rollup/IL1MessageQueue.sol";
 
-contract EnforcedTxGateway is OwnableUpgradeable, ReentrancyGuardUpgradeable, PausableUpgradeable {
+// solhint-disable reason-string
+
+contract EnforcedTxGateway is OwnableUpgradeable, ReentrancyGuardUpgradeable, PausableUpgradeable, EIP712Upgradeable {
     /**********
      * Events *
      **********/
@@ -19,6 +22,16 @@ contract EnforcedTxGateway is OwnableUpgradeable, ReentrancyGuardUpgradeable, Pa
     /// @param _newFeeVault The address of new fee vault contract.
     event UpdateFeeVault(address _oldFeeVault, address _newFeeVault);
 
+    /*************
+     * Constants *
+     *************/
+
+    // solhint-disable-next-line var-name-mixedcase
+    bytes32 private constant _ENFORCED_TX_TYPEHASH =
+        keccak256(
+            "EnforcedTransaction(address sender,address target,uint256 value,uint256 gasLimit,bytes data,uint256 nonce,uint256 deadline)"
+        );
+
     /*************
      * Variables *
      *************/
@@ -29,6 +42,11 @@ contract EnforcedTxGateway is OwnableUpgradeable, ReentrancyGuardUpgradeable, Pa
     /// @notice The address of fee vault contract.
     address public feeVault;
 
+    /// @notice Mapping from EOA address to current nonce.
+    /// @dev Every successful call to `sendTransaction` with signature increases `_sender`'s nonce by one.
+    /// This prevents a signature from being used multiple times.
+    mapping(address => uint256) public nonces;
+
     /***************
      * Constructor *
      ***************/
@@ -37,11 +55,22 @@ contract EnforcedTxGateway is OwnableUpgradeable, ReentrancyGuardUpgradeable, Pa
         OwnableUpgradeable.__Ownable_init();
         ReentrancyGuardUpgradeable.__ReentrancyGuard_init();
         PausableUpgradeable.__Pausable_init();
+        EIP712Upgradeable.__EIP712_init("EnforcedTxGateway", "1");
 
         messageQueue = _queue;
         feeVault = _feeVault;
     }
 
+    /*************************
+     * Public View Functions *
+     *************************/
+
+    /// @notice return the domain separator for the typed transaction
+    // solhint-disable-next-line func-name-mixedcase
+    function DOMAIN_SEPARATOR() external view returns (bytes32) {
+        return _domainSeparatorV4();
+    }
+
     /*****************************
      * Public Mutating Functions *
      *****************************/
@@ -58,6 +87,7 @@ contract EnforcedTxGateway is OwnableUpgradeable, ReentrancyGuardUpgradeable, Pa
         uint256 _gasLimit,
         bytes calldata _data
     ) external payable whenNotPaused {
+        // solhint-disable-next-line avoid-tx-origin
         require(msg.sender == tx.origin, "Only EOA senders are allowed to send enforced transaction");
 
         _sendTransaction(msg.sender, _target, _value, _gasLimit, _data, msg.sender);
@@ -70,6 +100,7 @@ contract EnforcedTxGateway is OwnableUpgradeable, ReentrancyGuardUpgradeable, Pa
     /// @param _value The value passed
     /// @param _gasLimit The maximum gas should be used for this transaction in L2.
     /// @param _data The calldata passed to target contract.
+    /// @param _deadline The deadline of the signature.
     /// @param _signature The signature for the transaction.
     /// @param _refundAddress The address to refund exceeded fee.
     function sendTransaction(
@@ -78,22 +109,23 @@ contract EnforcedTxGateway is OwnableUpgradeable, ReentrancyGuardUpgradeable, Pa
         uint256 _value,
         uint256 _gasLimit,
         bytes calldata _data,
+        uint256 _deadline,
         bytes memory _signature,
         address _refundAddress
     ) external payable whenNotPaused {
-        address _messageQueue = messageQueue;
-        uint256 _queueIndex = IL1MessageQueue(messageQueue).nextCrossDomainMessageIndex();
-        bytes32 _txHash = IL1MessageQueue(_messageQueue).computeTransactionHash(
-            _sender,
-            _queueIndex,
-            _value,
-            _target,
-            _gasLimit,
-            _data
+        // solhint-disable-next-line not-rely-on-time
+        require(block.timestamp <= _deadline, "signature expired");
+
+        uint256 _nonce = nonces[_sender];
+        bytes32 _structHash = keccak256(
+            abi.encode(_ENFORCED_TX_TYPEHASH, _sender, _target, _value, _gasLimit, keccak256(_data), _nonce, _deadline)
         );
+        unchecked {
+            nonces[_sender] = _nonce + 1;
+        }
 
-        bytes32 _signHash = ECDSAUpgradeable.toEthSignedMessageHash(_txHash);
-        address _signer = ECDSAUpgradeable.recover(_signHash, _signature);
+        bytes32 _hash = _hashTypedDataV4(_structHash);
+        address _signer = ECDSAUpgradeable.recover(_hash, _signature);
 
         // no need to check `_signer != address(0)`, since it is checked in `recover`.
         require(_signer == _sender, "Incorrect signature");