From c12cf86e0d85e089a8d3b2b9e733c2194ff89d45 Mon Sep 17 00:00:00 2001 From: cairo Date: Mon, 21 Oct 2024 13:44:22 +0200 Subject: [PATCH] Fuzz tampered tests for `ERC2771Forwarder` (#5258) Co-authored-by: Hadrien Croubois --- test/metatx/ERC2771Forwarder.t.sol | 270 +++++++++++++++++++-------- test/metatx/ERC2771Forwarder.test.js | 77 -------- 2 files changed, 192 insertions(+), 155 deletions(-) diff --git a/test/metatx/ERC2771Forwarder.t.sol b/test/metatx/ERC2771Forwarder.t.sol index d69b4750a39..e6baac6f030 100644 --- a/test/metatx/ERC2771Forwarder.t.sol +++ b/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))); } } diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index 8653ad708cb..bf6cfd10c4b 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/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 });