Add ERC2771Forwarder fuzz tests for avoiding loss of unused ETH (#4396)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco <fg@frang.io>

contracts/metatx/ERC2771Forwarder.sol

@@ -53,7 +53,7 @@ contract ERC2771Forwarder is EIP712, Nonces {
         bytes signature;
     }
 
-    bytes32 private constant _FORWARD_REQUEST_TYPEHASH =
+    bytes32 internal constant _FORWARD_REQUEST_TYPEHASH =
         keccak256(
             "ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)"
         );
@@ -255,7 +255,7 @@ contract ERC2771Forwarder is EIP712, Nonces {
                 abi.encodePacked(request.data, request.from)
             );
 
-            _checkForwardedGas(request);
+            _checkForwardedGas(gasleft(), request);
 
             emit ExecutedForwardRequest(signer, currentNonce, success);
         }
@@ -270,10 +270,10 @@ contract ERC2771Forwarder is EIP712, Nonces {
      *
      * 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.
+     * IMPORTANT: The `gasLeft` parameter should be measured exactly at 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 {
+    function _checkForwardedGas(uint256 gasLeft, ForwardRequestData calldata request) private pure {
         // 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
@@ -295,7 +295,7 @@ contract ERC2771Forwarder is EIP712, Nonces {
         // -    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) {
+        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

test/metatx/ERC2771Forwarder.t.sol

@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.19;
+
+import {Test} from "forge-std/Test.sol";
+import {ERC2771Forwarder} from "contracts/metatx/ERC2771Forwarder.sol";
+import {CallReceiverMock} from "contracts/mocks/CallReceiverMock.sol";
+
+struct ForwardRequest {
+    address from;
+    address to;
+    uint256 value;
+    uint256 gas;
+    uint256 nonce;
+    uint48 deadline;
+    bytes data;
+}
+
+contract ERC2771ForwarderMock is ERC2771Forwarder {
+    constructor(string memory name) ERC2771Forwarder(name) {}
+
+    function structHash(ForwardRequest calldata request) external view returns (bytes32) {
+        return
+            _hashTypedDataV4(
+                keccak256(
+                    abi.encode(
+                        _FORWARD_REQUEST_TYPEHASH,
+                        request.from,
+                        request.to,
+                        request.value,
+                        request.gas,
+                        request.nonce,
+                        request.deadline,
+                        keccak256(request.data)
+                    )
+                )
+            );
+    }
+}
+
+contract ERC2771ForwarderTest is Test {
+    ERC2771ForwarderMock internal _erc2771Forwarder;
+    CallReceiverMock internal _receiver;
+
+    uint256 internal _signerPrivateKey;
+    uint256 internal _relayerPrivateKey;
+
+    address internal _signer;
+    address internal _relayer;
+
+    uint256 internal constant _MAX_ETHER = 10_000_000; // To avoid overflow
+
+    function setUp() public {
+        _erc2771Forwarder = new ERC2771ForwarderMock("ERC2771Forwarder");
+        _receiver = new CallReceiverMock();
+
+        _signerPrivateKey = 0xA11CE;
+        _relayerPrivateKey = 0xB0B;
+
+        _signer = vm.addr(_signerPrivateKey);
+        _relayer = vm.addr(_relayerPrivateKey);
+    }
+
+    function _forgeRequestData(
+        uint256 value,
+        uint256 nonce,
+        uint48 deadline,
+        bytes memory data
+    ) private view returns (ERC2771Forwarder.ForwardRequestData memory) {
+        ForwardRequest memory request = ForwardRequest({
+            from: _signer,
+            to: address(_receiver),
+            value: value,
+            gas: 30000,
+            nonce: nonce,
+            deadline: deadline,
+            data: data
+        });
+
+        bytes32 digest = _erc2771Forwarder.structHash(request);
+        (uint8 v, bytes32 r, bytes32 s) = vm.sign(_signerPrivateKey, digest);
+        bytes memory signature = abi.encodePacked(r, s, v);
+
+        return
+            ERC2771Forwarder.ForwardRequestData({
+                from: request.from,
+                to: request.to,
+                value: request.value,
+                gas: request.gas,
+                deadline: request.deadline,
+                data: request.data,
+                signature: signature
+            });
+    }
+
+    function testExecuteAvoidsETHStuck(uint256 initialBalance, uint256 value, bool targetReverts) public {
+        initialBalance = bound(initialBalance, 0, _MAX_ETHER);
+        value = bound(value, 0, _MAX_ETHER);
+
+        vm.deal(address(_erc2771Forwarder), initialBalance);
+
+        uint256 nonce = _erc2771Forwarder.nonces(_signer);
+
+        vm.deal(address(this), value);
+
+        ERC2771Forwarder.ForwardRequestData memory requestData = _forgeRequestData({
+            value: value,
+            nonce: nonce,
+            deadline: uint48(block.timestamp + 1),
+            data: targetReverts
+                ? abi.encodeCall(CallReceiverMock.mockFunctionRevertsNoReason, ())
+                : abi.encodeCall(CallReceiverMock.mockFunction, ())
+        });
+
+        if (targetReverts) {
+            vm.expectRevert();
+        }
+
+        _erc2771Forwarder.execute{value: value}(requestData);
+        assertEq(address(_erc2771Forwarder).balance, initialBalance);
+    }
+
+    function testExecuteBatchAvoidsETHStuck(uint256 initialBalance, uint256 batchSize, uint256 value) public {
+        batchSize = bound(batchSize, 1, 10);
+        initialBalance = bound(initialBalance, 0, _MAX_ETHER);
+        value = bound(value, 0, _MAX_ETHER);
+
+        vm.deal(address(_erc2771Forwarder), initialBalance);
+        uint256 nonce = _erc2771Forwarder.nonces(_signer);
+
+        ERC2771Forwarder.ForwardRequestData[] memory batchRequestDatas = new ERC2771Forwarder.ForwardRequestData[](
+            batchSize
+        );
+
+        uint256 expectedRefund;
+
+        for (uint256 i = 0; i < batchSize; ++i) {
+            bytes memory data;
+            bool succeed = uint256(keccak256(abi.encodePacked(initialBalance, i))) % 2 == 0;
+
+            if (succeed) {
+                data = abi.encodeCall(CallReceiverMock.mockFunction, ());
+            } else {
+                expectedRefund += value;
+                data = abi.encodeCall(CallReceiverMock.mockFunctionRevertsNoReason, ());
+            }
+
+            batchRequestDatas[i] = _forgeRequestData({
+                value: value,
+                nonce: nonce + i,
+                deadline: uint48(block.timestamp + 1),
+                data: data
+            });
+        }
+
+        address payable refundReceiver = payable(address(0xebe));
+        uint256 totalValue = value * batchSize;
+
+        vm.deal(address(this), totalValue);
+        _erc2771Forwarder.executeBatch{value: totalValue}(batchRequestDatas, refundReceiver);
+
+        assertEq(address(_erc2771Forwarder).balance, initialBalance);
+        assertEq(refundReceiver.balance, expectedRefund);
+    }
+}