diff --git a/contracts/metatx/ERC2771Context.sol b/contracts/metatx/ERC2771Context.sol index 2c4d89f94..406a62bd6 100644 --- a/contracts/metatx/ERC2771Context.sol +++ b/contracts/metatx/ERC2771Context.sol @@ -7,6 +7,12 @@ import {Context} from "../utils/Context.sol"; /** * @dev Context variant with ERC2771 support. + * + * WARNING: Avoid using this pattern in contracts that rely in a specific calldata length as they'll + * be affected by any forwarder whose `msg.data` is suffixed with the `from` address according to the ERC2771 + * specification adding the address size in bytes (20) to the calldata size. An example of an unexpected + * behavior could be an unintended fallback (or another function) invocation while trying to invoke the `receive` + * function only accessible if `msg.data.length == 0`. */ abstract contract ERC2771Context is Context { /// @custom:oz-upgrades-unsafe-allow state-variable-immutable diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 84d736c74..125f80076 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -251,11 +251,19 @@ contract ERC2771Forwarder is EIP712, Nonces { // 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) - ); + uint256 reqGas = request.gas; + address to = request.to; + uint256 value = request.value; + bytes memory data = abi.encodePacked(request.data, request.from); - _checkForwardedGas(gasleft(), request); + uint256 gasLeft; + + assembly { + success := call(reqGas, to, value, add(data, 0x20), mload(data), 0, 0) + gasLeft := gas() + } + + _checkForwardedGas(gasLeft, request); emit ExecutedForwardRequest(signer, currentNonce, success); } diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index fa84ccdc3..26726e8df 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -41,12 +41,13 @@ contract('ERC2771Forwarder', function (accounts) { this.alice.address = web3.utils.toChecksumAddress(this.alice.getAddressString()); this.timestamp = await time.latest(); + this.receiver = await CallReceiverMock.new(); this.request = { from: this.alice.address, - to: constants.ZERO_ADDRESS, + to: this.receiver.address, value: '0', gas: '100000', - data: '0x', + data: this.receiver.contract.methods.mockFunction().encodeABI(), deadline: this.timestamp.toNumber() + 60, // 1 minute }; this.requestData = { @@ -64,6 +65,14 @@ contract('ERC2771Forwarder', function (accounts) { ethSigUtil.signTypedMessage(privateKey, { data: this.forgeData(request), }); + this.estimateRequest = request => + web3.eth.estimateGas({ + from: this.forwarder.address, + to: request.to, + data: web3.utils.encodePacked({ value: request.data, type: 'bytes' }, { value: request.from, type: 'address' }), + value: request.value, + gas: request.gas, + }); this.requestData.signature = this.sign(this.alice.getPrivateKey()); }); @@ -125,7 +134,8 @@ contract('ERC2771Forwarder', function (accounts) { it('emits an event and consumes nonce for a successful request', async function () { const receipt = await this.forwarder.execute(this.requestData); - expectEvent(receipt, 'ExecutedForwardRequest', { + await expectEvent.inTransaction(receipt.tx, this.receiver, 'MockFunctionCalled'); + await expectEvent.inTransaction(receipt.tx, this.forwarder, 'ExecutedForwardRequest', { signer: this.requestData.from, nonce: web3.utils.toBN(this.requestData.nonce), success: true, @@ -136,11 +146,9 @@ contract('ERC2771Forwarder', function (accounts) { }); 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(), + data: this.receiver.contract.methods.mockFunctionRevertsNoReason().encodeABI(), }; req.signature = this.sign(this.alice.getPrivateKey(), req); await expectRevertCustomError(this.forwarder.execute(req), 'FailedInnerCall', []); @@ -208,14 +216,11 @@ contract('ERC2771Forwarder', function (accounts) { }); 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.data = this.receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); + this.requestData.gas = 1_000_000; this.requestData.signature = this.sign(this.alice.getPrivateKey()); + const gasAvailable = 100_000; await expectRevert.assertion(this.forwarder.execute(this.requestData, { gas: gasAvailable })); const { transactions } = await web3.eth.getBlock('latest'); @@ -223,6 +228,32 @@ contract('ERC2771Forwarder', function (accounts) { expect(gasUsed).to.be.equal(gasAvailable); }); + + it('bubbles out of gas forced by the relayer', async function () { + // If there's an incentive behind executing requests, a malicious relayer could grief + // the forwarder by executing requests and providing a top-level call gas limit that + // is too low to successfully finish the request after the 63/64 rule. + + // We set the baseline to the gas limit consumed by a successful request if it was executed + // normally. Note this includes the 21000 buffer that also the relayer will be charged to + // start a request execution. + const estimate = await this.estimateRequest(this.request); + + // Because the relayer call consumes gas until the `CALL` opcode, the gas left after failing + // the subcall won't enough to finish the top level call (after testing), so we add a + // moderated buffer. + const gasAvailable = estimate + 2_000; + + // The subcall out of gas should be caught by the contract and then bubbled up consuming + // the available gas with an `invalid` opcode. + await expectRevert.outOfGas(this.forwarder.execute(this.requestData, { gas: gasAvailable })); + + const { transactions } = await web3.eth.getBlock('latest'); + const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + + // We assert that indeed the gas was totally consumed. + expect(gasUsed).to.be.equal(gasAvailable); + }); }); context('executeBatch', function () { @@ -252,6 +283,14 @@ contract('ERC2771Forwarder', function (accounts) { })); this.msgValue = batchValue(this.requestDatas); + + this.gasUntil = async reqIdx => { + const gas = 0; + const estimations = await Promise.all( + new Array(reqIdx + 1).fill().map((_, idx) => this.estimateRequest(this.requestDatas[idx])), + ); + return estimations.reduce((acc, estimation) => acc + estimation, gas); + }; }); context('with valid requests', function () { @@ -265,7 +304,8 @@ contract('ERC2771Forwarder', function (accounts) { it('emits events', async function () { for (const request of this.requestDatas) { - expectEvent(this.receipt, 'ExecutedForwardRequest', { + await expectEvent.inTransaction(this.receipt.tx, this.receiver, 'MockFunctionCalled'); + await expectEvent.inTransaction(this.receipt.tx, this.forwarder, 'ExecutedForwardRequest', { signer: request.from, nonce: web3.utils.toBN(request.nonce), success: true, @@ -428,6 +468,52 @@ contract('ERC2771Forwarder', function (accounts) { ); }); }); + + it('bubbles out of gas', async function () { + this.requestDatas[this.idx].data = this.receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); + this.requestDatas[this.idx].gas = 1_000_000; + this.requestDatas[this.idx].signature = this.sign( + this.signers[this.idx].getPrivateKey(), + this.requestDatas[this.idx], + ); + + const gasAvailable = 300_000; + await expectRevert.assertion( + this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS, { + gas: gasAvailable, + value: this.requestDatas.reduce((acc, { value }) => acc + Number(value), 0), + }), + ); + + const { transactions } = await web3.eth.getBlock('latest'); + const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + + expect(gasUsed).to.be.equal(gasAvailable); + }); + + it('bubbles out of gas forced by the relayer', async function () { + // Similarly to the single execute, a malicious relayer could grief requests. + + // We estimate until the selected request as if they were executed normally + const estimate = await this.gasUntil(this.requestDatas, this.idx); + + // We add a Buffer to account for all the gas that's used before the selected call. + // Note is slightly bigger because the selected request is not the index 0 and it affects + // the buffer needed. + const gasAvailable = estimate + 10_000; + + // The subcall out of gas should be caught by the contract and then bubbled up consuming + // the available gas with an `invalid` opcode. + await expectRevert.outOfGas( + this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS, { gas: gasAvailable }), + ); + + const { transactions } = await web3.eth.getBlock('latest'); + const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + + // We assert that indeed the gas was totally consumed. + expect(gasUsed).to.be.equal(gasAvailable); + }); }); }); });