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 add `msg.value` mismatch check

contracts/metatx/MinimalForwarder.sol

@@ -5,16 +5,21 @@ 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 request that include:
+ * * `from`: An address to operate on behalf of. It is required to be equal to `msg.sender`.
+ * * `to`: An address destination to call within the request.
+ * * `value`: The amount of ETH to attach within the requested call.
+ * * `gas`: The amount of gas limit that will be forwarded within 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.calldata` to send within the requested call.
  */
-contract MinimalForwarder is EIP712 {
+contract MinimalForwarder is EIP712, Nonces {
     using ECDSA for bytes32;
 
     struct ForwardRequest {
@@ -23,59 +28,85 @@ 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)");
-
-    mapping(address => uint256) private _nonces;
+    bytes32 private constant _FORWARD_REQUEST_TYPEHASH =
+        keccak256(
+            "ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)"
+        );
 
     /**
      * @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 request `value` doesn't match with the `msg.value`.
+     */
+    error MinimalForwaderMismatchedValue(uint256 msgValue, uint256 value);
+
+    /**
+     * @dev The list of requests length doesn't match with the list of signatures length.
      */
-    error MinimalForwarderInvalidNonce(address signer, uint256 current);
+    error MinimalForwaderInvalidBatchLength(uint256 requestsLength, uint256 signaturesLength);
 
-    constructor() EIP712("MinimalForwarder", "0.0.1") {}
+    /**
+     * @dev The request `deadline` has expired.
+     */
+    error MinimalForwarderExpiredRequest(uint256 deadline);
 
-    function getNonce(address from) public view returns (uint256) {
-        return _nonces[from];
-    }
+    /**
+     * @dev See {EIP7712-constructor}.
+     */
+    constructor(string memory name, string memory version) EIP712(name, version) {}
 
-    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.
+     */
     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) {
+        if (request.deadline < block.number) {
+            revert MinimalForwarderExpiredRequest(request.deadline);
+        }
 
-        if (!correctFrom) {
-            revert MinimalForwarderInvalidSigner(signer, req.from);
+        address signer = _recoverForwardRequestSigner(request, signature);
+        if (signer != request.from) {
+            revert MinimalForwarderInvalidSigner(signer, request.from);
         }
-        if (!correctNonce) {
-            revert MinimalForwarderInvalidNonce(signer, _nonces[req.from]);
+
+        if (msg.value != request.value) {
+            revert MinimalForwaderMismatchedValue(msg.value, request.value);
         }
 
-        _nonces[req.from] = req.nonce + 1;
+        _useCheckedNonce(request.from, request.nonce);
 
-        (bool success, bytes memory returndata) = req.to.call{gas: req.gas, value: req.value}(
-            abi.encodePacked(req.data, req.from)
+        // As a consequence of EIP-150, the maximum gas forwarded to a call is 63/64 of the remaining gas. So:
+        // - At most `gasleft() - floor(gasleft() / 64)` is passed.
+        // - At least `floor(gasleft() / 64)` is kept in the caller.
+        // The current gas available is saved for later checking.
+        uint256 gasBefore = gasleft();
+        (bool success, bytes memory returndata) = request.to.call{gas: request.gas, value: request.value}(
+            abi.encodePacked(request.data, request.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) {
+        // To avoid gas griefing attacks, as referenced in https://ronan.eth.limo/blog/ethereum-gas-dangers/
+        // A malicious relayer can attempt to manipulate the gas forwarded so that the underlying call reverts and
+        // the top-level call still passes.
+        // Such manipulation can be prevented by checking if `gasleft() < floor(gasBefore / 64)`. If so, we
+        // can assume an out of gas error was forced in the subcall. There's no need to process such transactions.
+        if (gasleft() < gasBefore / 64) {
             // 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
@@ -88,17 +119,50 @@ contract MinimalForwarder is EIP712 {
         return (success, returndata);
     }
 
-    function _recover(ForwardRequest calldata req, bytes calldata signature) internal view returns (address) {
+    /**
+     * @dev Validates if the provided request can be executed at `blockNumber` with `signature`.
+     */
+    function _validateAt(
+        uint256 blockNumber,
+        ForwardRequest calldata request,
+        bytes calldata signature
+    ) internal view virtual returns (bool alive, bool signerMatch, bool nonceMatch) {
+        address signer = _recoverForwardRequestSigner(request, signature);
+        return (request.deadline >= blockNumber, signer == request.from, nonces(request.from) == request.nonce);
+    }
+
+    /**
+     * @dev Same as {_validateAt} but for the current block.
+     */
+    function _validate(
+        ForwardRequest calldata request,
+        bytes calldata signature
+    ) internal view virtual returns (bool alive, bool signerMatch, bool nonceMatch) {
+        return _validateAt(block.number, request, signature);
+    }
+
+    /**
+     * @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 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);
-    }
 }

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,
         };