diff --git a/.changeset/blue-horses-do.md b/.changeset/blue-horses-do.md new file mode 100644 index 00000000000..9df604fe448 --- /dev/null +++ b/.changeset/blue-horses-do.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`ERC2771Forwarder`: Added `deadline` for expiring transactions, batching, and more secure handling of `msg.value`. 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 diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol new file mode 100644 index 00000000000..651fdce0b62 --- /dev/null +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -0,0 +1,289 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.9.0) (metatx/ERC2771Forwarder.sol) + +pragma solidity ^0.8.19; + +import "../utils/cryptography/ECDSA.sol"; +import "../utils/cryptography/EIP712.sol"; +import "../utils/Nonces.sol"; +import "../utils/Address.sol"; + +/** + * @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 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. + * * `data`: Encoded `msg.data` to send with the requested call. + */ +contract ERC2771Forwarder is EIP712, Nonces { + using ECDSA for bytes32; + + struct ForwardRequestData { + address from; + address to; + uint256 value; + uint256 gas; + uint48 deadline; + bytes data; + bytes signature; + } + + bytes32 private constant _FORWARD_REQUEST_TYPEHASH = + keccak256( + "ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)" + ); + + /** + * @dev Emitted when a `ForwardRequest` is executed. + * + * 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); + + /** + * @dev The request `from` doesn't match with the recovered `signer`. + */ + error ERC2771ForwarderInvalidSigner(address signer, address from); + + /** + * @dev The `requestedValue` doesn't match with the available `msgValue`. + */ + error ERC2771ForwarderMismatchedValue(uint256 requestedValue, uint256 msgValue); + + /** + * @dev The request `deadline` has expired. + */ + error ERC2771ForwarderExpiredRequest(uint48 deadline); + + /** + * @dev See {EIP712-constructor}. + */ + constructor(string memory name) EIP712(name, "1") {} + + /** + * @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. + * + * 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); + return alive && signerMatch; + } + + /** + * @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: + * + * - 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 { + // We make sure that msg.value and request.value match exactly. + // 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); + } + + if (!_execute(request, true)) { + revert Address.FailedInnerCall(); + } + } + + /** + * @dev Batch version of {execute} with optional refunding and atomic execution. + * + * 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 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 + * 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: + * + * - 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 + * 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, + address payable refundReceiver + ) public payable virtual { + bool atomic = refundReceiver == address(0); + + uint256 requestsValue; + uint256 refundValue; + + for (uint256 i; i < requests.length; ++i) { + requestsValue += requests[i].value; + bool success = _execute(requests[i], atomic); + if (!success) { + refundValue += requests[i].value; + } + } + + // 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); + } + + // 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 + // and refundReceiver is a known account. + Address.sendValue(refundReceiver, refundValue); + } + } + + /** + * @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 + ) internal view virtual returns (bool alive, bool signerMatch, address signer) { + 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}. + */ + function _recoverForwardRequestSigner(ForwardRequestData calldata request) internal view virtual returns (address) { + return + _hashTypedDataV4( + keccak256( + abi.encode( + _FORWARD_REQUEST_TYPEHASH, + request.from, + request.to, + request.value, + request.gas, + nonces(request.from), + request.deadline, + keccak256(request.data) + ) + ) + ).recover(request.signature); + } + + /** + * @dev Validates and executes a signed request returning the request call `success` value. + * + * Internal function without msg.value validation. + * + * Requirements: + * + * - 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. + * + * IMPORTANT: Using this function doesn't check that all the `msg.value` was sent, potentially + * leaving value stuck in the contract. + */ + function _execute( + ForwardRequestData calldata request, + bool requireValidRequest + ) internal virtual returns (bool success) { + (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 + if (requireValidRequest) { + if (!alive) { + revert ERC2771ForwarderExpiredRequest(request.deadline); + } + + if (!signerMatch) { + revert ERC2771ForwarderInvalidSigner(signer, request.from); + } + } + + // 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); + + (success, ) = request.to.call{gas: request.gas, value: request.value}( + abi.encodePacked(request.data, request.from) + ); + + _checkForwardedGas(request); + + emit ExecutedForwardRequest(signer, currentNonce, success); + } + } + + /** + * @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. + * + * 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(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 + // 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 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. + // If the subcall ran out of gas, then Y = X * 63 / 64 and gasleft() = X - Y = X / 64. + // 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 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 + // 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/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol deleted file mode 100644 index b5267aa108b..00000000000 --- a/contracts/metatx/MinimalForwarder.sol +++ /dev/null @@ -1,104 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.9.0) (metatx/MinimalForwarder.sol) - -pragma solidity ^0.8.19; - -import "../utils/cryptography/ECDSA.sol"; -import "../utils/cryptography/EIP712.sol"; - -/** - * @dev Simple minimal forwarder to be used together with an ERC2771 compatible contract. See {ERC2771Context}. - * - * 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. - */ -contract MinimalForwarder is EIP712 { - using ECDSA for bytes32; - - struct ForwardRequest { - address from; - address to; - uint256 value; - uint256 gas; - uint256 nonce; - 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; - - /** - * @dev The request `from` doesn't match with the recovered `signer`. - */ - error MinimalForwarderInvalidSigner(address signer, address from); - - /** - * @dev The request nonce doesn't match with the `current` nonce for the request signer. - */ - error MinimalForwarderInvalidNonce(address signer, uint256 current); - - constructor() EIP712("MinimalForwarder", "0.0.1") {} - - function getNonce(address from) public view returns (uint256) { - return _nonces[from]; - } - - 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; - } - - function execute( - ForwardRequest calldata req, - bytes calldata signature - ) public payable returns (bool, bytes memory) { - address signer = _recover(req, signature); - (bool correctFrom, bool correctNonce) = _validateReq(req, signer); - - if (!correctFrom) { - revert MinimalForwarderInvalidSigner(signer, req.from); - } - if (!correctNonce) { - revert MinimalForwarderInvalidNonce(signer, _nonces[req.from]); - } - - _nonces[req.from] = req.nonce + 1; - - (bool success, bytes memory returndata) = req.to.call{gas: req.gas, value: req.value}( - abi.encodePacked(req.data, req.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) { - // 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() - } - } - - return (success, returndata); - } - - function _recover(ForwardRequest calldata req, 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))) - ).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/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/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index 2628014f12c..1089de0050d 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._ */ diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index 6c298d3d9f0..3dd4b41532d 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -6,14 +6,16 @@ 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 = web3.utils.toBN(1).shln(48).subn(1).toString(); + beforeEach(async function () { - this.forwarder = await MinimalForwarder.new(); + this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder'); 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,14 +66,17 @@ 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, }; - 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 }); }); }); @@ -86,14 +92,17 @@ 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, }; - 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 }); }); }); diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js new file mode 100644 index 00000000000..fa84ccdc301 --- /dev/null +++ b/test/metatx/ERC2771Forwarder.test.js @@ -0,0 +1,433 @@ +const ethSigUtil = require('eth-sig-util'); +const Wallet = require('ethereumjs-wallet').default; +const { getDomain, domainType } = require('../helpers/eip712'); +const { expectRevertCustomError } = require('../helpers/customError'); + +const { constants, expectRevert, expectEvent, time } = require('@openzeppelin/test-helpers'); +const { expect } = require('chai'); + +const ERC2771Forwarder = artifacts.require('ERC2771Forwarder'); +const CallReceiverMock = artifacts.require('CallReceiverMock'); + +contract('ERC2771Forwarder', function (accounts) { + const [, refundReceiver, another] = accounts; + + const tamperedValues = { + from: another, + to: another, + value: web3.utils.toWei('0.5'), + data: '0x1742', + deadline: 0xdeadbeef, + }; + + beforeEach(async function () { + this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder'); + + this.domain = await getDomain(this.forwarder); + this.types = { + EIP712Domain: domainType(this.domain), + ForwardRequest: [ + { name: 'from', type: 'address' }, + { name: 'to', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'gas', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'deadline', type: 'uint48' }, + { name: 'data', type: 'bytes' }, + ], + }; + + this.alice = Wallet.generate(); + this.alice.address = web3.utils.toChecksumAddress(this.alice.getAddressString()); + + this.timestamp = await time.latest(); + this.request = { + from: this.alice.address, + to: constants.ZERO_ADDRESS, + value: '0', + gas: '100000', + 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.requestData, ...request }, + }); + this.sign = (privateKey, request) => + ethSigUtil.signTypedMessage(privateKey, { + data: this.forgeData(request), + }); + + this.requestData.signature = this.sign(this.alice.getPrivateKey()); + }); + + context('verify', function () { + context('with valid signature', 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), + ); + expect(await this.forwarder.verify(this.requestData)).to.be.equal(true); + expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( + web3.utils.toBN(this.requestData.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)).to.be.equal(false); + }); + } + + it('returns false with tampered signature', async function () { + const tamperedsign = web3.utils.hexToBytes(this.requestData.signature); + tamperedsign[42] ^= 0xff; + 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.requestData, + nonce: this.requestData.nonce + 1, + }; + 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.requestData, + deadline: this.timestamp - 1, + }; + req.signature = this.sign(this.alice.getPrivateKey(), req); + expect(await this.forwarder.verify(req)).to.be.equal(false); + }); + }); + }); + + context('execute', 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), + ); + }); + + 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, + }); + 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 () { + for (const [key, value] of Object.entries(tamperedValues)) { + it(`reverts with tampered ${key}`, async function () { + const data = this.forgeData({ [key]: value }); + await expectRevertCustomError( + this.forwarder.execute(data.message, { + value: key == 'value' ? value : 0, // To avoid MismatchedValue error + }), + 'ERC2771ForwarderInvalidSigner', + [ethSigUtil.recoverTypedSignature({ data, sig: this.requestData.signature }), data.message.from], + ); + }); + } + + it('reverts with tampered signature', async function () { + const tamperedSig = web3.utils.hexToBytes(this.requestData.signature); + tamperedSig[42] ^= 0xff; + 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.requestData); + + // And then fail due to an already used nonce + await expectRevertCustomError(this.forwarder.execute(this.requestData), 'ERC2771ForwarderInvalidSigner', [ + ethSigUtil.recoverTypedSignature({ + data: this.forgeData({ ...this.requestData, nonce: this.requestData.nonce + 1 }), + sig: this.requestData.signature, + }), + this.requestData.from, + ]); + }); + + it('reverts with valid signature for expired deadline', async function () { + const req = { + ...this.requestData, + deadline: this.timestamp - 1, + }; + req.signature = this.sign(this.alice.getPrivateKey(), req); + await expectRevertCustomError(this.forwarder.execute(req), 'ERC2771ForwarderExpiredRequest', [ + this.timestamp - 1, + ]); + }); + + it('reverts with valid signature but mismatched value', async function () { + const value = 100; + const req = { + ...this.requestData, + 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.requestData.to = receiver.address; + this.requestData.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); + this.requestData.gas = 1000000; + + 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]); + + expect(gasUsed).to.be.equal(gasAvailable); + }); + }); + + 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()); + + this.eve = Wallet.generate(); + this.eve.address = web3.utils.toChecksumAddress(this.eve.getAddressString()); + + this.signers = [this.alice, this.bob, this.eve]; + + this.requestDatas = await Promise.all( + this.signers.map(async ({ address }) => ({ + ...this.requestData, + from: address, + nonce: (await this.forwarder.nonces(address)).toString(), + value: web3.utils.toWei('10', 'gwei'), + })), + ); + + this.requestDatas = this.requestDatas.map((requestData, i) => ({ + ...requestData, + signature: this.sign(this.signers[i].getPrivateKey(), requestData), + })); + + this.msgValue = batchValue(this.requestDatas); + }); + + context('with valid requests', function () { + beforeEach(async function () { + 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 () { + for (const request of this.requestDatas) { + expectEvent(this.receipt, 'ExecutedForwardRequest', { + signer: request.from, + nonce: web3.utils.toBN(request.nonce), + success: true, + }); + } + }); + + it('increase nonces', async function () { + for (const request of this.requestDatas) { + expect(await this.forwarder.nonces(request.from)).to.be.bignumber.eq(web3.utils.toBN(request.nonce + 1)); + } + }); + }); + + context('with tampered requests', function () { + beforeEach(async function () { + this.idx = 1; // Tampered idx + }); + + 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], + ); + }); + + 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, this.refundReceiver, { value: this.msgValue }), + '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 non-current nonce', async function () { + // Execute first a request + await this.forwarder.execute(this.requestDatas[this.idx], { value: this.requestDatas[this.idx].value }); + + // 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, + ], + ); + }); + + 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], + ); + }); + }); + + 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); + }); + + 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 }); + + this.requestDatas[this.idx] = data.message; + + 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('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), + ); + }); + }); + }); + }); +}); diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js deleted file mode 100644 index c775c5e4403..00000000000 --- a/test/metatx/MinimalForwarder.test.js +++ /dev/null @@ -1,171 +0,0 @@ -const ethSigUtil = require('eth-sig-util'); -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 { expect } = require('chai'); - -const MinimalForwarder = artifacts.require('MinimalForwarder'); -const CallReceiverMock = artifacts.require('CallReceiverMock'); - -contract('MinimalForwarder', function (accounts) { - beforeEach(async function () { - this.forwarder = await MinimalForwarder.new(); - - this.domain = await getDomain(this.forwarder); - this.types = { - EIP712Domain: domainType(this.domain), - ForwardRequest: [ - { name: 'from', type: 'address' }, - { name: 'to', type: 'address' }, - { name: 'value', type: 'uint256' }, - { name: 'gas', type: 'uint256' }, - { name: 'nonce', type: 'uint256' }, - { 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', - }; - - beforeEach(async function () { - this.wallet = Wallet.generate(); - this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString()); - this.req = { - from: this.sender, - to: constants.ZERO_ADDRESS, - value: '0', - gas: '100000', - nonce: Number(await this.forwarder.getNonce(this.sender)), - data: '0x', - }; - this.forgeData = req => ({ - types: this.types, - domain: this.domain, - primaryType: 'ForwardRequest', - message: { ...this.req, ...req }, - }); - this.sign = req => - ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { - data: this.forgeData(req), - }); - }); - - 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)); - }); - - it('success', async function () { - expect(await this.forwarder.verify(this.req, 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)); - }); - }); - - 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 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); - }); - - it('returns false with valid signature for non-current nonce', async function () { - const req = { - ...this.req, - nonce: this.req.nonce + 1, - }; - const sig = this.sign(req); - expect(await this.forwarder.verify(req, sig)).to.be.equal(false); - }); - }); - }); - - 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)); - }); - - it('success', async function () { - await this.forwarder.execute(this.req, 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), - ); - }); - }); - - 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 tampered signature', async function () { - const tamperedSig = web3.utils.hexToBytes(this.sign()); - tamperedSig[42] ^= 0xff; - await expectRevertCustomError( - this.forwarder.execute(this.req, web3.utils.bytesToHex(tamperedSig)), - 'MinimalForwarderInvalidSigner', - [ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }), this.req.from], - ); - }); - - it('reverts with valid signature for non-current nonce', async function () { - const req = { - ...this.req, - nonce: this.req.nonce + 1, - }; - const sig = this.sign(req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderInvalidNonce', [ - this.req.from, - this.req.nonce, - ]); - }); - }); - - 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; - - await expectRevert.assertion(this.forwarder.execute(this.req, this.sign(), { gas: gasAvailable })); - - const { transactions } = await web3.eth.getBlock('latest'); - const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); - - expect(gasUsed).to.be.equal(gasAvailable); - }); - }); - }); -});