From 0fbd3713500d5ebd5a0ca505159140c63f8ff84f Mon Sep 17 00:00:00 2001 From: adam Date: Mon, 26 Aug 2024 17:03:58 -0400 Subject: [PATCH] fix: review fixes --- src/account/ModuleManagerInternals.sol | 2 +- src/account/ReferenceModularAccount.sol | 10 +++++----- src/helpers/Constants.sol | 2 +- src/interfaces/IModularAccount.sol | 6 +++--- standard/ERCs/erc-6900.md | 25 +++++++++++++++---------- test/utils/AccountTestBase.sol | 6 +++--- 6 files changed, 28 insertions(+), 23 deletions(-) diff --git a/src/account/ModuleManagerInternals.sol b/src/account/ModuleManagerInternals.sol index ee42012d..04130e41 100644 --- a/src/account/ModuleManagerInternals.sol +++ b/src/account/ModuleManagerInternals.sol @@ -286,7 +286,7 @@ abstract contract ModuleManagerInternals is IModularAccount { revert ArrayLengthMismatch(); } - // Hook uninstall data is provided in the order of pre-validation hooks, then permission hooks. + // Hook uninstall data is provided in the order of pre validation hooks, then permission hooks. uint256 hookIndex = 0; for (uint256 i = 0; i < _validationData.preValidationHooks.length; ++i) { bytes calldata hookData = hookUninstallDatas[hookIndex]; diff --git a/src/account/ReferenceModularAccount.sol b/src/account/ReferenceModularAccount.sol index cefc1d46..9be42767 100644 --- a/src/account/ReferenceModularAccount.sol +++ b/src/account/ReferenceModularAccount.sol @@ -442,7 +442,7 @@ contract ReferenceModularAccount is } } - // Run the pre hooks and copy their return data to the post hooks array, if an associated post-exec hook + // Run the pre hooks and copy their return data to the post hooks array, if an associated post exec hook // exists. for (uint256 i = 0; i < hooksLength; ++i) { HookConfig hookConfig = toHookConfig(executionHooks.at(i)); @@ -452,7 +452,7 @@ contract ReferenceModularAccount is preExecHookReturnData = _runPreExecHook(hookConfig.moduleEntity(), data); - // If there is an associated post-exec hook, save the return data. + // If there is an associated post exec hook, save the return data. if (hookConfig.hasPostHook()) { postHooksToRun[i].preExecHookReturnData = preExecHookReturnData; } @@ -523,15 +523,15 @@ contract ReferenceModularAccount is /** * Order of operations: * 1. Check if the sender is the entry point, the account itself, or the selector called is public. - * - Yes: Return an empty array, there are no post-permissionHooks. + * - Yes: Return an empty array, there are no post permissionHooks. * - No: Continue * 2. Check if the called selector (msg.sig) is included in the set of selectors the msg.sender can * directly call. * - Yes: Continue * - No: Revert, the caller is not allowed to call this selector * 3. If there are runtime validation hooks associated with this caller-sig combination, run them. - * 4. Run the pre-permissionHooks associated with this caller-sig combination, and return the - * post-permissionHooks to run later. + * 4. Run the pre permissionHooks associated with this caller-sig combination, and return the + * post permissionHooks to run later. */ function _checkPermittedCallerAndAssociatedHooks() internal diff --git a/src/helpers/Constants.sol b/src/helpers/Constants.sol index 4ad649c1..c65dc33d 100644 --- a/src/helpers/Constants.sol +++ b/src/helpers/Constants.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.25; // Index marking the start of the data for the validation function. uint8 constant RESERVED_VALIDATION_DATA_INDEX = type(uint8).max; -// Maximum number of pre-validation hooks that can be registered. +// Maximum number of pre validation hooks that can be registered. uint8 constant MAX_PRE_VALIDATION_HOOKS = type(uint8).max; // Magic value for the Entity ID of direct call validation. diff --git a/src/interfaces/IModularAccount.sol b/src/interfaces/IModularAccount.sol index 8ddebb75..6ce78d44 100644 --- a/src/interfaces/IModularAccount.sol +++ b/src/interfaces/IModularAccount.sol @@ -65,7 +65,7 @@ interface IModularAccount { /// @param selectors The selectors to install the validation function for. /// @param installData Optional data to be decoded and used by the module to setup initial module state. /// @param hooks Optional hooks to install, associated with the validation function. These may be - /// pre-validation hooks or execution hooks. The expected format is a bytes26 HookConfig, followed by the + /// pre validation hooks or execution hooks. The expected format is a bytes26 HookConfig, followed by the /// install data, if any. function installValidation( ValidationConfig validationConfig, @@ -79,8 +79,8 @@ interface IModularAccount { /// @param uninstallData Optional data to be decoded and used by the module to clear module data for the /// account. /// @param hookUninstallData Optional data to be used by hooks for cleanup. If any are provided, the array must - /// be of a length equal to existing pre-validation hooks plus permission hooks. Hooks are indexed by - /// pre-validation hook order first, then permission hooks. + /// be of a length equal to existing pre validation hooks plus permission hooks. Hooks are indexed by + /// pre validation hook order first, then permission hooks. function uninstallValidation( ModuleEntity validationFunction, bytes calldata uninstallData, diff --git a/standard/ERCs/erc-6900.md b/standard/ERCs/erc-6900.md index df09e990..86ac92ea 100644 --- a/standard/ERCs/erc-6900.md +++ b/standard/ERCs/erc-6900.md @@ -13,7 +13,7 @@ requires: 165, 4337 ## Abstract -This proposal standardizes smart contract accounts and account modules, which are smart contract interfaces that allow for composable logic within smart contract accounts. This proposal is compliant with [ERC-4337](./eip-4337.md). This standard emphasizes secure permissioning of modules, and maximal interoperability between all spec compliant accounts and modules. +This proposal standardizes smart contract accounts and account modules, which are smart contract interfaces that allow for composable logic within smart contract accounts. This proposal is compliant with [ERC-4337](./eip-4337.md). This standard emphasizes secure permissioning of modules, and maximal interoperability between all spec-compliant accounts and modules. This modular approach splits account functionality into three categories, implements them in external contracts, and defines an expected execution flow from accounts. @@ -29,7 +29,7 @@ We propose a standard that coordinates the implementation work between module de ![diagram showing relationship between accounts and modules with modular functions](../assets/eip-6900/MSCA_Shared_Components_Diagram.svg) -These modules can contain execution logic, validation functions, and hooks. Validation functions define the circumstances under which the smart contract account will approve actions taken on its behalf, while hooks allow for pre- and post-execution controls. +These modules can contain execution logic, validation functions, and hooks. Validation functions define the circumstances under which the smart contract account will approve actions taken on its behalf, while hooks allow for pre and post execution controls. Accounts adopting this standard will support modular, upgradable execution and validation logic. Defining this as a standard for smart contract accounts will make modules easier to develop securely and will allow for greater interoperability. @@ -45,8 +45,8 @@ The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "S - **Execution functions** execute custom logic allowed by the account. - **Hooks** execute custom logic and checks before and/or after an execution function or validation function. There are two types of hooks: - - **Validation Hook** functions run before a validation function. These can enforce permissions on actions authorized by a validation function. - - **Execution Hook** functions run can run before and/or after an execution function. The pre execution hook may optionally return data to be consumed by a post execution hook functions. + - **Validation hook** functions run before a validation function. These can enforce permissions on actions authorized by a validation function. + - **Execution hook** functions run can run before and/or after an execution function. The pre execution hook may optionally return data to be consumed by a post execution hook functions. - A **native function** refers to a function implemented by the modular account, as opposed to a function added by a module. - A **module** is a deployed smart contract that hosts any amount of the above three kinds of modular functions. - A module **manifest** describes the execution functions, interface ids, and hooks that should be installed on the account. @@ -67,8 +67,7 @@ Each step is modular, supporting different implementations, that allows for open - `IAccount.sol` from [ERC-4337](./eip-4337.md). - `IAccountExecute.sol` from [ERC-4337](./eip-4337.md). -- `IModularAccount.sol` to support installing and uninstalling modules. -- `IStandardExecutor.sol` to support open-ended execution. +- `IModularAccount.sol` to support module management and usage, and account identification. **Modular Smart Contract Accounts** **MAY** implement @@ -154,7 +153,7 @@ interface IModularAccount { /// @param selectors The selectors to install the validation function for. /// @param installData Optional data to be decoded and used by the module to setup initial module state. /// @param hooks Optional hooks to install, associated with the validation function. These may be - /// pre-validation hooks or execution hooks. The expected format is a bytes26 HookConfig, followed by the + /// pre validation hooks or execution hooks. The expected format is a bytes26 HookConfig, followed by the /// install data, if any. function installValidation( ValidationConfig validationConfig, @@ -168,8 +167,8 @@ interface IModularAccount { /// @param uninstallData Optional data to be decoded and used by the module to clear module data for the /// account. /// @param hookUninstallData Optional data to be used by hooks for cleanup. If any are provided, the array must - /// be of a length equal to existing pre-validation hooks plus permission hooks. Hooks are indexed by - /// pre-validation hook order first, then permission hooks. + /// be of a length equal to existing pre validation hooks plus permission hooks. Hooks are indexed by + /// pre validation hook order first, then permission hooks. function uninstallValidation( ModuleEntity validationFunction, bytes calldata uninstallData, @@ -186,6 +185,12 @@ interface IModularAccount { ExecutionManifest calldata manifest, bytes calldata moduleUninstallData ) external; + + /// @notice Return a unique identifier for the account implementation. + /// @dev This function MUST return a string in the format "vendor.account.semver". The vendor and account + /// names MUST NOT contain a period character. + /// @return The account ID. + function accountId() external view returns (string memory); } ``` @@ -583,7 +588,7 @@ The modular smart contract accounts themselves are trusted components. Installed Users should perform careful due diligence before installing a module and should be mindful of the fact that modules are potentially dangerous. The module's manifest can give users an understanding of the potential risks they are exposed to for that particular module. For instance, a request to install certain validation functions and/or hooks on certain execution selectors could potentially be a vector for DOS. -Execution hooks have no awareness of other execution hooks being performed in the same function selector execution setting. Since execution hooks can perform state changes, this reveals an important security consideration: An execution hook can only assure that at the time of its own execution, certain conditions are met, but this can not be generalized to the entire pre-execution context of potentially multiple pre-execution hooks. For example, a pre-execution hook cannot be assured that the storage it performed validation upon does not get further updated in subsequent pre-execution hooks. Even a post-execution hook potentially repeating the validation cannot assure that the storage remains unmodified because a prior post-execution hook may have reset the state. As long as the requirements checked by a module as part of an execution hook are only modifiable by the module itself, this can be considered safe. +Execution hooks have no awareness of other execution hooks being performed in the same function selector execution setting. Since execution hooks can perform state changes, this reveals an important security consideration: An execution hook can only assure that at the time of its own execution, certain conditions are met, but this can not be generalized to the entire pre execution context of potentially multiple pre execution hooks. For example, a pre execution hook cannot be assured that the storage it performed validation upon does not get further updated in subsequent pre execution hooks. Even a post execution hook potentially repeating the validation cannot assure that the storage remains unmodified because a prior post execution hook may have reset the state. As long as the requirements checked by a module as part of an execution hook are only modifiable by the module itself, this can be considered safe. ## Copyright diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 5ce4324a..f4e37462 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -229,7 +229,7 @@ abstract contract AccountTestBase is OptimizedTest { return sig; } - // overload for the case where there are no pre-validation hooks + // overload for the case where there are no pre validation hooks function _encodeSignature(ModuleEntity validationFunction, uint8 globalOrNot, bytes memory validationData) internal pure @@ -239,7 +239,7 @@ abstract contract AccountTestBase is OptimizedTest { return _encodeSignature(validationFunction, globalOrNot, emptyPreValidationHookData, validationData); } - // overload for the case where there are no pre-validation hooks + // overload for the case where there are no pre validation hooks function _encode1271Signature(ModuleEntity validationFunction, bytes memory validationData) internal pure @@ -249,7 +249,7 @@ abstract contract AccountTestBase is OptimizedTest { return _encode1271Signature(validationFunction, emptyPreValidationHookData, validationData); } - // helper function to pack pre-validation hook datas, according to the sparse calldata segment spec. + // helper function to pack pre validation hook datas, according to the sparse calldata segment spec. function _packPreHookDatas(PreValidationHookData[] memory preValidationHookData) internal pure