Alchemy Modular Account

The `SessionKeyData` struct of a session key defines the permission access of registered session keys. As this information has to be accessible as part of UserOp validation, it has to comply with the ERC-4337 storage restriction rules. The client developed a `PluginStorageLib` library that follows the `keccak256(A || X) + n` (`A` being the address of the account and `X` being an additional arbitrary key) storage access rule defined in the EIP, which essentially enables access to address-associated storage slots in the plugin's storage. See MSCA-30 for concerns regarding ERC-4337 storage access compliance. The key derivations to get to the address associated storage slots are defined in `SessionKeyPermissionsBase.sol#L70-L91` and implemented in the underlying `_sessionKeyIdOf()` function. The implementation of the `SessionKeyData` storage key differs slightly from the specs, resulting in a critical issue for the storage management of `SessionKeyData` structs. While the `associatedStorageKey` follows the spec with `12 padding zeros || associated address || SESSION_KEY_DATA_PREFIX || batch index`, it is not just appended with the `sessionKeyId` as the specs suggest, but with `bytes32(abi.encodePacked(SESSION_KEY_DATA_PREFIX, SessionKeyId.unwrap(id)))` (`SessionKeyPermissionsBase.sol#L133`). This statement will incorrectly slice off the 4 right-most bytes of the `sessionKeyId`, which turns out to be a `uint256` counter variable, incremented upwards from zero for each account. In the EVM, `uint256` variables are stored in little-endian notation, with the least significant bytes stored in the right-most bytes of the storage slot. Therefore, as a result of incorrectly slicing off the 4 least significant bytes of the key ID, meaning the first 0xffffffff = 4294967295 registered keys within each account share the same storage slot, meaning that they share the same `SessionKeyData` object defining a key's permissions. As a consequence, the entire access control around session keys is fully compromised for all MSCAs, as practically all keys an individual MSCA would ever issue share the same permissions. **SessionKeyData**: Specs: `... SESSION_KEY_DATA_PREFIX || batch index || sessionKeyId` Code: `... SESSION_KEY_DATA_PREFIX || batch index || **SESSION_KEY_DATA_PREFIX** || sessionKeyId with righ-most 4 bytes sliced off`

Part of the permissions that can be set for session keys is an ERC-20 spend limit, with which the owner of an MSCA can grant certain permissions to holders of the session key to spend tokens in possession of the MSCA. Notably, multiple contract calls can be executed within a single `executeWithSessionKey()` user operation through multiple entries in the array of `Call` structs. In a pre-execution hook, the calldata of each of the calls encoded in the user operation is analyzed for the `transfer()` and `approve()` selector, where the spending limit is adjusted based on the full `amount` parameter for transfers and on the supposed increase in allowance of the given address for approvals. However, the current implementation of the pre-execution hook incorrectly assumes that no prior calls part of this user operation have adjusted the allowance. The result of `IERC20(token).allowance(account, spender)` could incorrectly reflect the state at the time of execution of the current `Call` being validated. This enables any malicious holder of a session key with a non-zero ERC-20 token limit for a certain token to fully drain the MSCA's token balance.

The `MultiOwnerPlugin` defines the `runtimeValidationFunction()` function to protect the runtime calls to the native functions of MSCA. This function ensures that the caller is either the MSCA itself or one of the authorized owners. The choice of allowing MSCA to call the owner-only functions seems to be designed to allow the owner to uninstall the owner plugin and install another simultaneously in an `executeBatch()` call. However, such a design could pose risks to the MSCA and potentially allow an unauthorized party to execute the owner-only functions due to the design of plugin external calls. A plugin can be allowed to perform calls to external contracts, including the MSCA itself, via the `executeFromPluginExternal()` function. If a plugin is allowed to do so, it can, for example, execute `MSCA.upgradeToAndCall()`, to upgrade the account implementation without being an authorized owner. Note that `SessionKeyPlugin` is allowed to execute calls to any external contracts. While the `SessionKeyPermissionsPlugin` enforces specific permissions on the session keys, if any user with a session key with `ContractAccessControlType` of `NONE` type (i.e., no access control enforced on the called external contracts), they would be able to perform arbitrarily calls to the owner-only functions.

According to the MSCA specification, hooks are assumed to be idempotent, so deduplication can be performed when multiple hooks overlap. However, the pre-execution hook defined in `SessionKeyPermissionsPlugin` is not idempotent, specifically the `_updateLimitsPreExec()` function. If the `_updateLimitsPreExec()` function is executed more than once during the `executeWithSessionKey()` call, the spend limits will be reduced multiple times. Any plugin can add another plugin's hooks to the MSCA without permission from the providing plugin (except in the case of injected hooks). Therefore, a hook can be executed multiple times even though the providing plugin specifies it once in the manifest, which may lead to unexpected results for non-idempotent hooks.

In numerous instances, the MSCA is able to perform a call on itself ("self-call"). We identified three distinct ways in which self-calls can be achieved: 1. Via the successful call to the `execute()` and `executeBatch()` selectors, which require a passing `preNativeFunction()` check. In the reference implementation, these selectors are protected by the `MultiOwnerPlugin`'s runtime validation, so require the `msg.sender` to provide a valid signature or require the signer of the user operation to be a registered owner. 1. Via the `executeWithPluginExternal()` selector, self-calls can be achieved by all plugins with specifically the MSCA itself listed as a permitted address or with a plugin specifying an activated `permitAnyExternalAddress` flag in its manifest. For example, any holder of a session key registered in an MSCA with access control enabling calling the MSCA itself can do self-calls via `executeWithSessionKey()`, as it calls the `executeWIthPluginExternal()` selector. Such a key would need to be enabled to call the MSCA via `accessControlType=ALLOWLIST` and the MSCA specifically being registered as a callable address, or if the MSCA itself is not being registered in a configured `accesscontrolType=DENYLIST` or if the key can call arbitrary addresses via `accesscontrolType=NONE`. 1. If the MSCA itself were to add the `IPlugin` interface to its `supportedInterface` mapping through a plugin installation (see MSCA-23), self-calls could be achieved through `executeWithPlugin()` calls. The `_doRuntimeValidation()` function looks up the runtime validation function associated with the given function selector and performs the runtime validation call to the corresponding plugin. While generally, attempted runtime validation fails if no associated runtime validation is found, a special condition at `UpgradeableModularAccount.sol#L617-L625` is evaluated to check if the call should be allowed. According to the comments at `UpgradeableModularAccount.sol#L626-L630`, the MSCA should prohibit any runtime call with no associated runtime validation function except the cases of self-calls to `installPlugin()` or `upgradeToAndCall()`. However, due to the use of `||` at L622, which seems to be a logical error, any self-calls to the MSCA are allowed, causing a divergence between the code and the comments. As a result, a permitted plugin or owner can execute functions that should not be executed in runtime context due to this logical error in the code. For example, the `executeWithSessionKey()` function provided by the `SessionKeyPlugin` does not have a runtime validation function, as session keys are intended to be used in user operation context only. However, an owner of the MSCA can execute the `executeWithSessionKey()` function in runtime using self-calls via `executeBatch()` and potentially affect the token or gas usages of an arbitrary session key without obtaining a valid signature of the key. However, note that in the current design, the owner of MSCA can arbitrarily control the spending limits of session keys.

The ERC-4337 specification states the following: > If the account does not support signature aggregation, it MUST validate the signature is a valid signature of the `userOpHash`, and SHOULD return SIG_VALIDATION_FAILED (and not revert) on signature mismatch. Any other error should revert. Currently, there are cases in the signature verification process where `SIG_VALIDATION_FAILED` is returned even in the case of a malformed signature, which should not be the case. Currently, in 2 cases, the function does not revert when it should: 1. `MultiOwnerPlugin.userOpValidationFunction()` 2. `SessionKeyPlugin.userOpValidationFunction()`

Users can provide additional injected hooks not specified in the plugin manifest during the plugin installation. Injected hooks have the same behavior as permitted call hooks, which are executed when the plugin executes a permitted call to another plugin or an external contract. However, plugins with injected hooks that execute functions defined by themselves cannot be uninstalled after installation. While adding the injected hooks, the plugin creates a dependency on itself. Therefore, the plugin cannot be uninstalled since the plugin’s dependency count will be at least one at the time of the uninstall call, essentially causing a deadlock. This issue can further impact other plugins that the uninstallable plugin depends on, as they would also become uninstallable.

`PluginStorageLib` is a library for allocating and accessing ERC-4337 address-associated storage within plugins. There are multiple assumptions that are used through the library but are not enforced in the library code. In the current state of the codebase, the assumptions are followed, but as more plugins are developed, this cannot be guaranteed. Below is a list of current assumptions that will cause issues if not enforced: 1. The library allows a `keySize` of 256, which corresponds to how many words of memory are needed to look up a specific value. The `keySize` of max 2 is currently used, but the size of the key is not enforced. For example, if an address-associated storage key with `keySize = 2` is allocated, there is nothing stopping storage lookup with that memory allocation with only `input1` and no `input2`. Assure that the `keySize` corresponds to the number of key arguments provided. 2. The `key` variable size is currently unbound, which is not clear as it stores a slot in the memory and hence should be `bytes32,` or it will lead to unexpected outcomes. 3. One should not be allowed to have keys of a value of 0, as this could lead to collisions and unexpected behavior. Make sure that the `input` keys are checked and are not 0 inside the library. 4. Upper bits of assembly input are assumed to be 0s in `allocateAssociatedStorageKey()`, which might not be the case in `addr`, as it is an `address` and hence the upper bits are not guaranteed to be 0. Assure that the input is properly cleared in a similar manner to the `_allocateTempKeyBuffer()` function in `AssociatedLinkedListSetLib`. According to the latest [Solidity Inline Assembly documentation](https://docs.soliditylang.org/en/0.8.23/assembly.html) at the time of writing: > _"If you access variables of a type that spans less than 256 bits (for example `uint64`, `address`, or `bytes16`), you cannot make any assumptions about bits not part of the encoding of the type. Especially, do not assume them to be zero. To be safe, always clear the data properly before you use it in a context where this is important [...]"_

The `MultiOwnerMSCAFactory` uses `CREATE2` to generate the contract. As part of that call, the factory is passing the hash of a salt (provided by the caller) and the encoding of the list of owners. This means that if the owners are given in different orders, the addresses will be different. It is unclear if this is a problem or not

According to the inline documentation, the `_ASSOCIATED_STORAGE_PREFIX` should have a value calculated using `bytes4(keccak256("AssociatedLinkedListSet"))`. This value should be `0xf938c976`, but the current value is `0x9cc6c923`. It is unclear where this value came from.