Update docs

.changeset/rude-weeks-beg.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+`ERC2771Context` and `Context`: Introduce a `_contextPrefixLength()` getter, used to trim extra information appended to `msg.data`.

.changeset/strong-points-invent.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+`Multicall`: Make aware of non-canonical context (i.e. `msg.sender` is not `_msgSender()`), allowing compatibility with `ERC2771Context`.

contracts/metatx/ERC2771Context.sol

@@ -7,6 +7,10 @@ import "../utils/Context.sol";
 
 /**
  * @dev Context variant with ERC2771 support.
+ *
+ * WARNING: The usage of `delegatecall` in this contract is dangerous and may result in context corruption.
+ * Any forwarded request to this contract triggering a `delegatecall` to itself will result in an invalid {_msgSender}
+ * recovery.
  */
 abstract contract ERC2771Context is Context {
     /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
@@ -21,23 +25,30 @@ abstract contract ERC2771Context is Context {
         return forwarder == _trustedForwarder;
     }
 
-    function _msgSender() internal view virtual override returns (address sender) {
-        if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) {
-            // The assembly code is more direct than the Solidity version using `abi.decode`.
-            /// @solidity memory-safe-assembly
-            assembly {
-                sender := shr(96, calldataload(sub(calldatasize(), 20)))
-            }
+    function _msgSender() internal view virtual override returns (address) {
+        uint256 calldataLength = msg.data.length;
+        uint256 contextSuffixLength = _contextSuffixLength();
+        if (isTrustedForwarder(msg.sender) && calldataLength >= contextSuffixLength) {
+            return address(bytes20(msg.data[calldataLength - contextSuffixLength:]));
         } else {
             return super._msgSender();
         }
     }
 
     function _msgData() internal view virtual override returns (bytes calldata) {
-        if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) {
-            return msg.data[:msg.data.length - 20];
+        uint256 calldataLength = msg.data.length;
+        uint256 contextSuffixLength = _contextSuffixLength();
+        if (isTrustedForwarder(msg.sender) && calldataLength >= contextSuffixLength) {
+            return msg.data[:calldataLength - contextSuffixLength];
         } else {
             return super._msgData();
         }
     }
+
+    /**
+     * @dev ERC-2771 specifies the context as being a single address (20 bytes).
+     */
+    function _contextSuffixLength() internal view virtual override returns (uint256) {
+        return 20;
+    }
 }

contracts/mocks/ERC2771ContextMock.sol

@@ -3,10 +3,11 @@
 pragma solidity ^0.8.9;
 
 import "./ContextMock.sol";
+import "../utils/Multicall.sol";
 import "../metatx/ERC2771Context.sol";
 
 // By inheriting from ERC2771Context, Context's internal functions are overridden automatically
-contract ERC2771ContextMock is ContextMock, ERC2771Context {
+contract ERC2771ContextMock is ContextMock, ERC2771Context, Multicall {
     /// @custom:oz-upgrades-unsafe-allow constructor
     constructor(address trustedForwarder) ERC2771Context(trustedForwarder) {
         emit Sender(_msgSender()); // _msgSender() should be accessible during construction
@@ -19,4 +20,8 @@ contract ERC2771ContextMock is ContextMock, ERC2771Context {
     function _msgData() internal view override(Context, ERC2771Context) returns (bytes calldata) {
         return ERC2771Context._msgData();
     }
+
+    function _contextSuffixLength() internal view override(Context, ERC2771Context) returns (uint256) {
+        return ERC2771Context._contextSuffixLength();
+    }
 }

contracts/utils/Context.sol

@@ -21,4 +21,8 @@ abstract contract Context {
     function _msgData() internal view virtual returns (bytes calldata) {
         return msg.data;
     }
+
+    function _contextSuffixLength() internal view virtual returns (uint256) {
+        return 0;
+    }
 }

contracts/utils/Multicall.sol

@@ -4,21 +4,36 @@
 pragma solidity ^0.8.0;
 
 import "./Address.sol";
+import "./Context.sol";
 
 /**
  * @dev Provides a function to batch together multiple calls in a single external call.
  *
+ * Consider any assumption about calldata validation performed by the sender may be violated if it's not especially
+ * careful about sending transactions invoking {multicall}. For example, a relay address that filters function
+ * selectors won't filter calls nested within a {multicall} operation.
+ *
+ * NOTE: Since 5.0.1 and 4.9.4, this contract identifies non-canonical contexts (i.e. `msg.sender` is not {_msgSender}).
+ * If a non-canonical context is identified, the following self `delegatecall` appends the last bytes of `msg.data`
+ * to the subcall. This makes it safe to use with {ERC2771Context}. Contexts that don't affect the resolution of
+ * {_msgSender} are not propagated to subcalls.
+ *
  * _Available since v4.1._
  */
-abstract contract Multicall {
+abstract contract Multicall is Context {
     /**
      * @dev Receives and executes a batch of function calls on this contract.
      * @custom:oz-upgrades-unsafe-allow-reachable delegatecall
      */
     function multicall(bytes[] calldata data) external virtual returns (bytes[] memory results) {
+        bytes memory context = msg.sender == _msgSender()
+            ? new bytes(0)
+            : msg.data[msg.data.length - _contextSuffixLength():];
+
         results = new bytes[](data.length);
         for (uint256 i = 0; i < data.length; i++) {
             results[i] = Address.functionDelegateCall(address(this), data[i]);
+            results[i] = Address.functionDelegateCall(address(this), bytes.concat(data[i], context));
         }
         return results;
     }

docs/modules/api/pages/metatx.adoc

@@ -3,6 +3,7 @@
 :xref-ERC2771Context-isTrustedForwarder-address-: xref:metatx.adoc#ERC2771Context-isTrustedForwarder-address-
 :xref-ERC2771Context-_msgSender--: xref:metatx.adoc#ERC2771Context-_msgSender--
 :xref-ERC2771Context-_msgData--: xref:metatx.adoc#ERC2771Context-_msgData--
+:xref-ERC2771Context-_contextSuffixLength--: xref:metatx.adoc#ERC2771Context-_contextSuffixLength--
 :ERC2771Context: pass:normal[xref:metatx.adoc#ERC2771Context[`ERC2771Context`]]
 :xref-MinimalForwarder-constructor--: xref:metatx.adoc#MinimalForwarder-constructor--
 :xref-MinimalForwarder-getNonce-address-: xref:metatx.adoc#MinimalForwarder-getNonce-address-
@@ -23,6 +24,7 @@ NOTE: This document is better viewed at https://docs.openzeppelin.com/contracts/
 :isTrustedForwarder: pass:normal[xref:#ERC2771Context-isTrustedForwarder-address-[`++isTrustedForwarder++`]]
 :_msgSender: pass:normal[xref:#ERC2771Context-_msgSender--[`++_msgSender++`]]
 :_msgData: pass:normal[xref:#ERC2771Context-_msgData--[`++_msgData++`]]
+:_contextSuffixLength: pass:normal[xref:#ERC2771Context-_contextSuffixLength--[`++_contextSuffixLength++`]]
 
 [.contract]
 [[ERC2771Context]]
@@ -35,6 +37,10 @@ import "@openzeppelin/contracts/metatx/ERC2771Context.sol";
 
 Context variant with ERC2771 support.
 
+WARNING: The usage of `delegatecall` in this contract is dangerous and may result in context corruption.
+Any forwarded request to this contract triggering a `delegatecall` to itself will result in an invalid {_msgSender}
+recovery.
+
 [.contract-index]
 .Functions
 --
@@ -42,6 +48,7 @@ Context variant with ERC2771 support.
 * {xref-ERC2771Context-isTrustedForwarder-address-}[`++isTrustedForwarder(forwarder)++`]
 * {xref-ERC2771Context-_msgSender--}[`++_msgSender()++`]
 * {xref-ERC2771Context-_msgData--}[`++_msgData()++`]
+* {xref-ERC2771Context-_contextSuffixLength--}[`++_contextSuffixLength()++`]
 
 --
 
@@ -55,12 +62,18 @@ Context variant with ERC2771 support.
 
 [.contract-item]
 [[ERC2771Context-_msgSender--]]
-==== `[.contract-item-name]#++_msgSender++#++() → address sender++` [.item-kind]#internal#
+==== `[.contract-item-name]#++_msgSender++#++() → address++` [.item-kind]#internal#
 
 [.contract-item]
 [[ERC2771Context-_msgData--]]
 ==== `[.contract-item-name]#++_msgData++#++() → bytes++` [.item-kind]#internal#
 
+[.contract-item]
+[[ERC2771Context-_contextSuffixLength--]]
+==== `[.contract-item-name]#++_contextSuffixLength++#++() → uint256++` [.item-kind]#internal#
+
+ERC-2771 specifies the context as being a single address (20 bytes).
+
 == Utils
 
 :ForwardRequest: pass:normal[xref:#MinimalForwarder-ForwardRequest[`++ForwardRequest++`]]

docs/modules/api/pages/utils.adoc

@@ -393,6 +393,7 @@
 :xref-StorageSlot-getStringSlot-string-: xref:utils.adoc#StorageSlot-getStringSlot-string-
 :xref-StorageSlot-getBytesSlot-bytes32-: xref:utils.adoc#StorageSlot-getBytesSlot-bytes32-
 :xref-StorageSlot-getBytesSlot-bytes-: xref:utils.adoc#StorageSlot-getBytesSlot-bytes-
+:ERC2771Context: pass:normal[xref:metatx.adoc#ERC2771Context[`ERC2771Context`]]
 :xref-Multicall-multicall-bytes---: xref:utils.adoc#Multicall-multicall-bytes---
 = Utilities
 
@@ -5102,6 +5103,15 @@ import "@openzeppelin/contracts/utils/Multicall.sol";
 
 Provides a function to batch together multiple calls in a single external call.
 
+Consider any assumption about calldata validation performed by the sender may be violated if it's not especially
+careful about sending transactions invoking {multicall}. For example, a relay address that filters function
+selectors won't filter calls nested within a {multicall} operation.
+
+NOTE: Since 5.0.1 and 4.9.4, this contract identifies non-canonical contexts (i.e. `msg.sender` is not {_msgSender}).
+If a non-canonical context is identified, the following self `delegatecall` appends the last bytes of `msg.data`
+to the subcall. This makes it safe to use with {ERC2771Context}. Contexts that don't affect the resolution of
+{_msgSender} are not propagated to subcalls.
+
 _Available since v4.1._
 
 [.contract-index]

test/metatx/ERC2771Context.test.js

@@ -12,7 +12,7 @@ const ContextMockCaller = artifacts.require('ContextMockCaller');
 const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior');
 
 contract('ERC2771Context', function (accounts) {
-  const [, trustedForwarder] = accounts;
+  const [, trustedForwarder, other] = accounts;
 
   beforeEach(async function () {
     this.forwarder = await MinimalForwarder.new();
@@ -118,5 +118,58 @@ contract('ERC2771Context', function (accounts) {
       const data = recipient.contract.methods.msgDataShort().encodeABI();
       await expectEvent(receipt, 'DataShort', { data });
     });
+
+    it('multicall poison attack', async function () {
+      const attacker = Wallet.generate();
+      const attackerAddress = attacker.getChecksumAddressString();
+      const nonce = await this.forwarder.getNonce(attackerAddress);
+
+      const msgSenderCall = web3.eth.abi.encodeFunctionCall(
+        {
+          name: 'msgSender',
+          type: 'function',
+          inputs: [],
+        },
+        [],
+      );
+
+      const data = web3.eth.abi.encodeFunctionCall(
+        {
+          name: 'multicall',
+          type: 'function',
+          inputs: [
+            {
+              internalType: 'bytes[]',
+              name: 'data',
+              type: 'bytes[]',
+            },
+          ],
+        },
+        [[web3.utils.encodePacked({ value: msgSenderCall, type: 'bytes' }, { value: other, type: 'address' })]],
+      );
+
+      const req = {
+        from: attackerAddress,
+        to: this.recipient.address,
+        value: '0',
+        gas: '100000',
+        data,
+        nonce: Number(nonce),
+      };
+
+      const signature = await ethSigUtil.signTypedMessage(attacker.getPrivateKey(), {
+        data: {
+          types: this.types,
+          domain: this.domain,
+          primaryType: 'ForwardRequest',
+          message: req,
+        },
+      });
+
+      expect(await this.forwarder.verify(req, signature)).to.equal(true);
+
+      const receipt = await this.forwarder.execute(req, signature);
+      await expectEvent.inTransaction(receipt.tx, ERC2771ContextMock, 'Sender', { sender: attackerAddress });
+    });
   });
 });