Alchemy - Modular Account V2

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.

In the `getExecutionData()` function, all selectors that return `true` for `_isNativeFunction()` are marked as unlockable by global validation (`allowGlobalValidation = true`). However, this logic is flawed because `_isNativeFunction()` also returns `true` for many view functions, which are permissionless and not intended to be unlocked for global validation.

The `NativeTokenLimitModule.supportsInterface()`does not return `true` for `interfaceId == type(IValidationHookModule).interfaceId`, even though it provides validation hooks.

`NativeFunctionDelegate.isNativeFunction()` does not include all native selectors of the `ModularBase` contract. Crucial selectors are missing such as: * `invalidateDeferredValidationInstallNonce()`, * `performCreate()` and * `isValidSignature()` are missing, and also less crucial ones, such as: * `IERC721Receiver.onERC721Received.selector` * `IERC1155Receiver.onERC1155Received.selector` * `IERC1155Receiver.onERC1155BatchReceived.selector` and * ` IAccountExecute.executeUserOp.selector` Furthermore, `SemiModularAccountStorageOnly` also exposes an `initialize()` function and `ModularAccount` exposes an `initializeWithValidation()` that should technically instead be exposed by an overridden `_isNativeFunction()`.

The `LinkedListSetLib` library from the reference implementation has been adjusted to work with an underlying `SetValue` of type `bytes31` instead of `bytes30`. However, the castings in the `AccountStorage.toSetValue()` function have not been adjusted. Importantly, no information gets lost due to the erroneous casting, as the wrapped values are always less than 25 bytes long and also of type bytes, so only unused padding is omitted.

In `ModularAccountBase._computeDeferredValidationInstallTypedDataHash()`, the `deadline` is incorrectly masked to keep the least significant 4 bytes rather 6, truncating the upper 2 bytes. This limits possible deadline encodings to 2^32 - 1 UNIX seconds, some time in the year 2106.

The `KnownSelectorsLib.isIModuleFunction()` is missing a check for `preSignatureValidationHook()`.

The module allows account owners to set `validUntil` and `validAfter` timestamps for their operations. However, there is no validation to ensure that `validAfter` is less than or equal to `validUntil`. If an account owner inadvertently sets `validAfter` to a value greater than `validUntil`, the condition in `preRuntimeValidationHook()` will always revert.