Add `ERC2771Forwarder` as an enhanced successor to `MinimalForwarder` (#4346)

Co-authored-by: Francisco <fg@frang.io>

.changeset/blue-horses-do.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`ERC2771Forwarder`: Added `deadline` for expiring transactions, batching, and more secure handling of `msg.value`.

.github/workflows/checks.yml

@@ -100,6 +100,7 @@ jobs:
       - uses: crytic/slither-action@v0.3.0
         with:
           node-version: 18.15
+          slither-version: 0.9.3
 
   codespell:
     runs-on: ubuntu-latest

contracts/metatx/ERC2771Forwarder.sol

@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: MIT
+// OpenZeppelin Contracts (last updated v4.9.0) (metatx/ERC2771Forwarder.sol)
+
+pragma solidity ^0.8.19;
+
+import "../utils/cryptography/ECDSA.sol";
+import "../utils/cryptography/EIP712.sol";
+import "../utils/Nonces.sol";
+import "../utils/Address.sol";
+
+/**
+ * @dev A forwarder compatible with ERC2771 contracts. See {ERC2771Context}.
+ *
+ * This forwarder operates on forward requests that include:
+ *
+ * * `from`: An address to operate on behalf of. It is required to be equal to the request signer.
+ * * `to`: The address that should be called.
+ * * `value`: The amount of native token to attach with the requested call.
+ * * `gas`: The amount of gas limit that will be forwarded with the requested call.
+ * * `nonce`: A unique transaction ordering identifier to avoid replayability and request invalidation.
+ * * `deadline`: A timestamp after which the request is not executable anymore.
+ * * `data`: Encoded `msg.data` to send with the requested call.
+ */
+contract ERC2771Forwarder is EIP712, Nonces {
+    using ECDSA for bytes32;
+
+    struct ForwardRequestData {
+        address from;
+        address to;
+        uint256 value;
+        uint256 gas;
+        uint48 deadline;
+        bytes data;
+        bytes signature;
+    }
+
+    bytes32 private constant _FORWARD_REQUEST_TYPEHASH =
+        keccak256(
+            "ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)"
+        );
+
+    /**
+     * @dev Emitted when a `ForwardRequest` is executed.
+     *
+     * NOTE: An unsuccessful forward request could be due to an invalid signature, an expired deadline,
+     * or simply a revert in the requested call. The contract guarantees that the relayer is not able to force
+     * the requested call to run out of gas.
+     */
+    event ExecutedForwardRequest(address indexed signer, uint256 nonce, bool success);
+
+    /**
+     * @dev The request `from` doesn't match with the recovered `signer`.
+     */
+    error ERC2771ForwarderInvalidSigner(address signer, address from);
+
+    /**
+     * @dev The `requestedValue` doesn't match with the available `msgValue`.
+     */
+    error ERC2771ForwarderMismatchedValue(uint256 requestedValue, uint256 msgValue);
+
+    /**
+     * @dev The request `deadline` has expired.
+     */
+    error ERC2771ForwarderExpiredRequest(uint48 deadline);
+
+    /**
+     * @dev See {EIP712-constructor}.
+     */
+    constructor(string memory name) EIP712(name, "1") {}
+
+    /**
+     * @dev Returns `true` if a request is valid for a provided `signature` at the current block timestamp.
+     *
+     * A transaction is considered valid when it hasn't expired (deadline is not met), and the signer
+     * matches the `from` parameter of the signed request.
+     *
+     * NOTE: A request may return false here but it won't cause {executeBatch} to revert if a refund
+     * receiver is provided.
+     */
+    function verify(ForwardRequestData calldata request) public view virtual returns (bool) {
+        (bool alive, bool signerMatch, ) = _validate(request);
+        return alive && signerMatch;
+    }
+
+    /**
+     * @dev Executes a `request` on behalf of `signature`'s signer using the ERC-2771 protocol. The gas
+     * provided to the requested call may not be exactly the amount requested, but the call will not run
+     * out of gas. Will revert if the request is invalid or the call reverts, in this case the nonce is not consumed.
+     *
+     * Requirements:
+     *
+     * - The request value should be equal to the provided `msg.value`.
+     * - The request should be valid according to {verify}.
+     */
+    function execute(ForwardRequestData calldata request) public payable virtual {
+        // We make sure that msg.value and request.value match exactly.
+        // If the request is invalid or the call reverts, this whole function
+        // will revert, ensuring value isn't stuck.
+        if (msg.value != request.value) {
+            revert ERC2771ForwarderMismatchedValue(request.value, msg.value);
+        }
+
+        if (!_execute(request, true)) {
+            revert Address.FailedInnerCall();
+        }
+    }
+
+    /**
+     * @dev Batch version of {execute} with optional refunding and atomic execution.
+     *
+     * In case a batch contains at least one invalid request (see {verify}), the
+     * request will be skipped and the `refundReceiver` parameter will receive back the
+     * unused requested value at the end of the execution. This is done to prevent reverting
+     * the entire batch when a request is invalid or has already been submitted.
+     *
+     * If the `refundReceiver` is the `address(0)`, this function will revert when at least
+     * one of the requests was not valid instead of skipping it. This could be useful if
+     * a batch is required to get executed atomically (at least at the top-level). For example,
+     * refunding (and thus atomicity) can be opt-out if the relayer is using a service that avoids
+     * including reverted transactions.
+     *
+     * Requirements:
+     *
+     * - The sum of the requests' values should be equal to the provided `msg.value`.
+     * - All of the requests should be valid (see {verify}) when `refundReceiver` is the zero address.
+     *
+     * NOTE: Setting a zero `refundReceiver` guarantees an all-or-nothing requests execution only for
+     * the first-level forwarded calls. In case a forwarded request calls to a contract with another
+     * subcall, the second-level call may revert without the top-level call reverting.
+     */
+    function executeBatch(
+        ForwardRequestData[] calldata requests,
+        address payable refundReceiver
+    ) public payable virtual {
+        bool atomic = refundReceiver == address(0);
+
+        uint256 requestsValue;
+        uint256 refundValue;
+
+        for (uint256 i; i < requests.length; ++i) {
+            requestsValue += requests[i].value;
+            bool success = _execute(requests[i], atomic);
+            if (!success) {
+                refundValue += requests[i].value;
+            }
+        }
+
+        // The batch should revert if there's a mismatched msg.value provided
+        // to avoid request value tampering
+        if (requestsValue != msg.value) {
+            revert ERC2771ForwarderMismatchedValue(requestsValue, msg.value);
+        }
+
+        // Some requests with value were invalid (possibly due to frontrunning).
+        // To avoid leaving ETH in the contract this value is refunded.
+        if (refundValue != 0) {
+            // We know refundReceiver != address(0) && requestsValue == msg.value
+            // meaning we can ensure refundValue is not taken from the original contract's balance
+            // and refundReceiver is a known account.
+            Address.sendValue(refundReceiver, refundValue);
+        }
+    }
+
+    /**
+     * @dev Validates if the provided request can be executed at current block timestamp with
+     * the given `request.signature` on behalf of `request.signer`.
+     */
+    function _validate(
+        ForwardRequestData calldata request
+    ) internal view virtual returns (bool alive, bool signerMatch, address signer) {
+        signer = _recoverForwardRequestSigner(request);
+        return (request.deadline >= block.timestamp, signer == request.from, signer);
+    }
+
+    /**
+     * @dev Recovers the signer of an EIP712 message hash for a forward `request` and its corresponding `signature`.
+     * See {ECDSA-recover}.
+     */
+    function _recoverForwardRequestSigner(ForwardRequestData calldata request) internal view virtual returns (address) {
+        return
+            _hashTypedDataV4(
+                keccak256(
+                    abi.encode(
+                        _FORWARD_REQUEST_TYPEHASH,
+                        request.from,
+                        request.to,
+                        request.value,
+                        request.gas,
+                        nonces(request.from),
+                        request.deadline,
+                        keccak256(request.data)
+                    )
+                )
+            ).recover(request.signature);
+    }
+
+    /**
+     * @dev Validates and executes a signed request returning the request call `success` value.
+     *
+     * Internal function without msg.value validation.
+     *
+     * Requirements:
+     *
+     * - The caller must have provided enough gas to forward with the call.
+     * - The request must be valid (see {verify}) if the `requireValidRequest` is true.
+     *
+     * Emits an {ExecutedForwardRequest} event.
+     *
+     * IMPORTANT: Using this function doesn't check that all the `msg.value` was sent, potentially
+     * leaving value stuck in the contract.
+     */
+    function _execute(
+        ForwardRequestData calldata request,
+        bool requireValidRequest
+    ) internal virtual returns (bool success) {
+        (bool alive, bool signerMatch, address signer) = _validate(request);
+
+        // Need to explicitly specify if a revert is required since non-reverting is default for
+        // batches and reversion is opt-in since it could be useful in some scenarios
+        if (requireValidRequest) {
+            if (!alive) {
+                revert ERC2771ForwarderExpiredRequest(request.deadline);
+            }
+
+            if (!signerMatch) {
+                revert ERC2771ForwarderInvalidSigner(signer, request.from);
+            }
+        }
+
+        // Ignore an invalid request because requireValidRequest = false
+        if (signerMatch && alive) {
+            // Nonce should be used before the call to prevent reusing by reentrancy
+            uint256 currentNonce = _useNonce(signer);
+
+            (success, ) = request.to.call{gas: request.gas, value: request.value}(
+                abi.encodePacked(request.data, request.from)
+            );
+
+            _checkForwardedGas(request);
+
+            emit ExecutedForwardRequest(signer, currentNonce, success);
+        }
+    }
+
+    /**
+     * @dev Checks if the requested gas was correctly forwarded to the callee.
+     *
+     * As a consequence of https://eips.ethereum.org/EIPS/eip-150[EIP-150]:
+     * - At most `gasleft() - floor(gasleft() / 64)` is forwarded to the callee.
+     * - At least `floor(gasleft() / 64)` is kept in the caller.
+     *
+     * It reverts consuming all the available gas if the forwarded gas is not the requested gas.
+     *
+     * IMPORTANT: This function should be called exactly the end of the forwarded call. Any gas consumed
+     * in between will make room for bypassing this check.
+     */
+    function _checkForwardedGas(ForwardRequestData calldata request) private view {
+        // To avoid insufficient gas griefing attacks, as referenced in https://ronan.eth.limo/blog/ethereum-gas-dangers/
+        //
+        // A malicious relayer can attempt to shrink the gas forwarded so that the underlying call reverts out-of-gas
+        // but the forwarding itself still succeeds. In order to make sure that the subcall received sufficient gas,
+        // we will inspect gasleft() after the forwarding.
+        //
+        // Let X be the gas available before the subcall, such that the subcall gets at most X * 63 / 64.
+        // We can't know X after CALL dynamic costs, but we want it to be such that X * 63 / 64 >= req.gas.
+        // Let Y be the gas used in the subcall. gasleft() measured immediately after the subcall will be gasleft() = X - Y.
+        // If the subcall ran out of gas, then Y = X * 63 / 64 and gasleft() = X - Y = X / 64.
+        // Under this assumption req.gas / 63 > gasleft() is true is true if and only if
+        // req.gas / 63 > X / 64, or equivalently req.gas > X * 63 / 64.
+        // This means that if the subcall runs out of gas we are able to detect that insufficient gas was passed.
+        //
+        // We will now also see that req.gas / 63 > gasleft() implies that req.gas >= X * 63 / 64.
+        // The contract guarantees Y <= req.gas, thus gasleft() = X - Y >= X - req.gas.
+        // -    req.gas / 63 > gasleft()
+        // -    req.gas / 63 >= X - req.gas
+        // -    req.gas >= X * 63 / 64
+        // In other words if req.gas < X * 63 / 64 then req.gas / 63 <= gasleft(), thus if the relayer behaves honestly
+        // the forwarding does not revert.
+        if (gasleft() < request.gas / 63) {
+            // We explicitly trigger invalid opcode to consume all gas and bubble-up the effects, since
+            // neither revert or assert consume all gas since Solidity 0.8.0
+            // https://docs.soliditylang.org/en/v0.8.0/control-structures.html#panic-via-assert-and-error-via-require
+            /// @solidity memory-safe-assembly
+            assembly {
+                invalid()
+            }
+        }
+    }
+}

contracts/metatx/MinimalForwarder.sol

@@ -1,104 +0,0 @@
-// SPDX-License-Identifier: MIT
-// OpenZeppelin Contracts (last updated v4.9.0) (metatx/MinimalForwarder.sol)
-
-pragma solidity ^0.8.19;
-
-import "../utils/cryptography/ECDSA.sol";
-import "../utils/cryptography/EIP712.sol";
-
-/**
- * @dev Simple minimal forwarder to be used together with an ERC2771 compatible contract. See {ERC2771Context}.
- *
- * MinimalForwarder is mainly meant for testing, as it is missing features to be a good production-ready forwarder. This
- * contract does not intend to have all the properties that are needed for a sound forwarding system. A fully
- * functioning forwarding system with good properties requires more complexity. We suggest you look at other projects
- * such as the GSN which do have the goal of building a system like that.
- */
-contract MinimalForwarder is EIP712 {
-    using ECDSA for bytes32;
-
-    struct ForwardRequest {
-        address from;
-        address to;
-        uint256 value;
-        uint256 gas;
-        uint256 nonce;
-        bytes data;
-    }
-
-    bytes32 private constant _TYPEHASH =
-        keccak256("ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data)");
-
-    mapping(address => uint256) private _nonces;
-
-    /**
-     * @dev The request `from` doesn't match with the recovered `signer`.
-     */
-    error MinimalForwarderInvalidSigner(address signer, address from);
-
-    /**
-     * @dev The request nonce doesn't match with the `current` nonce for the request signer.
-     */
-    error MinimalForwarderInvalidNonce(address signer, uint256 current);
-
-    constructor() EIP712("MinimalForwarder", "0.0.1") {}
-
-    function getNonce(address from) public view returns (uint256) {
-        return _nonces[from];
-    }
-
-    function verify(ForwardRequest calldata req, bytes calldata signature) public view returns (bool) {
-        address signer = _recover(req, signature);
-        (bool correctFrom, bool correctNonce) = _validateReq(req, signer);
-        return correctFrom && correctNonce;
-    }
-
-    function execute(
-        ForwardRequest calldata req,
-        bytes calldata signature
-    ) public payable returns (bool, bytes memory) {
-        address signer = _recover(req, signature);
-        (bool correctFrom, bool correctNonce) = _validateReq(req, signer);
-
-        if (!correctFrom) {
-            revert MinimalForwarderInvalidSigner(signer, req.from);
-        }
-        if (!correctNonce) {
-            revert MinimalForwarderInvalidNonce(signer, _nonces[req.from]);
-        }
-
-        _nonces[req.from] = req.nonce + 1;
-
-        (bool success, bytes memory returndata) = req.to.call{gas: req.gas, value: req.value}(
-            abi.encodePacked(req.data, req.from)
-        );
-
-        // Validate that the relayer has sent enough gas for the call.
-        // See https://ronan.eth.limo/blog/ethereum-gas-dangers/
-        if (gasleft() <= req.gas / 63) {
-            // We explicitly trigger invalid opcode to consume all gas and bubble-up the effects, since
-            // neither revert or assert consume all gas since Solidity 0.8.0
-            // https://docs.soliditylang.org/en/v0.8.0/control-structures.html#panic-via-assert-and-error-via-require
-            /// @solidity memory-safe-assembly
-            assembly {
-                invalid()
-            }
-        }
-
-        return (success, returndata);
-    }
-
-    function _recover(ForwardRequest calldata req, bytes calldata signature) internal view returns (address) {
-        return
-            _hashTypedDataV4(
-                keccak256(abi.encode(_TYPEHASH, req.from, req.to, req.value, req.gas, req.nonce, keccak256(req.data)))
-            ).recover(signature);
-    }
-
-    function _validateReq(
-        ForwardRequest calldata req,
-        address signer
-    ) internal view returns (bool correctFrom, bool correctNonce) {
-        return (signer == req.from, _nonces[req.from] == req.nonce);
-    }
-}

contracts/metatx/README.adoc

@@ -9,4 +9,4 @@ NOTE: This document is better viewed at https://docs.openzeppelin.com/contracts/
 
 == Utils
 
-{{MinimalForwarder}}
+{{ERC2771Forwarder}}

contracts/utils/cryptography/EIP712.sol

@@ -110,7 +110,7 @@ abstract contract EIP712 is IERC5267 {
     }
 
     /**
-     * @dev See {EIP-5267}.
+     * @dev See {IERC-5267}.
      *
      * _Available since v4.9._
      */

test/metatx/ERC2771Context.test.js

@@ -6,14 +6,16 @@ const { expectEvent } = require('@openzeppelin/test-helpers');
 const { expect } = require('chai');
 
 const ERC2771ContextMock = artifacts.require('ERC2771ContextMock');
-const MinimalForwarder = artifacts.require('MinimalForwarder');
+const ERC2771Forwarder = artifacts.require('ERC2771Forwarder');
 const ContextMockCaller = artifacts.require('ContextMockCaller');
 
 const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior');
 
 contract('ERC2771Context', function (accounts) {
+  const MAX_UINT48 = web3.utils.toBN(1).shln(48).subn(1).toString();
+
   beforeEach(async function () {
-    this.forwarder = await MinimalForwarder.new();
+    this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder');
     this.recipient = await ERC2771ContextMock.new(this.forwarder.address);
 
     this.domain = await getDomain(this.forwarder);
@@ -25,6 +27,7 @@ contract('ERC2771Context', function (accounts) {
         { name: 'value', type: 'uint256' },
         { name: 'gas', type: 'uint256' },
         { name: 'nonce', type: 'uint256' },
+        { name: 'deadline', type: 'uint48' },
         { name: 'data', type: 'bytes' },
       ],
     };
@@ -63,14 +66,17 @@ contract('ERC2771Context', function (accounts) {
           to: this.recipient.address,
           value: '0',
           gas: '100000',
-          nonce: (await this.forwarder.getNonce(this.sender)).toString(),
+          nonce: (await this.forwarder.nonces(this.sender)).toString(),
+          deadline: MAX_UINT48,
           data,
         };
 
-        const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } });
-        expect(await this.forwarder.verify(req, sign)).to.equal(true);
+        req.signature = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), {
+          data: { ...this.data, message: req },
+        });
+        expect(await this.forwarder.verify(req)).to.equal(true);
 
-        const { tx } = await this.forwarder.execute(req, sign);
+        const { tx } = await this.forwarder.execute(req);
         await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Sender', { sender: this.sender });
       });
     });
@@ -86,14 +92,17 @@ contract('ERC2771Context', function (accounts) {
           to: this.recipient.address,
           value: '0',
           gas: '100000',
-          nonce: (await this.forwarder.getNonce(this.sender)).toString(),
+          nonce: (await this.forwarder.nonces(this.sender)).toString(),
+          deadline: MAX_UINT48,
           data,
         };
 
-        const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } });
-        expect(await this.forwarder.verify(req, sign)).to.equal(true);
+        req.signature = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), {
+          data: { ...this.data, message: req },
+        });
+        expect(await this.forwarder.verify(req)).to.equal(true);
 
-        const { tx } = await this.forwarder.execute(req, sign);
+        const { tx } = await this.forwarder.execute(req);
         await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Data', { data, integerValue, stringValue });
       });
     });

test/metatx/ERC2771Forwarder.test.js

@@ -0,0 +1,433 @@
+const ethSigUtil = require('eth-sig-util');
+const Wallet = require('ethereumjs-wallet').default;
+const { getDomain, domainType } = require('../helpers/eip712');
+const { expectRevertCustomError } = require('../helpers/customError');
+
+const { constants, expectRevert, expectEvent, time } = require('@openzeppelin/test-helpers');
+const { expect } = require('chai');
+
+const ERC2771Forwarder = artifacts.require('ERC2771Forwarder');
+const CallReceiverMock = artifacts.require('CallReceiverMock');
+
+contract('ERC2771Forwarder', function (accounts) {
+  const [, refundReceiver, another] = accounts;
+
+  const tamperedValues = {
+    from: another,
+    to: another,
+    value: web3.utils.toWei('0.5'),
+    data: '0x1742',
+    deadline: 0xdeadbeef,
+  };
+
+  beforeEach(async function () {
+    this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder');
+
+    this.domain = await getDomain(this.forwarder);
+    this.types = {
+      EIP712Domain: domainType(this.domain),
+      ForwardRequest: [
+        { name: 'from', type: 'address' },
+        { name: 'to', type: 'address' },
+        { name: 'value', type: 'uint256' },
+        { name: 'gas', type: 'uint256' },
+        { name: 'nonce', type: 'uint256' },
+        { name: 'deadline', type: 'uint48' },
+        { name: 'data', type: 'bytes' },
+      ],
+    };
+
+    this.alice = Wallet.generate();
+    this.alice.address = web3.utils.toChecksumAddress(this.alice.getAddressString());
+
+    this.timestamp = await time.latest();
+    this.request = {
+      from: this.alice.address,
+      to: constants.ZERO_ADDRESS,
+      value: '0',
+      gas: '100000',
+      data: '0x',
+      deadline: this.timestamp.toNumber() + 60, // 1 minute
+    };
+    this.requestData = {
+      ...this.request,
+      nonce: (await this.forwarder.nonces(this.alice.address)).toString(),
+    };
+
+    this.forgeData = request => ({
+      types: this.types,
+      domain: this.domain,
+      primaryType: 'ForwardRequest',
+      message: { ...this.requestData, ...request },
+    });
+    this.sign = (privateKey, request) =>
+      ethSigUtil.signTypedMessage(privateKey, {
+        data: this.forgeData(request),
+      });
+
+    this.requestData.signature = this.sign(this.alice.getPrivateKey());
+  });
+
+  context('verify', function () {
+    context('with valid signature', function () {
+      it('returns true without altering the nonce', async function () {
+        expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal(
+          web3.utils.toBN(this.requestData.nonce),
+        );
+        expect(await this.forwarder.verify(this.requestData)).to.be.equal(true);
+        expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal(
+          web3.utils.toBN(this.requestData.nonce),
+        );
+      });
+    });
+
+    context('with tampered values', function () {
+      for (const [key, value] of Object.entries(tamperedValues)) {
+        it(`returns false with tampered ${key}`, async function () {
+          expect(await this.forwarder.verify(this.forgeData({ [key]: value }).message)).to.be.equal(false);
+        });
+      }
+
+      it('returns false with tampered signature', async function () {
+        const tamperedsign = web3.utils.hexToBytes(this.requestData.signature);
+        tamperedsign[42] ^= 0xff;
+        this.requestData.signature = web3.utils.bytesToHex(tamperedsign);
+        expect(await this.forwarder.verify(this.requestData)).to.be.equal(false);
+      });
+
+      it('returns false with valid signature for non-current nonce', async function () {
+        const req = {
+          ...this.requestData,
+          nonce: this.requestData.nonce + 1,
+        };
+        req.signature = this.sign(this.alice.getPrivateKey(), req);
+        expect(await this.forwarder.verify(req)).to.be.equal(false);
+      });
+
+      it('returns false with valid signature for expired deadline', async function () {
+        const req = {
+          ...this.requestData,
+          deadline: this.timestamp - 1,
+        };
+        req.signature = this.sign(this.alice.getPrivateKey(), req);
+        expect(await this.forwarder.verify(req)).to.be.equal(false);
+      });
+    });
+  });
+
+  context('execute', function () {
+    context('with valid requests', function () {
+      beforeEach(async function () {
+        expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal(
+          web3.utils.toBN(this.requestData.nonce),
+        );
+      });
+
+      it('emits an event and consumes nonce for a successful request', async function () {
+        const receipt = await this.forwarder.execute(this.requestData);
+        expectEvent(receipt, 'ExecutedForwardRequest', {
+          signer: this.requestData.from,
+          nonce: web3.utils.toBN(this.requestData.nonce),
+          success: true,
+        });
+        expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal(
+          web3.utils.toBN(this.requestData.nonce + 1),
+        );
+      });
+
+      it('reverts with an unsuccessful request', async function () {
+        const receiver = await CallReceiverMock.new();
+        const req = {
+          ...this.requestData,
+          to: receiver.address,
+          data: receiver.contract.methods.mockFunctionRevertsNoReason().encodeABI(),
+        };
+        req.signature = this.sign(this.alice.getPrivateKey(), req);
+        await expectRevertCustomError(this.forwarder.execute(req), 'FailedInnerCall', []);
+      });
+    });
+
+    context('with tampered request', function () {
+      for (const [key, value] of Object.entries(tamperedValues)) {
+        it(`reverts with tampered ${key}`, async function () {
+          const data = this.forgeData({ [key]: value });
+          await expectRevertCustomError(
+            this.forwarder.execute(data.message, {
+              value: key == 'value' ? value : 0, // To avoid MismatchedValue error
+            }),
+            'ERC2771ForwarderInvalidSigner',
+            [ethSigUtil.recoverTypedSignature({ data, sig: this.requestData.signature }), data.message.from],
+          );
+        });
+      }
+
+      it('reverts with tampered signature', async function () {
+        const tamperedSig = web3.utils.hexToBytes(this.requestData.signature);
+        tamperedSig[42] ^= 0xff;
+        this.requestData.signature = web3.utils.bytesToHex(tamperedSig);
+        await expectRevertCustomError(this.forwarder.execute(this.requestData), 'ERC2771ForwarderInvalidSigner', [
+          ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }),
+          this.requestData.from,
+        ]);
+      });
+
+      it('reverts with valid signature for non-current nonce', async function () {
+        // Execute first a request
+        await this.forwarder.execute(this.requestData);
+
+        // And then fail due to an already used nonce
+        await expectRevertCustomError(this.forwarder.execute(this.requestData), 'ERC2771ForwarderInvalidSigner', [
+          ethSigUtil.recoverTypedSignature({
+            data: this.forgeData({ ...this.requestData, nonce: this.requestData.nonce + 1 }),
+            sig: this.requestData.signature,
+          }),
+          this.requestData.from,
+        ]);
+      });
+
+      it('reverts with valid signature for expired deadline', async function () {
+        const req = {
+          ...this.requestData,
+          deadline: this.timestamp - 1,
+        };
+        req.signature = this.sign(this.alice.getPrivateKey(), req);
+        await expectRevertCustomError(this.forwarder.execute(req), 'ERC2771ForwarderExpiredRequest', [
+          this.timestamp - 1,
+        ]);
+      });
+
+      it('reverts with valid signature but mismatched value', async function () {
+        const value = 100;
+        const req = {
+          ...this.requestData,
+          value,
+        };
+        req.signature = this.sign(this.alice.getPrivateKey(), req);
+        await expectRevertCustomError(this.forwarder.execute(req), 'ERC2771ForwarderMismatchedValue', [0, value]);
+      });
+    });
+
+    it('bubbles out of gas', async function () {
+      const receiver = await CallReceiverMock.new();
+      const gasAvailable = 100000;
+      this.requestData.to = receiver.address;
+      this.requestData.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI();
+      this.requestData.gas = 1000000;
+
+      this.requestData.signature = this.sign(this.alice.getPrivateKey());
+
+      await expectRevert.assertion(this.forwarder.execute(this.requestData, { gas: gasAvailable }));
+
+      const { transactions } = await web3.eth.getBlock('latest');
+      const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]);
+
+      expect(gasUsed).to.be.equal(gasAvailable);
+    });
+  });
+
+  context('executeBatch', function () {
+    const batchValue = requestDatas => requestDatas.reduce((value, request) => value + Number(request.value), 0);
+
+    beforeEach(async function () {
+      this.bob = Wallet.generate();
+      this.bob.address = web3.utils.toChecksumAddress(this.bob.getAddressString());
+
+      this.eve = Wallet.generate();
+      this.eve.address = web3.utils.toChecksumAddress(this.eve.getAddressString());
+
+      this.signers = [this.alice, this.bob, this.eve];
+
+      this.requestDatas = await Promise.all(
+        this.signers.map(async ({ address }) => ({
+          ...this.requestData,
+          from: address,
+          nonce: (await this.forwarder.nonces(address)).toString(),
+          value: web3.utils.toWei('10', 'gwei'),
+        })),
+      );
+
+      this.requestDatas = this.requestDatas.map((requestData, i) => ({
+        ...requestData,
+        signature: this.sign(this.signers[i].getPrivateKey(), requestData),
+      }));
+
+      this.msgValue = batchValue(this.requestDatas);
+    });
+
+    context('with valid requests', function () {
+      beforeEach(async function () {
+        for (const request of this.requestDatas) {
+          expect(await this.forwarder.verify(request)).to.be.equal(true);
+        }
+
+        this.receipt = await this.forwarder.executeBatch(this.requestDatas, another, { value: this.msgValue });
+      });
+
+      it('emits events', async function () {
+        for (const request of this.requestDatas) {
+          expectEvent(this.receipt, 'ExecutedForwardRequest', {
+            signer: request.from,
+            nonce: web3.utils.toBN(request.nonce),
+            success: true,
+          });
+        }
+      });
+
+      it('increase nonces', async function () {
+        for (const request of this.requestDatas) {
+          expect(await this.forwarder.nonces(request.from)).to.be.bignumber.eq(web3.utils.toBN(request.nonce + 1));
+        }
+      });
+    });
+
+    context('with tampered requests', function () {
+      beforeEach(async function () {
+        this.idx = 1; // Tampered idx
+      });
+
+      it('reverts with mismatched value', async function () {
+        this.requestDatas[this.idx].value = 100;
+        this.requestDatas[this.idx].signature = this.sign(
+          this.signers[this.idx].getPrivateKey(),
+          this.requestDatas[this.idx],
+        );
+        await expectRevertCustomError(
+          this.forwarder.executeBatch(this.requestDatas, another, { value: this.msgValue }),
+          'ERC2771ForwarderMismatchedValue',
+          [batchValue(this.requestDatas), this.msgValue],
+        );
+      });
+
+      context('when the refund receiver is the zero address', function () {
+        beforeEach(function () {
+          this.refundReceiver = constants.ZERO_ADDRESS;
+        });
+
+        for (const [key, value] of Object.entries(tamperedValues)) {
+          it(`reverts with at least one tampered request ${key}`, async function () {
+            const data = this.forgeData({ ...this.requestDatas[this.idx], [key]: value });
+
+            this.requestDatas[this.idx] = data.message;
+
+            await expectRevertCustomError(
+              this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }),
+              'ERC2771ForwarderInvalidSigner',
+              [
+                ethSigUtil.recoverTypedSignature({ data, sig: this.requestDatas[this.idx].signature }),
+                data.message.from,
+              ],
+            );
+          });
+        }
+
+        it('reverts with at least one tampered request signature', async function () {
+          const tamperedSig = web3.utils.hexToBytes(this.requestDatas[this.idx].signature);
+          tamperedSig[42] ^= 0xff;
+
+          this.requestDatas[this.idx].signature = web3.utils.bytesToHex(tamperedSig);
+
+          await expectRevertCustomError(
+            this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }),
+            'ERC2771ForwarderInvalidSigner',
+            [
+              ethSigUtil.recoverTypedSignature({
+                data: this.forgeData(this.requestDatas[this.idx]),
+                sig: this.requestDatas[this.idx].signature,
+              }),
+              this.requestDatas[this.idx].from,
+            ],
+          );
+        });
+
+        it('reverts with at least one valid signature for non-current nonce', async function () {
+          // Execute first a request
+          await this.forwarder.execute(this.requestDatas[this.idx], { value: this.requestDatas[this.idx].value });
+
+          // And then fail due to an already used nonce
+          await expectRevertCustomError(
+            this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }),
+            'ERC2771ForwarderInvalidSigner',
+            [
+              ethSigUtil.recoverTypedSignature({
+                data: this.forgeData({ ...this.requestDatas[this.idx], nonce: this.requestDatas[this.idx].nonce + 1 }),
+                sig: this.requestDatas[this.idx].signature,
+              }),
+              this.requestDatas[this.idx].from,
+            ],
+          );
+        });
+
+        it('reverts with at least one valid signature for expired deadline', async function () {
+          this.requestDatas[this.idx].deadline = this.timestamp.toNumber() - 1;
+          this.requestDatas[this.idx].signature = this.sign(
+            this.signers[this.idx].getPrivateKey(),
+            this.requestDatas[this.idx],
+          );
+          await expectRevertCustomError(
+            this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }),
+            'ERC2771ForwarderExpiredRequest',
+            [this.timestamp.toNumber() - 1],
+          );
+        });
+      });
+
+      context('when the refund receiver is a known address', function () {
+        beforeEach(async function () {
+          this.refundReceiver = refundReceiver;
+          this.initialRefundReceiverBalance = web3.utils.toBN(await web3.eth.getBalance(this.refundReceiver));
+          this.initialTamperedRequestNonce = await this.forwarder.nonces(this.requestDatas[this.idx].from);
+        });
+
+        for (const [key, value] of Object.entries(tamperedValues)) {
+          it(`ignores a request with tampered ${key} and refunds its value`, async function () {
+            const data = this.forgeData({ ...this.requestDatas[this.idx], [key]: value });
+
+            this.requestDatas[this.idx] = data.message;
+
+            const receipt = await this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, {
+              value: batchValue(this.requestDatas),
+            });
+            expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2);
+          });
+        }
+
+        it('ignores a request with a valid signature for non-current nonce', async function () {
+          // Execute first a request
+          await this.forwarder.execute(this.requestDatas[this.idx], { value: this.requestDatas[this.idx].value });
+          this.initialTamperedRequestNonce++; // Should be already incremented by the individual `execute`
+
+          // And then ignore the same request in a batch due to an already used nonce
+          const receipt = await this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, {
+            value: this.msgValue,
+          });
+          expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2);
+        });
+
+        it('ignores a request with a valid signature for expired deadline', async function () {
+          this.requestDatas[this.idx].deadline = this.timestamp.toNumber() - 1;
+          this.requestDatas[this.idx].signature = this.sign(
+            this.signers[this.idx].getPrivateKey(),
+            this.requestDatas[this.idx],
+          );
+
+          const receipt = await this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, {
+            value: this.msgValue,
+          });
+          expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2);
+        });
+
+        afterEach(async function () {
+          // The invalid request value was refunded
+          expect(await web3.eth.getBalance(this.refundReceiver)).to.be.bignumber.equal(
+            this.initialRefundReceiverBalance.add(web3.utils.toBN(this.requestDatas[this.idx].value)),
+          );
+
+          // The invalid request from's nonce was not incremented
+          expect(await this.forwarder.nonces(this.requestDatas[this.idx].from)).to.be.bignumber.eq(
+            web3.utils.toBN(this.initialTamperedRequestNonce),
+          );
+        });
+      });
+    });
+  });
+});

test/metatx/MinimalForwarder.test.js

@@ -1,171 +0,0 @@
-const ethSigUtil = require('eth-sig-util');
-const Wallet = require('ethereumjs-wallet').default;
-const { getDomain, domainType } = require('../helpers/eip712');
-const { expectRevertCustomError } = require('../helpers/customError');
-
-const { constants, expectRevert } = require('@openzeppelin/test-helpers');
-const { expect } = require('chai');
-
-const MinimalForwarder = artifacts.require('MinimalForwarder');
-const CallReceiverMock = artifacts.require('CallReceiverMock');
-
-contract('MinimalForwarder', function (accounts) {
-  beforeEach(async function () {
-    this.forwarder = await MinimalForwarder.new();
-
-    this.domain = await getDomain(this.forwarder);
-    this.types = {
-      EIP712Domain: domainType(this.domain),
-      ForwardRequest: [
-        { name: 'from', type: 'address' },
-        { name: 'to', type: 'address' },
-        { name: 'value', type: 'uint256' },
-        { name: 'gas', type: 'uint256' },
-        { name: 'nonce', type: 'uint256' },
-        { name: 'data', type: 'bytes' },
-      ],
-    };
-  });
-
-  context('with message', function () {
-    const tamperedValues = {
-      from: accounts[0],
-      to: accounts[0],
-      value: web3.utils.toWei('1'),
-      nonce: 1234,
-      data: '0x1742',
-    };
-
-    beforeEach(async function () {
-      this.wallet = Wallet.generate();
-      this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString());
-      this.req = {
-        from: this.sender,
-        to: constants.ZERO_ADDRESS,
-        value: '0',
-        gas: '100000',
-        nonce: Number(await this.forwarder.getNonce(this.sender)),
-        data: '0x',
-      };
-      this.forgeData = req => ({
-        types: this.types,
-        domain: this.domain,
-        primaryType: 'ForwardRequest',
-        message: { ...this.req, ...req },
-      });
-      this.sign = req =>
-        ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), {
-          data: this.forgeData(req),
-        });
-    });
-
-    context('verify', function () {
-      context('valid signature', function () {
-        beforeEach(async function () {
-          expect(await this.forwarder.getNonce(this.req.from)).to.be.bignumber.equal(web3.utils.toBN(this.req.nonce));
-        });
-
-        it('success', async function () {
-          expect(await this.forwarder.verify(this.req, this.sign())).to.be.equal(true);
-        });
-
-        afterEach(async function () {
-          expect(await this.forwarder.getNonce(this.req.from)).to.be.bignumber.equal(web3.utils.toBN(this.req.nonce));
-        });
-      });
-
-      context('with tampered values', function () {
-        for (const [key, value] of Object.entries(tamperedValues)) {
-          it(`returns false with tampered ${key}`, async function () {
-            expect(await this.forwarder.verify(this.forgeData({ [key]: value }).message, this.sign())).to.be.equal(
-              false,
-            );
-          });
-        }
-
-        it('returns false with tampered signature', async function () {
-          const tamperedsign = web3.utils.hexToBytes(this.sign());
-          tamperedsign[42] ^= 0xff;
-          expect(await this.forwarder.verify(this.req, web3.utils.bytesToHex(tamperedsign))).to.be.equal(false);
-        });
-
-        it('returns false with valid signature for non-current nonce', async function () {
-          const req = {
-            ...this.req,
-            nonce: this.req.nonce + 1,
-          };
-          const sig = this.sign(req);
-          expect(await this.forwarder.verify(req, sig)).to.be.equal(false);
-        });
-      });
-    });
-
-    context('execute', function () {
-      context('valid signature', function () {
-        beforeEach(async function () {
-          expect(await this.forwarder.getNonce(this.req.from)).to.be.bignumber.equal(web3.utils.toBN(this.req.nonce));
-        });
-
-        it('success', async function () {
-          await this.forwarder.execute(this.req, this.sign()); // expect to not revert
-        });
-
-        afterEach(async function () {
-          expect(await this.forwarder.getNonce(this.req.from)).to.be.bignumber.equal(
-            web3.utils.toBN(this.req.nonce + 1),
-          );
-        });
-      });
-
-      context('with tampered values', function () {
-        for (const [key, value] of Object.entries(tamperedValues)) {
-          it(`reverts with tampered ${key}`, async function () {
-            const sig = this.sign();
-            const data = this.forgeData({ [key]: value });
-            await expectRevertCustomError(this.forwarder.execute(data.message, sig), 'MinimalForwarderInvalidSigner', [
-              ethSigUtil.recoverTypedSignature({ data, sig }),
-              data.message.from,
-            ]);
-          });
-        }
-
-        it('reverts with tampered signature', async function () {
-          const tamperedSig = web3.utils.hexToBytes(this.sign());
-          tamperedSig[42] ^= 0xff;
-          await expectRevertCustomError(
-            this.forwarder.execute(this.req, web3.utils.bytesToHex(tamperedSig)),
-            'MinimalForwarderInvalidSigner',
-            [ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }), this.req.from],
-          );
-        });
-
-        it('reverts with valid signature for non-current nonce', async function () {
-          const req = {
-            ...this.req,
-            nonce: this.req.nonce + 1,
-          };
-          const sig = this.sign(req);
-          await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderInvalidNonce', [
-            this.req.from,
-            this.req.nonce,
-          ]);
-        });
-      });
-
-      it('bubble out of gas', async function () {
-        const receiver = await CallReceiverMock.new();
-        const gasAvailable = 100000;
-        this.req.to = receiver.address;
-        this.req.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI();
-        this.req.gas = 1000000;
-
-        await expectRevert.assertion(this.forwarder.execute(this.req, this.sign(), { gas: gasAvailable }));
-
-        const { transactions } = await web3.eth.getBlock('latest');
-        const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]);
-
-        expect(gasUsed).to.be.equal(gasAvailable);
-      });
-    });
-  });
-});