Alchemy - Modular Account V2

Quantstamp6 Dec 2024Files named in the report

Quantstamp reviewed Alchemy ERC (Alchemy - Modular Account V2) on 6 Dec 2024: 23 findings, 0 critical; 9 marked fixed, 14 acknowledged.

No open critical findings

Open the report Open Alchemy ERC’s security record

Report hosted on certificate.quantstamp.com.

Trustblock does not review code.

Findings23

Statuses are the security firm’s own, from its report.

Severity	Acknowledged	Fixed
Critical	0	0
High	0	2
Medium	0	0
Low	14	7

Low`executeBatch()` Can Be Used to Bypass Other Selector's Validation Hook ExecutionAcknowledged

With ERC-6900 v0.8, self-calls to the account are strictly reserved to indicate authorization to invoke some selector. Therefore, the ways in which the account can call itself are very important. The `executeBatch()` function is such a case. In case it performs a self-call, that call's selector is analyzed and has to also be able to be validated by the current validation. However, the actual validation, including the validation associated execution hooks, is then not performed, if invoked via a self-call, only the hooks for the outer `executeBatch()` call is performed. This is known and documented, but we wanted to emphasize it in the report, as well. If some entity has access to `executeBatch()`, but also to some by hooks heavily protected execution function, the hooks of that other selector can be bypassed by performing an `executeBatch()` self-call encoding that other execution function.

LowCode-Less (Pre-)Signature Validation Modules May Result in Always-Passing ValidationsAcknowledged

Within the `ExecutionLib` library, various low-level calls such as `delegatecall()`, `staticcall()`, and `call()` are made without sufficient safeguards, specifically a lack of: 1. `returnsize()` checks 2. `extcodesize()` checks <br> This oversight is concerning, particularly in the context of EIP-7702, where an Externally Owned Account (EOA) could temporarily function as a module when invoking `installExecution()` or `installValidation()`. This scenario could allow subsequent hook calls to succeed improperly without verification. A similar risk exists with standard module installations where no `installData` is used for callbacks or interface checks. Additionally, if a module self-destructs after installation, these unchecked calls may continue to pass, leading to potential security vulnerabilities.

LowConsider adding a `forceUninstallation` Flag that Requires `onInstall()`Callback SuccessAcknowledged

In our opinion, it is not optimal that based on user input issues, the `onUninstall()` callback to the module might not succeed, but cannot be re-attempted due to the modules any longer being registered on the account-side. While it is very reasonable to have a `try-catch` flow around the `onUninstall()` callback in case the module is blocking uninstallation, this should perhaps be specifically intended by a user, e.g. with a `forceUninstallation` flag indicating that a reverting module-callback is acceptable.

LowDeactivated Fallback Signer in `SemiModularAccountBase` Returned as Global ValidationAcknowledged

For semi-modular accounts, In case the fallback signer has been disabled through calls to `updateFallbackSignerData()`, the default validation is still returned as global validation in the `_isValidationGlobal()` function. The impact is minimal and does not enable a passing validation with the disabled fallback signer, because the `_getFallbackSigner()` call ultimately attempting to fetch the signer leads to a revert. However, as in another issue we recommend `getExecutionData()` to contain a global validation check, it might make sense to adjust the function to return false for disabled fallback signers.

LowDeferred Actions Allow Usage of Validation Functions in UserOp Context Without `isUserOpValidation` FlagAcknowledged

Deferred actions enable actions on behalf of the account to be delayed right up until their execution is needed. This means that there are 2 calls that are bounded together during the `_validateUserOp()` function. As these 2 calls can have different validation functions, the outer user op validation function is verfiried to be correct with the correct `isUserOpValidation` validation flag. Whilst for the inner deferred action call, the validation function is a signature verification one, hence `isSignatureValidation` flag is validated. This is however not fully correct as the deferred action call is also done in the user op context, hence the signature validation function should also have the `isUserOpValidation` set correctly and verified.

LowForwarding the `entityId` to the (Un-)Installation CallbacksAcknowledged

In the current module setup, most validation and validation-hook modules expect the `entityId` to be encoded within the `installData` passed to `onInstall()` and `uninstallData` passed to `onUninstall()`. This `entityId` links specific configurations and data within the module, allowing for stateful validation based on that `entityId`. This parameter is especially crucial for validation-associated hooks, while for regular execution hooks, it is typically less significant. Execution hooks are generally more stateless, and they currently do not involve an `onInstall()` callback for their installation, reducing the need for entityId encoding in these cases. Encoding the `entityId` in this way introduces potential inconsistencies, as the value in the `validationConfig` may not necessarily match the `entityId` provided in `installData` during `installValidation()` calls. If a module is installed multiple times with different `entityId` values, there is a risk of unintended interference. For example, if module A with `entityId` A is uninstalled but the `uninstallData` includes `entityId` B—an identifier for another installation of the same module—this could cause unintended deregistration on the module or account side, effectively "bricking" both modules by removing important data in both instances. While this risk is minor and mitigated by the fact that data fields are user-signed and validated, reducing the likelihood of intentional misuse, it still introduces a notable risk surface that could lead to user errors affecting account integrity.

LowImprovements to the Execution Installation FlowAcknowledged

The `installExecution()` function in the `ExecutionInstallDelegate` contract is responsible for setting up both execution functions and execution hooks in a modular account. In case `installData` is provided, the modules to be installed should be invoking `ModuleInstallCommonsLib.onInstall()` on all installed entities, however, for the execution hooks, there is currently no mechanism to do that. The absence of this call is compliant with the standard. However, developers who intend to have stateful execution hooks may not expect this. Hence, it may be beneficial to add an option to add install data to execution hooks. In case `installData` is provided `ModuleInstallCommonsLib.onInstall()` would then perform the interface check and the `onInstall()` call. Furthermore, the implementation only checks for the `IModule` interface when installing execution functions, which is insufficient for confirming that a module adheres to the necessary `IExecutionModule` functionality. This oversight can result in the installation of modules that do not support the required interfaces, even if install data is provided.

LowIncomplete Module Data Cleanup on UninstallAcknowledged

The `onUninstall()` functions in various modules may leave behind residual state data linked to `msg.sender` after uninstallation, especially since the external call is only performed if `onInstallData` is specified. While this incomplete cleanup is acknowledged in the documentation, it could pose security risks or lead to unexpected behavior in the case where the module is reinstalled with the same entityId. The danger is that previously set permissions or data may be unintentionally reused. Below is an overview of potential residual data for each module: - **`AllowlistModule`** - `allowlist`entries - `selector` mappings - **`NativeTokenLimitModule`** - `limits` - `specialPaymasters` - **`PaymasterGuardModule`** - `paymasters` - **`TimeRangeModule`** - `timeRanges` - **`SingleSignerValidationModule`** - `signers` - **`WebAuthnValidationModule`** - `signers`

LowIndividual Validation Hooks Cannot Be Removed From ConfigurationsAcknowledged

While it is possible to remove installed execution hooks from existing execution configurations individually, the same is not possible for validation hooks. Validation configurations are always removed in their entirety, with all associated hooks. This is in contrast with the possible hook extension of existing validation or execution configs. As additional validation hooks can be added, perhaps by another party that is authorized to do so, but not removed, this possibly creates a minor UX inconvenience, as an validation installation has to be fully removed and re-installed without one hook to mimic a single hook deinstallation.

LowInstalled Execution Functions Can Collide with Native FunctionsAcknowledged

The ERC-6900 standard currently clearly states that installations of execution selectors that are clashing with ERC-4337 and ERC-6900 selectors that are native to the account should revert: > An execution function selector MUST not conflict with native ERC-4337 and ERC-6900 functions. While it is checked for 4337 selectors, the native check is not performed. The assumption is that these selectors will not be invoked, as the native selectors will always be invoked instead. There is an extreme technical edge case, where a collision-clashed selector would suddenly become available in case of an account upgrade, where the native function was removed. Mainly, we want to point out the slight deviation from the standard.

Show 4 more

LowReduce Possibilities of Execution Reverts for `NativeTokenLimitModule `Acknowledged

The `NativeTokenLimitModule` provides a pre-execution hook that can use up the allowance for native token transfers under certain execution functions. As native transfers are rolled back in a reverting execution phase, it is a check that is correctly performed not as part of the validation phase, but in the execution phase, so that the spending limit is not changed in case a user operation, containing a native token spend, reverts in execution. However, this enables the possibility of predictable reverts in the execution phase, as the details of the native token spent remain fully unchecked in the validation phase. The possibility of execution reverts due to exceeded limits could be reduced, at least if only a single UserOp of that account is being bundled, with this slightly more restrictive approach: * Add a pre-validation hook to the module that reverts in case the checks currently performed in the `preExecutionHook()`are failing. Remove the current `preExecutionHook()` module and replace it with a validation associated post-only execution hook that performs the state change on the `limit` value. * Place the `preUserOpValidationHook()` that is doing gas limit checks on the account into a separate `GasLimitModule`. While this requires the configuration of two separate native token limit values (gas limit and token transfer limit), we deem this to be an improvement to the design.

LowSender-Parameter in Certain Cases Will Always Be Entrypoint Rather than `UserOp.sender`Acknowledged

All execution hooks in UserOperation settings encode the EntryPoint as the `sender`, rather than the `sender` specified in the UserOperation struct. Regular and validation-associated execution hooks are systematically invoked such that `msg.sender` resolves to the EntryPoint, thereby standardizing the context from which these hooks are triggered. This approach ensures uniform handling of sender identity across different execution paths, reinforcing the design intent to clearly signal the operational context to the module. This is consistently applied in both direct and deferred validation processes, where the EntryPoint remains the `sender` even during runtime validations and deferred actions like signature validation. Such consistency supports the integrity of operational flows and minimizes confusion regarding the origin of execution hooks. It is recommended to document this behavior explicitly to help developers understand that this pattern is a feature of system design, aimed at providing clarity and uniform context for execution hooks. Acknowledging this in the official documentation will aid in setting correct expectations and guide proper module implementation.

LowUnenforced Off-Chain Checks for Modules and Modular AccountAcknowledged

While technically not vulnerabilities, multiple risks in global validation and hook management create risks of unauthorized access, incomplete uninstallation, and potential account bricking: 1. Global validation modules inherently have access to sensitive native functions (e.g., `executeBatch()`, `upgradeToAndCall()`). As these functions have high privileges, end users must exercise caution when assigning validation modules globally. 1. Pre-runtime validation hooks do not necessarily need to be addresses with code and would pass for all parameters, as a low level `call()` is made without any `codesize` checks or return data checks 1. Execution hooks may not be fully removed during uninstallation, leaving residual hooks on fully removed selectors. 1. The last validation function, given sufficient privileges, can be uninstalled, which could effectively brick the account.

LowUnrelated Deferred Actions Could Be Dropped by Bundler to Apply Unused Gas Penalty to AccountAcknowledged

If some UserOperation were to be expected to include some independent deferred action that is not necessary for a passing validation phase, a bundler could drop that deferred action and making the account pay the 10% penalty for the unused gas that was intended to be consumed by the deferred action. However, as there is limited upside here and as deferred actions are most likely always needed to be included for such a UserOperation, this is only informational.