Add checks to ERC7579Utils.decodeBatch

contracts/account/utils/draft-ERC4337Utils.sol

@@ -99,33 +99,6 @@ library ERC4337Utils {
         return result;
     }
 
-    /// @dev Variant of {hash} that supports user operations in memory.
-    function hashMemory(
-        PackedUserOperation memory self,
-        address entrypoint,
-        uint256 chainid
-    ) internal pure returns (bytes32) {
-        bytes32 result = keccak256(
-            abi.encode(
-                keccak256(
-                    abi.encode(
-                        self.sender,
-                        self.nonce,
-                        keccak256(self.initCode),
-                        keccak256(self.callData),
-                        self.accountGasLimits,
-                        self.preVerificationGas,
-                        self.gasFees,
-                        keccak256(self.paymasterAndData)
-                    )
-                ),
-                entrypoint,
-                chainid
-            )
-        );
-        return result;
-    }
-
     /// @dev Returns `factory` from the {PackedUserOperation}, or address(0) if the initCode is empty or not properly formatted.
     function factory(PackedUserOperation calldata self) internal pure returns (address) {
         return self.initCode.length < 20 ? address(0) : address(bytes20(self.initCode[0:20]));

contracts/account/utils/draft-ERC7579Utils.sol

@@ -178,23 +178,27 @@ library ERC7579Utils {
         unchecked {
             uint256 bufferLength = executionCalldata.length;
 
-            // Check executionCalldata is not empty
+            // Check executionCalldata is not empty.
             if (bufferLength < 32) revert ERC7579DecodingError();
 
-            // Get the offset of the array, and check its valid
+            // Get the offset of the array.
             uint256 offset = uint256(bytes32(executionCalldata[0:32]));
 
-            // Array length is between offset and offset + 32. We check that this is in the buffer
-            // Since we know executionCalldata is at least 32, we can subtract with no overflow risk.
+            // The array length should be found at offset and be 32 bytes long. We check that this is within the
+            // buffer bounds. Since we know executionCalldata is at least 32, we can subtract with no overflow risk.
             if (offset > bufferLength - 32) revert ERC7579DecodingError();
 
-            // Get the array length
+            // Get the array length. offset + 32 is bounded by bufferLength so does not overflow.
             uint256 arrayLength = uint256(bytes32(executionCalldata[offset:offset + 32]));
             if (arrayLength > type(uint64).max) revert ERC7579DecodingError();
 
-            // Get the array as a bytes slice, and check its is long enough
+            // Get the array as a bytes slice, and check it is long enough:
+            // - each element of the array is an "offset pointer" to the data
+            //   - each offset pointer takes 32 bytes
+            //   - validity of the calldata at that location is checked when the array element is accessed.
+            // - `arrayLength * 32` does not overflow because `arrayLength` is less than `2**64`.
             bytes calldata executionArray = executionCalldata[offset + 32:];
-            if (executionArray.length < arrayLength * 96) revert ERC7579DecodingError();
+            if (executionArray.length < arrayLength * 32) revert ERC7579DecodingError();
 
             assembly ("memory-safe") {
                 executionBatch.offset := executionArray.offset

test/account/utils/draft-ERC7579Utils.t.sol

@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.24;
 
+// Parts of this test file are adapted for Adam Egyed (@adamegyed) proof of concept available at:
+// https://github.com/adamegyed/erc7579-execute-vulnerability/tree/4589a30ff139e143d6c57183ac62b5c029217a90
+//
 // solhint-disable no-console
 
 import {Test} from "forge-std/Test.sol";
@@ -161,7 +164,7 @@ contract ERC7579UtilsTest is Test {
 
         (uint8 v, bytes32 r, bytes32 s) = vm.sign(
             _ownerKey,
-            userOps[0].hashMemory(address(ENTRYPOINT), block.chainid).toEthSignedMessageHash()
+            this.hashUserOperation(userOps[0]).toEthSignedMessageHash()
         );
         userOps[0].signature = abi.encodePacked(r, s, v);
 
@@ -212,7 +215,7 @@ contract ERC7579UtilsTest is Test {
 
         (uint8 v, bytes32 r, bytes32 s) = vm.sign(
             _ownerKey,
-            userOps[0].hashMemory(address(ENTRYPOINT), block.chainid).toEthSignedMessageHash()
+            this.hashUserOperation(userOps[0]).toEthSignedMessageHash()
         );
         userOps[0].signature = abi.encodePacked(r, s, v);
 
@@ -270,7 +273,7 @@ contract ERC7579UtilsTest is Test {
 
         (uint8 v, bytes32 r, bytes32 s) = vm.sign(
             _ownerKey,
-            userOps[0].hashMemory(address(ENTRYPOINT), block.chainid).toEthSignedMessageHash()
+            this.hashUserOperation(userOps[0]).toEthSignedMessageHash()
         );
         userOps[0].signature = abi.encodePacked(r, s, v);
 
@@ -298,8 +301,9 @@ contract ERC7579UtilsTest is Test {
 
         // GOOD
         this.callDecodeBatch(abi.encode(0));
-        // Note: Solidity also supports this even though it's odd
-        uint256[] memory _1 = abi.decode(abi.encode(0), (uint256[])); _1;
+        // Note: Solidity also supports this even though it's odd. Offset 0 means array is at the same location, which
+        // is interpreted as an array of length 0, which doesn't require any more data
+        abi.decode(abi.encode(0), (uint256[]));
 
         // BAD: offset is out of bounds
         vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector);
@@ -310,18 +314,53 @@ contract ERC7579UtilsTest is Test {
 
         // BAD: reported array length extends beyond bounds
         vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector);
-        this.callDecodeBatch(abi.encode(32, 1, 0, 0));
+        this.callDecodeBatch(abi.encode(32, 1));
 
         // GOOD
-        this.callDecodeBatch(abi.encode(32, 1, 0, 0, 0));
+        this.callDecodeBatch(abi.encode(32, 1, 0));
 
         // GOOD
-        assertEq("", this.callDecodeBatchAndGetFirstBytes(abi.encode(32, 1, 0, 0, 96, 0)));
+        //
+        // 0000000000000000000000000000000000000000000000000000000000000020 (32) offset
+        // 0000000000000000000000000000000000000000000000000000000000000001 ( 1) array length
+        // 0000000000000000000000000000000000000000000000000000000000000020 (32) element 0 offset
+        // 000000000000000000000000xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (recipient) target for element #0
+        // 000000000000000000000000000000000000000000000000000000000000002a (42) value for element #0
+        // 0000000000000000000000000000000000000000000000000000000000000060 (96) offset to calldata for element #0
+        // 000000000000000000000000000000000000000000000000000000000000000c (12) length of the calldata for element #0
+        // 48656c6c6f20576f726c64210000000000000000000000000000000000000000 (..) buffer for the calldata for element #0
+        assertEq(
+            bytes("Hello World!"),
+            this.callDecodeBatchAndGetFirstBytes(
+                abi.encode(32, 1, 32, _recipient1, 42, 96, 12, bytes12(0x48656c6c6f20576f726c6421))
+            )
+        );
 
-        // this is invalid: the bytes field of the first element of the array points out of bounds
+        // This is invalid, the first element of the array points is out of bounds
         // but we allow it past initial validation, because solidity will validate later when the bytes field is accessed
-        bytes memory invalidDeeply = abi.encode(32, 1, 0, 0, 96);
+        //
+        // 0000000000000000000000000000000000000000000000000000000000000020 (32) offset
+        // 0000000000000000000000000000000000000000000000000000000000000001 ( 1) array length
+        // 0000000000000000000000000000000000000000000000000000000000000020 (32) element 0 offset
+        // <missing element>
+        bytes memory invalid = abi.encode(32, 1, 32);
+        this.callDecodeBatch(invalid);
+        vm.expectRevert();
+        this.callDecodeBatchAndGetFirst(invalid);
+
+        // this is invalid: the bytes field of the first element of the array is out of bounds
+        // but we allow it past initial validation, because solidity will validate later when the bytes field is accessed
+        //
+        // 0000000000000000000000000000000000000000000000000000000000000020 (32) offset
+        // 0000000000000000000000000000000000000000000000000000000000000001 ( 1) array length
+        // 0000000000000000000000000000000000000000000000000000000000000020 (32) element 0 offset
+        // 000000000000000000000000xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (recipient) target for element #0
+        // 000000000000000000000000000000000000000000000000000000000000002a (42) value for element #0
+        // 0000000000000000000000000000000000000000000000000000000000000060 (96) offset to calldata for element #0
+        // <missing data>
+        bytes memory invalidDeeply = abi.encode(32, 1, 32, _recipient1, 42, 96);
         this.callDecodeBatch(invalidDeeply);
+        // Note that this is ok because we don't return the value. Returning it would introduce a check that would fails.
         this.callDecodeBatchAndGetFirst(invalidDeeply);
         vm.expectRevert();
         this.callDecodeBatchAndGetFirstBytes(invalidDeeply);
@@ -335,10 +374,14 @@ contract ERC7579UtilsTest is Test {
         ERC7579Utils.decodeBatch(executionCalldata)[0];
     }
 
-    function callDecodeBatchAndGetFirstBytes(bytes calldata executionCalldata) public pure returns (bytes memory) {
+    function callDecodeBatchAndGetFirstBytes(bytes calldata executionCalldata) public pure returns (bytes calldata) {
         return ERC7579Utils.decodeBatch(executionCalldata)[0].callData;
     }
 
+    function hashUserOperation(PackedUserOperation calldata useroperation) public view returns (bytes32) {
+        return useroperation.hash(address(ENTRYPOINT), block.chainid);
+    }
+
     function _collectAndPrintLogs(bool includeTotalValue) internal {
         Vm.Log[] memory logs = vm.getRecordedLogs();
         for (uint256 i = 0; i < logs.length; i++) {