Merge pull request #235 from bcnmy/fix/decodeBatch-security

Secure batch decode implementation

contracts/lib/ExecLib.sol

@@ -29,20 +29,42 @@ library ExecLib {
         }
     }
 
-    function decodeBatch(bytes calldata callData) internal pure returns (Execution[] calldata executionBatch) {
-        /*
-         * Batch Call Calldata Layout
-         * Offset (in bytes)    | Length (in bytes) | Contents
-         * 0x0                  | 0x4               | bytes4 function selector
-         * 0x4                  | -                 |
-        abi.encode(IERC7579Execution.Execution[])
-         */
-        assembly ("memory-safe") {
-            let dataPointer := add(callData.offset, calldataload(callData.offset))
-
-            // Extract the ERC7579 Executions
-            executionBatch.offset := add(dataPointer, 32)
-            executionBatch.length := calldataload(dataPointer)
+    /**
+     * @notice Decode a batch of `Execution` executionBatch from a `bytes` calldata.
+     * @dev code is copied from solady's LibERC7579.sol
+     * https://github.com/Vectorized/solady/blob/740812cedc9a1fc11e17cb3d4569744367dedf19/src/accounts/LibERC7579.sol#L146
+     *      Credits to Vectorized and the Solady Team
+     */
+    function decodeBatch(bytes calldata executionCalldata) internal pure returns (Execution[] calldata executionBatch) {
+        /// @solidity memory-safe-assembly
+        assembly {
+            let u := calldataload(executionCalldata.offset)
+            let s := add(executionCalldata.offset, u)
+            let e := sub(add(executionCalldata.offset, executionCalldata.length), 0x20)
+            executionBatch.offset := add(s, 0x20)
+            executionBatch.length := calldataload(s)
+            if or(shr(64, u), gt(add(s, shl(5, executionBatch.length)), e)) {
+                mstore(0x00, 0xba597e7e) // `DecodingError()`.
+                revert(0x1c, 0x04)
+            }
+            if executionBatch.length {
+                // Perform bounds checks on the decoded `executionBatch`.
+                // Loop runs out-of-gas if `executionBatch.length` is big enough to cause overflows.
+                for { let i := executionBatch.length } 1 { } {
+                    i := sub(i, 1)
+                    let p := calldataload(add(executionBatch.offset, shl(5, i)))
+                    let c := add(executionBatch.offset, p)
+                    let q := calldataload(add(c, 0x40))
+                    let o := add(c, q)
+                    // forgefmt: disable-next-item
+                    if or(shr(64, or(calldataload(o), or(p, q))),
+                        or(gt(add(c, 0x40), e), gt(add(o, calldataload(o)), e))) {
+                        mstore(0x00, 0xba597e7e) // `DecodingError()`.
+                        revert(0x1c, 0x04)
+                    }
+                    if iszero(i) { break }
+                }
+            }
         }
     }
 

contracts/mocks/MockExecutor.sol

@@ -54,15 +54,18 @@ contract MockExecutor is IExecutor {
         address target,
         uint256 value,
         bytes calldata callData
-    ) external returns (bytes[] memory returnData) {
+    ) external returns (bytes[] memory) {
         (CallType callType, ) = ModeLib.decodeBasic(mode);
         bytes memory executionCallData;
         if (callType == CALLTYPE_SINGLE) {
             executionCallData = ExecLib.encodeSingle(target, value, callData);
+            return account.executeFromExecutor(mode, executionCallData);
         } else if (callType == CALLTYPE_BATCH) {
-            Execution[] memory execution = new Execution[](1);
+            Execution[] memory execution = new Execution[](2);
             execution[0] = Execution(target, 0, callData);
+            execution[1] = Execution(address(this), 0, executionCallData);
             executionCallData = ExecLib.encodeBatch(execution);
+            return account.executeFromExecutor(mode, executionCallData);
         }
         return account.executeFromExecutor(mode, ExecLib.encodeSingle(target, value, callData));
     }

test/foundry/unit/concrete/accountexecution/TestAccountExecution_ExecuteFromExecutor.t.sol

@@ -198,11 +198,8 @@ contract TestAccountExecution_ExecuteFromExecutor is TestAccountExecution_Base {
     /// @notice Tests execution with an unsupported call type via MockExecutor
     function test_RevertIf_ExecuteFromExecutor_UnsupportedCallType() public {
         ExecutionMode unsupportedMode = ExecutionMode.wrap(bytes32(abi.encodePacked(bytes1(0xee), bytes1(0x00), bytes4(0), bytes22(0))));
-        bytes memory executionCalldata = abi.encodePacked(address(counter), uint256(0), abi.encodeWithSelector(Counter.incrementNumber.selector));
 
         (CallType callType, , , ) = ModeLib.decode(unsupportedMode);
-        Execution[] memory execution = new Execution[](1);
-        execution[0] = Execution(address(mockExecutor), 0, executionCalldata);
 
         vm.expectRevert(abi.encodeWithSelector(UnsupportedCallType.selector, callType));
 
@@ -219,13 +216,10 @@ contract TestAccountExecution_ExecuteFromExecutor is TestAccountExecution_Base {
     function test_RevertIf_ExecuteFromExecutor_UnsupportedExecType_Batch() public {
         // Create an unsupported execution mode with an invalid execution type
         ExecutionMode unsupportedMode = ExecutionMode.wrap(bytes32(abi.encodePacked(CALLTYPE_BATCH, bytes1(0xff), bytes4(0), bytes22(0))));
-        bytes memory executionCalldata = abi.encodePacked(address(counter), uint256(0), abi.encodeWithSelector(Counter.incrementNumber.selector));
-
+
         // Decode the mode to extract the execution type for the expected revert
         (, ExecType execType, , ) = ModeLib.decode(unsupportedMode);
-        Execution[] memory execution = new Execution[](1);
-        execution[0] = Execution(address(mockExecutor), 0, executionCalldata);
-
+
         // Expect the revert with UnsupportedExecType error
         vm.expectRevert(abi.encodeWithSelector(UnsupportedExecType.selector, execType));