From 451ed01d5f6dd54da4b46b1369276ddc5f8c4cdd Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Mon, 3 Feb 2025 16:39:02 +0300 Subject: [PATCH] secure batch decode + test fix --- contracts/lib/ExecLib.sol | 50 +++++++++++++------ contracts/mocks/MockExecutor.sol | 7 ++- ...AccountExecution_ExecuteFromExecutor.t.sol | 10 +--- 3 files changed, 43 insertions(+), 24 deletions(-) diff --git a/contracts/lib/ExecLib.sol b/contracts/lib/ExecLib.sol index 1d4b4630..16c84df4 100644 --- a/contracts/lib/ExecLib.sol +++ b/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 } + } + } } } diff --git a/contracts/mocks/MockExecutor.sol b/contracts/mocks/MockExecutor.sol index 7efcb025..898a974c 100644 --- a/contracts/mocks/MockExecutor.sol +++ b/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)); } diff --git a/test/foundry/unit/concrete/accountexecution/TestAccountExecution_ExecuteFromExecutor.t.sol b/test/foundry/unit/concrete/accountexecution/TestAccountExecution_ExecuteFromExecutor.t.sol index f9a446cb..c6a0974f 100644 --- a/test/foundry/unit/concrete/accountexecution/TestAccountExecution_ExecuteFromExecutor.t.sol +++ b/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));