Add ERC2771Forwarder as an enhanced successor to MinimalForwarder

.changeset/blue-horses-do.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`MinimalForwarder`: Added `deadline` for expiring transactions and added `msg.value` mismatch check.

contracts/metatx/MinimalForwarder.sol

@@ -5,16 +5,22 @@ pragma solidity ^0.8.19;
 
 import "../utils/cryptography/ECDSA.sol";
 import "../utils/cryptography/EIP712.sol";
+import "../utils/Nonces.sol";
 
 /**
- * @dev Simple minimal forwarder to be used together with an ERC2771 compatible contract. See {ERC2771Context}.
+ * @dev A minimal implementation of a production-ready forwarder compatible with ERC2771 contracts.
  *
- * 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.
+ * 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 ETH 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 MinimalForwarder is EIP712 {
+contract MinimalForwarder is EIP712, Nonces {
     using ECDSA for bytes32;
 
     struct ForwardRequest {
@@ -23,82 +29,202 @@ contract MinimalForwarder is EIP712 {
         uint256 value;
         uint256 gas;
         uint256 nonce;
+        uint48 deadline;
         bytes data;
     }
 
-    bytes32 private constant _TYPEHASH =
-        keccak256("ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data)");
+    bytes32 private constant _FORWARD_REQUEST_TYPEHASH =
+        keccak256(
+            "ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)"
+        );
 
-    mapping(address => uint256) private _nonces;
+    /**
+     * @dev Emitted when a `ForwardRequest` is executed.
+     */
+    event ExecutedForwardRequest(address indexed signer, uint256 nonce, bool success, bytes returndata);
 
     /**
      * @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.
+     * @dev The requested `value` doesn't match with the available `msgValue`, leaving ETH stuck in the contract.
      */
-    error MinimalForwarderInvalidNonce(address signer, uint256 current);
+    error MinimalForwarderMismatchedValue(uint256 value, uint256 msgValue);
 
-    constructor() EIP712("MinimalForwarder", "0.0.1") {}
+    /**
+     * @dev The list of requests length doesn't match with the list of signatures length.
+     */
+    error MinimalForwarderInvalidBatchLength(uint256 requestsLength, uint256 signaturesLength);
 
-    function getNonce(address from) public view returns (uint256) {
-        return _nonces[from];
-    }
+    /**
+     * @dev The request `deadline` has expired.
+     */
+    error MinimalForwarderExpiredRequest(uint256 deadline);
+
+    /**
+     * @dev See {EIP712-constructor}.
+     */
+    constructor(string memory name) EIP712(name, "1") {}
 
-    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;
+    /**
+     * @dev Returns `true` if a request is valid for a provided `signature` at the current block.
+     */
+    function verify(ForwardRequest calldata request, bytes calldata signature) public view virtual returns (bool) {
+        (bool alive, bool signerMatch, bool nonceMatch) = _validate(request, signature);
+        return alive && signerMatch && nonceMatch;
     }
 
+    /**
+     * @dev Executes a `request` on behalf of `signature`'s signer guaranteeing that the forwarded call
+     * will receive the requested gas and no ETH is stuck in the contract.
+     */
     function execute(
-        ForwardRequest calldata req,
+        ForwardRequest calldata request,
         bytes calldata signature
-    ) public payable returns (bool, bytes memory) {
-        address signer = _recover(req, signature);
-        (bool correctFrom, bool correctNonce) = _validateReq(req, signer);
+    ) public payable virtual returns (bool, bytes memory) {
+        (bool success, bytes memory returndata) = _execute(request, signature);
 
-        if (!correctFrom) {
-            revert MinimalForwarderInvalidSigner(signer, req.from);
+        if (msg.value != request.value) {
+            revert MinimalForwarderMismatchedValue(request.value, msg.value);
         }
-        if (!correctNonce) {
-            revert MinimalForwarderInvalidNonce(signer, _nonces[req.from]);
+
+        return (success, returndata);
+    }
+
+    /**
+     * @dev Batch version of {execute}.
+     */
+    function executeBatch(ForwardRequest[] calldata requests, bytes[] calldata signatures) public payable virtual {
+        if (requests.length != signatures.length) {
+            revert MinimalForwarderInvalidBatchLength(requests.length, signatures.length);
         }
 
-        _nonces[req.from] = req.nonce + 1;
+        uint256 requestsValue;
 
-        (bool success, bytes memory returndata) = req.to.call{gas: req.gas, value: req.value}(
-            abi.encodePacked(req.data, req.from)
-        );
+        for (uint256 i; i < requests.length; ++i) {
+            requestsValue += requests[i].value;
+            _execute(requests[i], signatures[i]);
+        }
 
-        // 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()
-            }
+        if (msg.value != requestsValue) {
+            revert MinimalForwarderMismatchedValue(requestsValue, msg.value);
         }
+    }
 
-        return (success, returndata);
+    /**
+     * @dev Validates if the provided request can be executed at current block with `signature` on behalf of `signer`.
+     */
+    function _validate(
+        ForwardRequest calldata request,
+        bytes calldata signature
+    ) internal view virtual returns (bool alive, bool signerMatch, bool nonceMatch) {
+        address signer = _recoverForwardRequestSigner(request, signature);
+        return (request.deadline >= block.number, signer == request.from, nonces(request.from) == request.nonce);
     }
 
-    function _recover(ForwardRequest calldata req, bytes calldata signature) internal view returns (address) {
+    /**
+     * @dev Recovers the signer of an EIP712 message hash for a forward `request` and its corresponding `signature`.
+     * See {ECDSA-recover}.
+     */
+    function _recoverForwardRequestSigner(
+        ForwardRequest calldata request,
+        bytes calldata signature
+    ) internal view virtual returns (address) {
         return
             _hashTypedDataV4(
-                keccak256(abi.encode(_TYPEHASH, req.from, req.to, req.value, req.gas, req.nonce, keccak256(req.data)))
+                keccak256(
+                    abi.encode(
+                        _FORWARD_REQUEST_TYPEHASH,
+                        request.from,
+                        request.to,
+                        request.value,
+                        request.gas,
+                        request.nonce,
+                        request.deadline,
+                        keccak256(request.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);
+    /**
+     * @dev Validates and executes a signed request.
+     *
+     * Requirements:
+     *
+     * - The request's deadline must have not passed.
+     * - The request's from must be the request's signer.
+     * - The request's nonce must match the sender's nonce.
+     * - The caller must have provided enough gas to forward with the call.
+     *
+     * Emits an {ExecutedForwardRequest} event.
+     *
+     * IMPORTANT: Using this function doesn't check that all the `msg.value` was sent, potentially leaving
+     * ETH stuck in the contract.
+     */
+    function _execute(
+        ForwardRequest calldata request,
+        bytes calldata signature
+    ) internal virtual returns (bool success, bytes memory returndata) {
+        // The _validate function is intentionally avoided to keep the signer argument and the nonce check
+
+        if (request.deadline < block.number) {
+            revert MinimalForwarderExpiredRequest(request.deadline);
+        }
+
+        address signer = _recoverForwardRequestSigner(request, signature);
+        if (signer != request.from) {
+            revert MinimalForwarderInvalidSigner(signer, request.from);
+        }
+
+        _useCheckedNonce(request.from, request.nonce);
+
+        (success, returndata) = request.to.call{gas: request.gas, value: request.value}(
+            abi.encodePacked(request.data, request.from)
+        );
+
+        _checkForwardedGas(request);
+
+        emit ExecutedForwardRequest(signer, request.nonce, success, returndata);
+    }
+
+    /**
+     * @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(ForwardRequest 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
+        // and the top-level call still passes, so in order to make sure that the subcall received the requested gas,
+        // we let X be the available gas before the call and require that:
+        //
+        // - 63/64 * X >= req.gas  // Gas before call is enough to forward req.gas to callee
+        // - 63X >= 64req.gas
+        // - 63(X - req.gas) >= req.gas
+        // - (X - req.gas) >= req.gas/63
+        //
+        // Although we can't access X, we let Y be the actual gas used in the subcall so that `gasleft() == X - Y`, then
+        // we know that `X - req.gas <= X - Y`, thus `Y <= req.gas` and finally `X - req.gas <= gasleft()`.
+        // Therefore, any attempt to manipulate X to reduce the gas provided to the callee will result in the following
+        // invariant violated:
+        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/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

@@ -2,7 +2,7 @@ const ethSigUtil = require('eth-sig-util');
 const Wallet = require('ethereumjs-wallet').default;
 const { getDomain, domainType } = require('../helpers/eip712');
 
-const { expectEvent } = require('@openzeppelin/test-helpers');
+const { expectEvent, BN } = require('@openzeppelin/test-helpers');
 const { expect } = require('chai');
 
 const ERC2771ContextMock = artifacts.require('ERC2771ContextMock');
@@ -12,8 +12,10 @@ const ContextMockCaller = artifacts.require('ContextMockCaller');
 const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior');
 
 contract('ERC2771Context', function (accounts) {
+  const MAX_UINT48 = new BN('2').pow(new BN('48')).sub(new BN('1')).toString();
+
   beforeEach(async function () {
-    this.forwarder = await MinimalForwarder.new();
+    this.forwarder = await MinimalForwarder.new('MinimalForwarder', '0.0.1');
     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,7 +66,8 @@ 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,
         };
 
@@ -86,7 +90,8 @@ 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,
         };