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]);