Avoid returndatacopy in ERC2771Forwarder by calling via assembly (#4458)

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

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}(
+            uint256 reqGas = request.gas;
-                abi.encodePacked(request.data, request.from)
+            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);
         }

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: this.receiver.contract.methods.mockFunctionRevertsNoReason().encodeABI(),
-          data: 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();
+      this.requestData.data = this.receiver.contract.methods.mockFunctionOutOfGas().encodeABI();
-      const gasAvailable = 100000;
+      this.requestData.gas = 1_000_000;
-      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());
 
+      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);
+      });
     });
   });
 });