From 43b28343dc4acf4295495582604752106f57e3ca Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Thu, 14 Nov 2024 16:50:56 -0800 Subject: [PATCH 1/2] fix: [Quantstamp-ALC-14] fix execution data view for native functions --- src/account/ModularAccountBase.sol | 36 ++++++++++---------------- src/account/ModularAccountView.sol | 30 ++++++++++++++++++++- src/helpers/NativeFunctionDelegate.sol | 29 +++++++++++---------- src/interfaces/IModularAccountBase.sol | 19 ++++++++++++++ 4 files changed, 78 insertions(+), 36 deletions(-) create mode 100644 src/interfaces/IModularAccountBase.sol diff --git a/src/account/ModularAccountBase.sol b/src/account/ModularAccountBase.sol index ddce5660..cfe05cf4 100644 --- a/src/account/ModularAccountBase.sol +++ b/src/account/ModularAccountBase.sol @@ -27,6 +27,8 @@ import {UUPSUpgradeable} from "solady/utils/UUPSUpgradeable.sol"; import {ExecutionInstallDelegate} from "../helpers/ExecutionInstallDelegate.sol"; import {_coalescePreValidation, _coalesceValidation} from "../helpers/ValidationResHelpers.sol"; + +import {IModularAccountBase} from "../interfaces/IModularAccountBase.sol"; import { DensePostHookData, ExecutionLib, @@ -50,6 +52,7 @@ import {TokenReceiver} from "./TokenReceiver.sol"; /// deferred actions during validation. abstract contract ModularAccountBase is IModularAccount, + IModularAccountBase, ModularAccountView, AccountStorageInitializable, AccountBase, @@ -148,14 +151,12 @@ abstract contract ModularAccountBase is return execReturnData; } - /// @notice Create a contract. - /// @param value The value to send to the new contract constructor - /// @param initCode The initCode to deploy. - /// @return createdAddr The created contract address. + /// @inheritdoc IModularAccountBase function performCreate(uint256 value, bytes calldata initCode, bool isCreate2, bytes32 salt) external payable virtual + override wrapNativeFunction returns (address createdAddr) { @@ -339,8 +340,8 @@ abstract contract ModularAccountBase is _uninstallValidation(validationFunction, uninstallData, hookUninstallData); } - /// @notice May be validated by a global validation - function invalidateDeferredValidationInstallNonce(uint256 nonce) external wrapNativeFunction { + /// @inheritdoc IModularAccountBase + function invalidateDeferredValidationInstallNonce(uint256 nonce) external override wrapNativeFunction { getAccountStorage().deferredActionNonceUsed[nonce] = true; emit DeferredActionNonceInvalidated(nonce); } @@ -380,7 +381,8 @@ abstract contract ModularAccountBase is super.upgradeToAndCall(newImplementation, data); } - function isValidSignature(bytes32 hash, bytes calldata signature) public view returns (bytes4) { + /// @inheritdoc IERC1271 + function isValidSignature(bytes32 hash, bytes calldata signature) public view override returns (bytes4) { ModuleEntity sigValidation = ModuleEntity.wrap(bytes24(signature)); signature = signature[24:]; return _isValidSignature(sigValidation, hash, signature); @@ -870,21 +872,6 @@ abstract contract ModularAccountBase is return _1271_INVALID; } - function _globalValidationAllowed(bytes4 selector) internal view virtual returns (bool) { - if ( - selector == this.execute.selector || selector == this.executeBatch.selector - || selector == this.installExecution.selector || selector == this.uninstallExecution.selector - || selector == this.installValidation.selector || selector == this.uninstallValidation.selector - || selector == this.upgradeToAndCall.selector - || selector == this.invalidateDeferredValidationInstallNonce.selector - || selector == this.performCreate.selector - ) { - return true; - } - - return getAccountStorage().executionStorage[selector].allowGlobalValidation; - } - function _isValidationGlobal(ModuleEntity validationFunction) internal view virtual returns (bool) { return getAccountStorage().validationStorage[validationFunction].isGlobal; } @@ -1115,6 +1102,11 @@ abstract contract ModularAccountBase is return _globalValidationAllowed(selector) && _isValidationGlobal(validationFunction); } + function _globalValidationAllowed(bytes4 selector) internal view virtual returns (bool) { + return _isGlobalValidationAllowedNativeFunction(selector) + || getAccountStorage().executionStorage[selector].allowGlobalValidation; + } + function _selectorValidationApplies(bytes4 selector, ModuleEntity validationFunction) internal view diff --git a/src/account/ModularAccountView.sol b/src/account/ModularAccountView.sol index 91f8e7a6..c7940e14 100644 --- a/src/account/ModularAccountView.sol +++ b/src/account/ModularAccountView.sol @@ -8,7 +8,13 @@ import { ValidationDataView } from "@erc6900/reference-implementation/interfaces/IModularAccountView.sol"; +import {IModularAccount} from "@erc6900/reference-implementation/interfaces/IModularAccount.sol"; +import {IModularAccountView} from "@erc6900/reference-implementation/interfaces/IModularAccountView.sol"; +import {IAccountExecute} from "@eth-infinitism/account-abstraction/interfaces/IAccountExecute.sol"; +import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; + import {NativeFunctionDelegate} from "../helpers/NativeFunctionDelegate.sol"; +import {IModularAccountBase} from "../interfaces/IModularAccountBase.sol"; import {MemManagementLib} from "../libraries/MemManagementLib.sol"; import {ExecutionStorage, ValidationStorage, getAccountStorage} from "./AccountStorage.sol"; @@ -28,9 +34,13 @@ abstract contract ModularAccountView is IModularAccountView { // return ModularAccountViewLib.getExecutionData(selector, _isNativeFunction(selector)); ExecutionStorage storage executionStorage = getAccountStorage().executionStorage[selector]; - if (_isNativeFunction(uint32(selector))) { + if (_isGlobalValidationAllowedNativeFunction(selector)) { data.module = address(this); data.allowGlobalValidation = true; + } else if (_isNativeFunction(uint32(selector))) { + // native view functions + data.module = address(this); + data.skipRuntimeValidation = true; } else { data.module = executionStorage.module; data.skipRuntimeValidation = executionStorage.skipRuntimeValidation; @@ -68,4 +78,22 @@ abstract contract ModularAccountView is IModularAccountView { function _isNativeFunction(uint32 selector) internal view virtual returns (bool) { return _NATIVE_FUNCTION_DELEGATE.isNativeFunction(selector); } + + function _isGlobalValidationAllowedNativeFunction(bytes4 selector) internal view virtual returns (bool) { + if ( + selector == IModularAccount.execute.selector || selector == IModularAccount.executeBatch.selector + || selector == IAccountExecute.executeUserOp.selector + || selector == IModularAccount.executeWithRuntimeValidation.selector + || selector == IModularAccount.installExecution.selector + || selector == IModularAccount.uninstallExecution.selector + || selector == IModularAccount.installValidation.selector + || selector == IModularAccount.uninstallValidation.selector + || selector == UUPSUpgradeable.upgradeToAndCall.selector + || selector == IModularAccountBase.invalidateDeferredValidationInstallNonce.selector + || selector == IModularAccountBase.performCreate.selector + ) { + return true; + } + return false; + } } diff --git a/src/helpers/NativeFunctionDelegate.sol b/src/helpers/NativeFunctionDelegate.sol index 35c237e9..0a596f36 100644 --- a/src/helpers/NativeFunctionDelegate.sol +++ b/src/helpers/NativeFunctionDelegate.sol @@ -1,14 +1,17 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.26; +import {IModularAccount} from "@erc6900/reference-implementation/interfaces/IModularAccount.sol"; import {IModularAccountView} from "@erc6900/reference-implementation/interfaces/IModularAccountView.sol"; import {IAccount} from "@eth-infinitism/account-abstraction/interfaces/IAccount.sol"; +import {IAccountExecute} from "@eth-infinitism/account-abstraction/interfaces/IAccountExecute.sol"; import {IERC1155Receiver} from "@openzeppelin/contracts/interfaces/IERC1155Receiver.sol"; +import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol"; import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; -import {ModularAccountBase} from "../account/ModularAccountBase.sol"; +import {IModularAccountBase} from "../interfaces/IModularAccountBase.sol"; /// @title Native Function Delegate /// @author Alchemy @@ -20,18 +23,18 @@ contract NativeFunctionDelegate { // check against IAccount methods selector == uint32(IAccount.validateUserOp.selector) // check against ModularAccount methods - || selector == uint32(ModularAccountBase.installExecution.selector) - || selector == uint32(ModularAccountBase.uninstallExecution.selector) - || selector == uint32(ModularAccountBase.installValidation.selector) - || selector == uint32(ModularAccountBase.uninstallValidation.selector) - || selector == uint32(ModularAccountBase.execute.selector) - || selector == uint32(ModularAccountBase.executeBatch.selector) - || selector == uint32(ModularAccountBase.executeWithRuntimeValidation.selector) - || selector == uint32(ModularAccountBase.accountId.selector) - || selector == uint32(ModularAccountBase.performCreate.selector) - || selector == uint32(ModularAccountBase.invalidateDeferredValidationInstallNonce.selector) - || selector == uint32(ModularAccountBase.executeUserOp.selector) - || selector == uint32(ModularAccountBase.isValidSignature.selector) + || selector == uint32(IModularAccount.installExecution.selector) + || selector == uint32(IModularAccount.uninstallExecution.selector) + || selector == uint32(IModularAccount.installValidation.selector) + || selector == uint32(IModularAccount.uninstallValidation.selector) + || selector == uint32(IModularAccount.execute.selector) + || selector == uint32(IModularAccount.executeBatch.selector) + || selector == uint32(IModularAccount.executeWithRuntimeValidation.selector) + || selector == uint32(IModularAccount.accountId.selector) + || selector == uint32(IAccountExecute.executeUserOp.selector) + || selector == uint32(IModularAccountBase.performCreate.selector) + || selector == uint32(IModularAccountBase.invalidateDeferredValidationInstallNonce.selector) + || selector == uint32(IERC1271.isValidSignature.selector) // check against IModularAccountView methods || selector == uint32(IModularAccountView.getExecutionData.selector) || selector == uint32(IModularAccountView.getValidationData.selector) diff --git a/src/interfaces/IModularAccountBase.sol b/src/interfaces/IModularAccountBase.sol new file mode 100644 index 00000000..802e3e71 --- /dev/null +++ b/src/interfaces/IModularAccountBase.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.26; + +interface IModularAccountBase { + /// @notice Create a contract. + /// @param value The value to send to the new contract constructor + /// @param initCode The initCode to deploy. + /// @param isCreate2 The bool to indicate which method to use to deploy. + /// @param salt The salt for deployment. + /// @return createdAddr The created contract address. + function performCreate(uint256 value, bytes calldata initCode, bool isCreate2, bytes32 salt) + external + payable + returns (address createdAddr); + + /// @notice Invalidate a nonce for deferred actions + /// @param nonce the nonce to invalidate. + function invalidateDeferredValidationInstallNonce(uint256 nonce) external; +} From 927e9f2c697b7e287fa0f9ac4222c13c8bc3c87d Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Fri, 15 Nov 2024 14:51:14 -0800 Subject: [PATCH 2/2] add gas benchmark --- gas-snapshots/ModularAccount.json | 18 +++++++++--------- gas-snapshots/SemiModularAccount.json | 18 +++++++++--------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/gas-snapshots/ModularAccount.json b/gas-snapshots/ModularAccount.json index e573341e..6e877df8 100644 --- a/gas-snapshots/ModularAccount.json +++ b/gas-snapshots/ModularAccount.json @@ -1,16 +1,16 @@ { "Runtime_AccountCreation": "176125", - "Runtime_BatchTransfers": "92985", - "Runtime_Erc20Transfer": "78394", - "Runtime_InstallSessionKey_Case1": "423036", - "Runtime_NativeTransfer": "54543", + "Runtime_BatchTransfers": "93045", + "Runtime_Erc20Transfer": "78454", + "Runtime_InstallSessionKey_Case1": "423148", + "Runtime_NativeTransfer": "54603", "Runtime_UseSessionKey_Case1_Counter": "78606", "Runtime_UseSessionKey_Case1_Token": "111919", - "UserOp_BatchTransfers": "198306", - "UserOp_Erc20Transfer": "185271", - "UserOp_InstallSessionKey_Case1": "531243", - "UserOp_NativeTransfer": "161528", + "UserOp_BatchTransfers": "198366", + "UserOp_Erc20Transfer": "185331", + "UserOp_InstallSessionKey_Case1": "531355", + "UserOp_NativeTransfer": "161588", "UserOp_UseSessionKey_Case1_Counter": "195233", "UserOp_UseSessionKey_Case1_Token": "226217", - "UserOp_deferredValidation": "257908" + "UserOp_deferredValidation": "258080" } \ No newline at end of file diff --git a/gas-snapshots/SemiModularAccount.json b/gas-snapshots/SemiModularAccount.json index 7aee5cde..2e9dfa3e 100644 --- a/gas-snapshots/SemiModularAccount.json +++ b/gas-snapshots/SemiModularAccount.json @@ -1,16 +1,16 @@ { "Runtime_AccountCreation": "97745", - "Runtime_BatchTransfers": "88968", - "Runtime_Erc20Transfer": "74425", - "Runtime_InstallSessionKey_Case1": "421585", - "Runtime_NativeTransfer": "50584", + "Runtime_BatchTransfers": "89031", + "Runtime_Erc20Transfer": "74488", + "Runtime_InstallSessionKey_Case1": "421700", + "Runtime_NativeTransfer": "50647", "Runtime_UseSessionKey_Case1_Counter": "78956", "Runtime_UseSessionKey_Case1_Token": "112269", - "UserOp_BatchTransfers": "193459", - "UserOp_Erc20Transfer": "180495", - "UserOp_InstallSessionKey_Case1": "528763", - "UserOp_NativeTransfer": "156770", + "UserOp_BatchTransfers": "193522", + "UserOp_Erc20Transfer": "180558", + "UserOp_InstallSessionKey_Case1": "528878", + "UserOp_NativeTransfer": "156833", "UserOp_UseSessionKey_Case1_Counter": "195473", "UserOp_UseSessionKey_Case1_Token": "226457", - "UserOp_deferredValidation": "253953" + "UserOp_deferredValidation": "254131" } \ No newline at end of file