CVE-2023-40014: Improper Encoding or Escaping of Output (CWE-116)
value: OpenZeppelin Contracts is a library for secure smart contract development. Starting in version 4.0.0 and prior to version 4.9.3, contracts using `ERC2771Context` along with a custom trusted forwarder may see `_msgSender` return `address(0)` in calls that originate from the forwarder with calldata shorter than 20 bytes. This combination of circumstances does not appear to be common, in particular it is not the case for `MinimalForwarder` from OpenZeppelin Contracts, or any deployed forwarder the team is aware of, given that the signer address is appended to all calls that originate from these forwarders. The problem has been patched in v4.9.3.

CWE description: The product prepares a structured message for communication with another component, but encoding or escaping of the data is either missing or done incorrectly. As a result, the intended structure of the message is not preserved.
CVSS v3 base score: 5.3
Published: 2023-08-10T20:15Z
Repository: https://github.com/OpenZeppelin/openzeppelin-contracts
Language: Other
Files changed: .changeset/warm-guests-rule.md, contracts/metatx/ERC2771Context.sol, contracts/mocks/ContextMock.sol, test/metatx/ERC2771Context.test.js
Commit date: 2023-07-27 17:30:41 -0300
Commit message: Adjust ERC2771Context._msgData for msg.data.length < 20 (#4484)


Diff: diff --git a/.changeset/warm-guests-rule.md b/.changeset/warm-guests-rule.md
new file mode 100644
index 000000000..049da4dd5
--- /dev/null
+++ b/.changeset/warm-guests-rule.md
@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+`ERC2771Context`: Prevent revert in `_msgData()` when a call originating from a trusted forwarder is not long enough to contain the request signer address (i.e. `msg.data.length` is less than 20 bytes). Return the full calldata in that case.
diff --git a/contracts/metatx/ERC2771Context.sol b/contracts/metatx/ERC2771Context.sol
index 9ade6148d..2c4d89f94 100644
--- a/contracts/metatx/ERC2771Context.sol
+++ b/contracts/metatx/ERC2771Context.sol
@@ -32,11 +32,11 @@ abstract contract ERC2771Context is Context {
             return super._msgSender();
         }
     }
 
     function _msgData() internal view virtual override returns (bytes calldata) {
-        if (isTrustedForwarder(msg.sender)) {
+        if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) {
             return msg.data[:msg.data.length - 20];
         } else {
             return super._msgData();
         }
     }
diff --git a/contracts/mocks/ContextMock.sol b/contracts/mocks/ContextMock.sol
index fd105aa91..fb57535f7 100644
--- a/contracts/mocks/ContextMock.sol
+++ b/contracts/mocks/ContextMock.sol
@@ -14,10 +14,16 @@ contract ContextMock is Context {
     event Data(bytes data, uint256 integerValue, string stringValue);
 
     function msgData(uint256 integerValue, string memory stringValue) public {
         emit Data(_msgData(), integerValue, stringValue);
     }
+
+    event DataShort(bytes data);
+
+    function msgDataShort() public {
+        emit DataShort(_msgData());
+    }
 }
 
 contract ContextMockCaller {
     function callSender(ContextMock context) public {
         context.msgSender();
diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js
index a907e502c..0ec8d98dd 100644
--- a/test/metatx/ERC2771Context.test.js
+++ b/test/metatx/ERC2771Context.test.js
@@ -10,11 +10,11 @@ const ERC2771Forwarder = artifacts.require('ERC2771Forwarder');
 const ContextMockCaller = artifacts.require('ContextMockCaller');
 
 const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior');
 
 contract('ERC2771Context', function (accounts) {
-  const [, anotherAccount] = accounts;
+  const [, trustedForwarder] = accounts;
 
   const MAX_UINT48 = web3.utils.toBN(1).shln(48).subn(1).toString();
 
   beforeEach(async function () {
     this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder');
@@ -82,15 +82,15 @@ contract('ERC2771Context', function (accounts) {
         await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Sender', { sender: this.sender });
       });
 
       it('returns the original sender when calldata length is less than 20 bytes (address length)', async function () {
         // The forwarder doesn't produce calls with calldata length less than 20 bytes
-        const recipient = await ERC2771ContextMock.new(anotherAccount);
+        const recipient = await ERC2771ContextMock.new(trustedForwarder);
 
-        const { receipt } = await recipient.msgSender({ from: anotherAccount });
+        const { receipt } = await recipient.msgSender({ from: trustedForwarder });
 
-        await expectEvent(receipt, 'Sender', { sender: anotherAccount });
+        await expectEvent(receipt, 'Sender', { sender: trustedForwarder });
       });
     });
 
     describe('msgData', function () {
       it('returns the relayed transaction original data', async function () {
@@ -115,7 +115,17 @@ contract('ERC2771Context', function (accounts) {
 
         const { tx } = await this.forwarder.execute(req);
         await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Data', { data, integerValue, stringValue });
       });
     });
+
+    it('returns the full original data when calldata length is less than 20 bytes (address length)', async function () {
+      // The forwarder doesn't produce calls with calldata length less than 20 bytes
+      const recipient = await ERC2771ContextMock.new(trustedForwarder);
+
+      const { receipt } = await recipient.msgDataShort({ from: trustedForwarder });
+
+      const data = recipient.contract.methods.msgDataShort().encodeABI();
+      await expectEvent(receipt, 'DataShort', { data });
+    });
   });
 });

Vulnerable code:         if (isTrustedForwarder(msg.sender)) {
  const [, anotherAccount] = accounts;
        const recipient = await ERC2771ContextMock.new(anotherAccount);
        const { receipt } = await recipient.msgSender({ from: anotherAccount });
        await expectEvent(receipt, 'Sender', { sender: anotherAccount });
Fixed code: ---
'openzeppelin-solidity': patch
---

`ERC2771Context`: Prevent revert in `_msgData()` when a call originating from a trusted forwarder is not long enough to contain the request signer address (i.e. `msg.data.length` is less than 20 bytes). Return the full calldata in that case.
        if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) {

    event DataShort(bytes data);

    function msgDataShort() public {
        emit DataShort(_msgData());
    }
  const [, trustedForwarder] = accounts;
        const recipient = await ERC2771ContextMock.new(trustedForwarder);
        const { receipt } = await recipient.msgSender({ from: trustedForwarder });
        await expectEvent(receipt, 'Sender', { sender: trustedForwarder });

    it('returns the full original data when calldata length is less than 20 bytes (address length)', async function () {
      // The forwarder doesn't produce calls with calldata length less than 20 bytes
      const recipient = await ERC2771ContextMock.new(trustedForwarder);

      const { receipt } = await recipient.msgDataShort({ from: trustedForwarder });

      const data = recipient.contract.methods.msgDataShort().encodeABI();
      await expectEvent(receipt, 'DataShort', { data });
    });