Alchemy ERC security record

The `NativeTokenLimitModule` is intended to limit the native tokens some validation can access from the account. In case paymasters provided, the check is regularly skipped, as the paymaster is covering the gas costs, unless it is some special paymaster that has some existing ERC20 token allowance that withdraws e.g. ERC20 tokens equivalent to the gas consumed from the account. However, the decoded address is missing a check that it is a non-zero length `bytes` array of just zeroes. This enables a bypass in the `paymasters` registry, as in this case `userOp.paymasterAndData.length != 0` as well as `specialPaymasters[address(0x0)][msg.sender] == 0 `, if `address(0x0)` is not specifically registered as a `specialPaymaster`, which does not seem intended. EthInfinitism's EntryPoint internally treats a `paymasterAndData` field of zero length [the same way](https://github.com/eth-infinitism/account-abstraction/blob/6f02f5a28a20e804d0410b4b5b570dd4b076dcf9/contracts/core/EntryPoint.sol#L391) as `paymaster = address(0x0)`, `paymasterVerificationGasLimit = 0`and `paymasterPostOpGasLimit = 0`. The unpacking of the paymasterAndData variable [here](https://github.com/eth-infinitism/account-abstraction/blob/6f02f5a28a20e804d0410b4b5b570dd4b076dcf9/contracts/core/UserOperationLib.sol#L120) does not do any reverts on zero-values either. This enables all UserOperation-validations installed on the account that have this validation hook installed to fully drain the underlying account by simply encoding a 52 byte long `paymasterAndData` field containing only zeroes. Furthermore, in the `PaymasterGuardModule`, a similar check is performed. If this module were to be installed without the `onInstall()` callback or with an `installData` parameter encoding the zero address, native funds could be drained in the same way.

The modular account system supports deferred actions, enabling operations such as installing validation modules to be delayed until execution. Deferred actions, embedded in the `UserOperation`'s signature field, will likely only be used within the same `UserOperation` for validation installation. However, their design allows for arbitrary use, provided the outer `ValidationConfig` validates the selector. Currently, deferred action execution relies solely on signature verification through the outer `ValidationConfig`'s `validateSignature()` function. This bypasses critical validation steps, including pre-validation (`preUserOpValidationHook()`) and execution hooks, which enforce security policies like access control. Without these hooks, attackers can bypass the full validation process, enabling unauthorized operations and privilege escalation. In other words, as soon as a validation module is installed with `isSignatureValidation = true`, all validation-associated hooks and the main validation step that the module also has installed can be skipped by embedding a call to a deferred action and simply validating it via the signature validation path. We argue that this situation is significantly different from the intentional skipping of related hooks when calling `executeBatch()` (ALC-16), as `executeBatch()` is well understood as a powerful selector with very restricted access. Additionally, it should be noted that due to this issue, any validation module with access to `executeBatch()` and signature validation enables the checks imposed on `_checkExecuteBatchValidationApplicability()` to be bypassed, as the calldata validation is also skipped. Checking only that a `ValidationConfig` provides a valid signature is insufficient; deferred actions must trigger all associated validation hooks to maintain security.

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.

In a `mode` of `0`, `1` and `2`, the paymaster expects repayment in some ERC20 token and possibly returns excessive prepayments back to the `UserOp.sender`. However, reverts of the `postOp()` call can result in this process to no being properly completed, as the state from the execution phase, but not from the passed validation phase, is rolled back in those cases. * `mode 0` and `mode 2`: user fully pays upper bound pre-payment, but is not getting potential excess funds back. In these modes, the Paymaster temporarily holds the funds, so reverts are limited to strange, non-standard ERC-20 behavior. However, if such a case were to happen, these ERC-20 tokens would now be stuck in the Paymaster of a possibly significantly overcharged `maxTokenAmount` amount of tokens. There is no mechanism to redeem those tokens again for anyone. They can also not be withdrawn from the paymaster, as the paymaster itself is currently not designed to be holding ERC-20 tokens permanently, instead it normally forwards the charged amount to the `receiver` address. * `mode 1`: This mode expects an approval to be set as part of the execution phase. If that does not happen as simulated or if the tokens get sent out the account before the `postOp()` call, the Paymaster does not receive any reimbursement from the user, as in this mode, the transfer is only completed in the `postOp()` call after the execution phase. In the V0.6 Paymaster, the user therefore fully gets away with a free UserOperation, as reverts are not performed as that would harm Paymaster reputation. In the V0.7 Paymaster, there is a revert, rolling back the execution phase of the UserOperation, but this still poses a griefing attack on the Paymaster, as the Paymaster is still required to pay for the reverting UserOperation without getting reimbursed.

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.

As of V0.7, the EntryPoint will no longer forward a `PostOpMode` enum of type `PostOpMode.postOpReverted`to the paymaster; hence, it's check in the V0.7 paymaster in the `if` statement can be skipped. However, in V0.6, that parameter value would indicate that something malfunctioned in the intended flow of this Paymaster. Especially reverts in the first `postOp()` call in case `mode = 1`, i.e. in the case that the ERC-20-payment is supposed to be collected in the `postOp()` call, would indicate that the user was not billed for the sponsored transaction. Therefore, a special event should be emitted here that indicates to enable monitoring for potential abuses.

The EntryPoint v0.6 implementation contains a bug in the invocation of the Paymaster’s `postOp()` function, where reverts with less than 32 bytes of revert data cause the entire bundle to revert. This can result in significant financial loss for bundlers and a banned reputation for the reverting paymaster. The used `safeTransferFrom()` function would revert with a shorter revert message, causing the bug to get triggered. For more info see this [PR](https://github.com/eth-infinitism/account-abstraction/pull/293). In `mode = 0`, the paymaster only performs the `safeTransferFrom()` in the `postOp()` call, which could technically revert, leading to the whole transaction from the bundler reverting.