Cherrypick #5353 into release-v5.2 (#5367)

Signed-off-by: Hadrien Croubois <[email protected]> Co-authored-by: Ernesto García <[email protected]> Co-authored-by: Francisco Giordano <[email protected]>
OpenZeppelin · Dec 16, 2024 · ab96c48 · ab96c48
1 parent ac4198f
commit ab96c48
Show file tree

Hide file tree

Showing 11 changed files with 528 additions and 27 deletions.
diff --git a/.changeset/seven-insects-taste.md b/.changeset/seven-insects-taste.md
@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+`ERC7579Utils`: Add ABI decoding checks on calldata bounds within `decodeBatch`
diff --git a/contracts/account/utils/draft-ERC7579Utils.sol b/contracts/account/utils/draft-ERC7579Utils.sol
@@ -62,10 +62,13 @@ library ERC7579Utils {
     /// @dev The module type is not supported.
     error ERC7579UnsupportedModuleType(uint256 moduleTypeId);
 
+    /// @dev Input calldata not properly formatted and possibly malicious.
+    error ERC7579DecodingError();
+
     /// @dev Executes a single call.
     function execSingle(
-        ExecType execType,
-        bytes calldata executionCalldata
+        bytes calldata executionCalldata,
+        ExecType execType
     ) internal returns (bytes[] memory returnData) {
         (address target, uint256 value, bytes calldata callData) = decodeSingle(executionCalldata);
         returnData = new bytes[](1);
@@ -74,8 +77,8 @@ library ERC7579Utils {
 
     /// @dev Executes a batch of calls.
     function execBatch(
-        ExecType execType,
-        bytes calldata executionCalldata
+        bytes calldata executionCalldata,
+        ExecType execType
     ) internal returns (bytes[] memory returnData) {
         Execution[] calldata executionBatch = decodeBatch(executionCalldata);
         returnData = new bytes[](executionBatch.length);
@@ -92,8 +95,8 @@ library ERC7579Utils {
 
     /// @dev Executes a delegate call.
     function execDelegateCall(
-        ExecType execType,
-        bytes calldata executionCalldata
+        bytes calldata executionCalldata,
+        ExecType execType
     ) internal returns (bytes[] memory returnData) {
         (address target, bytes calldata callData) = decodeDelegate(executionCalldata);
         returnData = new bytes[](1);
@@ -170,12 +173,40 @@ library ERC7579Utils {
     }
 
     /// @dev Decodes a batch of executions. See {encodeBatch}.
+    ///
+    /// NOTE: This function runs some checks and will throw a {ERC7579DecodingError} if the input is not properly formatted.
     function decodeBatch(bytes calldata executionCalldata) internal pure returns (Execution[] calldata executionBatch) {
-        assembly ("memory-safe") {
-            let ptr := add(executionCalldata.offset, calldataload(executionCalldata.offset))
-            // Extract the ERC7579 Executions
-            executionBatch.offset := add(ptr, 32)
-            executionBatch.length := calldataload(ptr)
+        unchecked {
+            uint256 bufferLength = executionCalldata.length;
+
+            // Check executionCalldata is not empty.
+            if (bufferLength < 32) revert ERC7579DecodingError();
+
+            // Get the offset of the array (pointer to the array length).
+            uint256 arrayLengthOffset = uint256(bytes32(executionCalldata[0:32]));
+
+            // The array length (at arrayLengthOffset) should be 32 bytes long. We check that this is within the
+            // buffer bounds. Since we know bufferLength is at least 32, we can subtract with no overflow risk.
+            if (arrayLengthOffset > bufferLength - 32) revert ERC7579DecodingError();
+
+            // Get the array length. arrayLengthOffset + 32 is bounded by bufferLength so it does not overflow.
+            uint256 arrayLength = uint256(bytes32(executionCalldata[arrayLengthOffset:arrayLengthOffset + 32]));
+
+            // Check that the buffer is long enough to store the array elements as "offset pointer":
+            // - each element of the array is an "offset pointer" to the data.
+            // - each "offset pointer" (to an array element) takes 32 bytes.
+            // - validity of the calldata at that location is checked when the array element is accessed, so we only
+            //   need to check that the buffer is large enough to hold the pointers.
+            //
+            // Since we know bufferLength is at least arrayLengthOffset + 32, we can subtract with no overflow risk.
+            // Solidity limits length of such arrays to 2**64-1, this guarantees `arrayLength * 32` does not overflow.
+            if (arrayLength > type(uint64).max || bufferLength - arrayLengthOffset - 32 < arrayLength * 32)
+                revert ERC7579DecodingError();
+
+            assembly ("memory-safe") {
+                executionBatch.offset := add(add(executionCalldata.offset, arrayLengthOffset), 32)
+                executionBatch.length := arrayLength
+            }
         }
     }
 

diff --git a/foundry.toml b/foundry.toml
@@ -8,6 +8,7 @@ out = 'out'
 libs = ['node_modules', 'lib']
 test = 'test'
 cache_path  = 'cache_forge'
+fs_permissions = [{ access = "read", path = "./test/bin" }]
 
 [fuzz]
 runs = 5000

diff --git a/test/account/utils/draft-ERC4337Utils.test.js b/test/account/utils/draft-ERC4337Utils.test.js
@@ -3,11 +3,13 @@ const { expect } = require('chai');
 const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
 
 const { packValidationData, UserOperation } = require('../../helpers/erc4337');
+const { deployEntrypoint } = require('../../helpers/erc4337-entrypoint');
 const { MAX_UINT48 } = require('../../helpers/constants');
 const ADDRESS_ONE = '0x0000000000000000000000000000000000000001';
 
 const fixture = async () => {
-  const [authorizer, sender, entrypoint, factory, paymaster] = await ethers.getSigners();
+  const { entrypoint } = await deployEntrypoint();
+  const [authorizer, sender, factory, paymaster] = await ethers.getSigners();
   const utils = await ethers.deployContract('$ERC4337Utils');
   const SIG_VALIDATION_SUCCESS = await utils.$SIG_VALIDATION_SUCCESS();
   const SIG_VALIDATION_FAILED = await utils.$SIG_VALIDATION_FAILED();
@@ -167,11 +169,19 @@ describe('ERC4337Utils', function () {
   describe('hash', function () {
     it('returns the operation hash with specified entrypoint and chainId', async function () {
       const userOp = new UserOperation({ sender: this.sender, nonce: 1 });
-      const chainId = 0xdeadbeef;
+      const chainId = await ethers.provider.getNetwork().then(({ chainId }) => chainId);
+      const otherChainId = 0xdeadbeef;
 
+      // check that helper matches entrypoint logic
+      expect(this.entrypoint.getUserOpHash(userOp.packed)).to.eventually.equal(userOp.hash(this.entrypoint, chainId));
+
+      // check library against helper
       expect(this.utils.$hash(userOp.packed, this.entrypoint, chainId)).to.eventually.equal(
         userOp.hash(this.entrypoint, chainId),
       );
+      expect(this.utils.$hash(userOp.packed, this.entrypoint, otherChainId)).to.eventually.equal(
+        userOp.hash(this.entrypoint, otherChainId),
+      );
     });
   });