From 8a95166d945ac38a2166a1b824c7115fb93d7e01 Mon Sep 17 00:00:00 2001 From: adam Date: Thu, 10 Oct 2024 18:36:22 -0400 Subject: [PATCH] feat: skip return data on self call --- ...ModularAccount_Runtime_BatchTransfers.snap | 2 +- .../ModularAccount_Runtime_Erc20Transfer.snap | 2 +- ...count_Runtime_InstallSessionKey_Case1.snap | 2 +- ...ModularAccount_Runtime_NativeTransfer.snap | 2 +- ...t_Runtime_UseSessionKey_Case1_Counter.snap | 2 +- ...unt_Runtime_UseSessionKey_Case1_Token.snap | 2 +- .../ModularAccount_UserOp_BatchTransfers.snap | 2 +- .../ModularAccount_UserOp_Erc20Transfer.snap | 2 +- .../ModularAccount_UserOp_NativeTransfer.snap | 2 +- ...nt_UserOp_UseSessionKey_Case1_Counter.snap | 2 +- ...ount_UserOp_UseSessionKey_Case1_Token.snap | 2 +- ...ularAccount_UserOp_deferredValidation.snap | 2 +- ...ModularAccount_Runtime_BatchTransfers.snap | 2 +- ...iModularAccount_Runtime_Erc20Transfer.snap | 2 +- ...count_Runtime_InstallSessionKey_Case1.snap | 2 +- ...ModularAccount_Runtime_NativeTransfer.snap | 2 +- ...t_Runtime_UseSessionKey_Case1_Counter.snap | 2 +- ...unt_Runtime_UseSessionKey_Case1_Token.snap | 2 +- ...iModularAccount_UserOp_BatchTransfers.snap | 2 +- ...miModularAccount_UserOp_Erc20Transfer.snap | 2 +- ...ccount_UserOp_InstallSessionKey_Case1.snap | 2 +- ...iModularAccount_UserOp_NativeTransfer.snap | 2 +- ...nt_UserOp_UseSessionKey_Case1_Counter.snap | 2 +- ...ount_UserOp_UseSessionKey_Case1_Token.snap | 2 +- ...ularAccount_UserOp_deferredValidation.snap | 2 +- src/account/ModularAccountBase.sol | 28 ++++++++++---- src/libraries/ExecutionLib.sol | 37 ++++--------------- test/account/DirectCallsFromModule.t.sol | 2 +- 28 files changed, 54 insertions(+), 63 deletions(-) diff --git a/.forge-snapshots/ModularAccount_Runtime_BatchTransfers.snap b/.forge-snapshots/ModularAccount_Runtime_BatchTransfers.snap index fa2577c1e..81620e08b 100644 --- a/.forge-snapshots/ModularAccount_Runtime_BatchTransfers.snap +++ b/.forge-snapshots/ModularAccount_Runtime_BatchTransfers.snap @@ -1 +1 @@ -93449 \ No newline at end of file +93127 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_Runtime_Erc20Transfer.snap b/.forge-snapshots/ModularAccount_Runtime_Erc20Transfer.snap index 7e01d02ff..b40fed9a0 100644 --- a/.forge-snapshots/ModularAccount_Runtime_Erc20Transfer.snap +++ b/.forge-snapshots/ModularAccount_Runtime_Erc20Transfer.snap @@ -1 +1 @@ -78730 \ No newline at end of file +78552 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_Runtime_InstallSessionKey_Case1.snap b/.forge-snapshots/ModularAccount_Runtime_InstallSessionKey_Case1.snap index 99b27dc8f..b5f4c9d08 100644 --- a/.forge-snapshots/ModularAccount_Runtime_InstallSessionKey_Case1.snap +++ b/.forge-snapshots/ModularAccount_Runtime_InstallSessionKey_Case1.snap @@ -1 +1 @@ -424033 \ No newline at end of file +424043 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_Runtime_NativeTransfer.snap b/.forge-snapshots/ModularAccount_Runtime_NativeTransfer.snap index c519b1ccf..a51d6cdbb 100644 --- a/.forge-snapshots/ModularAccount_Runtime_NativeTransfer.snap +++ b/.forge-snapshots/ModularAccount_Runtime_NativeTransfer.snap @@ -1 +1 @@ -54870 \ No newline at end of file +54701 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Counter.snap b/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Counter.snap index aa2e3ffa0..b11e38a41 100644 --- a/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Counter.snap +++ b/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Counter.snap @@ -1 +1 @@ -79712 \ No newline at end of file +79540 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Token.snap b/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Token.snap index 10623b34c..64354daac 100644 --- a/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Token.snap +++ b/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Token.snap @@ -1 +1 @@ -113276 \ No newline at end of file +113098 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_BatchTransfers.snap b/.forge-snapshots/ModularAccount_UserOp_BatchTransfers.snap index e4cd94a53..f8918f329 100644 --- a/.forge-snapshots/ModularAccount_UserOp_BatchTransfers.snap +++ b/.forge-snapshots/ModularAccount_UserOp_BatchTransfers.snap @@ -1 +1 @@ -200346 \ No newline at end of file +198425 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_Erc20Transfer.snap b/.forge-snapshots/ModularAccount_UserOp_Erc20Transfer.snap index 92151ccd6..62aded4ee 100644 --- a/.forge-snapshots/ModularAccount_UserOp_Erc20Transfer.snap +++ b/.forge-snapshots/ModularAccount_UserOp_Erc20Transfer.snap @@ -1 +1 @@ -185789 \ No newline at end of file +185394 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_NativeTransfer.snap b/.forge-snapshots/ModularAccount_UserOp_NativeTransfer.snap index 362a03a5c..436151f5b 100644 --- a/.forge-snapshots/ModularAccount_UserOp_NativeTransfer.snap +++ b/.forge-snapshots/ModularAccount_UserOp_NativeTransfer.snap @@ -1 +1 @@ -161972 \ No newline at end of file +161675 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Counter.snap b/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Counter.snap index 065ee73dc..16ba32038 100644 --- a/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Counter.snap +++ b/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Counter.snap @@ -1 +1 @@ -196094 \ No newline at end of file +195915 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Token.snap b/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Token.snap index 472c16e0b..3c2f4150f 100644 --- a/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Token.snap +++ b/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Token.snap @@ -1 +1 @@ -227224 \ No newline at end of file +227039 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_deferredValidation.snap b/.forge-snapshots/ModularAccount_UserOp_deferredValidation.snap index 12cfab647..226c84076 100644 --- a/.forge-snapshots/ModularAccount_UserOp_deferredValidation.snap +++ b/.forge-snapshots/ModularAccount_UserOp_deferredValidation.snap @@ -1 +1 @@ -257304 \ No newline at end of file +257016 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_Runtime_BatchTransfers.snap b/.forge-snapshots/SemiModularAccount_Runtime_BatchTransfers.snap index b4d0e92f2..134271761 100644 --- a/.forge-snapshots/SemiModularAccount_Runtime_BatchTransfers.snap +++ b/.forge-snapshots/SemiModularAccount_Runtime_BatchTransfers.snap @@ -1 +1 @@ -89828 \ No newline at end of file +89614 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_Runtime_Erc20Transfer.snap b/.forge-snapshots/SemiModularAccount_Runtime_Erc20Transfer.snap index 6fc0c70ce..77e65f141 100644 --- a/.forge-snapshots/SemiModularAccount_Runtime_Erc20Transfer.snap +++ b/.forge-snapshots/SemiModularAccount_Runtime_Erc20Transfer.snap @@ -1 +1 @@ -75176 \ No newline at end of file +75094 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_Runtime_InstallSessionKey_Case1.snap b/.forge-snapshots/SemiModularAccount_Runtime_InstallSessionKey_Case1.snap index 0edbc28a2..4adee42c2 100644 --- a/.forge-snapshots/SemiModularAccount_Runtime_InstallSessionKey_Case1.snap +++ b/.forge-snapshots/SemiModularAccount_Runtime_InstallSessionKey_Case1.snap @@ -1 +1 @@ -422963 \ No newline at end of file +423069 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_Runtime_NativeTransfer.snap b/.forge-snapshots/SemiModularAccount_Runtime_NativeTransfer.snap index e4b032b54..f7cde7586 100644 --- a/.forge-snapshots/SemiModularAccount_Runtime_NativeTransfer.snap +++ b/.forge-snapshots/SemiModularAccount_Runtime_NativeTransfer.snap @@ -1 +1 @@ -51317 \ No newline at end of file +51244 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Counter.snap b/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Counter.snap index 99ac939b4..cb3462b52 100644 --- a/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Counter.snap +++ b/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Counter.snap @@ -1 +1 @@ -80150 \ No newline at end of file +80038 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Token.snap b/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Token.snap index 77b5b02fa..b51b43bf0 100644 --- a/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Token.snap +++ b/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Token.snap @@ -1 +1 @@ -113714 \ No newline at end of file +113596 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_BatchTransfers.snap b/.forge-snapshots/SemiModularAccount_UserOp_BatchTransfers.snap index bc6e91448..7e6925065 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_BatchTransfers.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_BatchTransfers.snap @@ -1 +1 @@ -195544 \ No newline at end of file +193755 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_Erc20Transfer.snap b/.forge-snapshots/SemiModularAccount_UserOp_Erc20Transfer.snap index 3a56129c9..e9fdcd8ff 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_Erc20Transfer.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_Erc20Transfer.snap @@ -1 +1 @@ -181114 \ No newline at end of file +180839 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_InstallSessionKey_Case1.snap b/.forge-snapshots/SemiModularAccount_UserOp_InstallSessionKey_Case1.snap index 3cdfa8137..d78c62286 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_InstallSessionKey_Case1.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_InstallSessionKey_Case1.snap @@ -1 +1 @@ -529814 \ No newline at end of file +529934 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_NativeTransfer.snap b/.forge-snapshots/SemiModularAccount_UserOp_NativeTransfer.snap index 9a37bd23b..a509ee65c 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_NativeTransfer.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_NativeTransfer.snap @@ -1 +1 @@ -157291 \ No newline at end of file +157114 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Counter.snap b/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Counter.snap index 0dcb01a51..8426f1a20 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Counter.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Counter.snap @@ -1 +1 @@ -196335 \ No newline at end of file +196336 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Token.snap b/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Token.snap index 6e00a5bd8..14840318b 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Token.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Token.snap @@ -1 +1 @@ -227477 \ No newline at end of file +227472 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_deferredValidation.snap b/.forge-snapshots/SemiModularAccount_UserOp_deferredValidation.snap index 822a34437..6ceed8a7d 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_deferredValidation.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_deferredValidation.snap @@ -1 +1 @@ -253744 \ No newline at end of file +253636 \ No newline at end of file diff --git a/src/account/ModularAccountBase.sol b/src/account/ModularAccountBase.sol index fc02461a4..75f14eb69 100644 --- a/src/account/ModularAccountBase.sol +++ b/src/account/ModularAccountBase.sol @@ -236,7 +236,7 @@ abstract contract ModularAccountBase is bytes memory callData = ExecutionLib.getExecuteUOCallData(callBuffer, userOp.callData); // Manually call self, without collecting return data unless there's a revert. - ExecutionLib.callSelfBubbleOnRevert(callData); + ExecutionLib.callBubbleOnRevert(address(this), 0, callData); ExecutionLib.doCachedPostHooks(postHookData); } @@ -250,7 +250,12 @@ abstract contract ModularAccountBase is wrapNativeFunction returns (bytes memory result) { - result = ExecutionLib.exec(target, value, data); + ExecutionLib.callBubbleOnRevertTransient(target, value, data); + + // Only return data if not called by the EntryPoint + if (msg.sender != address(_ENTRY_POINT)) { + result = ExecutionLib.collectReturnData(); + } } /// @inheritdoc IModularAccount @@ -262,11 +267,20 @@ abstract contract ModularAccountBase is wrapNativeFunction returns (bytes[] memory results) { + bool needReturnData = (msg.sender != address(_ENTRY_POINT)); + uint256 callsLength = calls.length; - results = new bytes[](callsLength); + + if (needReturnData) { + results = new bytes[](callsLength); + } for (uint256 i = 0; i < callsLength; ++i) { - results[i] = ExecutionLib.exec(calls[i].target, calls[i].value, calls[i].data); + ExecutionLib.callBubbleOnRevertTransient(calls[i].target, calls[i].value, calls[i].data); + + if (needReturnData) { + results[i] = ExecutionLib.collectReturnData(); + } } } @@ -301,8 +315,8 @@ abstract contract ModularAccountBase is // Execute the call, reusing the already-allocated RT call buffers, if it exists. // In practice, this is cheaper than attempting to coalesce the (possibly two) buffers. - bytes memory returnData = - ExecutionLib.exec(address(this), 0 wei, ExecutionLib.getCallData(rtCallBuffer, data)); + ExecutionLib.callBubbleOnRevert(address(this), 0 wei, ExecutionLib.getCallData(rtCallBuffer, data)); + bytes memory returnData = ExecutionLib.collectReturnData(); ExecutionLib.doCachedPostHooks(postHookData); @@ -460,7 +474,7 @@ abstract contract ModularAccountBase is validationData = uint256(deadline) << 160; // Call `installValidation` on the account. - ExecutionLib.callSelfBubbleOnRevertTransient(encodedData[38:]); + ExecutionLib.callBubbleOnRevertTransient(address(this), 0, encodedData[38:]); // Update the UserOp signature to the remaining bytes. userOpSignature = userOp.signature[33 + encodedDataLength + deferredInstallSigLength:]; diff --git a/src/libraries/ExecutionLib.sol b/src/libraries/ExecutionLib.sol index b8d52d492..467c5e663 100644 --- a/src/libraries/ExecutionLib.sol +++ b/src/libraries/ExecutionLib.sol @@ -31,39 +31,17 @@ library ExecutionLib { error PreRuntimeValidationHookFailed(address module, uint32 entityId, bytes revertReason); error RuntimeValidationFunctionReverted(address module, uint32 entityId, bytes revertReason); - /// @param target The address of the contract to call. - /// @param value The value to send with the call. - /// @param data The call data. - /// @return result The return data of the call, or the error message from the call if call reverts. - function exec(address target, uint256 value, bytes memory data) internal returns (bytes memory result) { - // Manually call, collecting return data. - assembly ("memory-safe") { - let success := call(gas(), target, value, add(data, 0x20), mload(data), codesize(), 0) - - // Allocate space for the return data, advancing the memory pointer to the nearest word - result := mload(0x40) - mstore(0x40, and(add(add(result, returndatasize()), 0x3f), not(0x1f))) - - // Copy the returned data to the allocated space. - mstore(result, returndatasize()) - returndatacopy(add(result, 0x20), 0, returndatasize()) - - // Revert if the call was not successful. - if iszero(success) { revert(add(result, 0x20), returndatasize()) } - } - } - - // Call the following function to address(this), without capturing any return data. + // Perform the following call, without capturing any return data. // If the call reverts, the revert message will be directly bubbled up. - function callSelfBubbleOnRevert(bytes memory callData) internal { + function callBubbleOnRevert(address target, uint256 value, bytes memory callData) internal { // Manually call, without collecting return data unless there's a revert. assembly ("memory-safe") { let success := call( gas(), - address(), + target, /*value*/ - 0, + value, /*argOffset*/ add(callData, 0x20), /*argSize*/ @@ -86,9 +64,8 @@ library ExecutionLib { } } - // Just like `callSelfBubbleOnRevert`, but with a `bytes calldata` parameter. This will be transiently stored - // in memory past-the-end of the used memory. - function callSelfBubbleOnRevertTransient(bytes calldata callData) internal { + // Transiently copy the call data to a memory, and perform a self-call. + function callBubbleOnRevertTransient(address target, uint256 value, bytes calldata callData) internal { bytes memory callToSelf; assembly ("memory-safe") { @@ -99,7 +76,7 @@ library ExecutionLib { calldatacopy(add(callToSelf, 0x20), callData.offset, callData.length) } - callSelfBubbleOnRevert(callToSelf); + callBubbleOnRevert(target, value, callToSelf); // Memory is discarded afterwards } diff --git a/test/account/DirectCallsFromModule.t.sol b/test/account/DirectCallsFromModule.t.sol index 4e4a96848..3efe876a1 100644 --- a/test/account/DirectCallsFromModule.t.sol +++ b/test/account/DirectCallsFromModule.t.sol @@ -93,7 +93,7 @@ contract DirectCallsFromModuleTest is AccountTestBase { { bytes memory encodedCall = abi.encodeCall(DirectCallModule.directCall, ()); - vm.prank(address(entryPoint)); + vm.prank(address(account1)); bytes memory result = account1.execute(address(_module), 0, encodedCall); assertTrue(_module.preHookRan());