Fix L-03-2

contracts/metatx/ERC2771Forwarder.sol

@@ -3,6 +3,7 @@
 
 pragma solidity ^0.8.20;
 
+import {ERC2771Context} from "./ERC2771Context.sol";
 import {ECDSA} from "../utils/cryptography/ECDSA.sol";
 import {EIP712} from "../utils/cryptography/EIP712.sol";
 import {Nonces} from "../utils/Nonces.sol";
@@ -92,6 +93,11 @@ contract ERC2771Forwarder is EIP712, Nonces {
      */
     error ERC2771ForwarderExpiredRequest(uint48 deadline);
 
+    /**
+     * @dev The request targert doesn't trust the `forwarder`.
+     */
+    error ERC2771UntrustfulTarget(address target, address forwarder);
+
     /**
      * @dev See {EIP712-constructor}.
      */
@@ -100,15 +106,15 @@ contract ERC2771Forwarder is EIP712, Nonces {
     /**
      * @dev Returns `true` if a request is valid for a provided `signature` at the current block timestamp.
      *
-     * A transaction is considered valid when it hasn't expired (deadline is not met), and the signer
-     * matches the `from` parameter of the signed request.
+     * A transaction is considered valid when the target trusts this forwarder, the request hasn't expired
+     * (deadline is not met), and the signer matches the `from` parameter of the signed request.
      *
      * NOTE: A request may return false here but it won't cause {executeBatch} to revert if a refund
      * receiver is provided.
      */
     function verify(ForwardRequestData calldata request) public view virtual returns (bool) {
-        (bool active, bool signerMatch, ) = _validate(request);
-        return active && signerMatch;
+        (bool isTrustedForwarder, bool active, bool signerMatch, ) = _validate(request);
+        return isTrustedForwarder && active && signerMatch;
     }
 
     /**
@@ -196,9 +202,15 @@ contract ERC2771Forwarder is EIP712, Nonces {
      */
     function _validate(
         ForwardRequestData calldata request
-    ) internal view virtual returns (bool active, bool signerMatch, address signer) {
+    ) internal view virtual returns (bool isTrustedForwarder, bool active, bool signerMatch, address signer) {
         (bool isValid, address recovered) = _recoverForwardRequestSigner(request);
-        return (request.deadline >= block.timestamp, isValid && recovered == request.from, recovered);
+
+        return (
+            _isTrustedByTarget(request.to),
+            request.deadline >= block.timestamp,
+            isValid && recovered == request.from,
+            recovered
+        );
     }
 
     /**
@@ -247,11 +259,15 @@ contract ERC2771Forwarder is EIP712, Nonces {
         ForwardRequestData calldata request,
         bool requireValidRequest
     ) internal virtual returns (bool success) {
-        (bool active, bool signerMatch, address signer) = _validate(request);
+        (bool isTrustedForwarder, bool active, bool signerMatch, address signer) = _validate(request);
 
         // Need to explicitly specify if a revert is required since non-reverting is default for
         // batches and reversion is opt-in since it could be useful in some scenarios
         if (requireValidRequest) {
+            if (!isTrustedForwarder) {
+                revert ERC2771UntrustfulTarget(request.to, address(this));
+            }
+
             if (!active) {
                 revert ERC2771ForwarderExpiredRequest(request.deadline);
             }
@@ -262,7 +278,7 @@ contract ERC2771Forwarder is EIP712, Nonces {
         }
 
         // Ignore an invalid request because requireValidRequest = false
-        if (signerMatch && active) {
+        if (isTrustedForwarder && signerMatch && active) {
             // Nonce should be used before the call to prevent reusing by reentrancy
             uint256 currentNonce = _useNonce(signer);
 
@@ -284,6 +300,56 @@ contract ERC2771Forwarder is EIP712, Nonces {
         }
     }
 
+    /**
+     * @dev Returns whether the target trusts this forwarder.
+     *
+     * This function performs a static call to the target contract calling the
+     * {ERC2771Context-isTrustedForwarder} function.
+     */
+    function _isTrustedByTarget(address target) private view returns (bool) {
+        bytes4 selector = ERC2771Context.isTrustedForwarder.selector;
+
+        // Calldata is always the same, and we can guarantee its length is 36 bytes
+        // - 4 bytes from the selector
+        // - 32 bytes from the abi encoded target address (padded to 32)
+        uint8 calldataLength = 36;
+
+        bool success;
+        bool returnValue;
+        /// @solidity memory-safe-assembly
+        assembly {
+            // Because the return value is a boolean (requires 1 byte copied to memory) and the
+            // calldata length is 24 bytes, we can safely reuse the scratch space in memory (0x00 - 0x3F).
+            // This avoids memory leakage of allocating the encoded calldata in memory during each
+            // `isTrustedForwarder` call.
+            let ptr := 0x00
+
+            // | Location  | Content  | Content (Hex)                                                      |
+            // |-----------|----------|--------------------------------------------------------------------|
+            // |           |          |                                                  selector ↓        |
+            // | 0x00:0x1F | selector | 0x00000000000000000000000000000000000000000000000000000000AAAAAAAA |
+            // |           |          |   ↓ PADDING        target ↓                                        |
+            // | 0x20:0x3F | target   | 0x000000000000000000000000BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB |
+            mstore(ptr, shr(224, selector))
+            mstore(add(ptr, 0x20), address())
+
+            // Perform the staticcal and save the result in the scratch space.
+            // | Location  | Content  | Content (Hex)                                                      |
+            // |-----------|----------|--------------------------------------------------------------------|
+            // |           |          |                                                           result ↓ |
+            // | 0x00:0x1F | selector | 0x0000000000000000000000000000000000000000000000000000000000000001 |
+            success := staticcall(gas(), target, add(ptr, 0x1c), calldataLength, 0, 0x20)
+
+            // Avoid strange returndatasizes (eg. an EOA)
+            if eq(returndatasize(), 0x20) {
+                // Copy from memory and clean the `returnValue` as a boolean.
+                returnValue := shr(255, shl(255, mload(0)))
+            }
+        }
+
+        return success && returnValue;
+    }
+
     /**
      * @dev Checks if the requested gas was correctly forwarded to the callee.
      *

contracts/mocks/CallReceiverMock.sol

@@ -59,3 +59,15 @@ contract CallReceiverMock {
         return "0x1234";
     }
 }
+
+contract CallReceiverMockTrustingForwarder is CallReceiverMock {
+    address private _trustedForwarder;
+
+    constructor(address trustedForwarder_) {
+        _trustedForwarder = trustedForwarder_;
+    }
+
+    function isTrustedForwarder(address forwarder) public view virtual returns (bool) {
+        return forwarder == _trustedForwarder;
+    }
+}

test/metatx/ERC2771Forwarder.t.sol

@@ -4,7 +4,7 @@ pragma solidity ^0.8.20;
 
 import {Test} from "forge-std/Test.sol";
 import {ERC2771Forwarder} from "contracts/metatx/ERC2771Forwarder.sol";
-import {CallReceiverMock} from "contracts/mocks/CallReceiverMock.sol";
+import {CallReceiverMockTrustingForwarder, CallReceiverMock} from "contracts/mocks/CallReceiverMock.sol";
 
 struct ForwardRequest {
     address from;
@@ -40,7 +40,7 @@ contract ERC2771ForwarderMock is ERC2771Forwarder {
 
 contract ERC2771ForwarderTest is Test {
     ERC2771ForwarderMock internal _erc2771Forwarder;
-    CallReceiverMock internal _receiver;
+    CallReceiverMockTrustingForwarder internal _receiver;
 
     uint256 internal _signerPrivateKey;
     uint256 internal _relayerPrivateKey;
@@ -52,7 +52,7 @@ contract ERC2771ForwarderTest is Test {
 
     function setUp() public {
         _erc2771Forwarder = new ERC2771ForwarderMock("ERC2771Forwarder");
-        _receiver = new CallReceiverMock();
+        _receiver = new CallReceiverMockTrustingForwarder(address(_erc2771Forwarder));
 
         _signerPrivateKey = 0xA11CE;
         _relayerPrivateKey = 0xB0B;

test/metatx/ERC2771Forwarder.test.js

@@ -7,14 +7,13 @@ const { constants, expectRevert, expectEvent, time } = require('@openzeppelin/te
 const { expect } = require('chai');
 
 const ERC2771Forwarder = artifacts.require('ERC2771Forwarder');
-const CallReceiverMock = artifacts.require('CallReceiverMock');
+const CallReceiverMockTrustingForwarder = artifacts.require('CallReceiverMockTrustingForwarder');
 
 contract('ERC2771Forwarder', function (accounts) {
   const [, refundReceiver, another] = accounts;
 
   const tamperedValues = {
     from: another,
-    to: another,
     value: web3.utils.toWei('0.5'),
     data: '0x1742',
     deadline: 0xdeadbeef,
@@ -41,7 +40,7 @@ contract('ERC2771Forwarder', function (accounts) {
     this.alice.address = web3.utils.toChecksumAddress(this.alice.getAddressString());
 
     this.timestamp = await time.latest();
-    this.receiver = await CallReceiverMock.new();
+    this.receiver = await CallReceiverMockTrustingForwarder.new(this.forwarder.address);
     this.request = {
       from: this.alice.address,
       to: this.receiver.address,
@@ -97,6 +96,10 @@ contract('ERC2771Forwarder', function (accounts) {
         });
       }
 
+      it('returns false with an untrustful to', async function () {
+        expect(await this.forwarder.verify(this.forgeData({ to: another }).message)).to.be.equal(false);
+      });
+
       it('returns false with tampered signature', async function () {
         const tamperedsign = web3.utils.hexToBytes(this.requestData.signature);
         tamperedsign[42] ^= 0xff;
@@ -169,6 +172,14 @@ contract('ERC2771Forwarder', function (accounts) {
         });
       }
 
+      it('reverts with an untrustful to', async function () {
+        const data = this.forgeData({ to: another });
+        await expectRevertCustomError(this.forwarder.execute(data.message), 'ERC2771UntrustfulTarget', [
+          data.message.to,
+          this.forwarder.address,
+        ]);
+      });
+
       it('reverts with tampered signature', async function () {
         const tamperedSig = web3.utils.hexToBytes(this.requestData.signature);
         tamperedSig[42] ^= 0xff;
@@ -360,6 +371,18 @@ contract('ERC2771Forwarder', function (accounts) {
           });
         }
 
+        it('reverts with at least one untrustful to', async function () {
+          const data = this.forgeData({ ...this.requestDatas[this.idx], to: another });
+
+          this.requestDatas[this.idx] = data.message;
+
+          await expectRevertCustomError(
+            this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }),
+            'ERC2771UntrustfulTarget',
+            [this.requestDatas[this.idx].to, this.forwarder.address],
+          );
+        });
+
         it('reverts with at least one tampered request signature', async function () {
           const tamperedSig = web3.utils.hexToBytes(this.requestDatas[this.idx].signature);
           tamperedSig[42] ^= 0xff;