From 4c1cd2264962c38f354b84c4410a3c81ea50516f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 12 Jun 2023 21:45:04 -0600 Subject: [PATCH 01/34] Minimal Forwarder --- contracts/metatx/MinimalForwarder.sol | 150 ++++++++++++++++++-------- test/metatx/MinimalForwarder.test.js | 98 ++++++++++++----- 2 files changed, 176 insertions(+), 72 deletions(-) diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index b5267aa108b..45f6a42b8ea 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/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,13 +28,14 @@ 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`. @@ -37,45 +43,70 @@ contract MinimalForwarder is EIP712 { 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); - } } diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index c775c5e4403..91ab720681b 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -3,7 +3,7 @@ const Wallet = require('ethereumjs-wallet').default; const { getDomain, domainType } = require('../helpers/eip712'); const { expectRevertCustomError } = require('../helpers/customError'); -const { constants, expectRevert } = require('@openzeppelin/test-helpers'); +const { constants, expectRevert, time } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const MinimalForwarder = artifacts.require('MinimalForwarder'); @@ -11,7 +11,7 @@ const CallReceiverMock = artifacts.require('CallReceiverMock'); contract('MinimalForwarder', function (accounts) { beforeEach(async function () { - this.forwarder = await MinimalForwarder.new(); + this.forwarder = await MinimalForwarder.new('MinimalForwarder', '0.0.1'); this.domain = await getDomain(this.forwarder); this.types = { @@ -22,6 +22,7 @@ contract('MinimalForwarder', function (accounts) { { name: 'value', type: 'uint256' }, { name: 'gas', type: 'uint256' }, { name: 'nonce', type: 'uint256' }, + { name: 'deadline', type: 'uint48' }, { name: 'data', type: 'bytes' }, ], }; @@ -34,43 +35,50 @@ contract('MinimalForwarder', function (accounts) { value: web3.utils.toWei('1'), nonce: 1234, data: '0x1742', + deadline: 0xabcdef, }; beforeEach(async function () { this.wallet = Wallet.generate(); this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString()); - this.req = { + this.blockNumber = await time.latestBlock(); + this.request = { from: this.sender, to: constants.ZERO_ADDRESS, value: '0', gas: '100000', - nonce: Number(await this.forwarder.getNonce(this.sender)), + nonce: Number(await this.forwarder.nonces(this.sender)), data: '0x', + deadline: this.blockNumber.toNumber() + 2, // Next + 1 }; - this.forgeData = req => ({ + this.forgeData = request => ({ types: this.types, domain: this.domain, primaryType: 'ForwardRequest', - message: { ...this.req, ...req }, + message: { ...this.request, ...request }, }); - this.sign = req => + this.sign = request => ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { - data: this.forgeData(req), + data: this.forgeData(request), }); }); context('verify', function () { context('valid signature', function () { beforeEach(async function () { - expect(await this.forwarder.getNonce(this.req.from)).to.be.bignumber.equal(web3.utils.toBN(this.req.nonce)); + expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( + web3.utils.toBN(this.request.nonce), + ); }); it('success', async function () { - expect(await this.forwarder.verify(this.req, this.sign())).to.be.equal(true); + expect(await this.forwarder.verify(this.request, this.sign())).to.be.equal(true); }); afterEach(async function () { - expect(await this.forwarder.getNonce(this.req.from)).to.be.bignumber.equal(web3.utils.toBN(this.req.nonce)); + expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( + web3.utils.toBN(this.request.nonce), + ); }); }); @@ -86,13 +94,22 @@ contract('MinimalForwarder', function (accounts) { it('returns false with tampered signature', async function () { const tamperedsign = web3.utils.hexToBytes(this.sign()); tamperedsign[42] ^= 0xff; - expect(await this.forwarder.verify(this.req, web3.utils.bytesToHex(tamperedsign))).to.be.equal(false); + expect(await this.forwarder.verify(this.request, web3.utils.bytesToHex(tamperedsign))).to.be.equal(false); }); it('returns false with valid signature for non-current nonce', async function () { const req = { - ...this.req, - nonce: this.req.nonce + 1, + ...this.request, + nonce: this.request.nonce + 1, + }; + const sig = this.sign(req); + expect(await this.forwarder.verify(req, sig)).to.be.equal(false); + }); + + it('returns false with valid signature for expired deadline', async function () { + const req = { + ...this.request, + nonce: this.request.nonce + 1, }; const sig = this.sign(req); expect(await this.forwarder.verify(req, sig)).to.be.equal(false); @@ -103,16 +120,18 @@ contract('MinimalForwarder', function (accounts) { context('execute', function () { context('valid signature', function () { beforeEach(async function () { - expect(await this.forwarder.getNonce(this.req.from)).to.be.bignumber.equal(web3.utils.toBN(this.req.nonce)); + expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( + web3.utils.toBN(this.request.nonce), + ); }); it('success', async function () { - await this.forwarder.execute(this.req, this.sign()); // expect to not revert + await this.forwarder.execute(this.request, this.sign()); // expect to not revert }); afterEach(async function () { - expect(await this.forwarder.getNonce(this.req.from)).to.be.bignumber.equal( - web3.utils.toBN(this.req.nonce + 1), + expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( + web3.utils.toBN(this.request.nonce + 1), ); }); }); @@ -133,33 +152,54 @@ contract('MinimalForwarder', function (accounts) { const tamperedSig = web3.utils.hexToBytes(this.sign()); tamperedSig[42] ^= 0xff; await expectRevertCustomError( - this.forwarder.execute(this.req, web3.utils.bytesToHex(tamperedSig)), + this.forwarder.execute(this.request, web3.utils.bytesToHex(tamperedSig)), 'MinimalForwarderInvalidSigner', - [ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }), this.req.from], + [ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }), this.request.from], ); }); it('reverts with valid signature for non-current nonce', async function () { const req = { - ...this.req, - nonce: this.req.nonce + 1, + ...this.request, + nonce: this.request.nonce + 1, + }; + const sig = this.sign(req); + await expectRevertCustomError(this.forwarder.execute(req, sig), 'InvalidAccountNonce', [ + this.request.from, + this.request.nonce, + ]); + }); + + it('reverts with valid signature for expired deadline', async function () { + const req = { + ...this.request, + deadline: this.blockNumber.toNumber() - 1, }; const sig = this.sign(req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderInvalidNonce', [ - this.req.from, - this.req.nonce, + await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderExpiredRequest', [ + this.blockNumber.toNumber() - 1, ]); }); + + it('reverts with valid signature but mismatched value', async function () { + const value = 100; + const req = { + ...this.request, + value, + }; + const sig = this.sign(req); + await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwaderMismatchedValue', [0, value]); + }); }); it('bubble out of gas', async function () { const receiver = await CallReceiverMock.new(); const gasAvailable = 100000; - this.req.to = receiver.address; - this.req.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); - this.req.gas = 1000000; + this.request.to = receiver.address; + this.request.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); + this.request.gas = 1000000; - await expectRevert.assertion(this.forwarder.execute(this.req, this.sign(), { gas: gasAvailable })); + await expectRevert.assertion(this.forwarder.execute(this.request, this.sign(), { gas: gasAvailable })); const { transactions } = await web3.eth.getBlock('latest'); const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); From 2432d0dcb2a54cbc55b420a9a4b568dfe639b202 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 13 Jun 2023 14:28:03 -0600 Subject: [PATCH 02/34] Add changeset --- .changeset/blue-horses-do.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/blue-horses-do.md diff --git a/.changeset/blue-horses-do.md b/.changeset/blue-horses-do.md new file mode 100644 index 00000000000..eea2ffeab64 --- /dev/null +++ b/.changeset/blue-horses-do.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`MinimalForwarder`: Added `deadline` for expiring transactions and add `msg.value` mismatch check From 6a87cec8b09983ffb09b9cd061af49f1aa547106 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 13 Jun 2023 18:23:12 -0600 Subject: [PATCH 03/34] Fix ERC2771 tests --- test/metatx/ERC2771Context.test.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index 6c298d3d9f0..bbf8142cc88 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/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, }; From 16b9ea88ea302eb34d9683131bb45449279373ad Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 20 Jun 2023 15:04:28 -0600 Subject: [PATCH 04/34] Add batching and better explain gas forwarding check --- contracts/metatx/MinimalForwarder.sol | 163 +++++++++++++++++--------- test/metatx/MinimalForwarder.test.js | 2 +- 2 files changed, 111 insertions(+), 54 deletions(-) diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index 45f6a42b8ea..8ffabd51852 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -10,14 +10,15 @@ import "../utils/Nonces.sol"; /** * @dev A minimal implementation of a production-ready forwarder compatible with ERC2771 contracts. * - * This forwarder operates on forward request that include: + * This forwarder operates on forward requests 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. + * * `data`: Encoded calldata to send within the requested call.' */ contract MinimalForwarder is EIP712, Nonces { using ECDSA for bytes32; @@ -43,14 +44,14 @@ contract MinimalForwarder is EIP712, Nonces { error MinimalForwarderInvalidSigner(address signer, address from); /** - * @dev The request `value` doesn't match with the `msg.value`. + * @dev The requested `value` doesn't match with the available `msgValue`, leaving ETH stuck in the contract. */ - error MinimalForwaderMismatchedValue(uint256 msgValue, uint256 value); + error MinimalForwarderMismatchedValue(uint256 value, uint256 msgValue); /** * @dev The list of requests length doesn't match with the list of signatures length. */ - error MinimalForwaderInvalidBatchLength(uint256 requestsLength, uint256 signaturesLength); + error MinimalForwarderInvalidBatchLength(uint256 requestsLength, uint256 signaturesLength); /** * @dev The request `deadline` has expired. @@ -58,7 +59,7 @@ contract MinimalForwarder is EIP712, Nonces { error MinimalForwarderExpiredRequest(uint256 deadline); /** - * @dev See {EIP7712-constructor}. + * @dev See {EIP712-constructor}. */ constructor(string memory name, string memory version) EIP712(name, version) {} @@ -71,79 +72,71 @@ contract MinimalForwarder is EIP712, Nonces { } /** - * @dev Executes a `request` on behalf of `signature`'s signer. + * @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 request, bytes calldata signature ) public payable virtual returns (bool, bytes memory) { - if (request.deadline < block.number) { - revert MinimalForwarderExpiredRequest(request.deadline); - } + (bool success, bytes memory returndata) = _execute(request, signature); - address signer = _recoverForwardRequestSigner(request, signature); - if (signer != request.from) { - revert MinimalForwarderInvalidSigner(signer, request.from); - } + _checkForwardedGas(gasleft(), request); if (msg.value != request.value) { - revert MinimalForwaderMismatchedValue(msg.value, request.value); - } - - _useCheckedNonce(request.from, request.nonce); - - // 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) - ); - - // 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 - /// @solidity memory-safe-assembly - assembly { - invalid() - } + revert MinimalForwarderMismatchedValue(request.value, msg.value); } return (success, returndata); } /** - * @dev Validates if the provided request can be executed at `blockNumber` with `signature`. + * @dev Batch version of {execute}. */ - 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); + function executeBatch( + ForwardRequest[] calldata requests, + bytes[] calldata signatures + ) public payable virtual returns (bool[] memory, bytes[] memory) { + if (requests.length != signatures.length) { + revert MinimalForwarderInvalidBatchLength(requests.length, signatures.length); + } + + uint256 requestsValue = 0; + bool[] memory successes = new bool[](requests.length); + bytes[] memory returndatas = new bytes[](requests.length); + + for (uint256 i; i < requests.length; ++i) { + (bool success, bytes memory returndata) = _execute(requests[i], signatures[i]); + requestsValue += requests[i].value; + successes[i] = success; + returndatas[i] = returndata; + } + + // Only check the last request because if the batch didn't go out-of-gas at this point, + // only the last call can be provided with less gas than requested. + _checkForwardedGas(gasleft(), requests[requests.length - 1]); + + if (msg.value != requestsValue) { + revert MinimalForwarderMismatchedValue(requestsValue, msg.value); + } + + return (successes, returndatas); } /** - * @dev Same as {_validateAt} but for the current block. + * @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) { - return _validateAt(block.number, request, signature); + address signer = _recoverForwardRequestSigner(request, signature); + return (request.deadline >= block.number, signer == request.from, nonces(request.from) == request.nonce); } /** - * @dev Recovers the signer of an EIP712 message hash for a forward `request` - * and its corresponding `signature`. See {ECDSA-recover}. + * @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, @@ -165,4 +158,68 @@ contract MinimalForwarder is EIP712, Nonces { ) ).recover(signature); } + + /** + * @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. + * + * IMPORTANT: Using this function does not guarantee the forwarded call will receive enough gas to complete. + * Similarly, it 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) private returns (bool, bytes memory) { + // 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); + + return request.to.call{gas: request.gas, value: request.value}(abi.encodePacked(request.data, request.from)); + } + + /** + * @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. + */ + function _checkForwardedGas(uint256 gasLeft, ForwardRequest 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 + // 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 `req.gas <= Y` and `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() + } + } + } } diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index 91ab720681b..7f0317b688a 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -188,7 +188,7 @@ contract('MinimalForwarder', function (accounts) { value, }; const sig = this.sign(req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwaderMismatchedValue', [0, value]); + await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderMismatchedValue', [0, value]); }); }); From 7c5d038d345a9298e7bd416f0b6b88d420375d37 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 20 Jun 2023 15:06:28 -0600 Subject: [PATCH 05/34] Lint --- test/metatx/MinimalForwarder.test.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index 7f0317b688a..09fb47883ec 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -188,7 +188,10 @@ contract('MinimalForwarder', function (accounts) { value, }; const sig = this.sign(req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderMismatchedValue', [0, value]); + await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderMismatchedValue', [ + 0, + value, + ]); }); }); From 5662e35e30ada69d44a15db991f07179fb5ddbe6 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 20 Jun 2023 15:35:40 -0600 Subject: [PATCH 06/34] Applied suggestions --- .changeset/blue-horses-do.md | 2 +- contracts/metatx/MinimalForwarder.sol | 12 ++++++------ contracts/utils/cryptography/EIP712.sol | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.changeset/blue-horses-do.md b/.changeset/blue-horses-do.md index eea2ffeab64..7ed08db8c3c 100644 --- a/.changeset/blue-horses-do.md +++ b/.changeset/blue-horses-do.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': major --- -`MinimalForwarder`: Added `deadline` for expiring transactions and add `msg.value` mismatch check +`MinimalForwarder`: Added `deadline` for expiring transactions and added `msg.value` mismatch check. diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index 8ffabd51852..b8232f81b8b 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -12,13 +12,13 @@ import "../utils/Nonces.sol"; * * This forwarder operates on forward requests 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. + * * `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 calldata to send within the requested call.' + * * `data`: Encoded `msg.data` to send with the requested call. */ contract MinimalForwarder is EIP712, Nonces { using ECDSA for bytes32; @@ -61,7 +61,7 @@ contract MinimalForwarder is EIP712, Nonces { /** * @dev See {EIP712-constructor}. */ - constructor(string memory name, string memory version) EIP712(name, version) {} + constructor(string memory name, string memory version) EIP712(name, "1") {} /** * @dev Returns `true` if a request is valid for a provided `signature` at the current block. diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index b9c9c9d195d..65d2a73e531 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/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._ */ From d1c75c8a63270a9299b51af189a77202cedb6b9a Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 20 Jun 2023 15:38:50 -0600 Subject: [PATCH 07/34] Fix test --- contracts/metatx/MinimalForwarder.sol | 2 +- test/metatx/MinimalForwarder.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index b8232f81b8b..7b152a56d42 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -61,7 +61,7 @@ contract MinimalForwarder is EIP712, Nonces { /** * @dev See {EIP712-constructor}. */ - constructor(string memory name, string memory version) EIP712(name, "1") {} + constructor(string memory name) EIP712(name, "1") {} /** * @dev Returns `true` if a request is valid for a provided `signature` at the current block. diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index 09fb47883ec..f630a114d1b 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -11,7 +11,7 @@ const CallReceiverMock = artifacts.require('CallReceiverMock'); contract('MinimalForwarder', function (accounts) { beforeEach(async function () { - this.forwarder = await MinimalForwarder.new('MinimalForwarder', '0.0.1'); + this.forwarder = await MinimalForwarder.new('MinimalForwarder'); this.domain = await getDomain(this.forwarder); this.types = { From dff99985e65d9029f16423fd42e6f46f989521ac Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 20 Jun 2023 20:27:23 -0600 Subject: [PATCH 08/34] Complete tests --- contracts/metatx/MinimalForwarder.sol | 57 ++-- test/metatx/MinimalForwarder.test.js | 419 +++++++++++++++++--------- 2 files changed, 305 insertions(+), 171 deletions(-) diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index 7b152a56d42..ad7b5f35acd 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -38,6 +38,11 @@ contract MinimalForwarder is EIP712, Nonces { "ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)" ); + /** + * @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`. */ @@ -81,8 +86,6 @@ contract MinimalForwarder is EIP712, Nonces { ) public payable virtual returns (bool, bytes memory) { (bool success, bytes memory returndata) = _execute(request, signature); - _checkForwardedGas(gasleft(), request); - if (msg.value != request.value) { revert MinimalForwarderMismatchedValue(request.value, msg.value); } @@ -93,34 +96,21 @@ contract MinimalForwarder is EIP712, Nonces { /** * @dev Batch version of {execute}. */ - function executeBatch( - ForwardRequest[] calldata requests, - bytes[] calldata signatures - ) public payable virtual returns (bool[] memory, bytes[] memory) { + function executeBatch(ForwardRequest[] calldata requests, bytes[] calldata signatures) public payable virtual { if (requests.length != signatures.length) { revert MinimalForwarderInvalidBatchLength(requests.length, signatures.length); } - uint256 requestsValue = 0; - bool[] memory successes = new bool[](requests.length); - bytes[] memory returndatas = new bytes[](requests.length); + uint256 requestsValue; for (uint256 i; i < requests.length; ++i) { - (bool success, bytes memory returndata) = _execute(requests[i], signatures[i]); requestsValue += requests[i].value; - successes[i] = success; - returndatas[i] = returndata; + _execute(requests[i], signatures[i]); } - // Only check the last request because if the batch didn't go out-of-gas at this point, - // only the last call can be provided with less gas than requested. - _checkForwardedGas(gasleft(), requests[requests.length - 1]); - if (msg.value != requestsValue) { revert MinimalForwarderMismatchedValue(requestsValue, msg.value); } - - return (successes, returndatas); } /** @@ -141,7 +131,7 @@ contract MinimalForwarder is EIP712, Nonces { function _recoverForwardRequestSigner( ForwardRequest calldata request, bytes calldata signature - ) internal view returns (address) { + ) internal view virtual returns (address) { return _hashTypedDataV4( keccak256( @@ -167,11 +157,17 @@ contract MinimalForwarder is EIP712, Nonces { * - 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 does not guarantee the forwarded call will receive enough gas to complete. - * Similarly, it doesn't check that all the `msg.value` was sent, potentially leaving ETH stuck in the contract. + * 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) private returns (bool, bytes memory) { + 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) { @@ -185,7 +181,13 @@ contract MinimalForwarder is EIP712, Nonces { _useCheckedNonce(request.from, request.nonce); - return request.to.call{gas: request.gas, value: request.value}(abi.encodePacked(request.data, request.from)); + (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); } /** @@ -196,8 +198,11 @@ contract MinimalForwarder is EIP712, Nonces { * - 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(uint256 gasLeft, ForwardRequest calldata request) private pure { + 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, @@ -209,10 +214,10 @@ contract MinimalForwarder is EIP712, Nonces { // - (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 `req.gas <= Y` and `X - req.gas <= gasleft()`. + // 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) { + 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 diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index f630a114d1b..c512c2d0301 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -3,13 +3,22 @@ const Wallet = require('ethereumjs-wallet').default; const { getDomain, domainType } = require('../helpers/eip712'); const { expectRevertCustomError } = require('../helpers/customError'); -const { constants, expectRevert, time } = require('@openzeppelin/test-helpers'); +const { constants, expectRevert, expectEvent, time } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const MinimalForwarder = artifacts.require('MinimalForwarder'); const CallReceiverMock = artifacts.require('CallReceiverMock'); contract('MinimalForwarder', function (accounts) { + const tamperedValues = { + from: accounts[0], + to: accounts[0], + value: web3.utils.toWei('1'), + nonce: 1234, + data: '0x1742', + deadline: 0xabcdef, + }; + beforeEach(async function () { this.forwarder = await MinimalForwarder.new('MinimalForwarder'); @@ -26,188 +35,308 @@ contract('MinimalForwarder', function (accounts) { { name: 'data', type: 'bytes' }, ], }; - }); - context('with message', function () { - const tamperedValues = { - from: accounts[0], - to: accounts[0], - value: web3.utils.toWei('1'), - nonce: 1234, - data: '0x1742', - deadline: 0xabcdef, + this.alice = Wallet.generate(); + this.alice.address = web3.utils.toChecksumAddress(this.alice.getAddressString()); + + this.blockNumber = await time.latestBlock(); + this.request = { + from: this.alice.address, + to: constants.ZERO_ADDRESS, + value: '0', + gas: '100000', + nonce: Number(await this.forwarder.nonces(this.alice.address)), + data: '0x', + deadline: this.blockNumber.toNumber() + 2, // Next + 1 }; - beforeEach(async function () { - this.wallet = Wallet.generate(); - this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString()); - this.blockNumber = await time.latestBlock(); - this.request = { - from: this.sender, - to: constants.ZERO_ADDRESS, - value: '0', - gas: '100000', - nonce: Number(await this.forwarder.nonces(this.sender)), - data: '0x', - deadline: this.blockNumber.toNumber() + 2, // Next + 1 - }; - this.forgeData = request => ({ - types: this.types, - domain: this.domain, - primaryType: 'ForwardRequest', - message: { ...this.request, ...request }, - }); - this.sign = request => - ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { - data: this.forgeData(request), - }); + this.forgeData = request => ({ + types: this.types, + domain: this.domain, + primaryType: 'ForwardRequest', + message: { ...this.request, ...request }, }); + this.sign = (privateKey, request) => + ethSigUtil.signTypedMessage(privateKey, { + data: this.forgeData(request), + }); - context('verify', function () { - context('valid signature', function () { - beforeEach(async function () { - expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( - web3.utils.toBN(this.request.nonce), - ); - }); + this.signature = this.sign(this.alice.getPrivateKey()); + }); - it('success', async function () { - expect(await this.forwarder.verify(this.request, this.sign())).to.be.equal(true); - }); + context('verify', function () { + context('with valid signature', function () { + beforeEach(async function () { + expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( + web3.utils.toBN(this.request.nonce), + ); + }); - afterEach(async function () { - expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( - web3.utils.toBN(this.request.nonce), + it('success', async function () { + expect(await this.forwarder.verify(this.request, this.signature)).to.be.equal(true); + }); + + afterEach(async function () { + expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( + web3.utils.toBN(this.request.nonce), + ); + }); + }); + + context('with tampered values', function () { + for (const [key, value] of Object.entries(tamperedValues)) { + it(`returns false with tampered ${key}`, async function () { + expect(await this.forwarder.verify(this.forgeData({ [key]: value }).message, this.signature)).to.be.equal( + false, ); }); + } + + it('returns false with tampered signature', async function () { + const tamperedsign = web3.utils.hexToBytes(this.signature); + tamperedsign[42] ^= 0xff; + expect(await this.forwarder.verify(this.request, web3.utils.bytesToHex(tamperedsign))).to.be.equal(false); }); - context('with tampered values', function () { - for (const [key, value] of Object.entries(tamperedValues)) { - it(`returns false with tampered ${key}`, async function () { - expect(await this.forwarder.verify(this.forgeData({ [key]: value }).message, this.sign())).to.be.equal( - false, - ); - }); - } + it('returns false with valid signature for non-current nonce', async function () { + const req = { + ...this.request, + nonce: this.request.nonce + 1, + }; + const sig = this.sign(this.alice.getPrivateKey(), req); + expect(await this.forwarder.verify(req, sig)).to.be.equal(false); + }); - it('returns false with tampered signature', async function () { - const tamperedsign = web3.utils.hexToBytes(this.sign()); - tamperedsign[42] ^= 0xff; - expect(await this.forwarder.verify(this.request, web3.utils.bytesToHex(tamperedsign))).to.be.equal(false); - }); + it('returns false with valid signature for expired deadline', async function () { + const req = { + ...this.request, + nonce: this.request.nonce + 1, + }; + const sig = this.sign(this.alice.getPrivateKey(), req); + expect(await this.forwarder.verify(req, sig)).to.be.equal(false); + }); + }); + }); - it('returns false with valid signature for non-current nonce', async function () { - const req = { - ...this.request, - nonce: this.request.nonce + 1, - }; - const sig = this.sign(req); - expect(await this.forwarder.verify(req, sig)).to.be.equal(false); - }); + context('execute', function () { + context('with valid signature', function () { + beforeEach(async function () { + expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( + web3.utils.toBN(this.request.nonce), + ); + }); - it('returns false with valid signature for expired deadline', async function () { - const req = { - ...this.request, - nonce: this.request.nonce + 1, - }; - const sig = this.sign(req); - expect(await this.forwarder.verify(req, sig)).to.be.equal(false); + it('succeeds', async function () { + const receipt = await this.forwarder.execute(this.request, this.signature); + expectEvent(receipt, 'ExecutedForwardRequest', { + signer: this.request.from, + nonce: web3.utils.toBN(this.request.nonce), + success: true, + returndata: null, }); }); + + afterEach(async function () { + expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( + web3.utils.toBN(this.request.nonce + 1), + ); + }); }); - context('execute', function () { - context('valid signature', function () { - beforeEach(async function () { - expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( - web3.utils.toBN(this.request.nonce), - ); + context('with tampered values', function () { + for (const [key, value] of Object.entries(tamperedValues)) { + it(`reverts with tampered ${key}`, async function () { + const sig = this.sign(this.alice.getPrivateKey()); + const data = this.forgeData({ [key]: value }); + await expectRevertCustomError(this.forwarder.execute(data.message, sig), 'MinimalForwarderInvalidSigner', [ + ethSigUtil.recoverTypedSignature({ data, sig }), + data.message.from, + ]); }); + } - it('success', async function () { - await this.forwarder.execute(this.request, this.sign()); // expect to not revert - }); + it('reverts with tampered signature', async function () { + const tamperedSig = web3.utils.hexToBytes(this.signature); + tamperedSig[42] ^= 0xff; + await expectRevertCustomError( + this.forwarder.execute(this.request, web3.utils.bytesToHex(tamperedSig)), + 'MinimalForwarderInvalidSigner', + [ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }), this.request.from], + ); + }); - afterEach(async function () { - expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( - web3.utils.toBN(this.request.nonce + 1), - ); - }); + it('reverts with valid signature for non-current nonce', async function () { + const req = { + ...this.request, + nonce: this.request.nonce + 1, + }; + const sig = this.sign(this.alice.getPrivateKey(), req); + await expectRevertCustomError(this.forwarder.execute(req, sig), 'InvalidAccountNonce', [ + this.request.from, + this.request.nonce, + ]); }); - context('with tampered values', function () { - for (const [key, value] of Object.entries(tamperedValues)) { - it(`reverts with tampered ${key}`, async function () { - const sig = this.sign(); - const data = this.forgeData({ [key]: value }); - await expectRevertCustomError(this.forwarder.execute(data.message, sig), 'MinimalForwarderInvalidSigner', [ - ethSigUtil.recoverTypedSignature({ data, sig }), - data.message.from, - ]); + it('reverts with valid signature for expired deadline', async function () { + const req = { + ...this.request, + deadline: this.blockNumber.toNumber() - 1, + }; + const sig = this.sign(this.alice.getPrivateKey(), req); + await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderExpiredRequest', [ + this.blockNumber.toNumber() - 1, + ]); + }); + + it('reverts with valid signature but mismatched value', async function () { + const value = 100; + const req = { + ...this.request, + value, + }; + const sig = this.sign(this.alice.getPrivateKey(), req); + await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderMismatchedValue', [0, value]); + }); + }); + + it('bubbles out of gas', async function () { + const receiver = await CallReceiverMock.new(); + const gasAvailable = 100000; + this.request.to = receiver.address; + this.request.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); + this.request.gas = 1000000; + + await expectRevert.assertion( + this.forwarder.execute(this.request, this.sign(this.alice.getPrivateKey()), { gas: gasAvailable }), + ); + + const { transactions } = await web3.eth.getBlock('latest'); + const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + + expect(gasUsed).to.be.equal(gasAvailable); + }); + }); + + context('executeBatch', function () { + beforeEach(async function () { + this.bob = Wallet.generate(); + this.bob.address = web3.utils.toChecksumAddress(this.bob.getAddressString()); + + this.eve = Wallet.generate(); + this.eve.address = web3.utils.toChecksumAddress(this.eve.getAddressString()); + + this.signers = [this.alice, this.bob, this.eve]; + + this.requests = await Promise.all( + this.signers.map(async ({ address }) => ({ + ...this.request, + from: address, + nonce: Number(await this.forwarder.nonces(address)), + })), + ); + + this.signatures = this.signers.map((signer, i) => this.sign(signer.getPrivateKey(), this.requests[i])); + }); + + it('reverts with mismatched lengths', async function () { + await expectRevertCustomError( + this.forwarder.executeBatch(this.requests, this.signatures.slice(0, -1)), + 'MinimalForwarderInvalidBatchLength', + [this.requests.length, this.signatures.length - 1], + ); + + await expectRevertCustomError( + this.forwarder.executeBatch(this.requests.slice(0, -1), this.signatures), + 'MinimalForwarderInvalidBatchLength', + [this.requests.length - 1, this.signatures.length], + ); + }); + + context('with valid signatures', function () { + it('succeeds', async function () { + const receipt = await this.forwarder.executeBatch(this.requests, this.signatures); + for (const request of this.requests) { + expectEvent(receipt, 'ExecutedForwardRequest', { + signer: request.from, + nonce: web3.utils.toBN(request.nonce), + success: true, + returndata: null, }); } + }); + }); + + context('with tampered values', function () { + beforeEach(async function () { + this.idx = 1; + }); + + for (const [key, value] of Object.entries(tamperedValues)) { + it(`reverts with at least one tampered request ${key}`, async function () { + const data = this.forgeData({ [key]: value }); + const sig = this.sign(this.signers[this.idx].getPrivateKey()); + + this.requests[this.idx] = data.message; + this.signatures[this.idx] = sig; - it('reverts with tampered signature', async function () { - const tamperedSig = web3.utils.hexToBytes(this.sign()); - tamperedSig[42] ^= 0xff; await expectRevertCustomError( - this.forwarder.execute(this.request, web3.utils.bytesToHex(tamperedSig)), + this.forwarder.executeBatch(this.requests, this.signatures), 'MinimalForwarderInvalidSigner', - [ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }), this.request.from], + [ethSigUtil.recoverTypedSignature({ data, sig }), data.message.from], ); }); + } - it('reverts with valid signature for non-current nonce', async function () { - const req = { - ...this.request, - nonce: this.request.nonce + 1, - }; - const sig = this.sign(req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'InvalidAccountNonce', [ - this.request.from, - this.request.nonce, - ]); - }); + it('reverts with at least one tampered request signature', async function () { + const tamperedSig = web3.utils.hexToBytes(this.signatures[this.idx]); + tamperedSig[42] ^= 0xff; - it('reverts with valid signature for expired deadline', async function () { - const req = { - ...this.request, - deadline: this.blockNumber.toNumber() - 1, - }; - const sig = this.sign(req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderExpiredRequest', [ - this.blockNumber.toNumber() - 1, - ]); - }); + this.signatures[this.idx] = web3.utils.bytesToHex(tamperedSig); - it('reverts with valid signature but mismatched value', async function () { - const value = 100; - const req = { - ...this.request, - value, - }; - const sig = this.sign(req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderMismatchedValue', [ - 0, - value, - ]); - }); + await expectRevertCustomError( + this.forwarder.executeBatch(this.requests, this.signatures), + 'MinimalForwarderInvalidSigner', + [ + ethSigUtil.recoverTypedSignature({ + data: this.forgeData(this.requests[this.idx]), + sig: this.signatures[this.idx], + }), + this.requests[this.idx].from, + ], + ); }); - it('bubble out of gas', async function () { - const receiver = await CallReceiverMock.new(); - const gasAvailable = 100000; - this.request.to = receiver.address; - this.request.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); - this.request.gas = 1000000; + it('reverts with at least one valid signature for non-current nonce', async function () { + const currentNonce = this.requests[this.idx].nonce; + this.requests[this.idx].nonce = this.requests[this.idx].nonce + 1; + this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); - await expectRevert.assertion(this.forwarder.execute(this.request, this.sign(), { gas: gasAvailable })); + await expectRevertCustomError( + this.forwarder.executeBatch(this.requests, this.signatures), + 'InvalidAccountNonce', + [this.requests[this.idx].from, currentNonce], + ); + }); - const { transactions } = await web3.eth.getBlock('latest'); - const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + it('reverts with at least one valid signature for expired deadline', async function () { + this.requests[this.idx].deadline = this.blockNumber.toNumber() - 1; + this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); + await expectRevertCustomError( + this.forwarder.executeBatch(this.requests, this.signatures), + 'MinimalForwarderExpiredRequest', + [this.blockNumber.toNumber() - 1], + ); + }); - expect(gasUsed).to.be.equal(gasAvailable); + it('reverts with at least one valid signature but mismatched value', async function () { + const value = 100; + this.requests[this.idx].value = value; + this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); + await expectRevertCustomError( + this.forwarder.executeBatch(this.requests, this.signatures), + 'MinimalForwarderMismatchedValue', + [0, value], + ); }); }); }); From 8da57efd43cf88325a5c7d22a91831e4d1b2e7bd Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 21 Jun 2023 14:45:02 -0600 Subject: [PATCH 09/34] Improve testing --- test/metatx/MinimalForwarder.test.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index c512c2d0301..333bb7b5a9d 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -339,5 +339,23 @@ contract('MinimalForwarder', function (accounts) { ); }); }); + + it('bubbles out of gas', async function () { + const receiver = await CallReceiverMock.new(); + const gasAvailable = 100000; + const idx = 0; + this.requests[idx].to = receiver.address; + this.requests[idx].data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); + this.requests[idx].gas = 1000000; + + this.signatures[idx] = this.sign(this.signers[idx].getPrivateKey(), this.requests[idx]); + + await expectRevert.assertion(this.forwarder.executeBatch(this.requests, this.signatures, { gas: gasAvailable })); + + const { transactions } = await web3.eth.getBlock('latest'); + const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + + expect(gasUsed).to.be.equal(gasAvailable); + }); }); }); From 5978b7e261230bd452bdebe7387d5cc255744dc4 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 22 Jun 2023 22:21:15 -0600 Subject: [PATCH 10/34] Rename MinimalForwarder to ERC2771Forwarder --- .changeset/blue-horses-do.md | 2 +- ...imalForwarder.sol => ERC2771Forwarder.sol} | 28 +++++++------- contracts/metatx/README.adoc | 2 +- test/metatx/ERC2771Context.test.js | 8 ++-- ...arder.test.js => ERC2771Forwarder.test.js} | 37 ++++++++++--------- 5 files changed, 40 insertions(+), 37 deletions(-) rename contracts/metatx/{MinimalForwarder.sol => ERC2771Forwarder.sol} (91%) rename test/metatx/{MinimalForwarder.test.js => ERC2771Forwarder.test.js} (92%) diff --git a/.changeset/blue-horses-do.md b/.changeset/blue-horses-do.md index 7ed08db8c3c..812f8c28eef 100644 --- a/.changeset/blue-horses-do.md +++ b/.changeset/blue-horses-do.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': major --- -`MinimalForwarder`: Added `deadline` for expiring transactions and added `msg.value` mismatch check. +`ERC2771Forwarder`: Added `deadline` for expiring transactions and added `msg.value` mismatch check. diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/ERC2771Forwarder.sol similarity index 91% rename from contracts/metatx/MinimalForwarder.sol rename to contracts/metatx/ERC2771Forwarder.sol index ad7b5f35acd..a9d3430348d 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.9.0) (metatx/MinimalForwarder.sol) +// OpenZeppelin Contracts (last updated v4.9.0) (metatx/ERC2771Forwarder.sol) pragma solidity ^0.8.19; @@ -8,7 +8,7 @@ import "../utils/cryptography/EIP712.sol"; import "../utils/Nonces.sol"; /** - * @dev A minimal implementation of a production-ready forwarder compatible with ERC2771 contracts. + * @dev An implementation of a production-ready forwarder compatible with ERC2771 contracts. * * This forwarder operates on forward requests that include: * @@ -20,7 +20,7 @@ import "../utils/Nonces.sol"; * * `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, Nonces { +contract ERC2771Forwarder is EIP712, Nonces { using ECDSA for bytes32; struct ForwardRequest { @@ -46,22 +46,22 @@ contract MinimalForwarder is EIP712, Nonces { /** * @dev The request `from` doesn't match with the recovered `signer`. */ - error MinimalForwarderInvalidSigner(address signer, address from); + error ERC2771ForwarderInvalidSigner(address signer, address from); /** * @dev The requested `value` doesn't match with the available `msgValue`, leaving ETH stuck in the contract. */ - error MinimalForwarderMismatchedValue(uint256 value, uint256 msgValue); + error ERC2771ForwarderMismatchedValue(uint256 value, uint256 msgValue); /** * @dev The list of requests length doesn't match with the list of signatures length. */ - error MinimalForwarderInvalidBatchLength(uint256 requestsLength, uint256 signaturesLength); + error ERC2771ForwarderInvalidBatchLength(uint256 requestsLength, uint256 signaturesLength); /** * @dev The request `deadline` has expired. */ - error MinimalForwarderExpiredRequest(uint256 deadline); + error ERC2771ForwarderExpiredRequest(uint256 deadline); /** * @dev See {EIP712-constructor}. @@ -84,12 +84,12 @@ contract MinimalForwarder is EIP712, Nonces { ForwardRequest calldata request, bytes calldata signature ) public payable virtual returns (bool, bytes memory) { - (bool success, bytes memory returndata) = _execute(request, signature); - if (msg.value != request.value) { - revert MinimalForwarderMismatchedValue(request.value, msg.value); + revert ERC2771ForwarderMismatchedValue(request.value, msg.value); } + (bool success, bytes memory returndata) = _execute(request, signature); + return (success, returndata); } @@ -98,7 +98,7 @@ contract MinimalForwarder is EIP712, Nonces { */ function executeBatch(ForwardRequest[] calldata requests, bytes[] calldata signatures) public payable virtual { if (requests.length != signatures.length) { - revert MinimalForwarderInvalidBatchLength(requests.length, signatures.length); + revert ERC2771ForwarderInvalidBatchLength(requests.length, signatures.length); } uint256 requestsValue; @@ -109,7 +109,7 @@ contract MinimalForwarder is EIP712, Nonces { } if (msg.value != requestsValue) { - revert MinimalForwarderMismatchedValue(requestsValue, msg.value); + revert ERC2771ForwarderMismatchedValue(requestsValue, msg.value); } } @@ -171,12 +171,12 @@ contract MinimalForwarder is EIP712, Nonces { // The _validate function is intentionally avoided to keep the signer argument and the nonce check if (request.deadline < block.number) { - revert MinimalForwarderExpiredRequest(request.deadline); + revert ERC2771ForwarderExpiredRequest(request.deadline); } address signer = _recoverForwardRequestSigner(request, signature); if (signer != request.from) { - revert MinimalForwarderInvalidSigner(signer, request.from); + revert ERC2771ForwarderInvalidSigner(signer, request.from); } _useCheckedNonce(request.from, request.nonce); diff --git a/contracts/metatx/README.adoc b/contracts/metatx/README.adoc index eccdeaf9740..9f25802e4d8 100644 --- a/contracts/metatx/README.adoc +++ b/contracts/metatx/README.adoc @@ -9,4 +9,4 @@ NOTE: This document is better viewed at https://docs.openzeppelin.com/contracts/ == Utils -{{MinimalForwarder}} +{{ERC2771Forwarder}} diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index bbf8142cc88..f0a87692b8f 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -2,20 +2,20 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; const { getDomain, domainType } = require('../helpers/eip712'); -const { expectEvent, BN } = require('@openzeppelin/test-helpers'); +const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const ERC2771ContextMock = artifacts.require('ERC2771ContextMock'); -const MinimalForwarder = artifacts.require('MinimalForwarder'); +const ERC2771Forwarder = artifacts.require('ERC2771Forwarder'); 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(); + const MAX_UINT48 = web3.utils.toBN(1).shln(48).subn(1).toString(); beforeEach(async function () { - this.forwarder = await MinimalForwarder.new('MinimalForwarder', '0.0.1'); + this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder'); this.recipient = await ERC2771ContextMock.new(this.forwarder.address); this.domain = await getDomain(this.forwarder); diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/ERC2771Forwarder.test.js similarity index 92% rename from test/metatx/MinimalForwarder.test.js rename to test/metatx/ERC2771Forwarder.test.js index 333bb7b5a9d..c1948e9bfff 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -6,10 +6,10 @@ const { expectRevertCustomError } = require('../helpers/customError'); const { constants, expectRevert, expectEvent, time } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const MinimalForwarder = artifacts.require('MinimalForwarder'); +const ERC2771Forwarder = artifacts.require('ERC2771Forwarder'); const CallReceiverMock = artifacts.require('CallReceiverMock'); -contract('MinimalForwarder', function (accounts) { +contract('ERC2771Forwarder', function (accounts) { const tamperedValues = { from: accounts[0], to: accounts[0], @@ -20,7 +20,7 @@ contract('MinimalForwarder', function (accounts) { }; beforeEach(async function () { - this.forwarder = await MinimalForwarder.new('MinimalForwarder'); + this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder'); this.domain = await getDomain(this.forwarder); this.types = { @@ -45,7 +45,7 @@ contract('MinimalForwarder', function (accounts) { to: constants.ZERO_ADDRESS, value: '0', gas: '100000', - nonce: Number(await this.forwarder.nonces(this.alice.address)), + nonce: (await this.forwarder.nonces(this.alice.address)).toString(), data: '0x', deadline: this.blockNumber.toNumber() + 2, // Next + 1 }; @@ -148,10 +148,13 @@ contract('MinimalForwarder', function (accounts) { it(`reverts with tampered ${key}`, async function () { const sig = this.sign(this.alice.getPrivateKey()); const data = this.forgeData({ [key]: value }); - await expectRevertCustomError(this.forwarder.execute(data.message, sig), 'MinimalForwarderInvalidSigner', [ - ethSigUtil.recoverTypedSignature({ data, sig }), - data.message.from, - ]); + await expectRevertCustomError( + this.forwarder.execute(data.message, sig, { + value: key == 'value' ? value : 0, // To avoid MismatchedValue error + }), + 'ERC2771ForwarderInvalidSigner', + [ethSigUtil.recoverTypedSignature({ data, sig }), data.message.from], + ); }); } @@ -160,7 +163,7 @@ contract('MinimalForwarder', function (accounts) { tamperedSig[42] ^= 0xff; await expectRevertCustomError( this.forwarder.execute(this.request, web3.utils.bytesToHex(tamperedSig)), - 'MinimalForwarderInvalidSigner', + 'ERC2771ForwarderInvalidSigner', [ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }), this.request.from], ); }); @@ -183,7 +186,7 @@ contract('MinimalForwarder', function (accounts) { deadline: this.blockNumber.toNumber() - 1, }; const sig = this.sign(this.alice.getPrivateKey(), req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderExpiredRequest', [ + await expectRevertCustomError(this.forwarder.execute(req, sig), 'ERC2771ForwarderExpiredRequest', [ this.blockNumber.toNumber() - 1, ]); }); @@ -195,7 +198,7 @@ contract('MinimalForwarder', function (accounts) { value, }; const sig = this.sign(this.alice.getPrivateKey(), req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderMismatchedValue', [0, value]); + await expectRevertCustomError(this.forwarder.execute(req, sig), 'ERC2771ForwarderMismatchedValue', [0, value]); }); }); @@ -241,13 +244,13 @@ contract('MinimalForwarder', function (accounts) { it('reverts with mismatched lengths', async function () { await expectRevertCustomError( this.forwarder.executeBatch(this.requests, this.signatures.slice(0, -1)), - 'MinimalForwarderInvalidBatchLength', + 'ERC2771ForwarderInvalidBatchLength', [this.requests.length, this.signatures.length - 1], ); await expectRevertCustomError( this.forwarder.executeBatch(this.requests.slice(0, -1), this.signatures), - 'MinimalForwarderInvalidBatchLength', + 'ERC2771ForwarderInvalidBatchLength', [this.requests.length - 1, this.signatures.length], ); }); @@ -281,7 +284,7 @@ contract('MinimalForwarder', function (accounts) { await expectRevertCustomError( this.forwarder.executeBatch(this.requests, this.signatures), - 'MinimalForwarderInvalidSigner', + 'ERC2771ForwarderInvalidSigner', [ethSigUtil.recoverTypedSignature({ data, sig }), data.message.from], ); }); @@ -295,7 +298,7 @@ contract('MinimalForwarder', function (accounts) { await expectRevertCustomError( this.forwarder.executeBatch(this.requests, this.signatures), - 'MinimalForwarderInvalidSigner', + 'ERC2771ForwarderInvalidSigner', [ ethSigUtil.recoverTypedSignature({ data: this.forgeData(this.requests[this.idx]), @@ -323,7 +326,7 @@ contract('MinimalForwarder', function (accounts) { this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); await expectRevertCustomError( this.forwarder.executeBatch(this.requests, this.signatures), - 'MinimalForwarderExpiredRequest', + 'ERC2771ForwarderExpiredRequest', [this.blockNumber.toNumber() - 1], ); }); @@ -334,7 +337,7 @@ contract('MinimalForwarder', function (accounts) { this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); await expectRevertCustomError( this.forwarder.executeBatch(this.requests, this.signatures), - 'MinimalForwarderMismatchedValue', + 'ERC2771ForwarderMismatchedValue', [0, value], ); }); From 3d5fe686425cf44b7f507f18cf7718cd1d15b6cf Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 22 Jun 2023 22:48:31 -0600 Subject: [PATCH 11/34] Avoid revert on nonce mismatch --- contracts/metatx/ERC2771Forwarder.sol | 18 ++++++++++----- test/metatx/ERC2771Forwarder.test.js | 33 +++++++++++++++------------ 2 files changed, 30 insertions(+), 21 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index a9d3430348d..3c7f3179e39 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -70,6 +70,9 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev Returns `true` if a request is valid for a provided `signature` at the current block. + * + * NOTE: A request with an invalid nonce will return false here but won't revert to prevent revert + * when a batch includes a request already executed by another relay. */ function verify(ForwardRequest calldata request, bytes calldata signature) public view virtual returns (bool) { (bool alive, bool signerMatch, bool nonceMatch) = _validate(request, signature); @@ -179,15 +182,18 @@ contract ERC2771Forwarder is EIP712, Nonces { revert ERC2771ForwarderInvalidSigner(signer, request.from); } - _useCheckedNonce(request.from, request.nonce); + // Avoid execution instead of reverting in case a batch includes an already executed request + if (nonces(request.from) == request.nonce) { + _useNonce(request.from); - (success, returndata) = request.to.call{gas: request.gas, value: request.value}( - abi.encodePacked(request.data, request.from) - ); + (success, returndata) = request.to.call{gas: request.gas, value: request.value}( + abi.encodePacked(request.data, request.from) + ); - _checkForwardedGas(request); + _checkForwardedGas(request); - emit ExecutedForwardRequest(signer, request.nonce, success, returndata); + emit ExecutedForwardRequest(signer, request.nonce, success, returndata); + } } /** diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index c1948e9bfff..fd2024cb2c4 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -168,16 +168,13 @@ contract('ERC2771Forwarder', function (accounts) { ); }); - it('reverts with valid signature for non-current nonce', async function () { + it('succeeds without executing the request with valid signature for non-current nonce', async function () { const req = { ...this.request, nonce: this.request.nonce + 1, }; const sig = this.sign(this.alice.getPrivateKey(), req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'InvalidAccountNonce', [ - this.request.from, - this.request.nonce, - ]); + await expectEvent.notEmitted(await this.forwarder.execute(req, sig), 'ExecutedForwardRequest'); }); it('reverts with valid signature for expired deadline', async function () { @@ -256,10 +253,13 @@ contract('ERC2771Forwarder', function (accounts) { }); context('with valid signatures', function () { - it('succeeds', async function () { - const receipt = await this.forwarder.executeBatch(this.requests, this.signatures); + beforeEach(async function () { + this.receipt = await this.forwarder.executeBatch(this.requests, this.signatures); + }); + + it('emits events', async function () { for (const request of this.requests) { - expectEvent(receipt, 'ExecutedForwardRequest', { + expectEvent(this.receipt, 'ExecutedForwardRequest', { signer: request.from, nonce: web3.utils.toBN(request.nonce), success: true, @@ -267,6 +267,12 @@ contract('ERC2771Forwarder', function (accounts) { }); } }); + + it('increase nonces', async function () { + for (const request of this.requests) { + expect(await this.forwarder.nonces(request.from)).to.be.bignumber.eq(web3.utils.toBN(request.nonce + 1)); + } + }); }); context('with tampered values', function () { @@ -309,16 +315,13 @@ contract('ERC2771Forwarder', function (accounts) { ); }); - it('reverts with at least one valid signature for non-current nonce', async function () { - const currentNonce = this.requests[this.idx].nonce; + it('succeeds with one valid signature for non-current nonce without executing the invalid request', async function () { this.requests[this.idx].nonce = this.requests[this.idx].nonce + 1; this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); - await expectRevertCustomError( - this.forwarder.executeBatch(this.requests, this.signatures), - 'InvalidAccountNonce', - [this.requests[this.idx].from, currentNonce], - ); + const receipt = await this.forwarder.executeBatch(this.requests, this.signatures); + + expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); }); it('reverts with at least one valid signature for expired deadline', async function () { From c781b6b4f24e65282998ba31ae90b27e2d38b422 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 22 Jun 2023 22:51:29 -0600 Subject: [PATCH 12/34] Use _validate in _execute --- contracts/metatx/ERC2771Forwarder.sol | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 3c7f3179e39..5e1ea7b444b 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -75,7 +75,7 @@ contract ERC2771Forwarder is EIP712, Nonces { * when a batch includes a request already executed by another relay. */ function verify(ForwardRequest calldata request, bytes calldata signature) public view virtual returns (bool) { - (bool alive, bool signerMatch, bool nonceMatch) = _validate(request, signature); + (bool alive, bool signerMatch, bool nonceMatch, ) = _validate(request, signature); return alive && signerMatch && nonceMatch; } @@ -122,9 +122,14 @@ contract ERC2771Forwarder is EIP712, Nonces { 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); + ) internal view virtual returns (bool alive, bool signerMatch, bool nonceMatch, address signer) { + signer = _recoverForwardRequestSigner(request, signature); + return ( + request.deadline >= block.number, + signer == request.from, + nonces(request.from) == request.nonce, + signer + ); } /** @@ -171,19 +176,18 @@ contract ERC2771Forwarder is EIP712, Nonces { 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 + (bool alive, bool signerMatch, bool nonceMatch, address signer) = _validate(request, signature); - if (request.deadline < block.number) { + if (!alive) { revert ERC2771ForwarderExpiredRequest(request.deadline); } - address signer = _recoverForwardRequestSigner(request, signature); - if (signer != request.from) { + if (!signerMatch) { revert ERC2771ForwarderInvalidSigner(signer, request.from); } // Avoid execution instead of reverting in case a batch includes an already executed request - if (nonces(request.from) == request.nonce) { + if (nonceMatch) { _useNonce(request.from); (success, returndata) = request.to.call{gas: request.gas, value: request.value}( From f5987eba66a0995ca2213d9f48fff8708e0da93b Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 23 Jun 2023 12:35:14 -0600 Subject: [PATCH 13/34] Use timestamp instead of block number --- contracts/metatx/ERC2771Forwarder.sol | 6 ++++-- test/metatx/ERC2771Forwarder.test.js | 14 +++++++------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 5e1ea7b444b..4f58c282855 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -61,7 +61,7 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev The request `deadline` has expired. */ - error ERC2771ForwarderExpiredRequest(uint256 deadline); + error ERC2771ForwarderExpiredRequest(uint48 deadline); /** * @dev See {EIP712-constructor}. @@ -125,7 +125,7 @@ contract ERC2771Forwarder is EIP712, Nonces { ) internal view virtual returns (bool alive, bool signerMatch, bool nonceMatch, address signer) { signer = _recoverForwardRequestSigner(request, signature); return ( - request.deadline >= block.number, + request.deadline >= block.timestamp, signer == request.from, nonces(request.from) == request.nonce, signer @@ -171,6 +171,8 @@ contract ERC2771Forwarder is EIP712, Nonces { * * IMPORTANT: Using this function doesn't check that all the `msg.value` was sent, potentially leaving * ETH stuck in the contract. + * + * NOTE: Execution won't revert for invalid signed nonces. */ function _execute( ForwardRequest calldata request, diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index fd2024cb2c4..0f683e77210 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -16,7 +16,7 @@ contract('ERC2771Forwarder', function (accounts) { value: web3.utils.toWei('1'), nonce: 1234, data: '0x1742', - deadline: 0xabcdef, + deadline: 0xdeadbeef, }; beforeEach(async function () { @@ -39,7 +39,7 @@ contract('ERC2771Forwarder', function (accounts) { this.alice = Wallet.generate(); this.alice.address = web3.utils.toChecksumAddress(this.alice.getAddressString()); - this.blockNumber = await time.latestBlock(); + this.timestamp = await time.latest(); this.request = { from: this.alice.address, to: constants.ZERO_ADDRESS, @@ -47,7 +47,7 @@ contract('ERC2771Forwarder', function (accounts) { gas: '100000', nonce: (await this.forwarder.nonces(this.alice.address)).toString(), data: '0x', - deadline: this.blockNumber.toNumber() + 2, // Next + 1 + deadline: this.timestamp.toNumber() + 60, // 1 minute }; this.forgeData = request => ({ @@ -180,11 +180,11 @@ contract('ERC2771Forwarder', function (accounts) { it('reverts with valid signature for expired deadline', async function () { const req = { ...this.request, - deadline: this.blockNumber.toNumber() - 1, + deadline: this.timestamp.toNumber() - 1, }; const sig = this.sign(this.alice.getPrivateKey(), req); await expectRevertCustomError(this.forwarder.execute(req, sig), 'ERC2771ForwarderExpiredRequest', [ - this.blockNumber.toNumber() - 1, + this.timestamp.toNumber() - 1, ]); }); @@ -325,12 +325,12 @@ contract('ERC2771Forwarder', function (accounts) { }); it('reverts with at least one valid signature for expired deadline', async function () { - this.requests[this.idx].deadline = this.blockNumber.toNumber() - 1; + this.requests[this.idx].deadline = this.timestamp.toNumber() - 1; this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); await expectRevertCustomError( this.forwarder.executeBatch(this.requests, this.signatures), 'ERC2771ForwarderExpiredRequest', - [this.blockNumber.toNumber() - 1], + [this.timestamp.toNumber() - 1], ); }); From 87e77b1d224d6cda89baa8074c607ac8e509466a Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 23 Jun 2023 19:52:35 -0600 Subject: [PATCH 14/34] Apply suggestions --- .changeset/blue-horses-do.md | 2 +- contracts/metatx/ERC2771Forwarder.sol | 93 +++++++++++++++------------ test/metatx/ERC2771Forwarder.test.js | 55 +++++++++------- 3 files changed, 82 insertions(+), 68 deletions(-) diff --git a/.changeset/blue-horses-do.md b/.changeset/blue-horses-do.md index 812f8c28eef..9df604fe448 100644 --- a/.changeset/blue-horses-do.md +++ b/.changeset/blue-horses-do.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': major --- -`ERC2771Forwarder`: Added `deadline` for expiring transactions and added `msg.value` mismatch check. +`ERC2771Forwarder`: Added `deadline` for expiring transactions, batching, and more secure handling of `msg.value`. diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 4f58c282855..66abd6add0f 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -23,12 +23,11 @@ import "../utils/Nonces.sol"; contract ERC2771Forwarder is EIP712, Nonces { using ECDSA for bytes32; - struct ForwardRequest { + struct ForwardRequestData { address from; address to; uint256 value; uint256 gas; - uint256 nonce; uint48 deadline; bytes data; } @@ -71,35 +70,41 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev Returns `true` if a request is valid for a provided `signature` at the current block. * - * NOTE: A request with an invalid nonce will return false here but won't revert to prevent revert - * when a batch includes a request already executed by another relay. + * NOTE: A request signed with an invalid nonce will return false here but won't revert by default + * to prevent revert when a batch includes a request already executed by another relay. This behavior can be opt-out. + * See {executeBatch}. */ - 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; + function verify(ForwardRequestData calldata request, bytes calldata signature) public view virtual returns (bool) { + (bool alive, bool signerMatch, ) = _validate(request, signature, nonces(request.from)); + return alive && signerMatch; } /** * @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. + * will receive the requested gas and no ETH is left stuck in the contract. */ function execute( - ForwardRequest calldata request, + ForwardRequestData calldata request, bytes calldata signature ) public payable virtual returns (bool, bytes memory) { if (msg.value != request.value) { revert ERC2771ForwarderMismatchedValue(request.value, msg.value); } - (bool success, bytes memory returndata) = _execute(request, signature); + (bool success, bytes memory returndata) = _execute(request, signature, _useNonce(request.from), true); return (success, returndata); } /** - * @dev Batch version of {execute}. + * @dev Batch version of {execute} that also includes an `atomic` parameter to indicate whether all requests are + * executed or none of them. */ - function executeBatch(ForwardRequest[] calldata requests, bytes[] calldata signatures) public payable virtual { + function executeBatch( + ForwardRequestData[] calldata requests, + bytes[] calldata signatures, + bool atomic + ) public payable virtual { if (requests.length != signatures.length) { revert ERC2771ForwarderInvalidBatchLength(requests.length, signatures.length); } @@ -108,7 +113,7 @@ contract ERC2771Forwarder is EIP712, Nonces { for (uint256 i; i < requests.length; ++i) { requestsValue += requests[i].value; - _execute(requests[i], signatures[i]); + _execute(requests[i], signatures[i], _useNonce(requests[i].from), atomic); } if (msg.value != requestsValue) { @@ -118,27 +123,28 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev Validates if the provided request can be executed at current block with `signature` on behalf of `signer`. + * + * Internal function without nonce validation. */ function _validate( - ForwardRequest calldata request, - bytes calldata signature - ) internal view virtual returns (bool alive, bool signerMatch, bool nonceMatch, address signer) { - signer = _recoverForwardRequestSigner(request, signature); - return ( - request.deadline >= block.timestamp, - signer == request.from, - nonces(request.from) == request.nonce, - signer - ); + ForwardRequestData calldata request, + bytes calldata signature, + uint256 nonce + ) internal view virtual returns (bool alive, bool signerMatch, address signer) { + signer = _recoverForwardRequestSigner(request, signature, nonce); + return (request.deadline >= block.timestamp, signer == request.from, signer); } /** * @dev Recovers the signer of an EIP712 message hash for a forward `request` and its corresponding `signature`. * See {ECDSA-recover}. + * + * Internal function without nonce validation. */ function _recoverForwardRequestSigner( - ForwardRequest calldata request, - bytes calldata signature + ForwardRequestData calldata request, + bytes calldata signature, + uint256 nonce ) internal view virtual returns (address) { return _hashTypedDataV4( @@ -149,7 +155,7 @@ contract ERC2771Forwarder is EIP712, Nonces { request.to, request.value, request.gas, - request.nonce, + nonce, request.deadline, keccak256(request.data) ) @@ -160,45 +166,48 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev Validates and executes a signed request. * + * Internal function without nonce and msg.value validation. + * * 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. - * - * NOTE: Execution won't revert for invalid signed nonces. */ function _execute( - ForwardRequest calldata request, - bytes calldata signature + ForwardRequestData calldata request, + bytes calldata signature, + uint256 nonce, + bool requireValidRequest ) internal virtual returns (bool success, bytes memory returndata) { - (bool alive, bool signerMatch, bool nonceMatch, address signer) = _validate(request, signature); + (bool alive, bool signerMatch, address signer) = _validate(request, signature, nonce); - if (!alive) { - revert ERC2771ForwarderExpiredRequest(request.deadline); - } + // Need to explicitly specify if a revert is required since non-reverting is default for + // batches and reversion is opt-in since it could be useful in some scenarios + if (requireValidRequest) { + if (!alive) { + revert ERC2771ForwarderExpiredRequest(request.deadline); + } - if (!signerMatch) { - revert ERC2771ForwarderInvalidSigner(signer, request.from); + if (!signerMatch) { + revert ERC2771ForwarderInvalidSigner(signer, request.from); + } } // Avoid execution instead of reverting in case a batch includes an already executed request - if (nonceMatch) { - _useNonce(request.from); - + if (signerMatch && alive) { (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); + emit ExecutedForwardRequest(signer, nonce, success, returndata); } } @@ -214,7 +223,7 @@ contract ERC2771Forwarder is EIP712, Nonces { * 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 { + function _checkForwardedGas(ForwardRequestData 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, diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index 0f683e77210..45fd365a39b 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -14,7 +14,6 @@ contract('ERC2771Forwarder', function (accounts) { from: accounts[0], to: accounts[0], value: web3.utils.toWei('1'), - nonce: 1234, data: '0x1742', deadline: 0xdeadbeef, }; @@ -146,14 +145,13 @@ contract('ERC2771Forwarder', function (accounts) { context('with tampered values', function () { for (const [key, value] of Object.entries(tamperedValues)) { it(`reverts with tampered ${key}`, async function () { - const sig = this.sign(this.alice.getPrivateKey()); const data = this.forgeData({ [key]: value }); await expectRevertCustomError( - this.forwarder.execute(data.message, sig, { + this.forwarder.execute(data.message, this.signature, { value: key == 'value' ? value : 0, // To avoid MismatchedValue error }), 'ERC2771ForwarderInvalidSigner', - [ethSigUtil.recoverTypedSignature({ data, sig }), data.message.from], + [ethSigUtil.recoverTypedSignature({ data, sig: this.signature }), data.message.from], ); }); } @@ -168,13 +166,20 @@ contract('ERC2771Forwarder', function (accounts) { ); }); - it('succeeds without executing the request with valid signature for non-current nonce', async function () { - const req = { - ...this.request, - nonce: this.request.nonce + 1, - }; - const sig = this.sign(this.alice.getPrivateKey(), req); - await expectEvent.notEmitted(await this.forwarder.execute(req, sig), 'ExecutedForwardRequest'); + it('reverts with valid signature for non-current nonce', async function () { + // Execute first a request + await this.forwarder.execute(this.request, this.signature); + + // And then fail due to an already used nonce + this.request.nonce++; + await expectRevertCustomError( + this.forwarder.execute(this.request, this.signature), + 'ERC2771ForwarderInvalidSigner', + [ + ethSigUtil.recoverTypedSignature({ data: this.forgeData(this.request), sig: this.signature }), + this.request.from, + ], + ); }); it('reverts with valid signature for expired deadline', async function () { @@ -240,13 +245,13 @@ contract('ERC2771Forwarder', function (accounts) { it('reverts with mismatched lengths', async function () { await expectRevertCustomError( - this.forwarder.executeBatch(this.requests, this.signatures.slice(0, -1)), + this.forwarder.executeBatch(this.requests, this.signatures.slice(0, -1), false), 'ERC2771ForwarderInvalidBatchLength', [this.requests.length, this.signatures.length - 1], ); await expectRevertCustomError( - this.forwarder.executeBatch(this.requests.slice(0, -1), this.signatures), + this.forwarder.executeBatch(this.requests.slice(0, -1), this.signatures, false), 'ERC2771ForwarderInvalidBatchLength', [this.requests.length - 1, this.signatures.length], ); @@ -254,7 +259,7 @@ contract('ERC2771Forwarder', function (accounts) { context('with valid signatures', function () { beforeEach(async function () { - this.receipt = await this.forwarder.executeBatch(this.requests, this.signatures); + this.receipt = await this.forwarder.executeBatch(this.requests, this.signatures, false); }); it('emits events', async function () { @@ -282,16 +287,14 @@ contract('ERC2771Forwarder', function (accounts) { for (const [key, value] of Object.entries(tamperedValues)) { it(`reverts with at least one tampered request ${key}`, async function () { - const data = this.forgeData({ [key]: value }); - const sig = this.sign(this.signers[this.idx].getPrivateKey()); + const data = this.forgeData({ ...this.requests[this.idx], [key]: value }); this.requests[this.idx] = data.message; - this.signatures[this.idx] = sig; await expectRevertCustomError( - this.forwarder.executeBatch(this.requests, this.signatures), + this.forwarder.executeBatch(this.requests, this.signatures, true), 'ERC2771ForwarderInvalidSigner', - [ethSigUtil.recoverTypedSignature({ data, sig }), data.message.from], + [ethSigUtil.recoverTypedSignature({ data, sig: this.signatures[this.idx] }), data.message.from], ); }); } @@ -303,7 +306,7 @@ contract('ERC2771Forwarder', function (accounts) { this.signatures[this.idx] = web3.utils.bytesToHex(tamperedSig); await expectRevertCustomError( - this.forwarder.executeBatch(this.requests, this.signatures), + this.forwarder.executeBatch(this.requests, this.signatures, true), 'ERC2771ForwarderInvalidSigner', [ ethSigUtil.recoverTypedSignature({ @@ -315,11 +318,11 @@ contract('ERC2771Forwarder', function (accounts) { ); }); - it('succeeds with one valid signature for non-current nonce without executing the invalid request', async function () { + it('succeeds ignoring a request with an invalid nonceZZZ', async function () { this.requests[this.idx].nonce = this.requests[this.idx].nonce + 1; this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); - const receipt = await this.forwarder.executeBatch(this.requests, this.signatures); + const receipt = await this.forwarder.executeBatch(this.requests, this.signatures, false); expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); }); @@ -328,7 +331,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requests[this.idx].deadline = this.timestamp.toNumber() - 1; this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); await expectRevertCustomError( - this.forwarder.executeBatch(this.requests, this.signatures), + this.forwarder.executeBatch(this.requests, this.signatures, true), 'ERC2771ForwarderExpiredRequest', [this.timestamp.toNumber() - 1], ); @@ -339,7 +342,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requests[this.idx].value = value; this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); await expectRevertCustomError( - this.forwarder.executeBatch(this.requests, this.signatures), + this.forwarder.executeBatch(this.requests, this.signatures, true), 'ERC2771ForwarderMismatchedValue', [0, value], ); @@ -356,7 +359,9 @@ contract('ERC2771Forwarder', function (accounts) { this.signatures[idx] = this.sign(this.signers[idx].getPrivateKey(), this.requests[idx]); - await expectRevert.assertion(this.forwarder.executeBatch(this.requests, this.signatures, { gas: gasAvailable })); + await expectRevert.assertion( + this.forwarder.executeBatch(this.requests, this.signatures, true, { gas: gasAvailable }), + ); const { transactions } = await web3.eth.getBlock('latest'); const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); From bdd061ddb5176614e33d913cb8778fa49cbba2ec Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 23 Jun 2023 20:28:53 -0600 Subject: [PATCH 15/34] Pack signature into request --- contracts/metatx/ERC2771Forwarder.sol | 33 ++-- test/metatx/ERC2771Forwarder.test.js | 209 +++++++++++++------------- 2 files changed, 113 insertions(+), 129 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 66abd6add0f..fc45de11fc1 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -30,6 +30,7 @@ contract ERC2771Forwarder is EIP712, Nonces { uint256 gas; uint48 deadline; bytes data; + bytes signature; } bytes32 private constant _FORWARD_REQUEST_TYPEHASH = @@ -74,8 +75,8 @@ contract ERC2771Forwarder is EIP712, Nonces { * to prevent revert when a batch includes a request already executed by another relay. This behavior can be opt-out. * See {executeBatch}. */ - function verify(ForwardRequestData calldata request, bytes calldata signature) public view virtual returns (bool) { - (bool alive, bool signerMatch, ) = _validate(request, signature, nonces(request.from)); + function verify(ForwardRequestData calldata request) public view virtual returns (bool) { + (bool alive, bool signerMatch, ) = _validate(request, nonces(request.from)); return alive && signerMatch; } @@ -83,15 +84,12 @@ contract ERC2771Forwarder is EIP712, Nonces { * @dev Executes a `request` on behalf of `signature`'s signer guaranteeing that the forwarded call * will receive the requested gas and no ETH is left stuck in the contract. */ - function execute( - ForwardRequestData calldata request, - bytes calldata signature - ) public payable virtual returns (bool, bytes memory) { + function execute(ForwardRequestData calldata request) public payable virtual returns (bool, bytes memory) { if (msg.value != request.value) { revert ERC2771ForwarderMismatchedValue(request.value, msg.value); } - (bool success, bytes memory returndata) = _execute(request, signature, _useNonce(request.from), true); + (bool success, bytes memory returndata) = _execute(request, _useNonce(request.from), true); return (success, returndata); } @@ -100,20 +98,12 @@ contract ERC2771Forwarder is EIP712, Nonces { * @dev Batch version of {execute} that also includes an `atomic` parameter to indicate whether all requests are * executed or none of them. */ - function executeBatch( - ForwardRequestData[] calldata requests, - bytes[] calldata signatures, - bool atomic - ) public payable virtual { - if (requests.length != signatures.length) { - revert ERC2771ForwarderInvalidBatchLength(requests.length, signatures.length); - } - + function executeBatch(ForwardRequestData[] calldata requests, bool atomic) public payable virtual { uint256 requestsValue; for (uint256 i; i < requests.length; ++i) { requestsValue += requests[i].value; - _execute(requests[i], signatures[i], _useNonce(requests[i].from), atomic); + _execute(requests[i], _useNonce(requests[i].from), atomic); } if (msg.value != requestsValue) { @@ -128,10 +118,9 @@ contract ERC2771Forwarder is EIP712, Nonces { */ function _validate( ForwardRequestData calldata request, - bytes calldata signature, uint256 nonce ) internal view virtual returns (bool alive, bool signerMatch, address signer) { - signer = _recoverForwardRequestSigner(request, signature, nonce); + signer = _recoverForwardRequestSigner(request, nonce); return (request.deadline >= block.timestamp, signer == request.from, signer); } @@ -143,7 +132,6 @@ contract ERC2771Forwarder is EIP712, Nonces { */ function _recoverForwardRequestSigner( ForwardRequestData calldata request, - bytes calldata signature, uint256 nonce ) internal view virtual returns (address) { return @@ -160,7 +148,7 @@ contract ERC2771Forwarder is EIP712, Nonces { keccak256(request.data) ) ) - ).recover(signature); + ).recover(request.signature); } /** @@ -181,11 +169,10 @@ contract ERC2771Forwarder is EIP712, Nonces { */ function _execute( ForwardRequestData calldata request, - bytes calldata signature, uint256 nonce, bool requireValidRequest ) internal virtual returns (bool success, bytes memory returndata) { - (bool alive, bool signerMatch, address signer) = _validate(request, signature, nonce); + (bool alive, bool signerMatch, address signer) = _validate(request, nonce); // Need to explicitly specify if a revert is required since non-reverting is default for // batches and reversion is opt-in since it could be useful in some scenarios diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index 45fd365a39b..1cdeb819280 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -44,40 +44,43 @@ contract('ERC2771Forwarder', function (accounts) { to: constants.ZERO_ADDRESS, value: '0', gas: '100000', - nonce: (await this.forwarder.nonces(this.alice.address)).toString(), data: '0x', deadline: this.timestamp.toNumber() + 60, // 1 minute }; + this.requestData = { + ...this.request, + nonce: (await this.forwarder.nonces(this.alice.address)).toString(), + }; this.forgeData = request => ({ types: this.types, domain: this.domain, primaryType: 'ForwardRequest', - message: { ...this.request, ...request }, + message: { ...this.requestData, ...request }, }); this.sign = (privateKey, request) => ethSigUtil.signTypedMessage(privateKey, { data: this.forgeData(request), }); - this.signature = this.sign(this.alice.getPrivateKey()); + this.requestData.signature = this.sign(this.alice.getPrivateKey()); }); context('verify', function () { context('with valid signature', function () { beforeEach(async function () { - expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( - web3.utils.toBN(this.request.nonce), + expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( + web3.utils.toBN(this.requestData.nonce), ); }); it('success', async function () { - expect(await this.forwarder.verify(this.request, this.signature)).to.be.equal(true); + expect(await this.forwarder.verify(this.requestData)).to.be.equal(true); }); afterEach(async function () { - expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( - web3.utils.toBN(this.request.nonce), + expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( + web3.utils.toBN(this.requestData.nonce), ); }); }); @@ -85,34 +88,33 @@ contract('ERC2771Forwarder', function (accounts) { context('with tampered values', function () { for (const [key, value] of Object.entries(tamperedValues)) { it(`returns false with tampered ${key}`, async function () { - expect(await this.forwarder.verify(this.forgeData({ [key]: value }).message, this.signature)).to.be.equal( - false, - ); + expect(await this.forwarder.verify(this.forgeData({ [key]: value }).message)).to.be.equal(false); }); } it('returns false with tampered signature', async function () { - const tamperedsign = web3.utils.hexToBytes(this.signature); + const tamperedsign = web3.utils.hexToBytes(this.requestData.signature); tamperedsign[42] ^= 0xff; - expect(await this.forwarder.verify(this.request, web3.utils.bytesToHex(tamperedsign))).to.be.equal(false); + this.requestData.signature = web3.utils.bytesToHex(tamperedsign); + expect(await this.forwarder.verify(this.requestData)).to.be.equal(false); }); it('returns false with valid signature for non-current nonce', async function () { const req = { - ...this.request, - nonce: this.request.nonce + 1, + ...this.requestData, + nonce: this.requestData.nonce + 1, }; - const sig = this.sign(this.alice.getPrivateKey(), req); - expect(await this.forwarder.verify(req, sig)).to.be.equal(false); + req.signature = this.sign(this.alice.getPrivateKey(), req); + expect(await this.forwarder.verify(req)).to.be.equal(false); }); it('returns false with valid signature for expired deadline', async function () { const req = { - ...this.request, - nonce: this.request.nonce + 1, + ...this.requestData, + nonce: this.requestData.nonce + 1, }; - const sig = this.sign(this.alice.getPrivateKey(), req); - expect(await this.forwarder.verify(req, sig)).to.be.equal(false); + req.signature = this.sign(this.alice.getPrivateKey(), req); + expect(await this.forwarder.verify(req)).to.be.equal(false); }); }); }); @@ -120,24 +122,24 @@ contract('ERC2771Forwarder', function (accounts) { context('execute', function () { context('with valid signature', function () { beforeEach(async function () { - expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( - web3.utils.toBN(this.request.nonce), + expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( + web3.utils.toBN(this.requestData.nonce), ); }); it('succeeds', async function () { - const receipt = await this.forwarder.execute(this.request, this.signature); + const receipt = await this.forwarder.execute(this.requestData); expectEvent(receipt, 'ExecutedForwardRequest', { - signer: this.request.from, - nonce: web3.utils.toBN(this.request.nonce), + signer: this.requestData.from, + nonce: web3.utils.toBN(this.requestData.nonce), success: true, returndata: null, }); }); afterEach(async function () { - expect(await this.forwarder.nonces(this.request.from)).to.be.bignumber.equal( - web3.utils.toBN(this.request.nonce + 1), + expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( + web3.utils.toBN(this.requestData.nonce + 1), ); }); }); @@ -147,48 +149,47 @@ contract('ERC2771Forwarder', function (accounts) { it(`reverts with tampered ${key}`, async function () { const data = this.forgeData({ [key]: value }); await expectRevertCustomError( - this.forwarder.execute(data.message, this.signature, { + this.forwarder.execute(data.message, { value: key == 'value' ? value : 0, // To avoid MismatchedValue error }), 'ERC2771ForwarderInvalidSigner', - [ethSigUtil.recoverTypedSignature({ data, sig: this.signature }), data.message.from], + [ethSigUtil.recoverTypedSignature({ data, sig: this.requestData.signature }), data.message.from], ); }); } it('reverts with tampered signature', async function () { - const tamperedSig = web3.utils.hexToBytes(this.signature); + const tamperedSig = web3.utils.hexToBytes(this.requestData.signature); tamperedSig[42] ^= 0xff; - await expectRevertCustomError( - this.forwarder.execute(this.request, web3.utils.bytesToHex(tamperedSig)), - 'ERC2771ForwarderInvalidSigner', - [ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }), this.request.from], - ); + this.requestData.signature = web3.utils.bytesToHex(tamperedSig); + await expectRevertCustomError(this.forwarder.execute(this.requestData), 'ERC2771ForwarderInvalidSigner', [ + ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }), + this.requestData.from, + ]); }); it('reverts with valid signature for non-current nonce', async function () { // Execute first a request - await this.forwarder.execute(this.request, this.signature); + await this.forwarder.execute(this.requestData); // And then fail due to an already used nonce - this.request.nonce++; - await expectRevertCustomError( - this.forwarder.execute(this.request, this.signature), - 'ERC2771ForwarderInvalidSigner', - [ - ethSigUtil.recoverTypedSignature({ data: this.forgeData(this.request), sig: this.signature }), - this.request.from, - ], - ); + this.requestData.nonce++; + await expectRevertCustomError(this.forwarder.execute(this.requestData), 'ERC2771ForwarderInvalidSigner', [ + ethSigUtil.recoverTypedSignature({ + data: this.forgeData(this.requestData), + sig: this.requestData.signature, + }), + this.requestData.from, + ]); }); it('reverts with valid signature for expired deadline', async function () { const req = { - ...this.request, + ...this.requestData, deadline: this.timestamp.toNumber() - 1, }; - const sig = this.sign(this.alice.getPrivateKey(), req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'ERC2771ForwarderExpiredRequest', [ + req.signature = this.sign(this.alice.getPrivateKey(), req); + await expectRevertCustomError(this.forwarder.execute(req), 'ERC2771ForwarderExpiredRequest', [ this.timestamp.toNumber() - 1, ]); }); @@ -196,24 +197,24 @@ contract('ERC2771Forwarder', function (accounts) { it('reverts with valid signature but mismatched value', async function () { const value = 100; const req = { - ...this.request, + ...this.requestData, value, }; - const sig = this.sign(this.alice.getPrivateKey(), req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'ERC2771ForwarderMismatchedValue', [0, value]); + req.signature = this.sign(this.alice.getPrivateKey(), req); + await expectRevertCustomError(this.forwarder.execute(req), 'ERC2771ForwarderMismatchedValue', [0, value]); }); }); it('bubbles out of gas', async function () { const receiver = await CallReceiverMock.new(); const gasAvailable = 100000; - this.request.to = receiver.address; - this.request.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); - this.request.gas = 1000000; + this.requestData.to = receiver.address; + this.requestData.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); + this.requestData.gas = 1000000; - await expectRevert.assertion( - this.forwarder.execute(this.request, this.sign(this.alice.getPrivateKey()), { gas: gasAvailable }), - ); + this.requestData.signature = this.sign(this.alice.getPrivateKey()); + + await expectRevert.assertion(this.forwarder.execute(this.requestData, { gas: gasAvailable })); const { transactions } = await web3.eth.getBlock('latest'); const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); @@ -232,38 +233,27 @@ contract('ERC2771Forwarder', function (accounts) { this.signers = [this.alice, this.bob, this.eve]; - this.requests = await Promise.all( + this.requestDatas = await Promise.all( this.signers.map(async ({ address }) => ({ - ...this.request, + ...this.requestData, from: address, - nonce: Number(await this.forwarder.nonces(address)), + nonce: (await this.forwarder.nonces(address)).toString(), })), ); - this.signatures = this.signers.map((signer, i) => this.sign(signer.getPrivateKey(), this.requests[i])); - }); - - it('reverts with mismatched lengths', async function () { - await expectRevertCustomError( - this.forwarder.executeBatch(this.requests, this.signatures.slice(0, -1), false), - 'ERC2771ForwarderInvalidBatchLength', - [this.requests.length, this.signatures.length - 1], - ); - - await expectRevertCustomError( - this.forwarder.executeBatch(this.requests.slice(0, -1), this.signatures, false), - 'ERC2771ForwarderInvalidBatchLength', - [this.requests.length - 1, this.signatures.length], - ); + this.requestDatas = this.requestDatas.map((requestData, i) => ({ + ...requestData, + signature: this.sign(this.signers[i].getPrivateKey(), requestData), + })); }); context('with valid signatures', function () { beforeEach(async function () { - this.receipt = await this.forwarder.executeBatch(this.requests, this.signatures, false); + this.receipt = await this.forwarder.executeBatch(this.requestDatas, false); }); it('emits events', async function () { - for (const request of this.requests) { + for (const request of this.requestDatas) { expectEvent(this.receipt, 'ExecutedForwardRequest', { signer: request.from, nonce: web3.utils.toBN(request.nonce), @@ -274,7 +264,7 @@ contract('ERC2771Forwarder', function (accounts) { }); it('increase nonces', async function () { - for (const request of this.requests) { + for (const request of this.requestDatas) { expect(await this.forwarder.nonces(request.from)).to.be.bignumber.eq(web3.utils.toBN(request.nonce + 1)); } }); @@ -287,51 +277,57 @@ contract('ERC2771Forwarder', function (accounts) { for (const [key, value] of Object.entries(tamperedValues)) { it(`reverts with at least one tampered request ${key}`, async function () { - const data = this.forgeData({ ...this.requests[this.idx], [key]: value }); + const data = this.forgeData({ ...this.requestDatas[this.idx], [key]: value }); - this.requests[this.idx] = data.message; + this.requestDatas[this.idx] = data.message; await expectRevertCustomError( - this.forwarder.executeBatch(this.requests, this.signatures, true), + this.forwarder.executeBatch(this.requestDatas, true), 'ERC2771ForwarderInvalidSigner', - [ethSigUtil.recoverTypedSignature({ data, sig: this.signatures[this.idx] }), data.message.from], + [ethSigUtil.recoverTypedSignature({ data, sig: this.requestDatas[this.idx].signature }), data.message.from], ); }); } it('reverts with at least one tampered request signature', async function () { - const tamperedSig = web3.utils.hexToBytes(this.signatures[this.idx]); + const tamperedSig = web3.utils.hexToBytes(this.requestDatas[this.idx].signature); tamperedSig[42] ^= 0xff; - this.signatures[this.idx] = web3.utils.bytesToHex(tamperedSig); + this.requestDatas[this.idx].signature = web3.utils.bytesToHex(tamperedSig); await expectRevertCustomError( - this.forwarder.executeBatch(this.requests, this.signatures, true), + this.forwarder.executeBatch(this.requestDatas, true), 'ERC2771ForwarderInvalidSigner', [ ethSigUtil.recoverTypedSignature({ - data: this.forgeData(this.requests[this.idx]), - sig: this.signatures[this.idx], + data: this.forgeData(this.requestDatas[this.idx]), + sig: this.requestDatas[this.idx].signature, }), - this.requests[this.idx].from, + this.requestDatas[this.idx].from, ], ); }); - it('succeeds ignoring a request with an invalid nonceZZZ', async function () { - this.requests[this.idx].nonce = this.requests[this.idx].nonce + 1; - this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); + it('succeeds ignoring a request with an invalid nonce', async function () { + this.requestDatas[this.idx].nonce = this.requestDatas[this.idx].nonce + 1; + this.requestDatas[this.idx].signature = this.sign( + this.signers[this.idx].getPrivateKey(), + this.requestDatas[this.idx], + ); - const receipt = await this.forwarder.executeBatch(this.requests, this.signatures, false); + const receipt = await this.forwarder.executeBatch(this.requestDatas, false); expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); }); it('reverts with at least one valid signature for expired deadline', async function () { - this.requests[this.idx].deadline = this.timestamp.toNumber() - 1; - this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); + this.requestDatas[this.idx].deadline = this.timestamp.toNumber() - 1; + this.requestDatas[this.idx].signature = this.sign( + this.signers[this.idx].getPrivateKey(), + this.requestDatas[this.idx], + ); await expectRevertCustomError( - this.forwarder.executeBatch(this.requests, this.signatures, true), + this.forwarder.executeBatch(this.requestDatas, true), 'ERC2771ForwarderExpiredRequest', [this.timestamp.toNumber() - 1], ); @@ -339,10 +335,13 @@ contract('ERC2771Forwarder', function (accounts) { it('reverts with at least one valid signature but mismatched value', async function () { const value = 100; - this.requests[this.idx].value = value; - this.signatures[this.idx] = this.sign(this.signers[this.idx].getPrivateKey(), this.requests[this.idx]); + this.requestDatas[this.idx].value = value; + this.requestDatas[this.idx].signature = this.sign( + this.signers[this.idx].getPrivateKey(), + this.requestDatas[this.idx], + ); await expectRevertCustomError( - this.forwarder.executeBatch(this.requests, this.signatures, true), + this.forwarder.executeBatch(this.requestDatas, true), 'ERC2771ForwarderMismatchedValue', [0, value], ); @@ -353,15 +352,13 @@ contract('ERC2771Forwarder', function (accounts) { const receiver = await CallReceiverMock.new(); const gasAvailable = 100000; const idx = 0; - this.requests[idx].to = receiver.address; - this.requests[idx].data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); - this.requests[idx].gas = 1000000; + this.requestDatas[idx].to = receiver.address; + this.requestDatas[idx].data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); + this.requestDatas[idx].gas = 1000000; - this.signatures[idx] = this.sign(this.signers[idx].getPrivateKey(), this.requests[idx]); + this.requestDatas[idx].signature = this.sign(this.signers[idx].getPrivateKey(), this.requestDatas[idx]); - await expectRevert.assertion( - this.forwarder.executeBatch(this.requests, this.signatures, true, { gas: gasAvailable }), - ); + await expectRevert.assertion(this.forwarder.executeBatch(this.requestDatas, true, { gas: gasAvailable })); const { transactions } = await web3.eth.getBlock('latest'); const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); From 376f3b2a6a998e57c65638ad6ed8cfee330e5494 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 23 Jun 2023 20:33:14 -0600 Subject: [PATCH 16/34] Fix ERC2771Context tests --- test/metatx/ERC2771Context.test.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index f0a87692b8f..3dd4b41532d 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -71,10 +71,12 @@ contract('ERC2771Context', function (accounts) { data, }; - const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - expect(await this.forwarder.verify(req, sign)).to.equal(true); + req.signature = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { + data: { ...this.data, message: req }, + }); + expect(await this.forwarder.verify(req)).to.equal(true); - const { tx } = await this.forwarder.execute(req, sign); + const { tx } = await this.forwarder.execute(req); await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Sender', { sender: this.sender }); }); }); @@ -95,10 +97,12 @@ contract('ERC2771Context', function (accounts) { data, }; - const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - expect(await this.forwarder.verify(req, sign)).to.equal(true); + req.signature = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { + data: { ...this.data, message: req }, + }); + expect(await this.forwarder.verify(req)).to.equal(true); - const { tx } = await this.forwarder.execute(req, sign); + const { tx } = await this.forwarder.execute(req); await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Data', { data, integerValue, stringValue }); }); }); From b94a10051d0e17cee6fbad99dd49993518c4c862 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 26 Jun 2023 18:28:13 -0600 Subject: [PATCH 17/34] Improve comments in _checkForwardedGas --- contracts/metatx/ERC2771Forwarder.sol | 35 +++++++++++++++------------ 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index fc45de11fc1..c11e1b75309 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -40,8 +40,10 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev Emitted when a `ForwardRequest` is executed. + * + * NOTE: */ - event ExecutedForwardRequest(address indexed signer, uint256 nonce, bool success, bytes returndata); + event ExecutedForwardRequest(address indexed signer, uint256 nonce, bool success); /** * @dev The request `from` doesn't match with the recovered `signer`. @@ -53,11 +55,6 @@ contract ERC2771Forwarder is EIP712, Nonces { */ error ERC2771ForwarderMismatchedValue(uint256 value, uint256 msgValue); - /** - * @dev The list of requests length doesn't match with the list of signatures length. - */ - error ERC2771ForwarderInvalidBatchLength(uint256 requestsLength, uint256 signaturesLength); - /** * @dev The request `deadline` has expired. */ @@ -212,19 +209,27 @@ contract ERC2771Forwarder is EIP712, Nonces { */ function _checkForwardedGas(ForwardRequestData 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: + // the define this model and adding a check: + // + // Let X be the gas available before the subcall, such that the subcall gets X * 63 / 64. + // We can't know X after CALL dynamic costs, but we want it to be such that X * 63 / 64 >= req.gas. + // Let Y be the gas used in the subcall gasleft() measured immediately after the subcall will be gasleft() = X - Y. + // If the subcall ran out of gas, then Y = X * 63 / 64 and gasleft() = X - Y = X / 64. + // Then we restrict the model by checking if req.gas / 63 > gasleft(), which is true is true if and only if + // req.gas / 63 > X / 64, or equivalently req.gas > X * 63 / 64. // - // - 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 + // This means that if the subcall runs out of gas we are able to detect that insufficient gas was passed. + // We will now also see that req.gas / 63 > gasleft() implies that req.gas >= X * 63 / 64. + // The contract guarantees Y <= req.gas, thus gasleft() = X - Y >= X - req.gas. + // - req.gas / 63 > gasleft() + // - req.gas / 63 >= X - req.gas + // - req.gas >= X * 63 / 64 // - // 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: + // In other words if req.gas < X * 63 / 64 then req.gas / 63 <= gasleft(), thus if the relayer behaves honestly + // the relay does not revert. 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 From 85acfdf562303d43ca1fb4923a0bb1d201685ee4 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 26 Jun 2023 19:39:47 -0600 Subject: [PATCH 18/34] Remove returndata --- contracts/metatx/ERC2771Forwarder.sol | 52 ++++++++++++++++++--------- test/metatx/ERC2771Forwarder.test.js | 2 -- 2 files changed, 35 insertions(+), 19 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index c11e1b75309..7a0f9ac42b9 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -41,7 +41,9 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev Emitted when a `ForwardRequest` is executed. * - * NOTE: + * NOTE: A non sucessful forwarded request should not be assumed as non out of gas exception because of + * {_checkForwardedGas}. Such function doesn't guarantee an out of gas exception won't be thrown, but instead + * it guarantees a relayer provided enough gas to cover the signer requested gas. */ event ExecutedForwardRequest(address indexed signer, uint256 nonce, bool success); @@ -68,9 +70,12 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev Returns `true` if a request is valid for a provided `signature` at the current block. * - * NOTE: A request signed with an invalid nonce will return false here but won't revert by default - * to prevent revert when a batch includes a request already executed by another relay. This behavior can be opt-out. - * See {executeBatch}. + * A transaction is considered valid when it hasn't expired (deadline is not met), and the signer + * matches the `from` parameter of the signed request. + * + * NOTE: A request may return false here but it optionally reverts in {batchExecute} to prevent + * reverts when a batch includes a request that was already frontrunned by another relay. This + * behavior is opt-in by using a flag in {executeBatch}. */ function verify(ForwardRequestData calldata request) public view virtual returns (bool) { (bool alive, bool signerMatch, ) = _validate(request, nonces(request.from)); @@ -81,26 +86,33 @@ contract ERC2771Forwarder is EIP712, Nonces { * @dev Executes a `request` on behalf of `signature`'s signer guaranteeing that the forwarded call * will receive the requested gas and no ETH is left stuck in the contract. */ - function execute(ForwardRequestData calldata request) public payable virtual returns (bool, bytes memory) { + function execute(ForwardRequestData calldata request) public payable virtual returns (bool) { + // This check can be before the call because _execute reverts with an invalid request. if (msg.value != request.value) { revert ERC2771ForwarderMismatchedValue(request.value, msg.value); } - (bool success, bytes memory returndata) = _execute(request, _useNonce(request.from), true); + (bool success, ) = _execute(request, _useNonce(request.from), true); - return (success, returndata); + return success; } /** * @dev Batch version of {execute} that also includes an `atomic` parameter to indicate whether all requests are - * executed or none of them. + * executed or the whole batch reverts if a single one of them is not a valid request as defined by {verify}. */ function executeBatch(ForwardRequestData[] calldata requests, bool atomic) public payable virtual { uint256 requestsValue; for (uint256 i; i < requests.length; ++i) { - requestsValue += requests[i].value; - _execute(requests[i], _useNonce(requests[i].from), atomic); + (, bool executed) = _execute(requests[i], _useNonce(requests[i].from), atomic); + if (executed) { + // Will never reallistically overflow given _execute will natively revert at + // some point due to insufficient balance. + unchecked { + requestsValue += requests[i].value; + } + } } if (msg.value != requestsValue) { @@ -109,7 +121,8 @@ contract ERC2771Forwarder is EIP712, Nonces { } /** - * @dev Validates if the provided request can be executed at current block with `signature` on behalf of `signer`. + * @dev Validates if the provided request can be executed at current block with `request.signature` + * on behalf of `request.signer`. * * Internal function without nonce validation. */ @@ -149,7 +162,11 @@ contract ERC2771Forwarder is EIP712, Nonces { } /** - * @dev Validates and executes a signed request. + * @dev Validates and executes a signed request returning a tuple with the request call `success` + * value and an boolean indicating if the request was `executed`. + * + * The `executed` boolean can be assumed as always true if the call doesn't revert and + * `requireValidRequest` is also true (valid as defined in {verify}). * * Internal function without nonce and msg.value validation. * @@ -168,7 +185,7 @@ contract ERC2771Forwarder is EIP712, Nonces { ForwardRequestData calldata request, uint256 nonce, bool requireValidRequest - ) internal virtual returns (bool success, bytes memory returndata) { + ) internal virtual returns (bool success, bool executed) { (bool alive, bool signerMatch, address signer) = _validate(request, nonce); // Need to explicitly specify if a revert is required since non-reverting is default for @@ -185,13 +202,14 @@ contract ERC2771Forwarder is EIP712, Nonces { // Avoid execution instead of reverting in case a batch includes an already executed request if (signerMatch && alive) { - (success, returndata) = request.to.call{gas: request.gas, value: request.value}( + executed = true; + (success, ) = request.to.call{gas: request.gas, value: request.value}( abi.encodePacked(request.data, request.from) ); _checkForwardedGas(request); - emit ExecutedForwardRequest(signer, nonce, success, returndata); + emit ExecutedForwardRequest(signer, nonce, success); } } @@ -218,7 +236,7 @@ contract ERC2771Forwarder is EIP712, Nonces { // We can't know X after CALL dynamic costs, but we want it to be such that X * 63 / 64 >= req.gas. // Let Y be the gas used in the subcall gasleft() measured immediately after the subcall will be gasleft() = X - Y. // If the subcall ran out of gas, then Y = X * 63 / 64 and gasleft() = X - Y = X / 64. - // Then we restrict the model by checking if req.gas / 63 > gasleft(), which is true is true if and only if + // Then we restrict the model by checking if req.gas / 63 > gasleft(), which is true is true if and only if // req.gas / 63 > X / 64, or equivalently req.gas > X * 63 / 64. // // This means that if the subcall runs out of gas we are able to detect that insufficient gas was passed. @@ -228,7 +246,7 @@ contract ERC2771Forwarder is EIP712, Nonces { // - req.gas / 63 >= X - req.gas // - 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 + // In other words if req.gas < X * 63 / 64 then req.gas / 63 <= gasleft(), thus if the relayer behaves honestly // the relay does not revert. if (gasleft() < request.gas / 63) { // We explicitly trigger invalid opcode to consume all gas and bubble-up the effects, since diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index 1cdeb819280..4f755b64fc1 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -133,7 +133,6 @@ contract('ERC2771Forwarder', function (accounts) { signer: this.requestData.from, nonce: web3.utils.toBN(this.requestData.nonce), success: true, - returndata: null, }); }); @@ -258,7 +257,6 @@ contract('ERC2771Forwarder', function (accounts) { signer: request.from, nonce: web3.utils.toBN(request.nonce), success: true, - returndata: null, }); } }); From 74d596155ece4c94c1f67186c86c48362c76d2ad Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 26 Jun 2023 21:13:03 -0600 Subject: [PATCH 19/34] Fix ETH left --- contracts/metatx/ERC2771Forwarder.sol | 53 +++++++++++++++++---------- test/metatx/ERC2771Forwarder.test.js | 17 +++++---- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 7a0f9ac42b9..bfce789b812 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -6,6 +6,7 @@ pragma solidity ^0.8.19; import "../utils/cryptography/ECDSA.sol"; import "../utils/cryptography/EIP712.sol"; import "../utils/Nonces.sol"; +import "../utils/Address.sol"; /** * @dev An implementation of a production-ready forwarder compatible with ERC2771 contracts. @@ -53,7 +54,7 @@ contract ERC2771Forwarder is EIP712, Nonces { error ERC2771ForwarderInvalidSigner(address signer, address from); /** - * @dev The requested `value` doesn't match with the available `msgValue`, leaving ETH stuck in the contract. + * @dev The requested `value` doesn't match with the available `msgValue`. */ error ERC2771ForwarderMismatchedValue(uint256 value, uint256 msgValue); @@ -92,31 +93,50 @@ contract ERC2771Forwarder is EIP712, Nonces { revert ERC2771ForwarderMismatchedValue(request.value, msg.value); } - (bool success, ) = _execute(request, _useNonce(request.from), true); - - return success; + return _execute(request, _useNonce(request.from), true); } /** - * @dev Batch version of {execute} that also includes an `atomic` parameter to indicate whether all requests are - * executed or the whole batch reverts if a single one of them is not a valid request as defined by {verify}. + * @dev Batch version of {execute} with optional atomic behavior and refunding. + * + * The `atomic` parameter indicates whether all requests are executed or the whole + * batch reverts if a single one of them is not a valid request as defined by {verify}. + * + * The `refundReceiver` is used for optionally send the funds back to a selected address in + * case there's ETH left in the contract at the end of the execution instead of reverting. If + * the provided refund receiver is the `address(0)`, the contract will revert with an + * {ERC2771ForwarderMismatchedValue} error. + * + * NOTE: A refund receiver will get all contract balance back. */ - function executeBatch(ForwardRequestData[] calldata requests, bool atomic) public payable virtual { + function executeBatch( + ForwardRequestData[] calldata requests, + bool atomic, + address payable refundReceiver + ) public payable virtual { uint256 requestsValue; + uint256 spentValue; for (uint256 i; i < requests.length; ++i) { - (, bool executed) = _execute(requests[i], _useNonce(requests[i].from), atomic); - if (executed) { + requestsValue += requests[i].value; + bool success = _execute(requests[i], _useNonce(requests[i].from), atomic); + if (success) { // Will never reallistically overflow given _execute will natively revert at // some point due to insufficient balance. unchecked { - requestsValue += requests[i].value; + spentValue += requests[i].value; } } } - if (msg.value != requestsValue) { - revert ERC2771ForwarderMismatchedValue(requestsValue, msg.value); + if (requestsValue != spentValue) { + if (refundReceiver == address(0)) { + revert ERC2771ForwarderMismatchedValue(requestsValue, msg.value); + } + + // To avoid unexpected reverts because a request was frontrunned leaving ETH in the contract + // the value is sent back instead of reverting. + Address.sendValue(refundReceiver, requestsValue - spentValue); } } @@ -162,11 +182,7 @@ contract ERC2771Forwarder is EIP712, Nonces { } /** - * @dev Validates and executes a signed request returning a tuple with the request call `success` - * value and an boolean indicating if the request was `executed`. - * - * The `executed` boolean can be assumed as always true if the call doesn't revert and - * `requireValidRequest` is also true (valid as defined in {verify}). + * @dev Validates and executes a signed request returning the request call `success` value. * * Internal function without nonce and msg.value validation. * @@ -185,7 +201,7 @@ contract ERC2771Forwarder is EIP712, Nonces { ForwardRequestData calldata request, uint256 nonce, bool requireValidRequest - ) internal virtual returns (bool success, bool executed) { + ) internal virtual returns (bool success) { (bool alive, bool signerMatch, address signer) = _validate(request, nonce); // Need to explicitly specify if a revert is required since non-reverting is default for @@ -202,7 +218,6 @@ contract ERC2771Forwarder is EIP712, Nonces { // Avoid execution instead of reverting in case a batch includes an already executed request if (signerMatch && alive) { - executed = true; (success, ) = request.to.call{gas: request.gas, value: request.value}( abi.encodePacked(request.data, request.from) ); diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index 4f755b64fc1..b111471b67a 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -5,6 +5,7 @@ const { expectRevertCustomError } = require('../helpers/customError'); const { constants, expectRevert, expectEvent, time } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); +const { ZERO_ADDRESS } = require('@openzeppelin/test-helpers/src/constants'); const ERC2771Forwarder = artifacts.require('ERC2771Forwarder'); const CallReceiverMock = artifacts.require('CallReceiverMock'); @@ -248,7 +249,7 @@ contract('ERC2771Forwarder', function (accounts) { context('with valid signatures', function () { beforeEach(async function () { - this.receipt = await this.forwarder.executeBatch(this.requestDatas, false); + this.receipt = await this.forwarder.executeBatch(this.requestDatas, false, ZERO_ADDRESS); }); it('emits events', async function () { @@ -280,7 +281,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[this.idx] = data.message; await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, true), + this.forwarder.executeBatch(this.requestDatas, true, ZERO_ADDRESS), 'ERC2771ForwarderInvalidSigner', [ethSigUtil.recoverTypedSignature({ data, sig: this.requestDatas[this.idx].signature }), data.message.from], ); @@ -294,7 +295,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[this.idx].signature = web3.utils.bytesToHex(tamperedSig); await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, true), + this.forwarder.executeBatch(this.requestDatas, true, ZERO_ADDRESS), 'ERC2771ForwarderInvalidSigner', [ ethSigUtil.recoverTypedSignature({ @@ -313,7 +314,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[this.idx], ); - const receipt = await this.forwarder.executeBatch(this.requestDatas, false); + const receipt = await this.forwarder.executeBatch(this.requestDatas, false, ZERO_ADDRESS); expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); }); @@ -325,7 +326,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[this.idx], ); await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, true), + this.forwarder.executeBatch(this.requestDatas, true, ZERO_ADDRESS), 'ERC2771ForwarderExpiredRequest', [this.timestamp.toNumber() - 1], ); @@ -339,7 +340,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[this.idx], ); await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, true), + this.forwarder.executeBatch(this.requestDatas, true, ZERO_ADDRESS), 'ERC2771ForwarderMismatchedValue', [0, value], ); @@ -356,7 +357,9 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[idx].signature = this.sign(this.signers[idx].getPrivateKey(), this.requestDatas[idx]); - await expectRevert.assertion(this.forwarder.executeBatch(this.requestDatas, true, { gas: gasAvailable })); + await expectRevert.assertion( + this.forwarder.executeBatch(this.requestDatas, true, ZERO_ADDRESS, { gas: gasAvailable }), + ); const { transactions } = await web3.eth.getBlock('latest'); const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); From c62d92756dafb92010c9c8d729ff93f916afe91f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 26 Jun 2023 21:43:46 -0600 Subject: [PATCH 20/34] Changed note --- contracts/metatx/ERC2771Forwarder.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index bfce789b812..931b1dd55a9 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -107,7 +107,9 @@ contract ERC2771Forwarder is EIP712, Nonces { * the provided refund receiver is the `address(0)`, the contract will revert with an * {ERC2771ForwarderMismatchedValue} error. * - * NOTE: A refund receiver will get all contract balance back. + * NOTE: The `atomic` flag guarantees an all-or-nothing behavior only for the first level forwarded + * calls. In case a call is forwarded to another contract, it may revert without the top-level call + * reverting. */ function executeBatch( ForwardRequestData[] calldata requests, From daafff8fb61383f6e46273a069d5e3d3f1c8613c Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 26 Jun 2023 21:46:48 -0600 Subject: [PATCH 21/34] Fix codespell --- contracts/metatx/ERC2771Forwarder.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 931b1dd55a9..102035bebd5 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -42,7 +42,7 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev Emitted when a `ForwardRequest` is executed. * - * NOTE: A non sucessful forwarded request should not be assumed as non out of gas exception because of + * NOTE: A non successful forwarded request should not be assumed as non out of gas exception because of * {_checkForwardedGas}. Such function doesn't guarantee an out of gas exception won't be thrown, but instead * it guarantees a relayer provided enough gas to cover the signer requested gas. */ From f3d5b44f08d0d1a18a48d6d4fd0917556e178596 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 27 Jun 2023 14:55:19 -0600 Subject: [PATCH 22/34] Fix ETH left in the contract --- contracts/metatx/ERC2771Forwarder.sol | 74 ++++++++++++++++----------- test/metatx/ERC2771Forwarder.test.js | 15 +++--- 2 files changed, 51 insertions(+), 38 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 102035bebd5..7aa85801c78 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -74,9 +74,8 @@ contract ERC2771Forwarder is EIP712, Nonces { * A transaction is considered valid when it hasn't expired (deadline is not met), and the signer * matches the `from` parameter of the signed request. * - * NOTE: A request may return false here but it optionally reverts in {batchExecute} to prevent - * reverts when a batch includes a request that was already frontrunned by another relay. This - * behavior is opt-in by using a flag in {executeBatch}. + * NOTE: A request may return false here but it won't revert in {batchExecute} if a refund receiver + * is provided. */ function verify(ForwardRequestData calldata request) public view virtual returns (bool) { (bool alive, bool signerMatch, ) = _validate(request, nonces(request.from)); @@ -86,9 +85,14 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev Executes a `request` on behalf of `signature`'s signer guaranteeing that the forwarded call * will receive the requested gas and no ETH is left stuck in the contract. + * + * Requirements: + * + * - The request value should be equal to the provided `msg.value` */ function execute(ForwardRequestData calldata request) public payable virtual returns (bool) { // This check can be before the call because _execute reverts with an invalid request. + // Otherwise, `msg.value` will be left in the contract's balance. if (msg.value != request.value) { revert ERC2771ForwarderMismatchedValue(request.value, msg.value); } @@ -97,48 +101,58 @@ contract ERC2771Forwarder is EIP712, Nonces { } /** - * @dev Batch version of {execute} with optional atomic behavior and refunding. + * @dev Batch version of {execute} with optional refunding and atomic execution. * - * The `atomic` parameter indicates whether all requests are executed or the whole - * batch reverts if a single one of them is not a valid request as defined by {verify}. + * In case a batch contains at least one invalid request (see {verify}), the + * request will be skipped and the `refundReceiver` parameter will receive back the + * unused requested value at the end of the execution. This is done to prevent unexpected + * reverts when a batch includes a request that was already frontrunned by another relayer. * - * The `refundReceiver` is used for optionally send the funds back to a selected address in - * case there's ETH left in the contract at the end of the execution instead of reverting. If - * the provided refund receiver is the `address(0)`, the contract will revert with an - * {ERC2771ForwarderMismatchedValue} error. + * If the `refundReceiver` is the `address(0)`, this function will revert when at least + * one of the requests was not valid instead of skipping it. This could be useful if + * a batch is required to get executed atomically (at least at the top-level). For example, + * refunding (and thus atomicity) can be opt-out if the relayer is using a service that avoids + * including reverted transactions. + * + * Requirements: * - * NOTE: The `atomic` flag guarantees an all-or-nothing behavior only for the first level forwarded - * calls. In case a call is forwarded to another contract, it may revert without the top-level call - * reverting. + * - The sum of the requests' values should be equal to the provided `msg.value` + * - All of the request should be valid (see {verify}) when `refundReceiver` is the zero address. + * + * NOTE: Setting a zero `refundReceiver` guarantees an all-or-nothing requests execution only for + * the first-level forwarded calls. In case a forwarded request calls to a contract with another + * subcall, the second-level call may revert without the top-level call reverting. */ function executeBatch( ForwardRequestData[] calldata requests, - bool atomic, address payable refundReceiver ) public payable virtual { + bool atomic = refundReceiver == address(0); + uint256 requestsValue; - uint256 spentValue; + uint256 refundValue; for (uint256 i; i < requests.length; ++i) { requestsValue += requests[i].value; bool success = _execute(requests[i], _useNonce(requests[i].from), atomic); - if (success) { - // Will never reallistically overflow given _execute will natively revert at - // some point due to insufficient balance. - unchecked { - spentValue += requests[i].value; - } + if (!success) { + refundValue += requests[i].value; } } - if (requestsValue != spentValue) { - if (refundReceiver == address(0)) { - revert ERC2771ForwarderMismatchedValue(requestsValue, msg.value); - } + // The batch should still revert if there's a mismatched msg.value provided + // to avoid request value tampering + if (requestsValue != msg.value) { + revert ERC2771ForwarderMismatchedValue(requestsValue, msg.value); + } - // To avoid unexpected reverts because a request was frontrunned leaving ETH in the contract - // the value is sent back instead of reverting. - Address.sendValue(refundReceiver, requestsValue - spentValue); + // To avoid unexpected reverts because a request was frontrunned leaving ETH in the contract + // the value is sent back instead of reverting. + if (refundValue != 0) { + // We know `refundReceiver != address(0) && requestsValue == msg.value` + // meaning we can ensure refundValue is not taken from the original contract's balance + // and refundReceiver is a known account. + Address.sendValue(refundReceiver, refundValue); } } @@ -196,8 +210,8 @@ contract ERC2771Forwarder is EIP712, Nonces { * * 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. + * IMPORTANT: Using this function doesn't check that all the `msg.value` was sent, potentially + * leaving ETH stuck in the contract. */ function _execute( ForwardRequestData calldata request, diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index b111471b67a..bdd67588d01 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -5,7 +5,6 @@ const { expectRevertCustomError } = require('../helpers/customError'); const { constants, expectRevert, expectEvent, time } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const { ZERO_ADDRESS } = require('@openzeppelin/test-helpers/src/constants'); const ERC2771Forwarder = artifacts.require('ERC2771Forwarder'); const CallReceiverMock = artifacts.require('CallReceiverMock'); @@ -249,7 +248,7 @@ contract('ERC2771Forwarder', function (accounts) { context('with valid signatures', function () { beforeEach(async function () { - this.receipt = await this.forwarder.executeBatch(this.requestDatas, false, ZERO_ADDRESS); + this.receipt = await this.forwarder.executeBatch(this.requestDatas, accounts[0]); }); it('emits events', async function () { @@ -281,7 +280,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[this.idx] = data.message; await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, true, ZERO_ADDRESS), + this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS), 'ERC2771ForwarderInvalidSigner', [ethSigUtil.recoverTypedSignature({ data, sig: this.requestDatas[this.idx].signature }), data.message.from], ); @@ -295,7 +294,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[this.idx].signature = web3.utils.bytesToHex(tamperedSig); await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, true, ZERO_ADDRESS), + this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS), 'ERC2771ForwarderInvalidSigner', [ ethSigUtil.recoverTypedSignature({ @@ -314,7 +313,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[this.idx], ); - const receipt = await this.forwarder.executeBatch(this.requestDatas, false, ZERO_ADDRESS); + const receipt = await this.forwarder.executeBatch(this.requestDatas, accounts[0]); expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); }); @@ -326,7 +325,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[this.idx], ); await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, true, ZERO_ADDRESS), + this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS), 'ERC2771ForwarderExpiredRequest', [this.timestamp.toNumber() - 1], ); @@ -340,7 +339,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[this.idx], ); await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, true, ZERO_ADDRESS), + this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS), 'ERC2771ForwarderMismatchedValue', [0, value], ); @@ -358,7 +357,7 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[idx].signature = this.sign(this.signers[idx].getPrivateKey(), this.requestDatas[idx]); await expectRevert.assertion( - this.forwarder.executeBatch(this.requestDatas, true, ZERO_ADDRESS, { gas: gasAvailable }), + this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS, { gas: gasAvailable }), ); const { transactions } = await web3.eth.getBlock('latest'); From cb8690e4167342c017fb72c7c655ae05834b5a34 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 27 Jun 2023 15:21:31 -0600 Subject: [PATCH 23/34] Fix nonces --- contracts/metatx/ERC2771Forwarder.sol | 36 +++++++++++---------------- test/metatx/ERC2771Forwarder.test.js | 6 ++++- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 7aa85801c78..1afc5b5aa0d 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -78,7 +78,7 @@ contract ERC2771Forwarder is EIP712, Nonces { * is provided. */ function verify(ForwardRequestData calldata request) public view virtual returns (bool) { - (bool alive, bool signerMatch, ) = _validate(request, nonces(request.from)); + (bool alive, bool signerMatch, ) = _validate(request); return alive && signerMatch; } @@ -88,7 +88,8 @@ contract ERC2771Forwarder is EIP712, Nonces { * * Requirements: * - * - The request value should be equal to the provided `msg.value` + * - The request value should be equal to the provided `msg.value`. + * - The request should be valid according to {verify}. */ function execute(ForwardRequestData calldata request) public payable virtual returns (bool) { // This check can be before the call because _execute reverts with an invalid request. @@ -97,7 +98,7 @@ contract ERC2771Forwarder is EIP712, Nonces { revert ERC2771ForwarderMismatchedValue(request.value, msg.value); } - return _execute(request, _useNonce(request.from), true); + return _execute(request, true); } /** @@ -117,7 +118,7 @@ contract ERC2771Forwarder is EIP712, Nonces { * Requirements: * * - The sum of the requests' values should be equal to the provided `msg.value` - * - All of the request should be valid (see {verify}) when `refundReceiver` is the zero address. + * - All of the requests should be valid (see {verify}) when `refundReceiver` is the zero address. * * NOTE: Setting a zero `refundReceiver` guarantees an all-or-nothing requests execution only for * the first-level forwarded calls. In case a forwarded request calls to a contract with another @@ -134,7 +135,7 @@ contract ERC2771Forwarder is EIP712, Nonces { for (uint256 i; i < requests.length; ++i) { requestsValue += requests[i].value; - bool success = _execute(requests[i], _useNonce(requests[i].from), atomic); + bool success = _execute(requests[i], atomic); if (!success) { refundValue += requests[i].value; } @@ -149,7 +150,7 @@ contract ERC2771Forwarder is EIP712, Nonces { // To avoid unexpected reverts because a request was frontrunned leaving ETH in the contract // the value is sent back instead of reverting. if (refundValue != 0) { - // We know `refundReceiver != address(0) && requestsValue == msg.value` + // We know refundReceiver != address(0) && requestsValue == msg.value // meaning we can ensure refundValue is not taken from the original contract's balance // and refundReceiver is a known account. Address.sendValue(refundReceiver, refundValue); @@ -157,29 +158,21 @@ contract ERC2771Forwarder is EIP712, Nonces { } /** - * @dev Validates if the provided request can be executed at current block with `request.signature` + * @dev Validates if the provided request can be executed at current timestamp with `request.signature` * on behalf of `request.signer`. - * - * Internal function without nonce validation. */ function _validate( - ForwardRequestData calldata request, - uint256 nonce + ForwardRequestData calldata request ) internal view virtual returns (bool alive, bool signerMatch, address signer) { - signer = _recoverForwardRequestSigner(request, nonce); + signer = _recoverForwardRequestSigner(request); return (request.deadline >= block.timestamp, signer == request.from, signer); } /** * @dev Recovers the signer of an EIP712 message hash for a forward `request` and its corresponding `signature`. * See {ECDSA-recover}. - * - * Internal function without nonce validation. */ - function _recoverForwardRequestSigner( - ForwardRequestData calldata request, - uint256 nonce - ) internal view virtual returns (address) { + function _recoverForwardRequestSigner(ForwardRequestData calldata request) internal view virtual returns (address) { return _hashTypedDataV4( keccak256( @@ -189,7 +182,7 @@ contract ERC2771Forwarder is EIP712, Nonces { request.to, request.value, request.gas, - nonce, + nonces(request.from), request.deadline, keccak256(request.data) ) @@ -215,10 +208,9 @@ contract ERC2771Forwarder is EIP712, Nonces { */ function _execute( ForwardRequestData calldata request, - uint256 nonce, bool requireValidRequest ) internal virtual returns (bool success) { - (bool alive, bool signerMatch, address signer) = _validate(request, nonce); + (bool alive, bool signerMatch, address signer) = _validate(request); // Need to explicitly specify if a revert is required since non-reverting is default for // batches and reversion is opt-in since it could be useful in some scenarios @@ -240,7 +232,7 @@ contract ERC2771Forwarder is EIP712, Nonces { _checkForwardedGas(request); - emit ExecutedForwardRequest(signer, nonce, success); + emit ExecutedForwardRequest(signer, _useNonce(signer), success); } } diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index bdd67588d01..d125c1ae379 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -307,7 +307,8 @@ contract('ERC2771Forwarder', function (accounts) { }); it('succeeds ignoring a request with an invalid nonce', async function () { - this.requestDatas[this.idx].nonce = this.requestDatas[this.idx].nonce + 1; + const correctNonce = await this.forwarder.nonces(this.requestDatas[this.idx].from); + this.requestDatas[this.idx].nonce = correctNonce + 1; this.requestDatas[this.idx].signature = this.sign( this.signers[this.idx].getPrivateKey(), this.requestDatas[this.idx], @@ -316,6 +317,9 @@ contract('ERC2771Forwarder', function (accounts) { const receipt = await this.forwarder.executeBatch(this.requestDatas, accounts[0]); expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); + expect(await this.forwarder.nonces(this.requestDatas[this.idx].from)).to.be.bignumber.eq( + web3.utils.toBN(correctNonce), + ); }); it('reverts with at least one valid signature for expired deadline', async function () { From 68ce4eb1f9acbc3a57f68ea74c3f3cf4f7ef706a Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 27 Jun 2023 15:52:56 -0600 Subject: [PATCH 24/34] Avoid reentrancy --- contracts/metatx/ERC2771Forwarder.sol | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 1afc5b5aa0d..b7610088b89 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -69,7 +69,7 @@ contract ERC2771Forwarder is EIP712, Nonces { constructor(string memory name) EIP712(name, "1") {} /** - * @dev Returns `true` if a request is valid for a provided `signature` at the current block. + * @dev Returns `true` if a request is valid for a provided `signature` at the current block timestamp. * * A transaction is considered valid when it hasn't expired (deadline is not met), and the signer * matches the `from` parameter of the signed request. @@ -117,7 +117,7 @@ contract ERC2771Forwarder is EIP712, Nonces { * * Requirements: * - * - The sum of the requests' values should be equal to the provided `msg.value` + * - The sum of the requests' values should be equal to the provided `msg.value`. * - All of the requests should be valid (see {verify}) when `refundReceiver` is the zero address. * * NOTE: Setting a zero `refundReceiver` guarantees an all-or-nothing requests execution only for @@ -158,8 +158,8 @@ contract ERC2771Forwarder is EIP712, Nonces { } /** - * @dev Validates if the provided request can be executed at current timestamp with `request.signature` - * on behalf of `request.signer`. + * @dev Validates if the provided request can be executed at current block timestamp with + * the given `request.signature` on behalf of `request.signer`. */ function _validate( ForwardRequestData calldata request @@ -193,13 +193,12 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev Validates and executes a signed request returning the request call `success` value. * - * Internal function without nonce and msg.value validation. + * Internal function without msg.value validation. * * Requirements: * - * - The request's deadline must have not passed. - * - The request's from must be the request's signer. * - The caller must have provided enough gas to forward with the call. + * - The request must be valid (see {verify}) if the `requireValidRequest` is true. * * Emits an {ExecutedForwardRequest} event. * @@ -226,13 +225,16 @@ contract ERC2771Forwarder is EIP712, Nonces { // Avoid execution instead of reverting in case a batch includes an already executed request if (signerMatch && alive) { + // Nonce should be used before the call to prevent reusing by reentrancy + uint256 currentNonce = _useNonce(signer); + (success, ) = request.to.call{gas: request.gas, value: request.value}( abi.encodePacked(request.data, request.from) ); _checkForwardedGas(request); - emit ExecutedForwardRequest(signer, _useNonce(signer), success); + emit ExecutedForwardRequest(signer, currentNonce, success); } } From 99a26c44149d6678122c9fceeeacaf4668f2ba49 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 27 Jun 2023 16:07:32 -0600 Subject: [PATCH 25/34] Apply suggestion --- contracts/metatx/ERC2771Forwarder.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index b7610088b89..5a2a8ac8a62 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -54,9 +54,9 @@ contract ERC2771Forwarder is EIP712, Nonces { error ERC2771ForwarderInvalidSigner(address signer, address from); /** - * @dev The requested `value` doesn't match with the available `msgValue`. + * @dev The `requestedValue` doesn't match with the available `msgValue`. */ - error ERC2771ForwarderMismatchedValue(uint256 value, uint256 msgValue); + error ERC2771ForwarderMismatchedValue(uint256 requestedValue, uint256 msgValue); /** * @dev The request `deadline` has expired. From be93a80acdf83be4e982bf87718e05490f9bd569 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 27 Jun 2023 16:20:38 -0600 Subject: [PATCH 26/34] Apply suggestion --- contracts/metatx/ERC2771Forwarder.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 5a2a8ac8a62..6f0a86431a3 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -92,8 +92,8 @@ contract ERC2771Forwarder is EIP712, Nonces { * - The request should be valid according to {verify}. */ function execute(ForwardRequestData calldata request) public payable virtual returns (bool) { - // This check can be before the call because _execute reverts with an invalid request. - // Otherwise, `msg.value` will be left in the contract's balance. + // We make sure that msg.value and request.value match exactly. + // If the request is invalid or the call reverts, requireValidRequest ensures this value won't be lost. if (msg.value != request.value) { revert ERC2771ForwarderMismatchedValue(request.value, msg.value); } From 5c039eac100fbdc92e4286b60abe204cc86c0994 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 27 Jun 2023 18:52:18 -0600 Subject: [PATCH 27/34] Improve tests --- test/metatx/ERC2771Forwarder.test.js | 229 ++++++++++++++++++--------- 1 file changed, 155 insertions(+), 74 deletions(-) diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index d125c1ae379..59a8c912467 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -10,10 +10,12 @@ const ERC2771Forwarder = artifacts.require('ERC2771Forwarder'); const CallReceiverMock = artifacts.require('CallReceiverMock'); contract('ERC2771Forwarder', function (accounts) { + const [, refundReceiver, another] = accounts; + const tamperedValues = { - from: accounts[0], - to: accounts[0], - value: web3.utils.toWei('1'), + from: another, + to: another, + value: web3.utils.toWei('0.5'), data: '0x1742', deadline: 0xdeadbeef, }; @@ -111,7 +113,7 @@ contract('ERC2771Forwarder', function (accounts) { it('returns false with valid signature for expired deadline', async function () { const req = { ...this.requestData, - nonce: this.requestData.nonce + 1, + deadline: this.timestamp - 1, }; req.signature = this.sign(this.alice.getPrivateKey(), req); expect(await this.forwarder.verify(req)).to.be.equal(false); @@ -120,7 +122,7 @@ contract('ERC2771Forwarder', function (accounts) { }); context('execute', function () { - context('with valid signature', function () { + context('with valid requests', function () { beforeEach(async function () { expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( web3.utils.toBN(this.requestData.nonce), @@ -143,7 +145,7 @@ contract('ERC2771Forwarder', function (accounts) { }); }); - context('with tampered values', function () { + context('with tampered request', function () { for (const [key, value] of Object.entries(tamperedValues)) { it(`reverts with tampered ${key}`, async function () { const data = this.forgeData({ [key]: value }); @@ -172,10 +174,9 @@ contract('ERC2771Forwarder', function (accounts) { await this.forwarder.execute(this.requestData); // And then fail due to an already used nonce - this.requestData.nonce++; await expectRevertCustomError(this.forwarder.execute(this.requestData), 'ERC2771ForwarderInvalidSigner', [ ethSigUtil.recoverTypedSignature({ - data: this.forgeData(this.requestData), + data: this.forgeData({ ...this.requestData, nonce: this.requestData.nonce + 1 }), sig: this.requestData.signature, }), this.requestData.from, @@ -185,11 +186,11 @@ contract('ERC2771Forwarder', function (accounts) { it('reverts with valid signature for expired deadline', async function () { const req = { ...this.requestData, - deadline: this.timestamp.toNumber() - 1, + deadline: this.timestamp - 1, }; req.signature = this.sign(this.alice.getPrivateKey(), req); await expectRevertCustomError(this.forwarder.execute(req), 'ERC2771ForwarderExpiredRequest', [ - this.timestamp.toNumber() - 1, + this.timestamp - 1, ]); }); @@ -223,6 +224,8 @@ contract('ERC2771Forwarder', function (accounts) { }); context('executeBatch', function () { + const batchValue = requestDatas => requestDatas.reduce((value, request) => value + Number(request.value), 0); + beforeEach(async function () { this.bob = Wallet.generate(); this.bob.address = web3.utils.toChecksumAddress(this.bob.getAddressString()); @@ -237,6 +240,7 @@ contract('ERC2771Forwarder', function (accounts) { ...this.requestData, from: address, nonce: (await this.forwarder.nonces(address)).toString(), + value: web3.utils.toWei('10', 'gwei'), })), ); @@ -244,11 +248,17 @@ contract('ERC2771Forwarder', function (accounts) { ...requestData, signature: this.sign(this.signers[i].getPrivateKey(), requestData), })); + + this.msgValue = batchValue(this.requestDatas); }); - context('with valid signatures', function () { + context('with valid requests', function () { beforeEach(async function () { - this.receipt = await this.forwarder.executeBatch(this.requestDatas, accounts[0]); + for (const request of this.requestDatas) { + expect(await this.forwarder.verify(request)).to.be.equal(true); + } + + this.receipt = await this.forwarder.executeBatch(this.requestDatas, another, { value: this.msgValue }); }); it('emits events', async function () { @@ -268,85 +278,153 @@ contract('ERC2771Forwarder', function (accounts) { }); }); - context('with tampered values', function () { + context('with tampered requests', function () { beforeEach(async function () { - this.idx = 1; + this.idx = 1; // Tampered idx }); - for (const [key, value] of Object.entries(tamperedValues)) { - it(`reverts with at least one tampered request ${key}`, async function () { - const data = this.forgeData({ ...this.requestDatas[this.idx], [key]: value }); + it('reverts with mismatched value', async function () { + this.requestDatas[this.idx].value = 100; + this.requestDatas[this.idx].signature = this.sign( + this.signers[this.idx].getPrivateKey(), + this.requestDatas[this.idx], + ); + await expectRevertCustomError( + this.forwarder.executeBatch(this.requestDatas, another, { value: this.msgValue }), + 'ERC2771ForwarderMismatchedValue', + [batchValue(this.requestDatas), this.msgValue], + ); + }); - this.requestDatas[this.idx] = data.message; + context('when the refund receiver is the zero address', function () { + beforeEach(function () { + this.refundReceiver = constants.ZERO_ADDRESS; + }); + + for (const [key, value] of Object.entries(tamperedValues)) { + it(`reverts with at least one tampered request ${key}`, async function () { + const data = this.forgeData({ ...this.requestDatas[this.idx], [key]: value }); + + this.requestDatas[this.idx] = data.message; + + await expectRevertCustomError( + this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), + 'ERC2771ForwarderInvalidSigner', + [ + ethSigUtil.recoverTypedSignature({ data, sig: this.requestDatas[this.idx].signature }), + data.message.from, + ], + ); + }); + } + + it('reverts with at least one tampered request signature', async function () { + const tamperedSig = web3.utils.hexToBytes(this.requestDatas[this.idx].signature); + tamperedSig[42] ^= 0xff; + + this.requestDatas[this.idx].signature = web3.utils.bytesToHex(tamperedSig); await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS), + this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), 'ERC2771ForwarderInvalidSigner', - [ethSigUtil.recoverTypedSignature({ data, sig: this.requestDatas[this.idx].signature }), data.message.from], + [ + ethSigUtil.recoverTypedSignature({ + data: this.forgeData(this.requestDatas[this.idx]), + sig: this.requestDatas[this.idx].signature, + }), + this.requestDatas[this.idx].from, + ], ); }); - } - it('reverts with at least one tampered request signature', async function () { - const tamperedSig = web3.utils.hexToBytes(this.requestDatas[this.idx].signature); - tamperedSig[42] ^= 0xff; + it('reverts with at least one valid signature for non-current nonce', async function () { + // Execute first a request + await this.forwarder.execute(this.requestDatas[this.idx], { value: this.requestDatas[this.idx].value }); - this.requestDatas[this.idx].signature = web3.utils.bytesToHex(tamperedSig); + // And then fail due to an already used nonce + await expectRevertCustomError( + this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), + 'ERC2771ForwarderInvalidSigner', + [ + ethSigUtil.recoverTypedSignature({ + data: this.forgeData({ ...this.requestDatas[this.idx], nonce: this.requestDatas[this.idx].nonce + 1 }), + sig: this.requestDatas[this.idx].signature, + }), + this.requestDatas[this.idx].from, + ], + ); + }); - await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS), - 'ERC2771ForwarderInvalidSigner', - [ - ethSigUtil.recoverTypedSignature({ - data: this.forgeData(this.requestDatas[this.idx]), - sig: this.requestDatas[this.idx].signature, - }), - this.requestDatas[this.idx].from, - ], - ); + it('reverts with at least one valid signature for expired deadline', async function () { + this.requestDatas[this.idx].deadline = this.timestamp.toNumber() - 1; + this.requestDatas[this.idx].signature = this.sign( + this.signers[this.idx].getPrivateKey(), + this.requestDatas[this.idx], + ); + await expectRevertCustomError( + this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), + 'ERC2771ForwarderExpiredRequest', + [this.timestamp.toNumber() - 1], + ); + }); }); - it('succeeds ignoring a request with an invalid nonce', async function () { - const correctNonce = await this.forwarder.nonces(this.requestDatas[this.idx].from); - this.requestDatas[this.idx].nonce = correctNonce + 1; - this.requestDatas[this.idx].signature = this.sign( - this.signers[this.idx].getPrivateKey(), - this.requestDatas[this.idx], - ); + context('when the refund receiver is a known address', function () { + beforeEach(async function () { + this.refundReceiver = refundReceiver; + this.initialRefundReceiverBalance = web3.utils.toBN(await web3.eth.getBalance(this.refundReceiver)); + this.initialTamperedRequestNonce = await this.forwarder.nonces(this.requestDatas[this.idx].from); + }); - const receipt = await this.forwarder.executeBatch(this.requestDatas, accounts[0]); + for (const [key, value] of Object.entries(tamperedValues)) { + it(`ignores a request with tampered ${key} and refunds its value`, async function () { + const data = this.forgeData({ ...this.requestDatas[this.idx], [key]: value }); - expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); - expect(await this.forwarder.nonces(this.requestDatas[this.idx].from)).to.be.bignumber.eq( - web3.utils.toBN(correctNonce), - ); - }); + this.requestDatas[this.idx] = data.message; - it('reverts with at least one valid signature for expired deadline', async function () { - this.requestDatas[this.idx].deadline = this.timestamp.toNumber() - 1; - this.requestDatas[this.idx].signature = this.sign( - this.signers[this.idx].getPrivateKey(), - this.requestDatas[this.idx], - ); - await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS), - 'ERC2771ForwarderExpiredRequest', - [this.timestamp.toNumber() - 1], - ); - }); + const receipt = await this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { + value: batchValue(this.requestDatas), + }); + expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); + }); + } - it('reverts with at least one valid signature but mismatched value', async function () { - const value = 100; - this.requestDatas[this.idx].value = value; - this.requestDatas[this.idx].signature = this.sign( - this.signers[this.idx].getPrivateKey(), - this.requestDatas[this.idx], - ); - await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS), - 'ERC2771ForwarderMismatchedValue', - [0, value], - ); + it('ignores a request with a valid signature for non-current nonce', async function () { + // Execute first a request + await this.forwarder.execute(this.requestDatas[this.idx], { value: this.requestDatas[this.idx].value }); + this.initialTamperedRequestNonce++; // Should be already incremented by the individual `execute` + + // And then ignore the same request in a batch due to an already used nonce + const receipt = await this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { + value: this.msgValue, + }); + expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); + }); + + it('ignores a request with a valid signature for expired deadline', async function () { + this.requestDatas[this.idx].deadline = this.timestamp.toNumber() - 1; + this.requestDatas[this.idx].signature = this.sign( + this.signers[this.idx].getPrivateKey(), + this.requestDatas[this.idx], + ); + + const receipt = await this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { + value: this.msgValue, + }); + expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); + }); + + afterEach(async function () { + // The invalid request value was refunded + expect(await web3.eth.getBalance(this.refundReceiver)).to.be.bignumber.equal( + this.initialRefundReceiverBalance.add(web3.utils.toBN(this.requestDatas[this.idx].value)), + ); + + // The invalid request from's nonce was not incremented + expect(await this.forwarder.nonces(this.requestDatas[this.idx].from)).to.be.bignumber.eq( + web3.utils.toBN(this.initialTamperedRequestNonce), + ); + }); }); }); @@ -361,7 +439,10 @@ contract('ERC2771Forwarder', function (accounts) { this.requestDatas[idx].signature = this.sign(this.signers[idx].getPrivateKey(), this.requestDatas[idx]); await expectRevert.assertion( - this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS, { gas: gasAvailable }), + this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS, { + gas: gasAvailable, + value: this.msgValue, + }), ); const { transactions } = await web3.eth.getBlock('latest'); From b7b985ee0bc4db1d9afaf335fcb27d73d81fd720 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 27 Jun 2023 19:47:54 -0600 Subject: [PATCH 28/34] Remove flaky test --- test/metatx/ERC2771Forwarder.test.js | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index 59a8c912467..ba3df8f356d 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -427,28 +427,5 @@ contract('ERC2771Forwarder', function (accounts) { }); }); }); - - it('bubbles out of gas', async function () { - const receiver = await CallReceiverMock.new(); - const gasAvailable = 100000; - const idx = 0; - this.requestDatas[idx].to = receiver.address; - this.requestDatas[idx].data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); - this.requestDatas[idx].gas = 1000000; - - this.requestDatas[idx].signature = this.sign(this.signers[idx].getPrivateKey(), this.requestDatas[idx]); - - await expectRevert.assertion( - this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS, { - gas: gasAvailable, - value: this.msgValue, - }), - ); - - const { transactions } = await web3.eth.getBlock('latest'); - const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); - - expect(gasUsed).to.be.equal(gasAvailable); - }); }); }); From 8f84f4e337bdbe774a2b749378705beb8cb23ecb Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 28 Jun 2023 12:08:46 -0600 Subject: [PATCH 29/34] Hardcode slither version to 0.9.3 --- .github/workflows/checks.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 122d3956413..15cc88e9600 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -100,6 +100,7 @@ jobs: - uses: crytic/slither-action@v0.3.0 with: node-version: 18.15 + slither-version: 0.9.3 codespell: runs-on: ubuntu-latest From 8a03caddb9d3e7497c754616beb75d58f3489f75 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 28 Jun 2023 13:30:22 -0600 Subject: [PATCH 30/34] Revert on unsuccessful execute --- contracts/metatx/ERC2771Forwarder.sol | 6 ++++-- test/metatx/ERC2771Forwarder.test.js | 16 ++++++++++++---- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 6f0a86431a3..37a2e0ee661 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -91,14 +91,16 @@ contract ERC2771Forwarder is EIP712, Nonces { * - The request value should be equal to the provided `msg.value`. * - The request should be valid according to {verify}. */ - function execute(ForwardRequestData calldata request) public payable virtual returns (bool) { + function execute(ForwardRequestData calldata request) public payable virtual { // We make sure that msg.value and request.value match exactly. // If the request is invalid or the call reverts, requireValidRequest ensures this value won't be lost. if (msg.value != request.value) { revert ERC2771ForwarderMismatchedValue(request.value, msg.value); } - return _execute(request, true); + if (!_execute(request, true)) { + revert Address.FailedInnerCall(); + } } /** diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index ba3df8f356d..d95c9916f09 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -129,20 +129,28 @@ contract('ERC2771Forwarder', function (accounts) { ); }); - it('succeeds', async function () { + it('emits an event and consumes nonce for a successful request', async function () { const receipt = await this.forwarder.execute(this.requestData); expectEvent(receipt, 'ExecutedForwardRequest', { signer: this.requestData.from, nonce: web3.utils.toBN(this.requestData.nonce), success: true, }); - }); - - afterEach(async function () { expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( web3.utils.toBN(this.requestData.nonce + 1), ); }); + + it('reverts with an unsuccessful request', async function () { + const receiver = await CallReceiverMock.new(); + const req = { + ...this.requestData, + to: receiver.address, + data: receiver.contract.methods.mockFunctionRevertsNoReason().encodeABI(), + }; + req.signature = this.sign(this.alice.getPrivateKey(), req); + await expectRevertCustomError(this.forwarder.execute(req), 'FailedInnerCall', []); + }); }); context('with tampered request', function () { From 95bcb57de2aa6033f54d993b81b16bea41c8798b Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 28 Jun 2023 19:52:44 -0600 Subject: [PATCH 31/34] Implement suggestions --- contracts/metatx/ERC2771Forwarder.sol | 31 ++++++++++++++------------- test/metatx/ERC2771Forwarder.test.js | 8 +------ 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 37a2e0ee661..682691f452f 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -9,13 +9,13 @@ import "../utils/Nonces.sol"; import "../utils/Address.sol"; /** - * @dev An implementation of a production-ready forwarder compatible with ERC2771 contracts. + * @dev A forwarder compatible with ERC2771 contracts. See {ERC2771Context}. * * 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. + * * `value`: The amount of native token 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. @@ -42,9 +42,9 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev Emitted when a `ForwardRequest` is executed. * - * NOTE: A non successful forwarded request should not be assumed as non out of gas exception because of - * {_checkForwardedGas}. Such function doesn't guarantee an out of gas exception won't be thrown, but instead - * it guarantees a relayer provided enough gas to cover the signer requested gas. + * NOTE: An unsuccessful forward request could be due to an invalid signature, an expired deadline, + * or simply a revert in the requested call. The contract guarantees that the relayer is not able to force + * the requested call to run out of gas. */ event ExecutedForwardRequest(address indexed signer, uint256 nonce, bool success); @@ -74,8 +74,8 @@ contract ERC2771Forwarder is EIP712, Nonces { * A transaction is considered valid when it hasn't expired (deadline is not met), and the signer * matches the `from` parameter of the signed request. * - * NOTE: A request may return false here but it won't revert in {batchExecute} if a refund receiver - * is provided. + * NOTE: A request may return false here but it won't cause {executeBatch} to revert if a refund + * receiver is provided. */ function verify(ForwardRequestData calldata request) public view virtual returns (bool) { (bool alive, bool signerMatch, ) = _validate(request); @@ -83,8 +83,9 @@ contract ERC2771Forwarder is EIP712, Nonces { } /** - * @dev Executes a `request` on behalf of `signature`'s signer guaranteeing that the forwarded call - * will receive the requested gas and no ETH is left stuck in the contract. + * @dev Executes a `request` on behalf of `signature`'s signer using the ERC-2771 protocol. The gas + * provided to the requested call may not be exactly the amount requested, but the call will not run + * out of gas. Will revert if the request is invalid or the call reverts, in this case the nonce is not consumed. * * Requirements: * @@ -108,8 +109,8 @@ contract ERC2771Forwarder is EIP712, Nonces { * * In case a batch contains at least one invalid request (see {verify}), the * request will be skipped and the `refundReceiver` parameter will receive back the - * unused requested value at the end of the execution. This is done to prevent unexpected - * reverts when a batch includes a request that was already frontrunned by another relayer. + * unused requested value at the end of the execution. This is done to prevent reverting + * the entire batch when a request is invalid or has already been submitted. * * If the `refundReceiver` is the `address(0)`, this function will revert when at least * one of the requests was not valid instead of skipping it. This could be useful if @@ -143,14 +144,14 @@ contract ERC2771Forwarder is EIP712, Nonces { } } - // The batch should still revert if there's a mismatched msg.value provided + // The batch should revert if there's a mismatched msg.value provided // to avoid request value tampering if (requestsValue != msg.value) { revert ERC2771ForwarderMismatchedValue(requestsValue, msg.value); } - // To avoid unexpected reverts because a request was frontrunned leaving ETH in the contract - // the value is sent back instead of reverting. + // Some requests with value were invalid (possibly due to frontrunning). + // To avoid leaving ETH in the contract this value is refunded. if (refundValue != 0) { // We know refundReceiver != address(0) && requestsValue == msg.value // meaning we can ensure refundValue is not taken from the original contract's balance @@ -225,7 +226,7 @@ contract ERC2771Forwarder is EIP712, Nonces { } } - // Avoid execution instead of reverting in case a batch includes an already executed request + // Ignore an invalid request because requireValidRequest = false if (signerMatch && alive) { // Nonce should be used before the call to prevent reusing by reentrancy uint256 currentNonce = _useNonce(signer); diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index d95c9916f09..fa84ccdc301 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -70,17 +70,11 @@ contract('ERC2771Forwarder', function (accounts) { context('verify', function () { context('with valid signature', function () { - beforeEach(async function () { + it('returns true without altering the nonce', async function () { expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( web3.utils.toBN(this.requestData.nonce), ); - }); - - it('success', async function () { expect(await this.forwarder.verify(this.requestData)).to.be.equal(true); - }); - - afterEach(async function () { expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( web3.utils.toBN(this.requestData.nonce), ); From 8b7e961b2d91f801c683b35545fe537e2c957024 Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 28 Jun 2023 23:52:27 -0300 Subject: [PATCH 32/34] tweak proof wording --- contracts/metatx/ERC2771Forwarder.sol | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 682691f452f..61a57ea371b 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -257,25 +257,24 @@ contract ERC2771Forwarder is EIP712, Nonces { // 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, - // the define this model and adding a check: + // but the forwarding itself still succeeds. In order to make sure that the subcall received sufficient gas, + // we will inspect gasleft() after the forwarding. // - // Let X be the gas available before the subcall, such that the subcall gets X * 63 / 64. + // Let X be the gas available before the subcall, such that the subcall gets at most X * 63 / 64. // We can't know X after CALL dynamic costs, but we want it to be such that X * 63 / 64 >= req.gas. - // Let Y be the gas used in the subcall gasleft() measured immediately after the subcall will be gasleft() = X - Y. + // Let Y be the gas used in the subcall. gasleft() measured immediately after the subcall will be gasleft() = X - Y. // If the subcall ran out of gas, then Y = X * 63 / 64 and gasleft() = X - Y = X / 64. - // Then we restrict the model by checking if req.gas / 63 > gasleft(), which is true is true if and only if + // Under this assumption req.gas / 63 > gasleft() is true is true if and only if // req.gas / 63 > X / 64, or equivalently req.gas > X * 63 / 64. - // // This means that if the subcall runs out of gas we are able to detect that insufficient gas was passed. + // // We will now also see that req.gas / 63 > gasleft() implies that req.gas >= X * 63 / 64. // The contract guarantees Y <= req.gas, thus gasleft() = X - Y >= X - req.gas. // - req.gas / 63 > gasleft() // - req.gas / 63 >= X - req.gas // - 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 relay does not revert. + // the forwarding does not revert. 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 From f72c2d5b54229bc34db6b1de58cdd37ea7f47b56 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 28 Jun 2023 23:56:47 -0300 Subject: [PATCH 33/34] change ETH -> value --- contracts/metatx/ERC2771Forwarder.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 61a57ea371b..14e2925bd03 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -206,7 +206,7 @@ contract ERC2771Forwarder is EIP712, Nonces { * 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. + * leaving value stuck in the contract. */ function _execute( ForwardRequestData calldata request, From 62d2342e5a5513f059574004cc74e4dcfe5274f5 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 29 Jun 2023 00:04:08 -0300 Subject: [PATCH 34/34] adjust comment after recent change --- contracts/metatx/ERC2771Forwarder.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 14e2925bd03..651fdce0b62 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -94,7 +94,8 @@ contract ERC2771Forwarder is EIP712, Nonces { */ function execute(ForwardRequestData calldata request) public payable virtual { // We make sure that msg.value and request.value match exactly. - // If the request is invalid or the call reverts, requireValidRequest ensures this value won't be lost. + // If the request is invalid or the call reverts, this whole function + // will revert, ensuring value isn't stuck. if (msg.value != request.value) { revert ERC2771ForwarderMismatchedValue(request.value, msg.value); }