Skip to content

Commit

Permalink
fix: review fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
adamegyed committed Aug 26, 2024
1 parent c1c2821 commit 0fbd371
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 23 deletions.
2 changes: 1 addition & 1 deletion src/account/ModuleManagerInternals.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
10 changes: 5 additions & 5 deletions src/account/ReferenceModularAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/helpers/Constants.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions src/interfaces/IModularAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
25 changes: 15 additions & 10 deletions standard/ERCs/erc-6900.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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.

Expand All @@ -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.
Expand All @@ -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

Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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);
}
```
Expand Down Expand Up @@ -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

Expand Down
6 changes: 3 additions & 3 deletions test/utils/AccountTestBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 0fbd371

Please sign in to comment.