Use invalid opcode to consume all gas in MinimalForwarder (#3035)

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
+            // 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()
+            }
+        }
 
         return (success, returndata);
     }

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 { transactions } = await web3.eth.getBlock('latest');
+        const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]);
+
+        expect(gasUsed).to.be.equal(gasAvailable);
+      });
     });
   });
 });