From dbcfb24ff2d6b035e1231a2b7f5d47b4a80d728e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 17 Dec 2021 19:31:46 +0100 Subject: [PATCH 1/3] use invalid opcode to consume all gas --- contracts/metatx/MinimalForwarder.sol | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index c20125522b2..41ec22d0a7b 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -50,9 +50,17 @@ contract MinimalForwarder is EIP712 { (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.link/blog/ethereum-gas-dangers/ - assert(gasleft() > req.gas / 63); + if (gasleft() <= req.gas / 63) { + // We explicitly trigger invalid opcode to consume all gas and bubble-up the effects, since + // Panic error do not 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 + assembly { + invalid() + } + } return (success, returndata); } From fcbd3190758443a6f01f7e6d5f41067528218957 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 20 Dec 2021 13:49:39 +0100 Subject: [PATCH 2/3] test out-of-gas-bubbling --- contracts/metatx/MinimalForwarder.sol | 12 +++---- test/metatx/MinimalForwarder.test.js | 48 ++++++++++++++++++--------- 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index 41ec22d0a7b..7f9090d6444 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -54,12 +54,12 @@ contract MinimalForwarder is EIP712 { // Validate that the relayer has sent enough gas for the call. // See https://ronan.eth.link/blog/ethereum-gas-dangers/ if (gasleft() <= req.gas / 63) { - // We explicitly trigger invalid opcode to consume all gas and bubble-up the effects, since - // Panic error do not 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 - assembly { - invalid() - } + // We explicitly trigger invalid opcode to consume all gas and bubble-up the effects, since + // Panic error do not 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 + assembly { + invalid() + } } return (success, returndata); diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index 6151f97ecf7..e7877d2974c 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -6,6 +6,7 @@ const { expectRevert, constants } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const MinimalForwarder = artifacts.require('MinimalForwarder'); +const CallReceiverMock = artifacts.require('CallReceiverMock'); const name = 'MinimalForwarder'; const version = '0.0.1'; @@ -44,7 +45,7 @@ contract('MinimalForwarder', function (accounts) { nonce: Number(await this.forwarder.getNonce(this.sender)), data: '0x', }; - this.sign = ethSigUtil.signTypedMessage( + this.sign = () => ethSigUtil.signTypedMessage( this.wallet.getPrivateKey(), { data: { @@ -65,7 +66,7 @@ contract('MinimalForwarder', function (accounts) { }); it('success', async function () { - expect(await this.forwarder.verify(this.req, this.sign)).to.be.equal(true); + expect(await this.forwarder.verify(this.req, this.sign())).to.be.equal(true); }); afterEach(async function () { @@ -76,27 +77,27 @@ contract('MinimalForwarder', function (accounts) { context('invalid signature', function () { it('tampered from', async function () { - expect(await this.forwarder.verify({ ...this.req, from: accounts[0] }, this.sign)) + expect(await this.forwarder.verify({ ...this.req, from: accounts[0] }, this.sign())) .to.be.equal(false); }); it('tampered to', async function () { - expect(await this.forwarder.verify({ ...this.req, to: accounts[0] }, this.sign)) + expect(await this.forwarder.verify({ ...this.req, to: accounts[0] }, this.sign())) .to.be.equal(false); }); it('tampered value', async function () { - expect(await this.forwarder.verify({ ...this.req, value: web3.utils.toWei('1') }, this.sign)) + expect(await this.forwarder.verify({ ...this.req, value: web3.utils.toWei('1') }, this.sign())) .to.be.equal(false); }); it('tampered nonce', async function () { - expect(await this.forwarder.verify({ ...this.req, nonce: this.req.nonce + 1 }, this.sign)) + expect(await this.forwarder.verify({ ...this.req, nonce: this.req.nonce + 1 }, this.sign())) .to.be.equal(false); }); it('tampered data', async function () { - expect(await this.forwarder.verify({ ...this.req, data: '0x1742' }, this.sign)) + expect(await this.forwarder.verify({ ...this.req, data: '0x1742' }, this.sign())) .to.be.equal(false); }); it('tampered signature', async function () { - const tamperedsign = web3.utils.hexToBytes(this.sign); + 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); @@ -112,7 +113,7 @@ contract('MinimalForwarder', function (accounts) { }); it('success', async function () { - await this.forwarder.execute(this.req, this.sign); // expect to not revert + await this.forwarder.execute(this.req, this.sign()); // expect to not revert }); afterEach(async function () { @@ -124,36 +125,36 @@ contract('MinimalForwarder', function (accounts) { context('invalid signature', function () { it('tampered from', async function () { await expectRevert( - this.forwarder.execute({ ...this.req, from: accounts[0] }, this.sign), + this.forwarder.execute({ ...this.req, from: accounts[0] }, this.sign()), 'MinimalForwarder: signature does not match request', ); }); it('tampered to', async function () { await expectRevert( - this.forwarder.execute({ ...this.req, to: accounts[0] }, this.sign), + this.forwarder.execute({ ...this.req, to: accounts[0] }, this.sign()), 'MinimalForwarder: signature does not match request', ); }); it('tampered value', async function () { await expectRevert( - this.forwarder.execute({ ...this.req, value: web3.utils.toWei('1') }, this.sign), + this.forwarder.execute({ ...this.req, value: web3.utils.toWei('1') }, this.sign()), 'MinimalForwarder: signature does not match request', ); }); it('tampered nonce', async function () { await expectRevert( - this.forwarder.execute({ ...this.req, nonce: this.req.nonce + 1 }, this.sign), + this.forwarder.execute({ ...this.req, nonce: this.req.nonce + 1 }, this.sign()), 'MinimalForwarder: signature does not match request', ); }); it('tampered data', async function () { await expectRevert( - this.forwarder.execute({ ...this.req, data: '0x1742' }, this.sign), + this.forwarder.execute({ ...this.req, data: '0x1742' }, this.sign()), 'MinimalForwarder: signature does not match request', ); }); it('tampered signature', async function () { - const tamperedsign = web3.utils.hexToBytes(this.sign); + const tamperedsign = web3.utils.hexToBytes(this.sign()); tamperedsign[42] ^= 0xff; await expectRevert( this.forwarder.execute(this.req, web3.utils.bytesToHex(tamperedsign)), @@ -161,6 +162,23 @@ contract('MinimalForwarder', function (accounts) { ); }); }); + + 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 { gasUsed } = await web3.eth.getBlock('latest') + .then(({ transactions }) => web3.eth.getTransactionReceipt(transactions.find(Boolean))); + + expect(gasUsed).to.be.equal(gasAvailable); + }); }); }); }); From 480f83d8025f30766bd79f470b6dca726194e3f7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 21 Dec 2021 23:20:12 +0100 Subject: [PATCH 3/3] address PR comments --- contracts/metatx/MinimalForwarder.sol | 2 +- test/metatx/MinimalForwarder.test.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index 7f9090d6444..817a42cbd56 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -55,7 +55,7 @@ contract MinimalForwarder is EIP712 { // See https://ronan.eth.link/blog/ethereum-gas-dangers/ if (gasleft() <= req.gas / 63) { // We explicitly trigger invalid opcode to consume all gas and bubble-up the effects, since - // Panic error do not consume all gas since Solidity 0.8.0 + // 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 assembly { invalid() diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index e7877d2974c..b8984e431e3 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -174,8 +174,8 @@ contract('MinimalForwarder', function (accounts) { this.forwarder.execute(this.req, this.sign(), { gas: gasAvailable }), ); - const { gasUsed } = await web3.eth.getBlock('latest') - .then(({ transactions }) => web3.eth.getTransactionReceipt(transactions.find(Boolean))); + const { transactions } = await web3.eth.getBlock('latest'); + const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); expect(gasUsed).to.be.equal(gasAvailable); });