Fuzz tampered tests for `ERC2771Forwarder` (#5258)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>

test/metatx/ERC2771Forwarder.t.sol

@@ -5,21 +5,24 @@ pragma solidity ^0.8.20;
 import {Test} from "forge-std/Test.sol";
 import {ERC2771Forwarder} from "@openzeppelin/contracts/metatx/ERC2771Forwarder.sol";
 import {CallReceiverMockTrustingForwarder, CallReceiverMock} from "@openzeppelin/contracts/mocks/CallReceiverMock.sol";
-
-struct ForwardRequest {
-    address from;
-    address to;
-    uint256 value;
-    uint256 gas;
-    uint256 nonce;
-    uint48 deadline;
-    bytes data;
+import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
+import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";
+
+enum TamperType {
+    FROM,
+    TO,
+    VALUE,
+    DATA,
+    SIGNATURE
 }
 
 contract ERC2771ForwarderMock is ERC2771Forwarder {
     constructor(string memory name) ERC2771Forwarder(name) {}
 
-    function structHash(ForwardRequest calldata request) external view returns (bytes32) {
+    function forwardRequestStructHash(
+        ERC2771Forwarder.ForwardRequestData calldata request,
+        uint256 nonce
+    ) external view returns (bytes32) {
         return
             _hashTypedDataV4(
                 keccak256(
@@ -29,7 +32,7 @@ contract ERC2771ForwarderMock is ERC2771Forwarder {
                         request.to,
                         request.value,
                         request.gas,
-                        request.nonce,
+                        nonce,
                         request.deadline,
                         keccak256(request.data)
                     )
@@ -39,127 +42,238 @@ contract ERC2771ForwarderMock is ERC2771Forwarder {
 }
 
 contract ERC2771ForwarderTest is Test {
+    using ECDSA for bytes32;
+
     ERC2771ForwarderMock internal _erc2771Forwarder;
     CallReceiverMockTrustingForwarder internal _receiver;
 
-    uint256 internal _signerPrivateKey;
-    uint256 internal _relayerPrivateKey;
-
-    address internal _signer;
-    address internal _relayer;
+    uint256 internal _signerPrivateKey = 0xA11CE;
+    address internal _signer = vm.addr(_signerPrivateKey);
 
     uint256 internal constant _MAX_ETHER = 10_000_000; // To avoid overflow
 
     function setUp() public {
         _erc2771Forwarder = new ERC2771ForwarderMock("ERC2771Forwarder");
         _receiver = new CallReceiverMockTrustingForwarder(address(_erc2771Forwarder));
+    }
 
-        _signerPrivateKey = 0xA11CE;
-        _relayerPrivateKey = 0xB0B;
-
-        _signer = vm.addr(_signerPrivateKey);
-        _relayer = vm.addr(_relayerPrivateKey);
+    // Forge a new ForwardRequestData
+    function _forgeRequestData() private view returns (ERC2771Forwarder.ForwardRequestData memory) {
+        return
+            _forgeRequestData({
+                value: 0,
+                deadline: uint48(block.timestamp + 1),
+                data: abi.encodeCall(CallReceiverMock.mockFunction, ())
+            });
     }
 
     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
+                from: _signer,
+                to: address(_receiver),
+                value: value,
+                gas: 30000,
+                deadline: deadline,
+                data: data,
+                signature: ""
             });
     }
 
-    function testExecuteAvoidsETHStuck(uint256 initialBalance, uint256 value, bool targetReverts) public {
-        initialBalance = bound(initialBalance, 0, _MAX_ETHER);
-        value = bound(value, 0, _MAX_ETHER);
+    // Sign a ForwardRequestData (in place) for a given nonce. Also returns it for convenience.
+    function _signRequestData(
+        ERC2771Forwarder.ForwardRequestData memory request,
+        uint256 nonce
+    ) private view returns (ERC2771Forwarder.ForwardRequestData memory) {
+        bytes32 digest = _erc2771Forwarder.forwardRequestStructHash(request, nonce);
+        (uint8 v, bytes32 r, bytes32 s) = vm.sign(_signerPrivateKey, digest);
+        request.signature = abi.encodePacked(r, s, v);
+        return request;
+    }
 
-        vm.deal(address(_erc2771Forwarder), initialBalance);
+    // Tamper a ForwardRequestData (in place). Also returns it for convenience.
+    function _tamperRequestData(
+        ERC2771Forwarder.ForwardRequestData memory request,
+        TamperType tamper
+    ) private returns (ERC2771Forwarder.ForwardRequestData memory) {
+        if (tamper == TamperType.FROM) request.from = vm.randomAddress();
+        else if (tamper == TamperType.TO) request.to = vm.randomAddress();
+        else if (tamper == TamperType.VALUE) request.value = vm.randomUint();
+        else if (tamper == TamperType.DATA) request.data = vm.randomBytes(4);
+        else if (tamper == TamperType.SIGNATURE) request.signature = vm.randomBytes(65);
+
+        return request;
+    }
 
-        uint256 nonce = _erc2771Forwarder.nonces(_signer);
+    // Predict the revert error for a tampered request, and expect it is emitted.
+    function _tamperedExpectRevert(
+        ERC2771Forwarder.ForwardRequestData memory request,
+        TamperType tamper,
+        uint256 nonce
+    ) private returns (ERC2771Forwarder.ForwardRequestData memory) {
+        if (tamper == TamperType.FROM) nonce = _erc2771Forwarder.nonces(request.from);
+
+        // predict revert
+        if (tamper == TamperType.TO) {
+            vm.expectRevert(
+                abi.encodeWithSelector(
+                    ERC2771Forwarder.ERC2771UntrustfulTarget.selector,
+                    request.to,
+                    address(_erc2771Forwarder)
+                )
+            );
+        } else {
+            (address recovered, , ) = _erc2771Forwarder.forwardRequestStructHash(request, nonce).tryRecover(
+                request.signature
+            );
+            vm.expectRevert(
+                abi.encodeWithSelector(ERC2771Forwarder.ERC2771ForwarderInvalidSigner.selector, recovered, request.from)
+            );
+        }
+        return request;
+    }
 
-        vm.deal(address(this), value);
+    function testExecuteAvoidsETHStuck(uint256 initialBalance, uint256 value, bool targetReverts) public {
+        initialBalance = bound(initialBalance, 0, _MAX_ETHER);
+        value = bound(value, 0, _MAX_ETHER);
 
-        ERC2771Forwarder.ForwardRequestData memory requestData = _forgeRequestData({
+        // create and sign request
+        ERC2771Forwarder.ForwardRequestData memory request = _forgeRequestData({
             value: value,
-            nonce: nonce,
             deadline: uint48(block.timestamp + 1),
             data: targetReverts
                 ? abi.encodeCall(CallReceiverMock.mockFunctionRevertsNoReason, ())
                 : abi.encodeCall(CallReceiverMock.mockFunction, ())
         });
+        _signRequestData(request, _erc2771Forwarder.nonces(_signer));
 
-        if (targetReverts) {
-            vm.expectRevert();
-        }
+        vm.deal(address(_erc2771Forwarder), initialBalance);
+        vm.deal(address(this), request.value);
+
+        if (targetReverts) vm.expectRevert();
+        _erc2771Forwarder.execute{value: value}(request);
 
-        _erc2771Forwarder.execute{value: value}(requestData);
         assertEq(address(_erc2771Forwarder).balance, initialBalance);
     }
 
     function testExecuteBatchAvoidsETHStuck(uint256 initialBalance, uint256 batchSize, uint256 value) public {
+        uint256 seed = uint256(keccak256(abi.encodePacked(initialBalance, batchSize, value)));
+
         batchSize = bound(batchSize, 1, 10);
         initialBalance = bound(initialBalance, 0, _MAX_ETHER);
         value = bound(value, 0, _MAX_ETHER);
 
+        address refundReceiver = address(0xebe);
+        uint256 refundExpected = 0;
+        uint256 nonce = _erc2771Forwarder.nonces(_signer);
+
+        // create an sign array or requests (that may fail)
+        ERC2771Forwarder.ForwardRequestData[] memory requests = new ERC2771Forwarder.ForwardRequestData[](batchSize);
+        for (uint256 i = 0; i < batchSize; ++i) {
+            bool failure = (seed >> i) & 0x1 == 0x1;
+
+            requests[i] = _forgeRequestData({
+                value: value,
+                deadline: uint48(block.timestamp + 1),
+                data: failure
+                    ? abi.encodeCall(CallReceiverMock.mockFunctionRevertsNoReason, ())
+                    : abi.encodeCall(CallReceiverMock.mockFunction, ())
+            });
+            _signRequestData(requests[i], nonce + i);
+
+            refundExpected += SafeCast.toUint(failure) * value;
+        }
+
+        // distribute ether
         vm.deal(address(_erc2771Forwarder), initialBalance);
+        vm.deal(address(this), value * batchSize);
+
+        // execute batch
+        _erc2771Forwarder.executeBatch{value: value * batchSize}(requests, payable(refundReceiver));
+
+        // check balances
+        assertEq(address(_erc2771Forwarder).balance, initialBalance);
+        assertEq(refundReceiver.balance, refundExpected);
+    }
+
+    function testVerifyTamperedValues(uint8 _tamper) public {
+        TamperType tamper = _asTamper(_tamper);
+
+        // create request, sign, tamper
+        ERC2771Forwarder.ForwardRequestData memory request = _forgeRequestData();
+        _signRequestData(request, 0);
+        _tamperRequestData(request, tamper);
+
+        // should not pass verification
+        assertFalse(_erc2771Forwarder.verify(request));
+    }
+
+    function testExecuteTamperedValues(uint8 _tamper) public {
+        TamperType tamper = _asTamper(_tamper);
+
+        // create request, sign, tamper, expect execution revert
+        ERC2771Forwarder.ForwardRequestData memory request = _forgeRequestData();
+        _signRequestData(request, 0);
+        _tamperRequestData(request, tamper);
+        _tamperedExpectRevert(request, tamper, 0);
+
+        vm.deal(address(this), request.value);
+        _erc2771Forwarder.execute{value: request.value}(request);
+    }
+
+    function testExecuteBatchTamperedValuesZeroReceiver(uint8 _tamper) public {
+        TamperType tamper = _asTamper(_tamper);
         uint256 nonce = _erc2771Forwarder.nonces(_signer);
 
-        ERC2771Forwarder.ForwardRequestData[] memory batchRequestDatas = new ERC2771Forwarder.ForwardRequestData[](
-            batchSize
-        );
+        // create an sign array or requests
+        ERC2771Forwarder.ForwardRequestData[] memory requests = new ERC2771Forwarder.ForwardRequestData[](3);
+        for (uint256 i = 0; i < requests.length; ++i) {
+            requests[i] = _forgeRequestData({
+                value: 0,
+                deadline: uint48(block.timestamp + 1),
+                data: abi.encodeCall(CallReceiverMock.mockFunction, ())
+            });
+            _signRequestData(requests[i], nonce + i);
+        }
 
-        uint256 expectedRefund;
+        // tamper with request[1] and expect execution revert
+        _tamperRequestData(requests[1], tamper);
+        _tamperedExpectRevert(requests[1], tamper, nonce + 1);
 
-        for (uint256 i = 0; i < batchSize; ++i) {
-            bytes memory data;
-            bool succeed = uint256(keccak256(abi.encodePacked(initialBalance, i))) % 2 == 0;
+        vm.deal(address(this), requests[1].value);
+        _erc2771Forwarder.executeBatch{value: requests[1].value}(requests, payable(address(0)));
+    }
 
-            if (succeed) {
-                data = abi.encodeCall(CallReceiverMock.mockFunction, ());
-            } else {
-                expectedRefund += value;
-                data = abi.encodeCall(CallReceiverMock.mockFunctionRevertsNoReason, ());
-            }
+    function testExecuteBatchTamperedValues(uint8 _tamper) public {
+        TamperType tamper = _asTamper(_tamper);
+        uint256 nonce = _erc2771Forwarder.nonces(_signer);
 
-            batchRequestDatas[i] = _forgeRequestData({
-                value: value,
-                nonce: nonce + i,
+        // create an sign array or requests
+        ERC2771Forwarder.ForwardRequestData[] memory requests = new ERC2771Forwarder.ForwardRequestData[](3);
+        for (uint256 i = 0; i < requests.length; ++i) {
+            requests[i] = _forgeRequestData({
+                value: 0,
                 deadline: uint48(block.timestamp + 1),
-                data: data
+                data: abi.encodeCall(CallReceiverMock.mockFunction, ())
             });
+            _signRequestData(requests[i], nonce + i);
         }
 
-        address payable refundReceiver = payable(address(0xebe));
-        uint256 totalValue = value * batchSize;
+        // tamper with request[1]
+        _tamperRequestData(requests[1], tamper);
 
-        vm.deal(address(this), totalValue);
-        _erc2771Forwarder.executeBatch{value: totalValue}(batchRequestDatas, refundReceiver);
+        // should not revert
+        vm.expectCall(address(_receiver), abi.encodeCall(CallReceiverMock.mockFunction, ()), 1);
 
-        assertEq(address(_erc2771Forwarder).balance, initialBalance);
-        assertEq(refundReceiver.balance, expectedRefund);
+        vm.deal(address(this), requests[1].value);
+        _erc2771Forwarder.executeBatch{value: requests[1].value}(requests, payable(address(0xebe)));
+    }
+
+    function _asTamper(uint8 _tamper) private pure returns (TamperType) {
+        return TamperType(bound(_tamper, uint8(TamperType.FROM), uint8(TamperType.SIGNATURE)));
     }
 }

test/metatx/ERC2771Forwarder.test.js

@@ -52,19 +52,6 @@ async function fixture() {
   };
 }
 
-// values or function to tamper with a signed request.
-const tamperedValues = {
-  from: ethers.Wallet.createRandom().address,
-  to: ethers.Wallet.createRandom().address,
-  value: ethers.parseEther('0.5'),
-  data: '0x1742',
-  signature: s => {
-    const t = ethers.toBeArray(s);
-    t[42] ^= 0xff;
-    return ethers.hexlify(t);
-  },
-};
-
 describe('ERC2771Forwarder', function () {
   beforeEach(async function () {
     Object.assign(this, await loadFixture(fixture));
@@ -81,15 +68,6 @@ describe('ERC2771Forwarder', function () {
     });
 
     describe('with tampered values', function () {
-      for (const [key, value] of Object.entries(tamperedValues)) {
-        it(`returns false with tampered ${key}`, async function () {
-          const request = await this.forgeRequest();
-          request[key] = typeof value == 'function' ? value(request[key]) : value;
-
-          expect(await this.forwarder.verify(request)).to.be.false;
-        });
-      }
-
       it('returns false with valid signature for non-current nonce', async function () {
         const request = await this.forgeRequest({ nonce: 1337n });
         expect(await this.forwarder.verify(request)).to.be.false;
@@ -127,24 +105,6 @@ describe('ERC2771Forwarder', function () {
     });
 
     describe('with tampered request', function () {
-      for (const [key, value] of Object.entries(tamperedValues)) {
-        it(`reverts with tampered ${key}`, async function () {
-          const request = await this.forgeRequest();
-          request[key] = typeof value == 'function' ? value(request[key]) : value;
-
-          const promise = this.forwarder.execute(request, { value: key == 'value' ? value : 0 });
-          if (key != 'to') {
-            await expect(promise)
-              .to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderInvalidSigner')
-              .withArgs(ethers.verifyTypedData(this.domain, this.types, request, request.signature), request.from);
-          } else {
-            await expect(promise)
-              .to.be.revertedWithCustomError(this.forwarder, 'ERC2771UntrustfulTarget')
-              .withArgs(request.to, this.forwarder);
-          }
-        });
-      }
-
       it('reverts with valid signature for non-current nonce', async function () {
         const request = await this.forgeRequest();
 
@@ -284,26 +244,6 @@ describe('ERC2771Forwarder', function () {
           this.refundReceiver = ethers.ZeroAddress;
         });
 
-        for (const [key, value] of Object.entries(tamperedValues)) {
-          it(`reverts with at least one tampered request ${key}`, async function () {
-            this.requests[idx][key] = typeof value == 'function' ? value(this.requests[idx][key]) : value;
-
-            const promise = this.forwarder.executeBatch(this.requests, this.refundReceiver, { value: this.value });
-            if (key != 'to') {
-              await expect(promise)
-                .to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderInvalidSigner')
-                .withArgs(
-                  ethers.verifyTypedData(this.domain, this.types, this.requests[idx], this.requests[idx].signature),
-                  this.requests[idx].from,
-                );
-            } else {
-              await expect(promise)
-                .to.be.revertedWithCustomError(this.forwarder, 'ERC2771UntrustfulTarget')
-                .withArgs(this.requests[idx].to, this.forwarder);
-            }
-          });
-        }
-
         it('reverts with at least one valid signature for non-current nonce', async function () {
           // Execute first a request
           await this.forwarder.execute(this.requests[idx], { value: this.requests[idx].value });
@@ -340,23 +280,6 @@ describe('ERC2771Forwarder', function () {
           this.initialTamperedRequestNonce = await this.forwarder.nonces(this.requests[idx].from);
         });
 
-        for (const [key, value] of Object.entries(tamperedValues)) {
-          it(`ignores a request with tampered ${key} and refunds its value`, async function () {
-            this.requests[idx][key] = typeof value == 'function' ? value(this.requests[idx][key]) : value;
-
-            const events = await this.forwarder
-              .executeBatch(this.requests, this.refundReceiver, { value: requestsValue(this.requests) })
-              .then(tx => tx.wait())
-              .then(receipt =>
-                receipt.logs.filter(
-                  log => log?.fragment?.type == 'event' && log?.fragment?.name == 'ExecutedForwardRequest',
-                ),
-              );
-
-            expect(events).to.have.lengthOf(this.requests.length - 1);
-          });
-        }
-
         it('ignores a request with a valid signature for non-current nonce', async function () {
           // Execute first a request
           await this.forwarder.execute(this.requests[idx], { value: this.requests[idx].value });