EIP 7702 support
#8
Conversation
- Created `ResourceLockValidator` module which can handle standard ECDSA signed `UserOperation` and `UserOperation` with `ResourceLock` merkle proofs. - Added test suite for `ResourceLockValidator`. - Added `soldeer` dependency manager and reinstalled required dependencies (updated imports as required). - Removed existing dependencies , `lib` folder and `remappings.txt`. - Added dependency configs to `foundry.toml`. - Added new `ModularTestBase` to replace `TestAdvancedUtils` and cleaned up functionality to make it more generic and extensible. - Reorganised tests into folders and added specific test utils if needed which extend `ModularTestBase`. - Created own `Constants` file which we can migrate to eventually to reduce dependency on ERC7579 implementation.
- Added `FOUNDRY_DISABLE_NIGHTLY_WARNING` param to .example.env. - Removed `deny_warnings` config from foundry.toml. - Fixed import in deploy script.
- Reworked wallet to remove `AccessController` and storing wallet owner inside wallet directly. - Created new `ECDSAValidator`, `GuardianRecoveryValidator` modules. The `GuardianRecoveryValidator` module encapsulates the `AccessController` guardianship mechanism. These modules allow for installing guardianship retroactively and also now allow for multiple owners of a wallet to not be the default if not required (can use `ECDSAValidator` instead). - Reworked `CredibleAccountModule` to split into two distinctive modules (`CredibleAccountValidator` and `CredibleAccountHook`). This is due to how we need to clear modules when calling `onRedelegation()` which is required for `EIP-7702`. `CredibleAccountValidator` now contains the storage which `CredibleAccountHook` can read from. Now need to `initialize()` `CredibleAccountValidator` with the `CredibleAccountHook` post deployment for checking installation. - Reworked `CredibleAccountValidator` storage of token lock data. Added extra functionality to pull data from contract. - Updated wallet implementation to support `EIP-7702` delegation. - Updated wallet factory to now use proxies for the wallet. - Added helpers and tests for new modules and `EIP-7702`. - Reworked `HookMultiPlexer` so we now no longer need to use `ERC7579HookBase`. Reworked data we pass to Hooks when installing. - Reworked `ModuleManager`. It now integrates `HookManager`. Reworked the `fallback` to support `onRecieved` for `ERC7579 and `ERC1155`. - Created `Types` folder for storage of different type data. - Added support for pre-validation Hooks. - Added support for `ERC7201` namespaced storage and `ERC7779`. - Added `Initializable` to manage initialization of the wallet implementation. - Removed `IHookLens` as no longer required. - Restructured repo.
- Small test refactoring
- Updated `_isEIP7702Account` function in `ModuleManager`. - Added `ModularEtherspotWalletHarness` for testing. - Added test for checking that post delegation account is flagged as `EIP-7702`.
cryptonoyaiba
requested review from
ch4r10t33r,
kanthgithub and
nikhilkumar1612
WalkthroughThis update introduces a major architectural overhaul of a modular smart contract wallet system. It removes legacy ERC-7579 core contracts, interfaces, and test utilities, replacing them with a new, modularized structure. The new system features dedicated contracts and interfaces for account storage, execution logic, module management, and registry integration. Validator modules for ECDSA, session keys, resource locks, and guardian recovery are refactored or added, each with enhanced lifecycle management, error handling, and event emission. Supporting libraries for bootstrap configuration, execution encoding, and initialization are introduced. Deployment scripts, documentation, and configuration files are updated to match the new modular design, with dependency management shifted from submodules to Soldeer. Comprehensive gas usage reporting is also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WalletFactory
participant ProxyWallet
participant Implementation
participant ValidatorModule
participant HookModule
participant EntryPoint
User->>WalletFactory: createAccount(owner, salt, initCode)
WalletFactory->>WalletFactory: getAddress(salt, initCode)
alt Proxy not deployed
WalletFactory->>ProxyWallet: Deploy via CREATE2 (initCode)
ProxyWallet->>Implementation: Delegate call initializeAccount(data)
Implementation->>ValidatorModule: onInstall(data)
Implementation->>HookModule: onInstall(data)
end
WalletFactory->>User: return ProxyWallet address
User->>EntryPoint: submitUserOp(UserOperation)
EntryPoint->>ProxyWallet: validateUserOp(userOp, hash, missingFunds)
ProxyWallet->>ValidatorModule: validateUserOp(userOp, hash)
ValidatorModule->>ProxyWallet: return validationData
EntryPoint->>ProxyWallet: executeUserOp(userOp, hash)
ProxyWallet->>Implementation: execute logic (may call HookModule)
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
src/interfaces/base/IHook.sol (1)
1-17: 🛠️ Refactor suggestionNew hook interface supports pre/post execution validation.
This new interface defines the contract for modules that implement pre- and post-execution hooks, enabling transaction validation before execution and follow-up actions after execution. This aligns with the PR objective of supporting pre-validation hooks and enhancing the modular architecture.
However, there's room for improvement in the documentation and error handling:
Consider adding:
- NatSpec documentation for each function explaining expected behavior
- Documentation on error handling expectations (should it revert on failure?)
- Events for important state changes or hook executions
// SPDX-License-Identifier: MIT pragma solidity ^0.8.27; import {IModule} from "./IModule.sol"; interface IHook is IModule { /*////////////////////////////////////////////////////////////// FUNCTIONS //////////////////////////////////////////////////////////////*/ + /** + * @notice Performs checks before the execution of a transaction + * @param msgSender The address that initiated the transaction + * @param msgValue The amount of ETH sent with the transaction + * @param msgData The calldata of the transaction + * @return hookData Data that will be passed to postCheck after execution + * @dev Should revert if the pre-execution check fails + */ function preCheck(address msgSender, uint256 msgValue, bytes calldata msgData) external returns (bytes memory hookData); + /** + * @notice Performs checks after the execution of a transaction + * @param hookData Data returned from the preCheck function + * @dev Should revert if the post-execution check fails + */ function postCheck(bytes calldata hookData) external; }
🧹 Nitpick comments (52)
README.md (1)
57-57: Fix the duplicate semicolon in the pragma statement.There's an extra semicolon in the pragma statement which should be removed.
-pragma solidity ^0.8.27;; +pragma solidity ^0.8.27;script/MultipleOwnerECDSAValidator.s.sol (1)
32-36: Consider removing commented-out code.There are commented-out lines that appear to be debugging code. Since this is a finalized deployment script, consider removing these lines for cleanliness.
console2.log("MultipleOwnerECDSAValidator deployed at address", address(multipleOwnerECDSAValidator)); - // bytes memory valCode = address(MultipleOwnerECDSAValidator) - // .code; - // console2.logBytes(valCode);src/interfaces/base/IValidator.sol (1)
1-24: Well structured interface that aligns with the modular wallet design.The interface is clean and follows good Solidity practices for defining validation capabilities for user operations and signatures. It properly inherits from
IModulein line with the architectural changes for EIP-7702 support.Consider enhancing documentation with NatSpec comments for improved readability and developer experience:
interface IValidator is IModule { + /// @notice Error thrown when an invalid target address is provided error InvalidTargetAddress(address target); + /// @notice Validates a user operation according to ERC-4337 + /// @param userOp The packed user operation to validate + /// @param userOpHash The hash of the user operation + /// @return validationResult A uint256 value indicating the validation result (0 for success) function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash) external returns (uint256); + /// @notice Validates a signature with a specified sender (extends ERC-1271 functionality) + /// @param sender The address of the sender + /// @param hash The hash of the data to validate + /// @param data The signature data + /// @return magicValue The magic value (0x1626ba7e) if signature is valid function isValidSignatureWithSender(address sender, bytes32 hash, bytes calldata data) external view returns (bytes4);src/libraries/SentinelList.sol (1)
42-47: Well-implemented helper function to prevent initialization errors.The new
safePushfunction nicely encapsulates the initialization check and push operation in a single call, reducing the chance of errors when using the library.Consider adding NatSpec documentation to provide context and usage guidelines:
+ /** + * @notice Safely adds a new entry to the list, initializing the list if necessary + * @param self The SentinelList storage reference + * @param newEntry The address to add to the list + */ function safePush(SentinelList storage self, address newEntry) internal { if (!alreadyInitialized({self: self})) { init({self: self}); } push({self: self, newEntry: newEntry}); }src/interfaces/ercs/IERC7779.sol (1)
1-32:❓ Verification inconclusive
Well-defined interface supporting EIP-7702 implementation requirements.
This interface provides the necessary functions for interoperable delegated accounts, which is central to the EIP-7702 support mentioned in the PR. The comments provide clear explanations of the purpose and implementation details.
Consider converting the documentation to NatSpec format for better tooling integration:
interface IERC7779 { - /* - * @dev Externally shares the storage bases that has been used throughout the account. - * Majority of 7702 accounts will have their distinctive storage base to reduce the - chance of storage collision. - * This allows the external entities to know what the storage base is of the account. - * Wallets willing to redelegate already-delegated accounts should call - accountStorageBase() to check if it confirms with the account it plans to redelegate. - * - * The bytes32 array should be stored at the storage slot: - keccak(keccak('InteroperableDelegatedAccount.ERC.Storage')-1) & ~0xff - * This is an append-only array so newly redelegated accounts should not overwrite the - storage at this slot, but just append their base to the array. - * This append operation should be done during the initialization of the account. - */ + /** + * @dev Externally shares the storage bases that has been used throughout the account. + * Majority of 7702 accounts will have their distinctive storage base to reduce the + * chance of storage collision. + * This allows the external entities to know what the storage base is of the account. + * Wallets willing to redelegate already-delegated accounts should call + * accountStorageBase() to check if it confirms with the account it plans to redelegate. + * + * The bytes32 array should be stored at the storage slot: + * keccak(keccak('InteroperableDelegatedAccount.ERC.Storage')-1) & ~0xff + * This is an append-only array so newly redelegated accounts should not overwrite the + * storage at this slot, but just append their base to the array. + * This append operation should be done during the initialization of the account. + * @return An array of storage bases used by the account + */ function accountStorageBases() external view returns (bytes32[] memory); - /* - * @dev Function called before redelegation. - * This function should prepare the account for a delegation to a different - implementation. - * This function could be triggered by the new wallet that wants to redelegate an already - delegated EOA. - * It should uninitialize storages if needed and execute wallet-specific logic to prepare - for redelegation. - * msg.sender should be the owner of the account. - */ + /** + * @dev Function called before redelegation. + * This function should prepare the account for a delegation to a different + * implementation. + * This function could be triggered by the new wallet that wants to redelegate an already + * delegated EOA. + * It should uninitialize storages if needed and execute wallet-specific logic to prepare + * for redelegation. + * msg.sender should be the owner of the account. + * @return true if the redelegation preparation was successful + */ function onRedelegation() external returns (bool); }Also, ensure implementations enforce the ownership check mentioned in the comments for the
onRedelegation()function to prevent unauthorized redelegation.Are there any existing checks in the implementation to verify that msg.sender is the owner when calling onRedelegation()? Please examine any classes that implement this interface.
🏁 Script executed:
#!/bin/bash # Search for implementations of the onRedelegation function to verify ownership checks rg -A 10 "function onRedelegation\(\)" --type solLength of output: 79
I generated an updated search using file globs to look for implementations of the
onRedelegation()function but did not locate any explicit ownership checks (i.e. verifying thatmsg.senderis the account owner). This means we cannot conclusively infer that all implementing contracts are enforcing the ownership restriction as described in the documentation.Below is the rewritten review comment incorporating the NatSpec conversion (based on the diff snippet) and a reminder to verify that ownership checks are in place within all implementations:
Action Required: Verify Ownership Enforcement on onRedelegation Implementation
The interface is now documented with NatSpec, which improves tooling integration. The updated comments clearly explain the intended behavior:
interface IERC7779 { - /* - * @dev Externally shares the storage bases that has been used throughout the account. - * Majority of 7702 accounts will have their distinctive storage base to reduce the - chance of storage collision. - * This allows the external entities to know what the storage base is of the account. - * Wallets willing to redelegate already-delegated accounts should call - accountStorageBase() to check if it confirms with the account it plans to redelegate. - * - * The bytes32 array should be stored at the storage slot: - keccak(keccak('InteroperableDelegatedAccount.ERC.Storage')-1) & ~0xff - * This is an append-only array so newly redelegated accounts should not overwrite the - storage at this slot, but just append their base to the array. - * This append operation should be done during the initialization of the account. - */ + /** + * @dev Externally shares the storage bases that has been used throughout the account. + * Majority of 7702 accounts will have their distinctive storage base to reduce the + * chance of storage collision. + * This allows the external entities to know what the storage base of the account is. + * Wallets willing to redelegate already-delegated accounts should call + * accountStorageBase() to validate the account before redelegation. + * + * The bytes32 array should be stored at the storage slot: + * keccak(keccak('InteroperableDelegatedAccount.ERC.Storage')-1) & ~0xff + * This is an append-only array so that newly redelegated accounts append their base + * instead of overwriting existing storage. + * This append operation should be executed during account initialization. + * @return An array of storage bases used by the account. + */ function accountStorageBases() external view returns (bytes32[] memory); - /* - * @dev Function called before redelegation. - * This function should prepare the account for a delegation to a different - implementation. - * This function could be triggered by the new wallet that wants to redelegate an already - delegated EOA. - * It should uninitialize storages if needed and execute wallet-specific logic to prepare - for redelegation. - * msg.sender should be the owner of the account. - */ + /** + * @dev Function called before redelegation. + * Prepares the account for delegation to a different implementation. + * Typically, this is triggered by a new wallet attempting to redelegate an already delegated EOA. + * The function should uninitialize storages where necessary and execute wallet-specific + * logic to prepare for redelegation. + * **Note:** Implementations must enforce that `msg.sender` is the account owner. + * @return true if the redelegation preparation was successful. + */ function onRedelegation() external returns (bool); }Next Steps:
- Manual Verification: Please ensure that all contracts implementing this interface enforce the ownership check (i.e. that
msg.senderis the owner) when executingonRedelegation().- Implementation Update: If any implementation lacks this enforcement, update it to include a check such as
require(msg.sender == owner, "Unauthorized redelegation").
src/interfaces/modules/IERC20SessionKeyValidator.sol (1)
69-69: Function name and return value are inconsistentThe function is named
isSessionKeyLivebut returns a value namedpaused, which is confusing. These opposing terms could lead to misinterpretation of the return value.- function isSessionKeyLive(address _sessionKey) external view returns (bool paused); + function isSessionKeyLive(address _sessionKey) external view returns (bool isLive);src/interfaces/wallet/IModularEtherspotWallet.sol (2)
12-28: Good standards composition with clear error handling.The interface successfully combines multiple ERC standards (7579, 4337, 7779) to create a comprehensive wallet interface. The custom errors are well-defined with appropriate parameters and descriptions, making error handling clear for implementers.
Consider adding a parameter to the
AccountInitializationFailederror to provide more context about why initialization failed, which would help with debugging.- error AccountInitializationFailed(); + error AccountInitializationFailed(bytes32 reason);
2-2: Consider upgrading the Solidity version for consistency.The pragma version (
^0.8.21) is different from other files in the repository (e.g.,^0.8.27in AccountStorage.sol). For consistency and to take advantage of the latest features and bugfixes, consider updating to the same version used in other files.-pragma solidity ^0.8.21; +pragma solidity ^0.8.27;src/core/RegistryAdapter.sol (1)
26-34: Registry configuration lacks removal mechanism.The
setRegistryfunction allows setting a registry and configuring attesters, but there's no way to unset or change the registry once it's configured. Consider adding functionality to remove or update the registry if needed.function setRegistry(IERC7484 registry, address[] calldata attesters, uint8 threshold) external onlyEntryPointOrSelf { $registry = registry; if (attesters.length > 0) { registry.trustAttesters(threshold, attesters); } } +/** + * @notice Removes the current registry configuration + * @dev Can only be called by the entry point or the account itself + */ +function removeRegistry() external onlyEntryPointOrSelf { + $registry = IERC7484(address(0)); + emit ERC7484RegistryConfigured(address(this), address(0)); +}src/interfaces/base/IModule.sol (1)
16-21: Consider adding event emissions for module lifecycle.While the interface defines lifecycle hooks, it doesn't include events for these important state changes. Consider adding event definitions for module installation and uninstallation to improve transparency and enable off-chain tracking of module lifecycle.
interface IModule { /*////////////////////////////////////////////////////////////// ERRORS //////////////////////////////////////////////////////////////*/ error AlreadyInitialized(address smartAccount); error NotInitialized(address smartAccount); + /*////////////////////////////////////////////////////////////// + EVENTS + //////////////////////////////////////////////////////////////*/ + + event ModuleInstalled(address indexed smartAccount, bytes data); + event ModuleUninstalled(address indexed smartAccount, bytes data); /*////////////////////////////////////////////////////////////// FUNCTIONS //////////////////////////////////////////////////////////////*/ function onInstall(bytes calldata data) external; function onUninstall(bytes calldata data) external; function isModuleType(uint256 moduleTypeId) external view returns (bool); function isInitialized(address smartAccount) external view returns (bool); }src/core/BaseAccount.sol (1)
49-65: Consider verifying call success or clarifying intent.Using inline assembly to call
caller()with_missingAccountFundsand ignoring the call result is an accepted approach if the EntryPoint handles failures. However, you may want to add inline comments or documentation clarifying that this is by design so readers understand why errors are intentionally ignored.src/factory/FactoryStaker.sol (1)
51-53: Consider validating _withdrawTo for accidental burns.To avoid inadvertently sending withdrawn stake to the zero address, consider reverting or warning if
_withdrawTo == address(0). Although it’s valid to “burn” funds intentionally, you might want an additional safety check here.src/interfaces/modules/IECDSAValidator.sol (1)
64-73: Missing ERC-1271 magic value constants in documentationWhile the function signature is correct, the documentation mentions returning
ERC1271_MAGIC_VALUEorERC1271_INVALIDwithout specifying their values. Consider adding the actual hex values in the documentation for clarity./// @param hash The hash of the data that was signed /// @param signature The signature to validate -/// @return bytes4 ERC1271_MAGIC_VALUE if valid, ERC1271_INVALID otherwise +/// @return bytes4 ERC1271_MAGIC_VALUE (0x1626ba7e) if valid, ERC1271_INVALID (0xffffffff) otherwisedocs/RESOURCE_LOCK_VALIDATOR.md (1)
87-87: Improve punctuation usage.Static analysis flags a possible punctuation issue. Consider removing or refining the dash in "Error Conditions -
RLV_AlreadyInstalled" for cleaner formatting.-## Error Conditions - - `RLV_AlreadyInstalled`: Validator ... +## Error Conditions +- `RLV_AlreadyInstalled`: Validator ...🧰 Tools
🪛 LanguageTool
[uncategorized] ~87-~87: Loose punctuation mark.
Context: ...ror Conditions -RLV_AlreadyInstalled: Validator already installed for account...(UNLIKELY_OPENING_PUNCTUATION)
script/DeployAllAndSetup.s.sol (1)
136-243: Consider abstracting repeated deployment logic.Lines deploying each validator/hook follow a near-identical pattern. Factoring this out into a helper function or loop would reduce repetition and make the script more maintainable.
src/modules/hooks/ModuleIsolationHook.sol (1)
66-91: Duplicate signature checks across call types.
integrityCheckrepeats the signature extraction andbannedSigslookup for each call type. Although it’s correct, consider reducing repetition by extracting a helper to parse the function selector.src/libraries/BootstrapLib.sol (2)
25-38: Consider parameter validation in_buildSingleConfig.The logic checks for a zero module address, but does not validate
_data. If empty data is disallowed, consider reverting or clarifying usage in documentation so clients consistently know when to pass empty data.
73-81: Add unit tests for empty config constructors.These helper functions return empty structs and arrays, which are prone to edge cases. Adding test coverage ensures no unexpected behavior when no modules are installed.
Do you want me to open a new issue to track these specific unit tests?
src/core/ERC7779Adapter.sol (1)
36-42: Validate zero-value storage base if needed.Currently
_addStorageBasepushes the givenstorageBaseunconditionally. If a zero storage base is invalid, consider adding a revert or a check to avoid silently pushing an unintended entry.src/test/TestUniswapV2.sol (2)
17-23: Mock logic for swap might need more granularity.You hardcode
amountOut = amountOutMin + 1 ether; while suitable for a mock, overly large increments could cause confusion. If you require different scenarios, consider parameterizing the premium aboveamountOutMin.
35-50: Check re-entrancy in mock swaps if used in more complex tests.Even though this is a simple mock, if you extend functionality or store state in the future, consider adding re-entrancy protection. It helps reflect actual DEX behavior more accurately.
script/CredibleAccountScript.s.sol (1)
1-18: Good script setup with clear imports and constants.The script imports necessary contracts and defines constants with descriptive names. However, the EXPECTED address constants are all set to zero address, and their validation code is commented out.
Consider either:
- Removing the unused EXPECTED address constants, or
- Uncommenting the validation logic and providing actual expected addresses for production deployments
src/interfaces/core/IModuleManager.sol (1)
87-138: Comprehensive function signatures
Functions for paginating, installing, and checking modules/hook usage are thoughtfully laid out. However, the interface is quite substantial; you might consider splitting it if it grows more in future to keep it maintainable.src/modules/hooks/HookMultiPlexer.sol (3)
43-52: Commented-out constructor code
Leaving old, commented-out code can confuse maintainers. Consider removing it if it's no longer needed.- // constructor(IERC7484 _registry) { - // REGISTRY = _registry; - // } + // Remove if not needed
66-125: onInstall function complexity
The setup flow is clear, decoding and installing a variety of hooks. However, this method is quite large and might benefit from smaller helper functions to improve maintainability and reduce complexity.
131-152: onUninstall teardown
This logic properly cleans up installed hooks but partially duplicates iteration steps with_removeHook. Avoid potential double processing by removing hooks before clearing arrays, or consider a single unified flowsheet.src/modules/validators/MultipleOwnerECDSAValidator.sol (2)
23-35: Efficient struct definition but consider indexing strategies for large ownership sets.
Storing both an array and a mapping is an effective way to maintain O(1) lookups. However, note that removing owners requires a linear scan, so for large arrays, consider a more sophisticated data structure or a mapping of owner ⇒ index to reduce loop overhead.
117-141:removeOwnerfunction is correct but O(N) removal can be optimized.
- Ensures the caller is a valid owner.
- Blocks removing the last owner.
- Traverses the array to remove the target.
Consider indexing or data structures with faster owner removal if the set can grow large.src/core/ModuleManager.sol (1)
520-589: Detailed fallback execution logic.
- Verifies if the caller is allowed if whitelisted.
- Supports
STATICCALLand standard call patterns, forwarding revert messages.- Handles ERC token reception safely by returning known signatures.
- This large function is logically cohesive, but if it grows further, consider factoring parts out for maintainability.
src/modules/hooks/CredibleAccountHook.sol (4)
1-3: Consider aligning the Solidity version across the project.
Usingpragma solidity ^0.8.27;is acceptable, but ensure that other contracts and dependencies also align with or support this version in case of potential version mismatch.
22-24: Add more fields to HookStorage if you anticipate expansions.
Currently,HookStorageonly storesowner. If future expansions to store timestamps, roles, or additional states are expected, planning a more flexible approach could simplify versioning.
52-60: Validate input data structure foronInstall.
Your code checksdata.lengthand decodesscwfrom[12:32]bytes. Consider validating that any additional parameters indataare also conforming to expected structure.
91-106: Potential out-of-range revert condition.
When checkingwalletBalance < preCheckLocked && walletBalance < postCheckLocked, it reverts. Consider clarifying whether the logical AND is correct or if you intended an OR condition (for partial compliance).src/core/ExecutionHelper.sol (4)
1-3: Check for project-wide Solidity version consistency.
Ensure other contracts in the repository also use^0.8.27or a compatible version to avoid compilation conflicts.
28-50: Potential extension or fallback forUnsupportedCallType.
If new call types are introduced in the future, consider an extensible or fallback pattern for unrecognized_callTypeto reduce refactoring overhead.
182-198: No checks for re-entrancy or repeated calls
Consider adding re-entrancy protection if user-supplied calls can trigger state changes that re-invoke these methods.Also applies to: 200-209, 211-225
227-242: Using inline assembly is efficient but might be risky.
Inline assembly is powerful yet error-prone. Consider adding more in-code comments for maintainability, especially around addresses and memory offsets.Also applies to: 248-342
src/libraries/HookMultiPlexerLib.sol (4)
2-2: Consider consolidating or standardizing the project’s pragma statement.
You’re using^0.8.27; verify the rest of the codebase uses the same to avoid compilation conflicts.
92-98:postCheckSubHookusage.
You’re passing additional parameters (address(this),msg.sender) inabi.encodePacked. Confirm that subhooks expecting these extra fields interpret them correctly.
169-180: Efficient searching withindexOfandcontains.
Linear search is simple but can be expensive for large arrays. If you expect large arrays, consider a more optimal data structure (e.g., mapping or a sorted array + binary search).Also applies to: 182-184, 194-205
410-470: Batching logic is well structured for signature hooks.
Sorting and uniquifying the target signatures helps avoid duplicates, improving overall efficiency. Consider adding event logs if critical to debugging.src/modules/validators/GuardianRecoveryValidator.sol (2)
173-197: Consider indexing for array removal.When removing an owner in
removeOwner, the loop performs a swap-and-pop pattern:vs.owners[i] = vs.owners[vs.owners.length - 1]; vs.owners.pop();This is a standard approach but reorders the array. Confirm that stable ordering of owners is not required for off-chain indexing or user interfaces.
392-411: Check aggregator or bundled signature flow.The
validateUserOpfunction defaults to ECDSA (65-byte) checks or fails otherwise. If future expansions require aggregator-based or multiple-sig recoveries, an additional signature handling path might be needed.src/modules/validators/ERC20SessionKeyValidator.sol (2)
100-122: Validate session time checks.
enableSessionKeyrecordsvalidAfterandvalidUntilbut does not impose immediate runtime checks for the current block. You rely on bundlers to process these periods viapackValidationData. Make sure off-chain components (e.g., bundlers) correctly reject expired or not-yet-active session keys.
178-223: Verify advanced session key usage.
validateSessionKeyParamscombines checks for single-call and batch-call usage. It discards user operations if any call is disallowed. This all-or-nothing approach is typically desired for batch execution. Consider partial reverts only if you need partial batch success.src/interfaces/modules/IGuardianRecoveryValidator.sol (1)
39-44: Clarify difference between 'NotOwnerOrGuardian' and 'NotOwnerOrGuardianOrSelf'.Having both
GRV_NotOwnerOrGuardianandGRV_NotOwnerOrGuardianOrSelfis logical to distinguish who can perform certain actions. Ensure that overlapping conditions don’t cause confusion or an accidental mismatch.src/modules/validators/CredibleAccountValidator.sol (4)
36-37: Clarify the relationship betweenhookMultiPlexerandcaHook.
Currently,hookMultiPlexeris set in the constructor, whilecaHookis assigned separately ininitialize(). This split may be intentional, but consider documenting that thecaHookalso depends on the same multiplexer being properly initialized.
108-113: Consider adding a guard against repeatedinitializecalls.
The functioninitialize(CredibleAccountHook _caHook)lacks an explicit check preventing multiple invocations. If calling it more than once is unintended, consider adding a guard (e.g., verifying_credibleAccountStorage[owner].initialized) to avoid unintentional re-initialization.
153-184: Potential partial usage limitation inenableSessionKey.
The function locks a fixed amount of each token for the session. If the design requires partial usage or dynamic token amounts, consider adding support for incremental locking. Otherwise, the all-or-nothing locking approach is fine but should be clearly documented.
186-215: Ensure session key disablement covers all edge cases.
The current logic reverts if locked tokens are unclaimed and the key is still valid. This is correct for preserving locked amounts but might inconvenience valid use cases (e.g., emergency disable). Clarify or confirm that emergency disabling is not needed.Do you need a fallback path for forced disablement if locked tokens remain?
src/modules/validators/SessionKeyValidator.sol (2)
82-126: Refine checks inenableSessionKey.
The function properly checks for zero addresses and preexisting session keys. Consider further clarifying:
- Time intervals: confirm that
_sessionData.validUntil>_sessionData.validAfter.- Whether partial permission usage or expansion is allowed in subsequent calls.
134-148: Evaluate forced disablement of session keys with active uses.
disableSessionKeyfully removes the session key and all permissions, reverting if the key does not exist. Consider whether a forced disable path is needed even if the session is still valid or has unexercised uses, especially for emergency cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
soldeer.lockis excluded by!**/*.lock
📒 Files selected for processing (107)
.env.example(1 hunks).gitignore(1 hunks).gitmodules(0 hunks)README.md(2 hunks)docs/RESOURCE_LOCK_VALIDATOR.md(1 hunks)foundry.toml(1 hunks)lib/account-abstraction(0 hunks)lib/forge-std(0 hunks)lib/openzeppelin-contracts(0 hunks)lib/solady(0 hunks)lib/solarray(0 hunks)remappings.txt(0 hunks)script/CredibleAccountScript.s.sol(1 hunks)script/DeployAllAndSetup.s.sol(1 hunks)script/ERC1155FallbackHandler.s.sol(1 hunks)script/ERC20SessionKeyValidator.s.sol(1 hunks)script/ModularEtherspotWallet.s.sol(1 hunks)script/ModularEtherspotWalletFactory.s.sol(1 hunks)script/MultipleOwnerECDSAValidator.s.sol(1 hunks)script/NonDeterministicDeployAllAndSetup.s.sol(3 hunks)script/SessionKeyValidator.s.sol(1 hunks)script/StakeWalletFactory.s.sol(1 hunks)script/UnlockFactoryStake.s.sol(1 hunks)script/WithdrawFactoryStake.s.sol(1 hunks)src/ERC7579/core/AccountBase.sol(0 hunks)src/ERC7579/core/ExecutionHelper.sol(0 hunks)src/ERC7579/core/HookManager.sol(0 hunks)src/ERC7579/core/ModuleManager.sol(0 hunks)src/ERC7579/core/Receiver.sol(0 hunks)src/ERC7579/interfaces/IERC7579Module.sol(0 hunks)src/ERC7579/interfaces/IMSA.sol(0 hunks)src/ERC7579/libs/ExecutionLib.sol(0 hunks)src/ERC7579/libs/ModuleTypeLib.sol(0 hunks)src/ERC7579/test/Bootstrap.t.sol(0 hunks)src/ERC7579/test/dependencies/EntryPoint.sol(0 hunks)src/ERC7579/test/mocks/MockDelegateTarget.sol(0 hunks)src/ERC7579/test/mocks/MockExecutor.sol(0 hunks)src/ERC7579/test/mocks/MockFallbackHandler.sol(0 hunks)src/ERC7579/test/mocks/MockTarget.sol(0 hunks)src/ERC7579/utils/Bootstrap.sol(0 hunks)src/access/AccessController.sol(0 hunks)src/core/AccountStorage.sol(1 hunks)src/core/BaseAccount.sol(1 hunks)src/core/ERC7779Adapter.sol(1 hunks)src/core/ExecutionHelper.sol(1 hunks)src/core/ModuleManager.sol(1 hunks)src/core/RegistryAdapter.sol(1 hunks)src/factory/FactoryStaker.sol(1 hunks)src/factory/ModularEtherspotWalletFactory.sol(1 hunks)src/interfaces/IAccessController.sol(0 hunks)src/interfaces/ICredibleAccountModule.sol(0 hunks)src/interfaces/IHookLens.sol(0 hunks)src/interfaces/IModularEtherspotWallet.sol(0 hunks)src/interfaces/IProofVerifier.sol(0 hunks)src/interfaces/base/IERC4337Account.sol(1 hunks)src/interfaces/base/IERC7579Account.sol(7 hunks)src/interfaces/base/IExecutor.sol(1 hunks)src/interfaces/base/IFallback.sol(1 hunks)src/interfaces/base/IHook.sol(1 hunks)src/interfaces/base/IModule.sol(1 hunks)src/interfaces/base/IPreValidationHook.sol(1 hunks)src/interfaces/base/IValidator.sol(1 hunks)src/interfaces/core/IAccountStorage.sol(1 hunks)src/interfaces/core/IExecutionHelper.sol(1 hunks)src/interfaces/core/IModuleManager.sol(1 hunks)src/interfaces/ercs/IERC7484.sol(2 hunks)src/interfaces/ercs/IERC7779.sol(1 hunks)src/interfaces/factory/IFactoryStaker.sol(1 hunks)src/interfaces/factory/IModularEtherspotWalletFactory.sol(1 hunks)src/interfaces/modules/IECDSAValidator.sol(1 hunks)src/interfaces/modules/IERC20SessionKeyValidator.sol(4 hunks)src/interfaces/modules/IGuardianRecoveryValidator.sol(1 hunks)src/interfaces/modules/IHookMultiPlexer.sol(1 hunks)src/interfaces/modules/IMultipleOwnerECDSAValidator.sol(1 hunks)src/interfaces/modules/IResourceLockValidator.sol(1 hunks)src/interfaces/modules/ISessionKeyValidator.sol(9 hunks)src/interfaces/wallet/IModularEtherspotWallet.sol(1 hunks)src/libraries/ArrayLib.sol(1 hunks)src/libraries/BootstrapLib.sol(1 hunks)src/libraries/ExecutionLib.sol(1 hunks)src/libraries/HashLib.sol(1 hunks)src/libraries/HookMultiPlexerLib.sol(15 hunks)src/libraries/Initializable.sol(1 hunks)src/libraries/ModeLib.sol(3 hunks)src/libraries/SentinelList.sol(2 hunks)src/modules/fallbacks/ERC1155FallbackHandler.sol(2 hunks)src/modules/hooks/CredibleAccountHook.sol(1 hunks)src/modules/hooks/HookMultiPlexer.sol(16 hunks)src/modules/hooks/ModuleIsolationHook.sol(6 hunks)src/modules/validators/CredibleAccountModule.sol(0 hunks)src/modules/validators/CredibleAccountValidator.sol(1 hunks)src/modules/validators/ECDSAValidator.sol(1 hunks)src/modules/validators/ERC20SessionKeyValidator.sol(4 hunks)src/modules/validators/GuardianRecoveryValidator.sol(1 hunks)src/modules/validators/MultipleOwnerECDSAValidator.sol(1 hunks)src/modules/validators/ResourceLockValidator.sol(1 hunks)src/modules/validators/SessionKeyValidator.sol(16 hunks)src/test/TestCounter.sol(1 hunks)src/test/TestERC1155.sol(2 hunks)src/test/TestERC20.sol(1 hunks)src/test/TestERC721.sol(1 hunks)src/test/TestExecutor.sol(0 hunks)src/test/TestUSDC.sol(1 hunks)src/test/TestUniswapV2.sol(2 hunks)src/test/TestUniswapV3.sol(3 hunks)src/test/TestWETH.sol(2 hunks)src/test/mocks/MockDelegateTarget.sol(1 hunks)
⛔ Files not processed due to max files limit (35)
- src/test/mocks/MockExecutor.sol
- src/test/mocks/MockFallbackHandler.sol
- src/test/mocks/MockHook.sol
- src/test/mocks/MockModule.sol
- src/test/mocks/MockRegistry.sol
- src/test/mocks/MockTarget.sol
- src/test/mocks/MockValidator.sol
- src/types/Constants.sol
- src/types/Enums.sol
- src/types/Structs.sol
- src/types/Types.sol
- src/utils/Bootstrap.sol
- src/utils/ERC4337Utils.sol
- src/utils/ERC7484RegistryAdapter.sol
- src/utils/ERC7579HookBase.sol
- src/utils/ModularEtherspotWalletProxy.sol
- src/utils/ProofVerifier.sol
- src/utils/TrustedForwarder.sol
- src/wallet/FactoryStaker.sol
- src/wallet/ModularEtherspotWallet.sol
- src/wallet/ModularEtherspotWalletFactory.sol
- test/ModularTestBase.sol
- test/TestAdvancedUtils.t.sol
- test/harnesses/CredibleAccountModuleHarness.sol
- test/harnesses/ModularEtherspotWalletHarness.sol
- test/harnesses/SessionKeyValidatorHarness.sol
- test/modules/CredibleAccount/concrete/CredibleAccountTest.t.sol
- test/modules/CredibleAccount/utils/CredibleAccountTestUtils.sol
- test/modules/CredibleAccountModule/concrete/CredibleAccountModule.t.sol
- test/modules/CredibleAccountModule/fuzz/CredibleAccountModule.t.sol
- test/modules/CredibleAccountModule/utils/CredibleAccountModuleTestUtils.sol
- test/modules/ECDSAValidator/concrete/ECDSAValidator.t.sol
- test/modules/ECDSAValidator/fuzz/ECDSAValidator.t.sol
- test/modules/ERC1155FallbackHandler/concrete/ERC1155FallbackHandler.t.sol
- test/modules/ERC20SessionKeyValidator.t.sol
💤 Files with no reviewable changes (31)
- lib/openzeppelin-contracts
- lib/solady
- src/interfaces/IHookLens.sol
- lib/solarray
- lib/account-abstraction
- lib/forge-std
- src/ERC7579/core/Receiver.sol
- src/interfaces/IModularEtherspotWallet.sol
- src/ERC7579/test/mocks/MockDelegateTarget.sol
- src/ERC7579/test/mocks/MockTarget.sol
- remappings.txt
- .gitmodules
- src/interfaces/IProofVerifier.sol
- src/ERC7579/core/AccountBase.sol
- src/ERC7579/core/HookManager.sol
- src/ERC7579/libs/ExecutionLib.sol
- src/ERC7579/interfaces/IMSA.sol
- src/ERC7579/test/Bootstrap.t.sol
- src/interfaces/IAccessController.sol
- src/access/AccessController.sol
- src/ERC7579/test/mocks/MockFallbackHandler.sol
- src/test/TestExecutor.sol
- src/ERC7579/utils/Bootstrap.sol
- src/ERC7579/test/mocks/MockExecutor.sol
- src/ERC7579/libs/ModuleTypeLib.sol
- src/modules/validators/CredibleAccountModule.sol
- src/ERC7579/interfaces/IERC7579Module.sol
- src/ERC7579/test/dependencies/EntryPoint.sol
- src/interfaces/ICredibleAccountModule.sol
- src/ERC7579/core/ExecutionHelper.sol
- src/ERC7579/core/ModuleManager.sol
🧰 Additional context used
🪛 LanguageTool
docs/RESOURCE_LOCK_VALIDATOR.md
[uncategorized] ~87-~87: Loose punctuation mark.
Context: ...ror Conditions - RLV_AlreadyInstalled: Validator already installed for account...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (219)
.env.example (1)
3-3: Addition of Foundry nightly warning suppression is appropriate.Adding
FOUNDRY_DISABLE_NIGHTLY_WARNING=1is a standard and safe way to suppress Foundry nightly warnings, improving developer experience.src/libraries/HashLib.sol (1)
1-11: HashLib is well-implemented and follows best practices.The library provides a clear, correct, and reusable hashing utility for typed initialization data.
src/libraries/ArrayLib.sol (1)
2-67: NatSpec comments and style improvements enhance code quality.The added documentation and style updates improve clarity and maintainability without altering logic.
src/test/TestERC1155.sol (1)
2-17: Pragma and formatting updates are appropriate.The pragma update and function signature formatting modernize the test contract without affecting functionality.
src/test/TestWETH.sol (1)
2-21: Pragma, import path, and formatting updates are correct.These changes improve compatibility and code style without affecting logic.
src/test/TestCounter.sol (1)
2-2: Pragma version update is safe and improves compatibility.Upgrading to
^0.8.27is a standard and beneficial change for broader compiler support..gitignore (1)
22-23: Dependency ignore rule updated correctly.Ignoring the
dependenciesfolder is appropriate for Soldeer-managed dependencies.src/test/TestERC20.sol (1)
2-4: Pragma and import path updates are correct and beneficial.Switching to
^0.8.27and using the@openzeppelinpackage path are both best practices for modern Solidity projects.src/test/TestUSDC.sol (1)
2-4: Pragma and import path updates are correct and beneficial.These changes modernize the code and ensure compatibility with the updated dependency management.
src/interfaces/base/IExecutor.sol (1)
1-6: New marker interface is well-structured and aligns with modular design.Introducing
IExecutoras an extension ofIModuleis a clean and extensible approach.src/interfaces/base/IFallback.sol (1)
6-6: Well-designed marker interface.This interface follows the Interface Segregation Principle by creating a specific marker interface for fallback handlers. This design helps with type safety and module classification in your modular architecture without adding unnecessary function requirements.
README.md (1)
18-26: Good addition of dependency management information.Adding clear instructions for Soldeer package management improves the developer experience and aligns with modern Solidity development practices.
src/test/TestERC721.sol (3)
2-2: Good practice using caret versioning for Solidity pragma.Updating to a caret range (
^0.8.27) allows for compatibility with minor version updates while ensuring minimum required features are available.
4-4: Improved import path using scoped package name.Using the scoped package import (
@openzeppelin/contracts/...) instead of a relative path makes the code more compatible with package managers like Soldeer that you've adopted.
10-10: Clean event declaration formatting.The single-line event declaration improves code readability and consistency.
script/MultipleOwnerECDSAValidator.s.sol (5)
8-10: Improved documentation with NatSpec comments.The NatSpec-style documentation provides better clarity about the script's purpose and authorship.
13-14: More concise variable declarations.Condensing the immutable SALT and constant EXPECTED_MULTIPLE_OWNER_ECDSA_VALIDATOR declarations to single lines makes the code more readable.
20-22: Improved code section demarcation.The comment formatting clearly separates the deployment section, enhancing readability.
28-29: Cleaner deployment logic.Combining the contract creation and address verification steps makes the deployment logic more concise without sacrificing clarity.
39-40: Improved console log formatting.The condensed console logging makes the output more readable.
src/test/mocks/MockDelegateTarget.sol (1)
4-9: Well-implemented mock with proper error handling.This mock contract correctly propagates revert reasons from low-level calls, which is essential for effective testing. The implementation follows best practices by:
- Capturing both success status and return data
- Converting return data to a string for clear error messages
- Using require to revert with the original error message
This is an improvement over simpler mocks that don't provide detailed error information.
src/interfaces/modules/IHookMultiPlexer.sol (1)
2-22: Interface update is clean and maintains compatibilityThe update to the Solidity version, local import, and formatting improves maintainability and aligns with the codebase refactor. No issues found.
script/SessionKeyValidator.s.sol (1)
2-34: Script formatting and clarity improvedThe script's formatting and comments are improved for clarity. No functional changes—deployment logic remains correct.
script/UnlockFactoryStake.s.sol (1)
8-39: Consistent formatting and import path updateThe script is now more consistent and readable. No functional changes—stake unlocking logic is unaffected.
script/WithdrawFactoryStake.s.sol (1)
8-45: Formatting and import path improvementsThe script is clearer and more consistent. No functional changes—stake withdrawal logic is preserved.
script/ERC20SessionKeyValidator.s.sol (1)
8-46: Improved script clarity and consistencyThe script's formatting and comments are improved for clarity. Deployment logic is unchanged and correct.
script/NonDeterministicDeployAllAndSetup.s.sol (6)
8-8: Import path update aligns with repository restructuring.The import path for Bootstrap has been changed to reflect the new location at "../src/utils/Bootstrap.sol", which is consistent with the PR's repository restructuring objective.
10-10: Import path correctly reflects new factory location.The ModularEtherspotWalletFactory import path has been updated to match its new location in the src/factory directory, in line with the repository structure refinement mentioned in the PR objectives.
13-22: Improved NatSpec documentation and deployment instructions.The updated comments provide clearer documentation about the script's purpose and more detailed instructions for running it, including chain-specific considerations. This enhances developer experience and script usability.
50-51: Constructor call updated to support new EIP-7702 architecture.The ModularEtherspotWallet constructor now receives the entryPoint parameter, which aligns with the PR objective of updating the wallet implementation to support EIP-7702 delegation. The logging is also improved for better readability.
57-58: Factory initialization updated for new architecture.The ModularEtherspotWalletFactory constructor now receives the implementation address and DEPLOYER address, reflecting the changes in the factory initialization requirements to support the new wallet architecture mentioned in PR objectives.
79-81: Improved stake information retrieval.The stake information retrieval has been simplified to a single line, making the code more concise while maintaining the same functionality.
script/StakeWalletFactory.s.sol (8)
8-8: Import path correctly reflects new factory location.The ModularEtherspotWalletFactory import path has been updated to match its new location in the src/factory directory, in line with the repository structure refinement mentioned in the PR objectives.
10-13: Improved NatSpec documentation.The NatSpec comments have been updated to use triple-slash format and provide clearer documentation about the script's purpose.
15-17: Constants formatted consistently.The constant declarations have been formatted consistently, making the code more readable and maintainable.
22-24: More direct factory instantiation.The factory instantiation has been simplified, making the code more concise while maintaining the same functionality.
25-29: Improved section formatting and logging.The addition of section delimiters and updated console log message make the script's output more readable and structured.
34-35: Simplified stake info retrieval.The call to entryPoint.getDepositInfo has been simplified to a single-line expression, making the code more concise.
38-41: Added section delimiter for script completion.The addition of a section delimiter for script completion improves readability and provides clear separation between script sections.
16-16:❓ Verification inconclusive
Verify hardcoded factory address across environments.
The script uses a hardcoded address for EXPECTED_WALLET_FACTORY. Ensure this approach works with your deployment strategy, especially if deployments are non-deterministic.
Consider implementing a more flexible approach, such as reading the address from an environment variable or a configuration file:
🏁 Script executed:
#!/bin/bash # Check if the hardcoded factory address is used elsewhere grep -r "0x2A40091f044e48DEB5C0FCbc442E443F3341B451" --include="*.sol" .Length of output: 476
Address Configuration Verification
I've verified that the hardcoded factory address (0x2A40091f044e48DEB5C0FCbc442E443F3341B451) is consistently used across several scripts:
- script/ModularEtherspotWalletFactory.s.sol (as
EXPECTED_FACTORY)- script/StakeWalletFactory.s.sol (as
EXPECTED_WALLET_FACTORY)- script/UnlockFactoryStake.s.sol (as
FACTORY)Please ensure that using this consistent hardcoded value aligns with your deployment strategy, especially if your deployments may be non-deterministic. If you anticipate the need for flexibility across environments, consider refactoring these scripts to fetch the factory address from an environment variable or a configuration file.
src/interfaces/ercs/IERC7484.sol (2)
16-16: Consistent function declaration formatting.The checkForAccount function declaration has been reformatted from multi-line to single-line, improving consistency across the interface.
35-35: Consistent function declaration formatting.The overloaded check function declaration has been reformatted from multi-line to single-line, improving consistency across the interface.
src/interfaces/modules/IResourceLockValidator.sol (1)
1-23: Well-structured interface with clear documentationThe interface is well-designed with comprehensive NatSpec comments and follows best practices. It properly extends the base
IValidatorinterface and defines module-specific events with a clear prefix convention (RLV_).I like how the events include indexed parameters for efficient filtering and have clear descriptive comments explaining their purpose.
script/ERC1155FallbackHandler.s.sol (3)
15-16: Code style improvement with consolidated variable declarationsThe variable declarations have been consolidated into single lines, improving code readability.
23-25: Better code organization with clear section markersAdding consistent comment block formatting for the deployment section improves code readability and organization.
31-43: Improved logging with accurate contract referencesThe log messages have been updated to correctly reference "ERC1155FallbackHandler" in the deployment logs.
Verify the expected fallback handler address
The
EXPECTED_ERC1155_FALLBACK_HANDLERis set toaddress(0), which means the deployment will always execute sinceaddress(0)never has code. If this is intentional (e.g., for testing or initial setup), this is fine, but if this script is meant for production deployments, you should set this to the expected deterministic address.script/ModularEtherspotWalletFactory.s.sol (6)
7-7: Updated import path reflects improved code organizationThe import path has been updated from
"../src/wallet/ModularEtherspotWalletFactory.sol"to"../src/factory/ModularEtherspotWalletFactory.sol", reflecting better code organization by moving the factory contract to a more appropriate directory.
9-12: Improved NatSpec comment formattingThe NatSpec comments have been updated to use the triple-slash style, which is more idiomatic for Solidity documentation.
14-17: Consolidated variable declarations improve readabilityThe variable declarations have been consolidated and clearly formatted, making the code more readable and maintainable.
23-25: Better code organization with clear section markersAdding consistent comment block formatting for the deployment section improves code readability and organization.
33-44: Improved code formatting and loggingThe contract creation and logging statements have been reformatted for better readability, and the logs now provide clearer information about the deployment status.
47-47: Explicit broadcast stop is a good practiceAdding an explicit
vm.stopBroadcast()call at the end of the script ensures proper cleanup and prevents unintended transactions, which is a good security practice.script/ModularEtherspotWallet.s.sol (8)
6-7: Added EntryPoint interface import for EIP-7702 supportThe import of
IEntryPointis essential for supporting EIP-7702, allowing the wallet to interact with the entry point contract.
9-13: Improved NatSpec comment formattingThe NatSpec comments have been updated to use the triple-slash style, which is more idiomatic for Solidity documentation.
15-18: Added EntryPoint address constant for deterministic deploymentsAdding the
ENTRY_POINT_07constant is a good practice for making the entry point address explicit and maintains the script's deterministic deployment approach.
21-22: Initialize EntryPoint interface for constructor parameterCreating an
IEntryPointinstance from the address constant is necessary for passing it to the wallet constructor, supporting EIP-7702 integration.
26-28: Better code organization with clear section markersAdding consistent comment block formatting for the deployment section improves code readability and organization.
36-36: Updated constructor call to support EIP-7702The key functional change is passing the entry point interface to the wallet constructor, which is essential for EIP-7702 support and aligns with the PR's objectives of reworking the wallet architecture.
40-46: Improved logging messagesThe log messages have been updated to provide clearer information about the deployment status.
49-50: Explicit broadcast stop is a good practiceAdding an explicit
vm.stopBroadcast()call at the end of the script ensures proper cleanup and prevents unintended transactions, which is a good security practice.src/libraries/SentinelList.sol (1)
72-72: Formatting change maintains functionality.The reformatting of this function signature from multi-line to single-line doesn't affect functionality and aligns with the consistent code style being applied throughout the codebase.
src/test/TestUniswapV3.sol (4)
2-2: Pragma version updated to align with project requirements.The update to Solidity ^0.8.27 aligns with the project's standardization efforts. Ensure all dependent tools and environments support this version.
4-4: Import path properly updated to use scoped package.Changing to
@openzeppelin/contractsfollows modern import conventions and ensures consistency with other files in the project.
22-23: Added blank line improves readability between struct definitions.This formatting change improves code readability by visually separating the two struct declarations.
38-38: Code reformatting maintains functionality while improving consistency.The reformatting of event declarations, function signatures, and statement merging are purely stylistic changes that maintain the original functionality while aligning with a consistent code style across the codebase.
Also applies to: 40-43, 48-52, 58-58
src/interfaces/factory/IFactoryStaker.sol (1)
1-30: Well-designed interface with clear responsibilities and documentationThis interface establishes a clean API for managing staking operations with ERC-4337 EntryPoint contracts. The NatSpec documentation is thorough, and the function signatures are well-designed with appropriate parameter naming.
I particularly appreciate the security-related documentation notes using
@custom:securitytags that explain important considerations for each function.src/libraries/Initializable.sol (1)
1-35:❓ Verification inconclusive
Good implementation with a potential cross-chain compatibility concern
The library is well-designed and uses transient storage via
tloadandtstoreopcodes (EIP-1153) to manage initialization state.
🌐 Web query:
When were tload and tstore opcodes (EIP-1153) introduced and which networks support them?💡 Result:
The TLOAD and TSTORE opcodes, introduced by EIP-1153, became available as part of the Ethereum network's Cancun hard fork. This upgrade implements "transient storage," a more gas-efficient mechanism for temporary data storage during transactions, where the stored data is erased at the end of each transaction[1][4][7].
Supported Networks
As of now, the Ethereum mainnet supports these opcodes following the Cancun upgrade. There is no evidence that other blockchain networks have adopted these specific EVM modifications at this time. Additionally, Solidity 0.8.24 includes support for these opcodes, although their use is currently limited to inline assembly in the compiler[6][9][10].
Citations:
- 1: https://consensys.io/blog/ethereum-evolved-dencun-upgrade-part-4-eip-7514-and-eip-1153
- 2: https://etherworld.co/2024/01/08/eip-1153-and-transient-storage/
- 3: https://eips.ethereum.org/EIPS/eip-1153
- 4: https://sequence.xyz/blog/transient-storage
- 5: https://etherworld.co/2022/12/13/transient-storage-for-beginners/
- 6: https://soliditylang.org/blog/2024/01/26/transient-storage/
- 7: https://threesigma.xyz/blog/defi-transient-storage-data-handling
- 8: https://confucian.xlog.page/EIP-1153?locale=en
- 9: https://forum.soliditylang.org/t/tload-tstore-opcodes-implementation/2171
- 10: https://blog.bingx.com/bingx-insights/detailed-guide-to-the-ethereum-cancun-upgrade/
Implementation is solid; verify network compatibility for transient storage
- The library correctly uses the EIP-1153 transient storage opcodes (
tloadandtstore) to manage initialization state.- Note that these opcodes were introduced during the Ethereum Cancun hard fork and are currently supported on the Ethereum mainnet.
- If there's any intention to deploy on networks other than Ethereum mainnet, please verify that those networks support these opcodes to avoid potential compatibility issues.
src/interfaces/modules/IERC20SessionKeyValidator.sol (3)
2-2: LGTM: Updated Solidity versionThe upgrade to Solidity ^0.8.27 aligns with the broader architectural changes and enables using newer language features.
5-5: LGTM: Improved import organizationReorganizing imports to use a relative path for IValidator and removing the unused IERC7579Account import improves code quality.
60-60: LGTM: Reformatted function signaturesFunction signatures have been reformatted from multi-line to single-line style, which is more consistent with the codebase styling.
Also applies to: 76-77, 81-81, 86-86, 92-94, 114-117
foundry.toml (4)
4-4: Good configuration updates for modern EVM featuresUpdating to EVM version "cancun" enables newer EVM features like transient storage used in the Initializable library. The reduction in optimizer runs from 1000 to 256 is reasonable and may slightly improve compile times while maintaining good optimization.
Also applies to: 7-11
12-20: Clear and explicit dependency management with remappingsThe explicit remappings improve reproducibility and make dependency paths clear. Disabling auto-detection of remappings helps prevent unexpected behavior.
Also applies to: 21-21
27-34: Good Soldeer configuration for dependency managementThe Soldeer configuration settings provide clear control over dependency management behavior, disabling automatic generation and regeneration of remappings for more predictable builds.
35-41:❓ Verification inconclusive
Well-defined dependencies with specific versions
Using specific versions and git references for dependencies ensures reproducible builds and prevents unexpected breaking changes. The move from Git submodules to Soldeer for dependency management is a modern approach that aligns with Foundry best practices.
🏁 Script executed:
#!/bin/bash # Verify if any dependencies have newer versions available # Check for newer version of solady curl -s https://api.github.com/repos/Vectorized/solady/releases/latest | jq -r '.tag_name' # Check for newer version of OpenZeppelin contracts curl -s https://api.github.com/repos/OpenZeppelin/openzeppelin-contracts/releases/latest | jq -r '.tag_name' # Check for newer version of forge-std curl -s https://api.github.com/repos/foundry-rs/forge-std/releases/latest | jq -r '.tag_name'Length of output: 315
Dependency Version Pinning Verification
The explicit versioning in your
foundry.tomlclearly ensures reproducible builds and aligns with Foundry best practices. However, our verification indicates that some dependencies might be behind their latest releases:
- solady: Currently pinned to
0.1.8while the latest release isv0.1.14- @openzeppelin-contracts: Currently set to
5.2.0while the latest release isv5.3.0- forge-std: Up-to-date at
1.9.6Please verify if retaining the older versions for solady and OpenZeppelin contracts is intentional (e.g., for compatibility or stability reasons). Otherwise, consider upgrading them if the newer releases offer important fixes or improvements. Also, ensure that any update is accompanied by thorough testing to maintain build reproducibility.
src/core/AccountStorage.sol (3)
19-23: Excellent use of ERC-7201 namespaced storage pattern.The implementation correctly uses inline assembly to access a specific storage slot defined by the
ETHERSPOT_STORAGE_SLOTconstant, which follows the ERC-7201 pattern for namespaced storage. This approach effectively prevents storage collisions between different components.
28-30: Clean storage initialization check implementation.The
isStorageInitialized()function provides a straightforward way to check initialization status and prevent re-initialization of the account, which is a good security practice.
34-36: Well-defined storage initialization function.The
_markStorageInitialized()function serves a clear purpose and follows the pattern established in the rest of the contract. Make sure this function is called during the account initialization process to properly set up the storage state.src/core/RegistryAdapter.sol (1)
18-24: Secure and flexible registry check implementation.The
withRegistrymodifier correctly handles the case where no registry is set by checking for a zero address, allowing the contract to work with or without a registry. This provides flexibility in deployment configurations.src/interfaces/base/IModule.sol (1)
4-23: Well-designed modular interface with clear lifecycle hooks.The
IModuleinterface provides a clean and comprehensive foundation for the modular smart account architecture. It includes:
- Essential lifecycle hooks (
onInstall,onUninstall)- Type checking functionality (
isModuleType)- Initialization state verification (
isInitialized)- Appropriate custom errors for initialization state issues
This design allows for flexible module extensions while maintaining a consistent interface across different module types.
src/core/BaseAccount.sol (1)
30-37: Restricting caller to EntryPoint is appropriate.The
onlyEntryPointmodifier correctly enforces that calls can only come from the configured EntryPoint, throwing a custom error otherwise. This aligns with ERC-4337’s design.src/libraries/ModeLib.sol (2)
63-70: Verify left-shift logic for correct field extraction.The inline assembly uses
shl(...)to isolate fields from a 32-byte ModeCode. Typically, extracting a byte from the lower end of the word would use right shifts (e.g.,shr(...)) or bit masks. Confirm that the distribution of bits inModeCodematches your intended memory layout, as using a left shift might discard these bits instead of extracting them.
73-80: Encoding approach is consistent with the design.The
encodefunction usesabi.encodePackedand then wraps the result inModeCode. This is a concise way to build the final 32-byte representation. Ensure all components (CallType, ExecType, ModeSelector, ModePayload) are laid out in the order you expect.src/factory/FactoryStaker.sol (1)
33-35: Zero EntryPoint address guarded correctly.The check for
_epAddress == address(0)ensures you do not interact with an uninitialized EntryPoint. This is a proper safeguard to prevent staking to a null address.src/interfaces/base/IERC7579Account.sol (2)
4-4: Import refactoring looks good.The import statement has been simplified to only include
ModeCodefrom the local types file, which aligns with the broader refactoring to separate ERC-7579 account functionality from ERC-4337 user operation handling.
20-20: Clean function declaration formatting.The function declarations have been reformatted to a more compact single-line style without changing their signatures or semantics. This improves code readability and consistency throughout the interface.
Also applies to: 32-35, 45-45, 55-55, 65-65, 71-71, 90-93, 101-101
src/interfaces/factory/IModularEtherspotWalletFactory.sol (1)
1-37: Well-structured interface with comprehensive documentation.This new interface is well-designed with thorough NatSpec comments and clearly defined responsibilities. It extends
IFactoryStakerfor staking functionality and provides the core methods needed for deterministic wallet creation using CREATE2.The event
ModularAccountDeployedappropriately captures the essential information when a new account is deployed. The function signatures have clear parameter names and return values, making the interface easy to understand and implement.src/interfaces/modules/ISessionKeyValidator.sol (4)
2-2: Solidity version update is appropriate.Upgrading to pragma ^0.8.27 is sensible given the project's modernization efforts and ensures compatibility with newer Solidity features.
5-6: Import paths refactored correctly.The import statements have been properly updated to reflect the new modular design approach, importing
IValidatorfrom the base interface path and struct definitions from the types folder.
8-11: Enhanced documentation improves interface clarity.The added NatSpec documentation for the interface provides valuable context about its purpose and relationship to the validator interface, enhancing maintainability and developer experience.
28-28: Function and event declarations reformatted consistently.The reformatting of function and event declarations to single-line signatures improves code consistency and readability without changing functionality.
Also applies to: 33-33, 39-39, 46-46, 52-52, 60-61, 76-76, 103-103, 141-141, 147-147, 154-154, 163-163, 168-168, 174-174, 180-180, 204-204, 224-227
src/interfaces/core/IAccountStorage.sol (4)
1-8: Appropriate imports for storage components.The imports correctly include all necessary dependencies for the storage interface, including the sentinel list library and relevant interface definitions. This sets up a solid foundation for the modular storage system.
9-13: Comprehensive NatSpec documentation.The interface documentation clearly explains the purpose and pattern (ERC-7201 namespaced storage) being used, which is excellent for maintainability and developer understanding.
18-26: Well-structured FallbackHandler definition.The FallbackHandler struct is well-designed with clear field names and documentation. It properly encapsulates all the necessary information for fallback handling including the handler address, call type, and allowed callers array.
28-45: Comprehensive AccountStorage design.The AccountStorage struct effectively organizes all components needed for a modular account, using appropriate data structures like SentinelList for validators and executors. The storage layout is well-thought-out with clear separation of concerns between different module types.
The use of mapping with named values (
mapping(bytes4 selector => FallbackHandler fallbackHandler)) is a good choice for readability and is supported by the specified Solidity version.src/interfaces/base/IPreValidationHook.sol (1)
7-12: Well-defined interface for ERC1271 pre-validation hooksThis interface correctly defines a view function for ERC1271 pre-validation that takes a sender address, a hash to verify, and arbitrary calldata. The return values are appropriate for signature validation purposes.
src/interfaces/base/IERC4337Account.sol (2)
7-50: Thorough implementation of ERC4337 validateUserOp with detailed documentationThe function signature and documentation correctly implement the ERC4337 standard for account abstraction. The comprehensive documentation clearly explains the validation process, including how to handle signature failures, nonce validation, and the structure of the returned validation data.
52-63: Correct implementation of executeUserOp with clear documentationThe function signature is properly defined as external and payable. The documentation clearly explains that this function executes the operation after successful validation and how it should process the call data.
src/interfaces/core/IExecutionHelper.sol (2)
15-35: Comprehensive event definitions for execution trackingThe events are well-designed to cover all execution scenarios (success, failure, batch failure, delegate call failure) with appropriate parameters for logging important information. The indexed parameters are chosen appropriately for efficient filtering.
41-71: Exhaustive error definitions for execution failure scenariosThe error definitions are comprehensive and well-documented, covering all potential failure modes for execution. Each error includes appropriate context parameters to help with debugging. The error messages follow a consistent naming pattern, which helps with error handling and identification.
src/interfaces/modules/IECDSAValidator.sol (2)
15-28: Well-structured error definitions with clear messagingThe error definitions clearly articulate various validation failure scenarios with appropriate parameters. The naming convention with the "ECDSA_" prefix ensures clear error source identification.
34-46: Appropriate events for validator lifecycle and ownership changesThe events properly track the validator's lifecycle (enable/disable) and ownership changes with relevant indexed parameters for efficient filtering and tracking.
docs/RESOURCE_LOCK_VALIDATOR.md (2)
1-86: Documentation clarity check.These sections provide a clear explanation of the validator’s features, methods, and security considerations. There are no apparent logic or consistency issues.
88-111: Overall good documentation.The remainder of the file is well-structured, with thorough coverage of integration steps, dependencies, and error conditions. No functional issues spotted.
script/DeployAllAndSetup.s.sol (1)
19-36: Good script documentation.The top-level docstrings concisely explain the script’s purpose, including references to forging commands and helpful tips.
src/modules/hooks/ModuleIsolationHook.sol (4)
5-6: Author annotation changes.No issues with updated authorship details.
10-16: Import statements.Imports are organized properly to align with the new modular structure.
39-44: Contains function logic.Straightforward linear search is acceptable here given the small array size of banned signatures.
101-102: Module type and initialization checks.These succinct helpers are correctly implemented.
Also applies to: 108-110
src/libraries/BootstrapLib.sol (2)
13-19: Good use of custom errors for clarity.Defining specific error types improves readability and clarifies why the function reverts. Ensure all call sites handle these errors properly with relevant test coverage.
54-68: Ensure consistent usage of_buildMultipleConfigs.While the loop reliably calls
_buildSingleConfig, verify that downstream code expects the encoded installation data in the same format for every module. Any discrepancy in how modules parse data could cause issues at install.src/core/ERC7779Adapter.sol (1)
54-58: Caution withonRedelegationexternal call.Enforcing
msg.sender == address(this)ensures only internal calls, but confirm no external calls can be crafted to bypass this check (e.g., via DELEGATECALL). Adding explicit commentary clarifies the expected use.src/test/TestUniswapV2.sol (1)
52-53: Ensure test coverage forunwrapWETH9.While straightforward, confirm you test for boundary scenarios (e.g.,
amountMinimumof zero, absent or failing WETH9).src/libraries/ExecutionLib.sol (3)
19-54: Verify decoding logic with thorough tests.This assembly-based approach is highly optimized but can be fragile. Rigorously test boundary cases (empty array, maximal lengths, invalid offsets) to ensure no hidden edge cases.
56-58: Encoding consistency inencodeBatch.All batch consumers should decode data using the same schema. Verify that no separate code path assumes a different format.
70-76: Confirm consistent usage ofencodeSingle.Ensure the receiving functions handle
abi.encodePacked(_target, _value, _callData)consistently. If any variable-length data is appended incorrectly, it may break subsequent decodes.src/modules/fallbacks/ERC1155FallbackHandler.sol (10)
2-2: Upgraded Solidity version requirement.The pragma directive was changed from a fixed version to a flexible version constraint (
^0.8.27), allowing compatibility with versions from 0.8.27 up to (but not including) 0.9.0. This follows good practice for library code.
4-5: Updated imports to align with new modular architecture.Replaced the external IERC7579Module import with the new modular interface structure. This change supports the architectural shift to a more modular approach described in the PR objectives.
7-10: Added comprehensive NatSpec documentation.Good addition of thorough contract documentation explaining the purpose and behavior of the fallback handler.
16-17: Added documentation for state variable.Clear explanation of the initialization tracking mapping, which improves code readability.
23-45: Enhanced event documentation with NatSpec.All events now have detailed NatSpec comments explaining their purpose and parameters, which significantly improves developer experience.
51-65: Added detailed function documentation for onERC1155Received.Comprehensive NatSpec comments explaining parameters, return values, and behavior.
67-84: Added detailed function documentation for onERC1155BatchReceived.Comprehensive NatSpec comments explaining parameters, return values, and behavior.
86-92: Added module lifecycle management: onInstall function.Proper implementation of installation logic that sets the initialization flag and emits an event.
94-100: Added module lifecycle management: onUninstall function.Proper implementation of uninstallation logic that clears the initialization flag and emits an event.
102-107: Added module type identification.The isModuleType function correctly checks against the imported module type constant.
src/interfaces/modules/IMultipleOwnerECDSAValidator.sol (6)
1-10: Well-structured interface with comprehensive documentation.The interface is properly defined with complete NatSpec comments explaining its purpose and relationship to the overall architecture. It correctly extends the base IValidator interface.
15-34: Well-defined error types with clear descriptions.The error definitions follow a consistent naming convention (MOECDSA prefix) and include descriptive parameters. Each error has a clear NatSpec comment explaining when it's thrown.
40-50: Properly defined events with indexed parameters.Events are well-structured with the same consistent naming convention. Appropriate parameters are indexed for efficient filtering in event logs.
56-60: Clear ownership check function definition.The isOwner function is well-documented with clear parameter and return value descriptions.
62-72: Owner management functions with access control documentation.Both addOwner and removeOwner functions have clear documentation indicating they can only be called by the wallet itself or existing owners, providing important access control information.
74-83: ERC-1271 signature validation function.The isValidSignatureWithSender function is properly defined with clear documentation of its purpose and parameters, properly implementing the ERC-1271 standard for signature validation.
script/CredibleAccountScript.s.sol (5)
19-30: Script initialization with proper environment variable usage.The script correctly uses vm.envUint for the private key and implements proper logging.
49-63: CredibleAccountValidator deployment with correct parameters.The validator is deployed with the deployer address and hook multiplexer. Same validation comment as above.
65-79: CredibleAccountHook deployment with validator dependency.The hook is correctly deployed with a reference to the validator. Same validation comment as above.
81-90: Validator initialization with proper verification.The script correctly initializes the validator with the deployed hook and verifies the initialization worked by checking the validator's caHook address.
92-98: Clean script completion.The script properly logs completion and stops broadcasting.
src/factory/ModularEtherspotWalletFactory.sol (5)
1-13: Well-structured factory contract with appropriate inheritance.The contract properly implements the factory interface and inherits staking functionality. The imports are organized and appropriate for the contract's functionality.
18-19: Immutable implementation address.Good practice to use an immutable variable for the implementation address, which can't be changed after deployment and saves gas.
25-30: Clean constructor with proper inheritance.The constructor correctly initializes the implementation address and passes the owner to the FactoryStaker base contract.
36-59: Robust account creation with existing account handling.The createAccount function properly:
- Checks if an account already exists at the predicted address
- Uses CREATE2 for deterministic deployment
- Passes the implementation and initialization data to the proxy
- Emits an event with the deployed account and owner
- Handles payment forwarding with the
value: msg.valuesyntaxThis implementation aligns with the EIP-7702 support objectives mentioned in the PR.
61-81: Correct CREATE2 address computation.The getAddress function correctly implements the CREATE2 address computation formula, properly encoding the proxy creation code and initialization data.
src/interfaces/core/IModuleManager.sol (4)
1-3: Use of MIT license and Solidity ^0.8.27
These lines cleanly declare an MIT license and a suitably recent Solidity compiler version.
4-10: Interface structure and doc commentary
The interface is clearly named and includes concise docstrings, which improves readability for downstream contracts.
11-59: Extensive custom errors
Having well-defined custom errors with relevant parameters (e.g.,InvalidModule(address module)) makes debugging more transparent and saves gas compared to revert strings.
60-85: Event definitions
Emitting events for fallback calls, hook uninstallation failures, and Ether receipts is beneficial for on-chain observability and debugging.src/modules/hooks/HookMultiPlexer.sol (5)
2-2: Solidity version statement
Using solidity ^0.8.27 ensures up-to-date language features and overflow/underflow checks.
28-35: Event definitions
Emitting events for hook addition, removal, and module initialization/uninitialization offers solid transparency.
304-337: preCheck hooking complexity
Collecting hooks, sorting, and removing duplicates before calling each subHook is logical. However, repeated array operations can be gas-heavy if used in frequently called paths. Consider verifying performance suits typical use cases and possibly caching partial results.
346-364: postCheck function
Decoding and iterating over the hooks for post-execution checks is well-conceived. Be mindful of potential memory usage if the number of hooks grows significantly, though this approach is typically acceptable for modular systems.
373-386: isModuleType and metadata
Providing the module type, name, and version is concise and helps identify modules easily in a heterogeneous system.src/modules/validators/ResourceLockValidator.sol (12)
1-3: License and recent compiler version
Declaring MIT license and using Solidity ^0.8.27 are consistent with modern best practices.
4-18: Import statements
Pulling inECDSAandMerkleProofLibfrom recognized libraries enhances reliability and avoids reinventing cryptographic primitives.
22-26: Contract documentation
The contract purpose and Merkle-proof-based resource lock approach are well documented.
35-52: Custom errors
Clear custom errors such asRLV_AlreadyInstalledandRLV_InvalidOwnerhelp identify misconfiguration at runtime.
62-96: onInstall ownership extraction
The owner extraction logic covers various data layouts. Ensure that the fallback extraction logic cannot be tricked by malformed data.
98-107: onUninstall data cleanup
Deleting validator storage is straightforward, ensuring no residual ownership remains.
109-149: validateUserOp with Merkle proof
The fallback from standard ECDSA verification to a hashed root check for resource-locked signatures is carefully implemented. Ensure that external code using this method enumerates the correct signature format (65 bytes for normal sig, more for proof).
151-188: ERC-1271 signature checks
Supporting both normal signatures and Merkle root signatures in the ERC-1271 flow parallels the userOp approach, preserving consistency.
190-195: Module type check
Clean mapping ofMODULE_TYPE_VALIDATORensures external frameworks can identify this Validator module.
197-202: isInitialized
Concise method verifying if a validator is installed. No immediate concerns.
208-264: Resource lock parsing
The_getResourceLockfunction decodes the call data meticulously. Be mindful that full rely onIERC7579Account.execute.selectoris correct for all usage contexts, and that partial inheritance isn’t interfering.
266-291: Resource lock hashing
The keccak-based approach to combine chainId, addresses, timestamps, and token data array is consistent. No issues with collisions are apparent.src/modules/validators/MultipleOwnerECDSAValidator.sol (11)
1-14: Well-structured imports and constants usage.
All external dependencies and constants are clearly referenced, keeping the code modular and focused. Good job ensuring references toECDSA,PackedUserOperation, and your constants remain consistent.
16-20: Concise documentation.
The docstrings comprehensively outline the purpose and scope of this validator, facilitating quick understanding.
41-42: Global mapping is straightforward.
This mapping to track validator configurations per smart account is clean and aligns with modular architecture principles.
48-68: Initialization logic looks robust.
- Correctly checks for re-initialization.
- Decodes owners array and prevents empty or zero-address owners.
- Properly emits events upon successful addition.
70-79: Uninstallation logic is safe and consistent.
- Uses
_isInitializedto verify states.- Clears storage and emits a disable event.
This maintains a clean lifecycle.
81-86:isInitializedmethod straightforwardly checks theenabledflag.
Serves well for confirming readiness of the module.
88-93: Module type identification and ownership query are clean.
isModuleTypeensures consistency with the new module-based architecture.isOwnerleverages the_isOwnerinternal check properly.Also applies to: 95-101
103-115:addOwnerfunction with correct authorization and validation.
- Verifies caller is either the SCW or an existing owner.
- Disallows duplicates or zero addresses.
- Emits the relevant event.
All criteria are covered.
143-163: Signature validation approach is consistent with standard ECDSA flows.
- Attempts recovery from both raw and
toEthSignedMessageHash.- Returns success/failure codes in line with the constants.
165-187: ERC-1271 validation covers both raw and EIP-191 signatures.
This dual path ensures broader compatibility for verifying off-chain signatures.
193-205: Internal checks_isInitializedand_isOwnerare compact and clear.
No redundant logic. Straight to the point.src/core/ModuleManager.sol (13)
1-3: License declaration and pragma.
Well formatted and in line with standard conventions.
4-38: Comprehensive imports centralize external dependencies.
Using specialized libraries likeExcessivelySafeCallhighlights attention to safe external calls.
43-48: Modifiers for restricted access and hooking are well-organized.
onlyExecutorModuleensures untrusted calls don’t slip through.withHookpattern properly wraps pre/post checks for transactions.Also applies to: 50-62
68-79: Initialization logic diligently sets up validators and executors.
- Uses sentinel lists for flexible module management.
- Appropriately checks for EIP-7702–deployed accounts and extends storage.
85-103: Robust_isEIP7702Accountcheck.
- Verifies code size is 23 bytes and checks prefix.
- Effectively gates EIP-7702–specific storage logic.
108-117: Validator install/uninstall patterns with safe external calls.
- Linked list usage keeps modules flexible.
_tryUninstallValidatorssystematically cleans them in a loop, giving each a chance to runonUninstall.- Potential gas consideration if many validators exist, but the pattern remains typical.
Also applies to: 119-137, 139-153
155-167: Executor management mirrors validator logic.
- Maintains consistent patterns with
_installExecutor,_uninstallExecutor, and_tryUninstallExecutors.- Good synergy, ensuring a uniform approach.
Also applies to: 169-181, 183-189
269-280: Hook installation/uninstallation with graceful fallback.
- Single hook enforced; safe calls to
onUninstallwrapped in a try/catch for partial failures._tryUninstallHookproperly resets state even if the external call fails.Also applies to: 281-294, 296-309
340-351: Pre-validation hook architecture.
- Distinguishes ERC-1271/4337 hooks.
- Defensively uninstalls hooks in
_tryUninstallPreValidationHooks, capturing errors while ensuring state is cleared._setPreValidationHookapproach is straightforward.Also applies to: 353-368, 370-377, 379-400, 402-413
415-432: Pre-validation hooks_withPreValidationHook
- Returns potentially updated hashes/signatures via modular hooks.
- This is a flexible design to incorporate multiple signature schemes.
Also applies to: 434-451
457-466: Fallback and receive functions integrate well with hooking.
receive()emits an event to track raw ETH transfers.fallback()is wrapped with thewithHookmodifier, ensuring the pre-/post-check pattern remains consistent.
467-497: Fallback handler installation/uninstallation.
- Extracts function selector, call type, and allowed callers.
- Forbids
DELEGATECALLto mitigate potential security concerns.- Uses
excessivelySafeCallforonUninstall.Also applies to: 499-518
590-603: Fallback handler checks and retrieval.
_isFallbackHandlerInstalledandgetFallbackHandlercomplement the fallback management.- Code is consistent and straightforward for module retrieval.
Also applies to: 605-617
src/modules/validators/ECDSAValidator.sol (10)
1-4: License, pragma, and doc comments.
Clear licensing and versioning. The docstrings succinctly explain single-owner ECDSA validation.
5-14: Imports and constants use.
Organized references toECDSA,PackedUserOperation, and your shared constants.
24-30: Straightforward storage for single-owner configuration.
Maintaining a singleValidatorStorageper account is consistent with a minimal approach for single-owner.
36-69: Installation logic covers both minimal and bootstrap data formats.
- Handles varying data lengths gracefully.
- Ensures zero address is invalid.
- Emits event for clarity.
71-80: Uninstall method cleans the mapping.
Removes the stored owner address, preserving a clean lifecycle pattern.
82-87: Initialization check, module type check, and ownership retrieval.
- All methods remain consistent with the updated module-based architecture.
isOwnerleverages_isOwnerinternally, no duplication.Also applies to: 89-95, 97-103
105-117:changeOwnermethod is properly gated and logs changes.
- Allows the current SCW or the existing owner to update ownership.
- Prevents zero addresses.
119-139:validateUserOphandles multi-path ECDSA signature checks.
- Checks raw signature first, then EIP-191 style.
- Returns standardized success/failure codes.
141-162: ERC-1271 method reuses the same multi-step check.
- Minimizes code duplication while ensuring broad signature coverage.
168-174: Private_isOwnerfunction is concise.
Ensures the validator is active simply by storing a non-zero owner.src/modules/hooks/CredibleAccountHook.sol (4)
16-16: Immutable reference to validator is good for efficiency.
StoringCredibleAccountValidatoras an immutable reference is a solid choice to save gas, as its address cannot change post-deployment.
36-39: Well-defined error messages.
Defining custom errors for more granular revert reasons is a best practice in recent Solidity versions. This is a good approach for providing clarity.
82-90: ConfirmmsgSenderusage inpreCheck.
You wrap the originalmsg.senderwithmsgDatain the call. Ensure no mismatch occurs if external calls or forwarders altermsg.sender.
112-114: Efficient internal helper functions.
_isInitializedand_walletTokenBalancemaintain a clear separation of concerns. It's good practice to keep these as small, cohesive helpers.Also applies to: 116-118
src/core/ExecutionHelper.sol (4)
16-21: Clear contract documentation.
The docstring concisely states this contract’s purpose to provide “enhanced execution management.” This clarity is helpful for maintainers.
52-93: Batch call logic looks solid.
The separate flows for_execType(default vs try) are logically structured and give good coverage for partial failures. Great job.
95-134: Single call execution is well-structured.
The approach for capturing or ignoring return data is consistent with the batch version and helps keep the code DRY.
348-359: Clean handling of_executevs_tryExecute.
Your approach to handle success or revert in_execute, and partial tolerance in_tryExecute, is consistent and keeps the call flow easy to follow.Also applies to: 361-376
src/libraries/HookMultiPlexerLib.sol (5)
5-6: Appropriate usage of imports for modular design.
ImportingIHookand other base types keeps the library’s scope tight, encouraging code reuse.Also applies to: 9-12
41-44:preCheckSubHooksloop design.
The code iterates oversubHooks, callingpreCheckSubHookfor each. This is straightforward; ensure you handle possible hook reverts gracefully, which you already do withSubHookPreCheckError.Also applies to: 53-54
68-71: Originalmsg.senderwrapping inpreCheckSubHook.
You encode the originalmsg.senderinside the call. Ensure that downstream hooks honor or parse these fields correctly, especially if proxies or forwarders are involved.Also applies to: 77-80
357-361: Selective forward compatibility.
isExecutioncoversexecute.selectorandexecuteFromExecutor.selector. If new execution methods appear, ensure you update or handle those scenarios accordingly.
363-400:appendExecutionHookorganizes sub-hooks effectively.
The segmented approach for single, batch, and delegate call flows is well structured. Keeping the value hooks separated from target sig hooks is a clean design.src/modules/validators/GuardianRecoveryValidator.sol (4)
81-129: Ensure external calls trust assumptions remain consistent.The
onInstallfunction decodes bootstrap data and initializes the module. While it looks robust, consider the implications if external calls attempt to install this validator multiple times for the same smart account. The guardif (validatorStorage[msg.sender].enabled) {...}ensures only a single initialization, which is correct. Just verify that no re-entrancy or race conditions can occur if multiple installs are attempted in separate transactions.
285-314: One active proposal at a time.The logic enforces a single proposal by discarding the current one if unresolved. This can simplify state tracking but may block concurrent proposals. If concurrency is desired in future expansions, storing pending proposals in a more flexible structure may be needed.
316-361: Validate edge cases with guardian cosign.
guardianCosignincrements the approval count and checks for a quorum. If multiple guardians sign within the same block, ensure there's no unexpected concurrency scenario (though it is generally safe, as calls are sequentially processed in the EVM).
641-662: Timelock enforcement can be extended.Within
_discardCurrentProposal, a timelock is respected only for guardians. If an owner or the wallet itself discards a proposal immediately, that might bypass guardian timelocks. Verify that this aligns with intended design requirements and does not pose a security gap.src/modules/validators/ERC20SessionKeyValidator.sol (3)
153-164: Pause/unpause logic is correct.The
toggleSessionKeyPausefunction cleanly flips the session’slivestatus. This straightforward approach appears correct, but confirm external integrators handle paused session key states gracefully.
248-254: Explicit block-time checks on validation.In
validateUserOp, the session’svalidAfterandvalidUntilvalues are only encoded into the validation data. If direct on-chain re-checking of block timestamps is required, integrate it here; otherwise, ensure external bundlers faithfully respect the result.
283-294: Robust cleanup in onUninstall.The loop that deletes all session keys for the smart account is straightforward and effective. Keep an eye on potential gas costs if a large number of session keys accumulate.
src/modules/validators/CredibleAccountValidator.sol (1)
119-130: Validate module prerequisites inonInstall.
The checks inonInstallensure the correct hook is active and that the hook multiplexer has the right settings. This is good, but you may also want to verify thatcaHookhas been initialized before installing. If a user callsonInstallwithout having setcaHook, it might cause confusion or unexpected behaviors if used later.src/modules/validators/SessionKeyValidator.sol (2)
158-164: Good modular approach inrotateSessionKey.
Rotating old keys while enabling new ones is a clean design. The reuse ofdisableSessionKeyandenableSessionKeyhelps reduce code duplication.
250-252: Confirm correct enforcement of param conditions in_checkCondition.
Condition checks ensure calls meet specific criteria. Verify that strict type matching (bytes32-level comparison) is acceptable for your use case. If dealing with smaller integral types or structured data, consider decoding and comparing typed values to avoid accidental mismatches.Also applies to: 531-536
| $registry = registry; | ||
| if (attesters.length > 0) { | ||
| registry.trustAttesters(threshold, attesters); | ||
| } |
There was a problem hiding this comment.
Event emission missing in registry configuration.
The setRegistry function defines an event (ERC7484RegistryConfigured), but doesn't emit it when the registry is set. Add the event emission for proper logging and off-chain tracking.
function setRegistry(IERC7484 registry, address[] calldata attesters, uint8 threshold)
external
onlyEntryPointOrSelf
{
$registry = registry;
+ emit ERC7484RegistryConfigured(address(this), address(registry));
if (attesters.length > 0) {
registry.trustAttesters(threshold, attesters);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $registry = registry; | |
| if (attesters.length > 0) { | |
| registry.trustAttesters(threshold, attesters); | |
| } | |
| function setRegistry(IERC7484 registry, address[] calldata attesters, uint8 threshold) | |
| external | |
| onlyEntryPointOrSelf | |
| { | |
| $registry = registry; | |
| emit ERC7484RegistryConfigured(address(this), address(registry)); | |
| if (attesters.length > 0) { | |
| registry.trustAttesters(threshold, attesters); | |
| } | |
| } |
| interface IPreValidationHookERC4337 is IModule { | ||
| function preValidationHookERC4337( | ||
| PackedUserOperation calldata userOp, | ||
| uint256 missingAccountFunds, | ||
| bytes32 userOpHash | ||
| ) external returns (bytes32 hookHash, bytes memory hookSignature); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Well-defined interface for ERC4337 pre-validation hooks with one consideration
The interface is well-designed, but unlike its ERC1271 counterpart, this function is not marked as view despite having similar behavior. Consider whether this function should modify state - if not, marking it as view would be more appropriate and gas-efficient.
Consider reviewing the ERC4337 standard to confirm whether this function should be able to modify state:
🌐 Web query:
Does ERC4337 preValidation hook modify state or should it be a view function?
💡 Result:
The preValidation hook in ERC-4337 does not inherently modify state and therefore could be designed as a view function. However, its actual implementation depends on the specific requirements and use cases, which may vary.
Key Points:
-
Purpose of PreValidation Hooks: These hooks are executed before the main validation functions and are typically used for checks such as gas metering, session key limits, and permission validations. They do not perform operations like signature verification directly but set the stage for further validations[4][9].
-
View Function Suitability: Typically, operations that purely read data and perform checks without altering blockchain state are implemented as view functions. PreValidation hooks align with this concept because their core purpose is to ensure that user operations meet predefined criteria before proceeding[4][9].
-
Exceptions for State Modifications: If a specific implementation of a preValidation hook requires updating internal state (e.g., for bookkeeping or preparing context for later phases), it would not qualify as a view function. However, such modifications seem outside the usual purpose of preValidation hooks as defined in the ERC-4337 lifecycle[4][9].
Conclusion:
By default, ERC-4337 preValidation hooks should be implemented as view functions since their role is primarily to validate and ensure compliance without modifying state. Implementations that deviate from this pattern would be exceptions based on custom design choices.
Citations:
- 1: https://medium.portto.com/erc-6900-modular-account-abstraction-what-it-means-for-smart-contract-wallets-cd0b1fd2a885
- 2: https://blog.particle.network/the-final-piece-of-erc-4337-omnichain-account-abstraction/
- 3: https://docs.stackup.sh/docs/erc-4337-overview
- 4: https://github.com/alchemyplatform/modular-account
- 5: https://evacodes.com/blog/erc4337-account-abstraction
- 6: https://www.alchemy.com/blog/erc-4337-gas-estimation
- 7: https://www.youtube.com/watch?v=v_V4Wqcn7vE
- 8: https://www.alchemy.com/blog/account-abstraction-paymasters
- 9: https://github.com/ethereum/ercs/blob/master/ERCS/erc-4337.md
- 10: https://www.blocknative.com/blog/account-abstraction-erc-4337-guide
Action Required: Consider Marking the Function as view
The ERC4337 pre-validation hook is intended for non-modifying checks, and guidelines suggest it should be implemented as a view function for gas efficiency—unless your design specifically requires state changes. Please review the implementation need for state modifications. If no state changes are necessary, update the interface accordingly:
- File:
src/interfaces/base/IPreValidationHook.sol(Lines 14-20) - Suggested Change:
interface IPreValidationHookERC4337 is IModule { function preValidationHookERC4337( PackedUserOperation calldata userOp, uint256 missingAccountFunds, bytes32 userOpHash ) external view returns (bytes32 hookHash, bytes memory hookSignature); }
| console2.log("Staking factory contract with EntryPoint..."); | ||
| factory.addStake{value: FACTORY_STAKE}(address(entryPoint), 86400); | ||
| IStakeManager.DepositInfo memory info = entryPoint.getDepositInfo(EXPECTED_FACTORY); | ||
| console2.log("Staked amount:", info.stake); | ||
| console2.log("Factory staked!"); | ||
|
|
There was a problem hiding this comment.
Potential deposit info mismatch.
After calling factory.addStake{value: FACTORY_STAKE}(address(entryPoint), 86400), the code retrieves deposit info for EXPECTED_FACTORY instead of the actual factory address. This might yield incorrect staking info logs.
- IStakeManager.DepositInfo memory info = entryPoint.getDepositInfo(EXPECTED_FACTORY);
+ IStakeManager.DepositInfo memory info = entryPoint.getDepositInfo(address(factory));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console2.log("Staking factory contract with EntryPoint..."); | |
| factory.addStake{value: FACTORY_STAKE}(address(entryPoint), 86400); | |
| IStakeManager.DepositInfo memory info = entryPoint.getDepositInfo(EXPECTED_FACTORY); | |
| console2.log("Staked amount:", info.stake); | |
| console2.log("Factory staked!"); | |
| console2.log("Staking factory contract with EntryPoint..."); | |
| factory.addStake{value: FACTORY_STAKE}(address(entryPoint), 86400); | |
| IStakeManager.DepositInfo memory info = entryPoint.getDepositInfo(address(factory)); | |
| console2.log("Staked amount:", info.stake); | |
| console2.log("Factory staked!"); |
| * msg.sender should be the owner of the account. | ||
| */ | ||
| function onRedelegation() external returns (bool) { | ||
| require(msg.sender == address(this), NonAuthorizedOnRedelegationCaller()); |
There was a problem hiding this comment.
Inconsistent usage of custom error in require.
When using a custom error like NonAuthorizedOnRedelegationCaller(), you should revert directly rather than passing it to require(). For instance:
- require(msg.sender == address(this), NonAuthorizedOnRedelegationCaller());
+ if (msg.sender != address(this)) revert NonAuthorizedOnRedelegationCaller();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require(msg.sender == address(this), NonAuthorizedOnRedelegationCaller()); | |
| if (msg.sender != address(this)) revert NonAuthorizedOnRedelegationCaller(); |
| function decodeSingle(bytes calldata _executionCalldata) | ||
| internal | ||
| pure | ||
| returns (address target, uint256 value, bytes calldata callData) | ||
| { | ||
| target = address(bytes20(_executionCalldata[0:20])); | ||
| value = uint256(bytes32(_executionCalldata[20:52])); | ||
| callData = _executionCalldata[52:]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check single decode out-of-bounds.
Slicing _executionCalldata[52:] for callData presumes at least 52 bytes in _executionCalldata. Revert or annotate if _executionCalldata could be shorter.
| console2.log("Starting deployment sequence..."); | ||
|
|
||
| /*////////////////////////////////////////////////////////////// | ||
| Deploy HookMultiPlexer | ||
| //////////////////////////////////////////////////////////////*/ | ||
|
|
||
| console2.log("Deploying HookMultiPlexer..."); | ||
| // if (EXPECTED_MULTIPLEXER_ADDRESS.code.length == 0) { | ||
| hookMultiPlexer = new HookMultiPlexer{salt: SALT}(); | ||
| // if (address(hookMultiPlexer) != EXPECTED_MULTIPLEXER_ADDRESS) { | ||
| // revert("Unexpected HookMultiPlexer address!!!"); | ||
| // } else { | ||
| console2.log("HookMultiPlexer deployed at address", address(hookMultiPlexer)); | ||
| // } | ||
| // } else { | ||
| // console2.log("HookMultiPlexer already deployed at address", EXPECTED_MULTIPLEXER_ADDRESS); | ||
| // } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
HookMultiPlexer deployment with commented-out validation.
The deployment logic works, but the address validation is commented out. This might be intentional for development but should be addressed before production use.
If this script is intended for production deployment, consider uncommenting and implementing the address validation logic, or explicitly documenting why the validation is disabled.
🏁 Script executed:
#!/bin/bash
# Check if this script is used in production environments by looking for references
grep -r "CredibleAccountSetupScript" --include="*.sol" .Length of output: 141
Action Required: Address Commented-out Address Validation in Production Deployment Script
The grep results confirm that the CredibleAccountScript.s.sol file is used in production (it references CredibleAccountSetupScript), so leaving the address validation commented out poses a risk. Please either:
- Uncomment and fully implement the address validation logic (lines 31-47) to check that the deployed
HookMultiPlexeraddress matches the expected value. - OR add clear documentation explaining why the validation is intentionally disabled—with assurances that any necessary production checks are performed elsewhere.
Affected Location:
- File:
script/CredibleAccountScript.s.sol, Lines: 31-47
| revert CredibleAccountHook_InvalidData(); | ||
| } | ||
| address scw = address(bytes20(data[12:32])); | ||
| if (!_isInitialized(scw)) revert NotInitialized(scw); |
There was a problem hiding this comment.
Missing definition for NotInitialized(scw).
Similar to AlreadyInitialized, this error is referenced but not declared here. Verify it's defined elsewhere or properly imported.
| revert CredibleAccountHook_InvalidData(); | ||
| } | ||
| address scw = address(bytes20(data[12:32])); | ||
| if (_isInitialized(scw)) revert AlreadyInitialized(scw); |
There was a problem hiding this comment.
Missing definition for AlreadyInitialized(scw).
This error is used but never declared in this contract or the imported code visible here. Confirm that it is declared elsewhere or import it if needed.
| function join(address[] memory hooks, SignatureHooks storage $sigHooks) internal view { | ||
| uint256 sigsLength = $sigHooks.allSigs.length; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Chaining hooks with join.
Extending arrays in memory is an elegant approach but keep an eye on potential out-of-gas scenarios if _targetHooks arrays become large.
| function _validateTokenData(address _sessionKey, address _wallet, uint256 _amount, address _token) | ||
| internal | ||
| returns (bool) | ||
| { | ||
| CredibleAccountStorage storage store = _credibleAccountStorage[_wallet]; | ||
| if (!store.sessionTokens[_sessionKey].contains(_token)) return false; | ||
| uint256 lockedAmount = store.sessionLockedAmounts[_sessionKey][_token]; | ||
| uint256 claimedAmount = store.sessionClaimedAmounts[_sessionKey][_token]; | ||
| if (_walletTokenBalance(_wallet, _token) >= _amount && _amount == lockedAmount && claimedAmount < lockedAmount) | ||
| { | ||
| uint256 newClaim = _amount - claimedAmount; | ||
| store.sessionClaimedAmounts[_sessionKey][_token] += newClaim; | ||
| store.claimedAmounts[_token] += newClaim; | ||
| emit CredibleAccountValidator_TokenClaimed(_sessionKey, _wallet, _token, newClaim); | ||
| _updateTokenStatus(_wallet, _token); | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Revisit strict matching of lockedAmount in _validateTokenData.
The code requires amount == lockedAmount, which mandates that the user must spend the entire locked amount in one transaction. If partial usage is a requirement, consider adjusting this check to subtract partial amounts and keep track of the remainder. Otherwise, document that transactions must consume the entire locked amount.
- if (_walletTokenBalance(_wallet, _token) >= _amount && _amount == lockedAmount && claimedAmount < lockedAmount) {
+ // Example partial usage approach:
+ if (_walletTokenBalance(_wallet, _token) >= _amount && _amount <= (lockedAmount - claimedAmount)) {Committable suggestion skipped: line range outside the PR's diff.
- Added gas snapshot
- ResourceLockValidator now handles nonce management for each smart account for valid Merkle proof appended signatures (signatures relating to ResourceLocks). It will not update for standard ECDSA validation to reduce gas costs for standard validation. - Fixed test utils to implement new changes. - Added test case for valid updating of nonce.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/modules/validators/ResourceLockValidator.sol (5)
44-45: Consider using ERC-7201 namespaced storage for the validator storage mappingThe storage pattern could benefit from using ERC-7201 namespaced storage patterns to prevent potential storage collisions, especially considering this is part of a modular system with multiple validators.
- /// @notice Maps smart account addresses to their validator configuration - mapping(address => RLVValidatorStorage) public validatorStorage; + /// @dev Storage namespace for the Resource Lock Validator + bytes32 constant RESOURCE_LOCK_STORAGE_POSITION = keccak256("etherspot.modules.validator.resourcelock.storage"); + + /// @notice Get the validator storage + /// @return s The validator storage struct + function _getStorage() internal pure returns (mapping(address => RLVValidatorStorage) storage s) { + bytes32 position = RESOURCE_LOCK_STORAGE_POSITION; + assembly { + s.slot := position + } + } + + /// @notice Maps smart account addresses to their validator configuration + function validatorStorage(address account) public view returns (RLVValidatorStorage memory) { + return _getStorage()[account]; + }
112-116: Consider adding proper type checking for empty data parameterThe
onUninstallfunction accepts adataparameter but doesn't use it. It would be good to either document why it's unused or to check for empty input.function onUninstall(bytes calldata data) external override { if (!_isInitialized(msg.sender)) revert RLV_NotInstalled(msg.sender); + // The data parameter is unused but required by the interface + // We could optionally verify it's empty: require(data.length == 0, "RLV_InvalidUninstallData"); delete validatorStorage[msg.sender]; emit RLV_ValidatorDisabled(msg.sender); }
124-159: Improve signature validation logic to reduce code duplicationThe signature validation contains duplicated logic between standard and proof-based validation flows. Consider refactoring to extract common logic into a helper function.
function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash) external override returns (uint256) { bytes calldata signature = userOp.signature; address walletOwner = validatorStorage[msg.sender].owner; + + // Helper function for standard signature validation + function _validateStandardSignature(address owner, bytes32 hash, bytes calldata sig) pure returns (bool) { + if (owner == ECDSA.recover(hash, sig)) { + return true; + } + bytes32 sigHash = ECDSA.toEthSignedMessageHash(hash); + return owner == ECDSA.recover(sigHash, sig); + } + // Standard signature length - no proof packing if (signature.length == 65) { - // standard ECDSA recover - if (walletOwner == ECDSA.recover(userOpHash, signature)) { + if (_validateStandardSignature(walletOwner, userOpHash, signature)) { return SIG_VALIDATION_SUCCESS; } - bytes32 sigHash = ECDSA.toEthSignedMessageHash(userOpHash); - address recoveredSigner = ECDSA.recover(sigHash, signature); - if (walletOwner != recoveredSigner) return SIG_VALIDATION_FAILED; - return SIG_VALIDATION_SUCCESS; + return SIG_VALIDATION_FAILED; } // or if signature.length >= 65 (standard signature length + proof packing) ResourceLock memory rl = _getResourceLock(userOp.callData); bytes memory ecdsaSignature = signature[0:65]; bytes32 root = bytes32(signature[65:97]); // 32 bytes bytes32[] memory proof = abi.decode(signature[97:], (bytes32[])); // Rest of bytes in signature if (!MerkleProofLib.verify(proof, root, _buildResourceLockHash(rl))) { revert RLV_ResourceLockHashNotInProof(); } // check proof is signed - if (walletOwner == ECDSA.recover(root, ecdsaSignature)) { + if (_validateStandardSignature(walletOwner, root, ecdsaSignature)) { _incrementNonce(msg.sender); return SIG_VALIDATION_SUCCESS; } - bytes32 sigRoot = ECDSA.toEthSignedMessageHash(root); - address recoveredMSigner = ECDSA.recover(sigRoot, ecdsaSignature); - if (walletOwner != recoveredMSigner) return SIG_VALIDATION_FAILED; return SIG_VALIDATION_SUCCESS; }
217-219: Add view function to get the owner address for a smart accountThere's a getter for the nonce but no getter for the owner address, which would be useful for front-end applications and verification.
+ /// @notice Returns owner address for specific smart account + /// @param smartAccount Address of the smart account to check + /// @return address Current owner address + function getOwner(address smartAccount) external view returns (address) { + return validatorStorage[smartAccount].owner; + } /// @notice Returns nonce for specific smart account /// @param smartAccount Address of the smart account to check /// @return uint256 Current unused nonce function getNonce(address smartAccount) external view returns (uint256) { return validatorStorage[smartAccount].nonce; }
284-300: Optimize gas usage in hashing operations by using abi.encodePackedFor keccak256 hashing where unique encoding is guaranteed, using abi.encodePacked instead of abi.encode can save gas.
function _buildResourceLockHash(ResourceLock memory _lock) internal pure returns (bytes32) { return keccak256( - abi.encode( + abi.encodePacked( _lock.chainId, _lock.smartWallet, _lock.sessionKey, _lock.validAfter, _lock.validUntil, _hashTokenData(_lock.tokenData), _lock.nonce ) ); }Note: Only use abi.encodePacked if the values are of fixed length or if there's no risk of hash collision. In this case, since most values are fixed-length types, it should be safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/interfaces/modules/IResourceLockValidator.sol(1 hunks)src/modules/validators/ResourceLockValidator.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/interfaces/modules/IResourceLockValidator.sol
🔇 Additional comments (1)
src/modules/validators/ResourceLockValidator.sol (1)
313-318:❌ Incorrect review comment
Add reentrancy protection to _incrementNonce function
The _incrementNonce function emits an event after updating the nonce. This could potentially be exploited in a reentrancy attack if called from a function lacking reentrancy protection.
I need to verify if reentrancy is a concern in this context by checking if this function is called from external functions that might be subject to reentrancy attacks:
🏁 Script executed:
#!/bin/bash # Check for functions that call _incrementNonce and analyze reentrancy protection # First, find all references to _incrementNonce in the file echo "References to _incrementNonce in ResourceLockValidator.sol:" grep -n "_incrementNonce" src/modules/validators/ResourceLockValidator.sol # Check for nonReentrant modifier usage in the contract echo -e "\nChecking for reentrancy protection:" grep -n "nonReentrant" src/modules/validators/ResourceLockValidator.sol # Check if any external/public function calls _incrementNonce echo -e "\nFunctions that might call _incrementNonce:" rg -A 5 -B 5 "function.*external.*returns.*\{" src/modules/validators/ResourceLockValidator.solLength of output: 1815
🏁 Script executed:
#!/bin/bash # Display 5 lines before and after the call site of _incrementNonce for context rg -n -C5 "_incrementNonce" src/modules/validators/ResourceLockValidator.solLength of output: 1203
🏁 Script executed:
#!/bin/bash # Show full context around the call site to identify the enclosing function and its modifiers sed -n '100,200p' src/modules/validators/ResourceLockValidator.solLength of output: 5161
No reentrancy risk in _incrementNonce
The
_incrementNoncefunction isinternal, updates only its own storage slot, and emits an event. It’s only ever called invalidateUserOpbefore any external calls (all signature and proof verifications are pure library calls), so there’s no opportunity for reentrancy here. You can safely ignore the suggestion to add reentrancy protection for this function.Likely an incorrect or invalid review comment.
| function onInstall(bytes calldata _data) external override { | ||
| address owner; | ||
| // Check if data starts with a function selector (bootstrap method) | ||
| if (_data.length >= 4 && _data[0] != 0) { | ||
| if (_data.length >= 24) { | ||
| // Try to extract owner from the last 20 bytes | ||
| owner = address(bytes20(_data[_data.length - 20:])); | ||
| if (owner == address(0)) { | ||
| bytes calldata actualData = _data[4:]; | ||
| if (actualData.length >= 32) { | ||
| owner = abi.decode(actualData, (address)); | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| // Direct method - extract from the end or decode directly | ||
| if (_data.length >= 20) { | ||
| owner = address(bytes20(_data[_data.length - 20:])); | ||
| } else if (_data.length == 32) { | ||
| owner = abi.decode(_data, (address)); | ||
| } | ||
| } | ||
| if (owner == address(0)) { | ||
| revert RLV_InvalidOwner(); | ||
| } | ||
| if (validatorStorage[msg.sender].owner != address(0)) { | ||
| revert RLV_AlreadyInstalled(msg.sender, validatorStorage[msg.sender].owner); | ||
| } | ||
| validatorStorage[msg.sender].owner = owner; | ||
| emit RLV_ValidatorEnabled(msg.sender, owner); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify owner extraction logic in the onInstall method
The owner extraction logic is overly complex and difficult to follow, with multiple nested conditions. This can make maintenance challenging and increases the risk of bugs.
function onInstall(bytes calldata _data) external override {
address owner;
- // Check if data starts with a function selector (bootstrap method)
- if (_data.length >= 4 && _data[0] != 0) {
- if (_data.length >= 24) {
- // Try to extract owner from the last 20 bytes
- owner = address(bytes20(_data[_data.length - 20:]));
- if (owner == address(0)) {
- bytes calldata actualData = _data[4:];
- if (actualData.length >= 32) {
- owner = abi.decode(actualData, (address));
- }
- }
- }
- } else {
- // Direct method - extract from the end or decode directly
- if (_data.length >= 20) {
- owner = address(bytes20(_data[_data.length - 20:]));
- } else if (_data.length == 32) {
- owner = abi.decode(_data, (address));
- }
- }
+ // Extract owner from different potential formats
+ if (_data.length == 0) {
+ revert RLV_InvalidOwner();
+ }
+
+ // Handle bootstrap method (starts with function selector)
+ if (_data.length >= 4 && _data[0] != 0) {
+ bytes calldata actualData = _data[4:];
+ if (actualData.length >= 32) {
+ owner = abi.decode(actualData, (address));
+ }
+ }
+ // Handle direct methods
+ else if (_data.length == 32) {
+ owner = abi.decode(_data, (address));
+ }
+ // Extract from last 20 bytes as fallback
+ else if (_data.length >= 20) {
+ owner = address(bytes20(_data[_data.length - 20:]));
+ }
if (owner == address(0)) {
revert RLV_InvalidOwner();
}
if (validatorStorage[msg.sender].owner != address(0)) {
revert RLV_AlreadyInstalled(msg.sender, validatorStorage[msg.sender].owner);
}
validatorStorage[msg.sender].owner = owner;
emit RLV_ValidatorEnabled(msg.sender, owner);
}Committable suggestion skipped: line range outside the PR's diff.
| function _getArrayInfo(bytes calldata _data) internal pure returns (uint256 offset, uint256 length) { | ||
| offset = uint256(bytes32(_data[260:292])); | ||
| length = uint256(bytes32(_data[100 + offset:132 + offset])); | ||
| } |
There was a problem hiding this comment.
Hardcoded array offsets in _getArrayInfo are fragile and need better documentation
The function uses hardcoded offsets (260, 292, 100, 132) without clear explanation of how these values were determined, making the code fragile and difficult to maintain.
/// @notice Extracts array information from calldata
/// @dev Helper function to navigate complex calldata structures
/// @param _data The calldata to extract information from
/// @return offset The offset of the array in the calldata
/// @return length The length of the array
function _getArrayInfo(bytes calldata _data) internal pure returns (uint256 offset, uint256 length) {
- offset = uint256(bytes32(_data[260:292]));
- length = uint256(bytes32(_data[100 + offset:132 + offset]));
+ // Retrieves the array offset from a specific position in the calldata
+ // These offsets correspond to:
+ // - 260: Position where the array offset is stored (bytes from start of the calldata)
+ // - 292: End position of the array offset (260 + 32 bytes)
+ // The array offset tells us where the array starts relative to the base position
+ offset = uint256(bytes32(_data[260:292]));
+
+ // The first 32 bytes at the array position contain the array length
+ // - 100: Base position in the calldata for array data
+ // - 132: End position of the array length (100 + 32 bytes)
+ length = uint256(bytes32(_data[100 + offset:132 + offset]));
+
+ // NOTE: These hardcoded offsets are based on the exact calldata structure of the resource lock.
+ // Any changes to the calldata structure will require updating these values.
}Consider refactoring this to use a more robust approach, possibly using ABI encoding/decoding patterns that are less dependent on fixed offsets.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function _getArrayInfo(bytes calldata _data) internal pure returns (uint256 offset, uint256 length) { | |
| offset = uint256(bytes32(_data[260:292])); | |
| length = uint256(bytes32(_data[100 + offset:132 + offset])); | |
| } | |
| /// @notice Extracts array information from calldata | |
| /// @dev Helper function to navigate complex calldata structures | |
| /// @param _data The calldata to extract information from | |
| /// @return offset The offset of the array in the calldata | |
| /// @return length The length of the array | |
| function _getArrayInfo(bytes calldata _data) internal pure returns (uint256 offset, uint256 length) { | |
| // Retrieves the array offset from a specific position in the calldata | |
| // These offsets correspond to: | |
| // - 260: Position where the array offset is stored (bytes from start of the calldata) | |
| // - 292: End position of the array offset (260 + 32 bytes) | |
| // The array offset tells us where the array starts relative to the base position | |
| offset = uint256(bytes32(_data[260:292])); | |
| // The first 32 bytes at the array position contain the array length | |
| // - 100: Base position in the calldata for array data | |
| // - 132: End position of the array length (100 + 32 bytes) | |
| length = uint256(bytes32(_data[100 + offset:132 + offset])); | |
| // NOTE: These hardcoded offsets are based on the exact calldata structure of the resource lock. | |
| // Any changes to the calldata structure will require updating these values. | |
| } |
| function isValidSignatureWithSender(address sender, bytes32 hash, bytes calldata signature) | ||
| external | ||
| view | ||
| override | ||
| returns (bytes4) | ||
| { | ||
| address walletOwner = validatorStorage[msg.sender].owner; | ||
| if (signature.length == 65) { | ||
| if (walletOwner == ECDSA.recover(hash, signature)) { | ||
| return ERC1271_MAGIC_VALUE; | ||
| } | ||
| bytes32 sigHash = ECDSA.toEthSignedMessageHash(hash); | ||
| address recoveredSigner = ECDSA.recover(sigHash, signature); | ||
| if (walletOwner != recoveredSigner) return ERC1271_INVALID; | ||
| return ERC1271_MAGIC_VALUE; | ||
| } | ||
| bytes memory ecdsaSig = signature[0:65]; | ||
| bytes32 root = bytes32(signature[65:97]); | ||
| bytes32[] memory proof = abi.decode(signature[97:], (bytes32[])); | ||
| if (!MerkleProofLib.verify(proof, root, hash)) { | ||
| revert RLV_ResourceLockHashNotInProof(); | ||
| } | ||
| // simple ecdsa verification | ||
| if (walletOwner == ECDSA.recover(root, ecdsaSig)) { | ||
| return ERC1271_MAGIC_VALUE; | ||
| } | ||
| bytes32 sigRoot = ECDSA.toEthSignedMessageHash(root); | ||
| address recoveredMSigner = ECDSA.recover(sigRoot, ecdsaSig); | ||
| if (walletOwner != recoveredMSigner) return ERC1271_INVALID; | ||
| return ERC1271_MAGIC_VALUE; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate signature validation logic between validateUserOp and isValidSignatureWithSender
There's significant duplication between validateUserOp and isValidSignatureWithSender. Consider extracting the common validation logic into a shared internal function.
The functions have almost identical signature verification logic but different return types. Extract the common logic:
+ /// @notice Internal function to validate signatures with or without Merkle proofs
+ /// @dev Used by both validateUserOp and isValidSignatureWithSender
+ /// @param _owner The owner address to verify against
+ /// @param _hash The hash to verify the signature for
+ /// @param _signature The signature data (with optional Merkle proof)
+ /// @param _verifyMerkleProof Whether to verify against a Merkle root (true) or directly (false)
+ /// @return bool True if signature is valid, false otherwise
+ function _validateSignature(
+ address _owner,
+ bytes32 _hash,
+ bytes memory _signature,
+ bool _verifyMerkleProof
+ ) internal view returns (bool) {
+ if (_signature.length == 65) {
+ if (_owner == ECDSA.recover(_hash, _signature)) {
+ return true;
+ }
+ bytes32 sigHash = ECDSA.toEthSignedMessageHash(_hash);
+ return _owner == ECDSA.recover(sigHash, _signature);
+ }
+
+ bytes memory ecdsaSig = _signature[0:65];
+ bytes32 root = bytes32(_signature[65:97]);
+ bytes32[] memory proof = abi.decode(_signature[97:], (bytes32[]));
+
+ if (_verifyMerkleProof && !MerkleProofLib.verify(proof, root, _hash)) {
+ revert RLV_ResourceLockHashNotInProof();
+ }
+
+ if (_owner == ECDSA.recover(root, ecdsaSig)) {
+ return true;
+ }
+ bytes32 sigRoot = ECDSA.toEthSignedMessageHash(root);
+ return _owner == ECDSA.recover(sigRoot, ecdsaSig);
+ }
function isValidSignatureWithSender(address sender, bytes32 hash, bytes calldata signature)
external
view
override
returns (bytes4)
{
address walletOwner = validatorStorage[msg.sender].owner;
- if (signature.length == 65) {
- if (walletOwner == ECDSA.recover(hash, signature)) {
- return ERC1271_MAGIC_VALUE;
- }
- bytes32 sigHash = ECDSA.toEthSignedMessageHash(hash);
- address recoveredSigner = ECDSA.recover(sigHash, signature);
- if (walletOwner != recoveredSigner) return ERC1271_INVALID;
- return ERC1271_MAGIC_VALUE;
- }
- bytes memory ecdsaSig = signature[0:65];
- bytes32 root = bytes32(signature[65:97]);
- bytes32[] memory proof = abi.decode(signature[97:], (bytes32[]));
- if (!MerkleProofLib.verify(proof, root, hash)) {
- revert RLV_ResourceLockHashNotInProof();
- }
- // simple ecdsa verification
- if (walletOwner == ECDSA.recover(root, ecdsaSig)) {
- return ERC1271_MAGIC_VALUE;
- }
- bytes32 sigRoot = ECDSA.toEthSignedMessageHash(root);
- address recoveredMSigner = ECDSA.recover(sigRoot, ecdsaSig);
- if (walletOwner != recoveredMSigner) return ERC1271_INVALID;
- return ERC1271_MAGIC_VALUE;
+ return _validateSignature(walletOwner, hash, signature, true)
+ ? ERC1271_MAGIC_VALUE
+ : ERC1271_INVALID;
}Then modify the validateUserOp function similarly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isValidSignatureWithSender(address sender, bytes32 hash, bytes calldata signature) | |
| external | |
| view | |
| override | |
| returns (bytes4) | |
| { | |
| address walletOwner = validatorStorage[msg.sender].owner; | |
| if (signature.length == 65) { | |
| if (walletOwner == ECDSA.recover(hash, signature)) { | |
| return ERC1271_MAGIC_VALUE; | |
| } | |
| bytes32 sigHash = ECDSA.toEthSignedMessageHash(hash); | |
| address recoveredSigner = ECDSA.recover(sigHash, signature); | |
| if (walletOwner != recoveredSigner) return ERC1271_INVALID; | |
| return ERC1271_MAGIC_VALUE; | |
| } | |
| bytes memory ecdsaSig = signature[0:65]; | |
| bytes32 root = bytes32(signature[65:97]); | |
| bytes32[] memory proof = abi.decode(signature[97:], (bytes32[])); | |
| if (!MerkleProofLib.verify(proof, root, hash)) { | |
| revert RLV_ResourceLockHashNotInProof(); | |
| } | |
| // simple ecdsa verification | |
| if (walletOwner == ECDSA.recover(root, ecdsaSig)) { | |
| return ERC1271_MAGIC_VALUE; | |
| } | |
| bytes32 sigRoot = ECDSA.toEthSignedMessageHash(root); | |
| address recoveredMSigner = ECDSA.recover(sigRoot, ecdsaSig); | |
| if (walletOwner != recoveredMSigner) return ERC1271_INVALID; | |
| return ERC1271_MAGIC_VALUE; | |
| } | |
| /// @notice Internal function to validate signatures with or without Merkle proofs | |
| /// @dev Used by both validateUserOp and isValidSignatureWithSender | |
| /// @param _owner The owner address to verify against | |
| /// @param _hash The hash to verify the signature for | |
| /// @param _signature The signature data (with optional Merkle proof) | |
| /// @param _verifyMerkleProof Whether to verify against a Merkle root (true) or directly (false) | |
| /// @return bool True if the signature is valid, false otherwise | |
| function _validateSignature( | |
| address _owner, | |
| bytes32 _hash, | |
| bytes memory _signature, | |
| bool _verifyMerkleProof | |
| ) internal view returns (bool) { | |
| if (_signature.length == 65) { | |
| // simple ECDSA | |
| if (_owner == ECDSA.recover(_hash, _signature)) { | |
| return true; | |
| } | |
| bytes32 sigHash = ECDSA.toEthSignedMessageHash(_hash); | |
| return _owner == ECDSA.recover(sigHash, _signature); | |
| } | |
| // extract Merkle‐proof form: [ecdsaSig (65)] [root (32)] [proof array] | |
| bytes memory ecdsaSig = _signature[0:65]; | |
| bytes32 root = bytes32(_signature[65:97]); | |
| bytes32[] memory proof = abi.decode(_signature[97:], (bytes32[])); | |
| if (_verifyMerkleProof && !MerkleProofLib.verify(proof, root, _hash)) { | |
| revert RLV_ResourceLockHashNotInProof(); | |
| } | |
| // verify against root | |
| if (_owner == ECDSA.recover(root, ecdsaSig)) { | |
| return true; | |
| } | |
| bytes32 sigRoot = ECDSA.toEthSignedMessageHash(root); | |
| return _owner == ECDSA.recover(sigRoot, ecdsaSig); | |
| } | |
| function isValidSignatureWithSender( | |
| address sender, | |
| bytes32 hash, | |
| bytes calldata signature | |
| ) | |
| external | |
| view | |
| override | |
| returns (bytes4) | |
| { | |
| address walletOwner = validatorStorage[msg.sender].owner; | |
| return _validateSignature(walletOwner, hash, signature, true) | |
| ? ERC1271_MAGIC_VALUE | |
| : ERC1271_INVALID; | |
| } |
| function _getResourceLock(bytes calldata _callData) internal view returns (ResourceLock memory) { | ||
| if (bytes4(_callData[:4]) == IERC7579Account.execute.selector) { | ||
| (CallType calltype,,,) = ModeLib.decode(ModeCode.wrap(bytes32(_callData[4:36]))); | ||
| if (calltype == CALLTYPE_SINGLE) { | ||
| (,, bytes calldata execData) = ExecutionLib.decodeSingle(_callData[100:]); | ||
| (uint256 arrayOffset, uint256 arrayLength) = _getArrayInfo(execData); | ||
| TokenData[] memory td = new TokenData[](arrayLength); | ||
| for (uint256 i; i < arrayLength; ++i) { | ||
| td[i] = _getSingleTokenData(execData, 132 + arrayOffset + (i * 64)); | ||
| } | ||
| address scw = address(uint160(uint256(bytes32(execData[132:164])))); | ||
| return ResourceLock({ | ||
| chainId: uint256(bytes32(execData[100:132])), | ||
| smartWallet: scw, | ||
| sessionKey: address(uint160(uint256(bytes32(execData[164:196])))), | ||
| validAfter: uint48(uint256(bytes32(execData[196:228]))), | ||
| validUntil: uint48(uint256(bytes32(execData[228:260]))), | ||
| tokenData: td, | ||
| nonce: validatorStorage[scw].nonce | ||
| }); | ||
| } | ||
| revert RLV_OnlyCallTypeSingle(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve robustness of _getResourceLock function
The _getResourceLock function has several issues:
- It only works with a specific calldata structure
- It doesn't handle potential out-of-bounds errors when accessing calldata
- It returns an uninitialized ResourceLock if the execute selector isn't matched
function _getResourceLock(bytes calldata _callData) internal view returns (ResourceLock memory) {
+ ResourceLock memory emptyLock;
+
+ // Ensure calldata has enough length for selector
+ if (_callData.length < 4) {
+ return emptyLock;
+ }
+
if (bytes4(_callData[:4]) == IERC7579Account.execute.selector) {
+ // Ensure calldata has enough length for mode code
+ if (_callData.length < 36) {
+ return emptyLock;
+ }
+
(CallType calltype,,,) = ModeLib.decode(ModeCode.wrap(bytes32(_callData[4:36])));
if (calltype == CALLTYPE_SINGLE) {
+ // Ensure calldata has enough length for execution data
+ if (_callData.length < 100) {
+ return emptyLock;
+ }
+
(,, bytes calldata execData) = ExecutionLib.decodeSingle(_callData[100:]);
+
+ // Validate minimum execution data length
+ if (execData.length < 292) { // 260 + 32 for the array offset
+ return emptyLock;
+ }
+
(uint256 arrayOffset, uint256 arrayLength) = _getArrayInfo(execData);
+
+ // Validate array data is within bounds
+ uint256 expectedDataLength = 132 + arrayOffset + (arrayLength * 64);
+ if (execData.length < expectedDataLength) {
+ return emptyLock;
+ }
+
TokenData[] memory td = new TokenData[](arrayLength);
for (uint256 i; i < arrayLength; ++i) {
td[i] = _getSingleTokenData(execData, 132 + arrayOffset + (i * 64));
}
+
+ // Ensure we have enough data for the smart wallet address
+ if (execData.length < 164) {
+ return emptyLock;
+ }
+
address scw = address(uint160(uint256(bytes32(execData[132:164]))));
return ResourceLock({
chainId: uint256(bytes32(execData[100:132])),
smartWallet: scw,
sessionKey: address(uint160(uint256(bytes32(execData[164:196])))),
validAfter: uint48(uint256(bytes32(execData[196:228]))),
validUntil: uint48(uint256(bytes32(execData[228:260]))),
tokenData: td,
nonce: validatorStorage[scw].nonce
});
}
revert RLV_OnlyCallTypeSingle();
}
+ // Return empty ResourceLock struct or consider reverting with an error
+ return emptyLock;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function _getResourceLock(bytes calldata _callData) internal view returns (ResourceLock memory) { | |
| if (bytes4(_callData[:4]) == IERC7579Account.execute.selector) { | |
| (CallType calltype,,,) = ModeLib.decode(ModeCode.wrap(bytes32(_callData[4:36]))); | |
| if (calltype == CALLTYPE_SINGLE) { | |
| (,, bytes calldata execData) = ExecutionLib.decodeSingle(_callData[100:]); | |
| (uint256 arrayOffset, uint256 arrayLength) = _getArrayInfo(execData); | |
| TokenData[] memory td = new TokenData[](arrayLength); | |
| for (uint256 i; i < arrayLength; ++i) { | |
| td[i] = _getSingleTokenData(execData, 132 + arrayOffset + (i * 64)); | |
| } | |
| address scw = address(uint160(uint256(bytes32(execData[132:164])))); | |
| return ResourceLock({ | |
| chainId: uint256(bytes32(execData[100:132])), | |
| smartWallet: scw, | |
| sessionKey: address(uint160(uint256(bytes32(execData[164:196])))), | |
| validAfter: uint48(uint256(bytes32(execData[196:228]))), | |
| validUntil: uint48(uint256(bytes32(execData[228:260]))), | |
| tokenData: td, | |
| nonce: validatorStorage[scw].nonce | |
| }); | |
| } | |
| revert RLV_OnlyCallTypeSingle(); | |
| } | |
| } | |
| function _getResourceLock(bytes calldata _callData) internal view returns (ResourceLock memory) { | |
| ResourceLock memory emptyLock; | |
| // Ensure calldata has enough length for selector | |
| if (_callData.length < 4) { | |
| return emptyLock; | |
| } | |
| if (bytes4(_callData[:4]) == IERC7579Account.execute.selector) { | |
| // Ensure calldata has enough length for mode code | |
| if (_callData.length < 36) { | |
| return emptyLock; | |
| } | |
| (CallType calltype,,,) = ModeLib.decode( | |
| ModeCode.wrap(bytes32(_callData[4:36])) | |
| ); | |
| if (calltype == CALLTYPE_SINGLE) { | |
| // Ensure calldata has enough length for execution data | |
| if (_callData.length < 100) { | |
| return emptyLock; | |
| } | |
| (,, bytes calldata execData) = ExecutionLib.decodeSingle( | |
| _callData[100:] | |
| ); | |
| // Validate minimum execution data length | |
| if (execData.length < 292) { // 260 + 32 for the array offset | |
| return emptyLock; | |
| } | |
| (uint256 arrayOffset, uint256 arrayLength) = _getArrayInfo(execData); | |
| // Validate array data is within bounds | |
| uint256 expectedDataLength = 132 + arrayOffset + (arrayLength * 64); | |
| if (execData.length < expectedDataLength) { | |
| return emptyLock; | |
| } | |
| TokenData[] memory td = new TokenData[](arrayLength); | |
| for (uint256 i; i < arrayLength; ++i) { | |
| td[i] = _getSingleTokenData( | |
| execData, | |
| 132 + arrayOffset + (i * 64) | |
| ); | |
| } | |
| // Ensure we have enough data for the smart wallet address | |
| if (execData.length < 164) { | |
| return emptyLock; | |
| } | |
| address scw = address( | |
| uint160(uint256(bytes32(execData[132:164]))) | |
| ); | |
| return ResourceLock({ | |
| chainId: uint256(bytes32(execData[100:132])), | |
| smartWallet: scw, | |
| sessionKey: address(uint160(uint256(bytes32(execData[164:196])))), | |
| validAfter: uint48(uint256(bytes32(execData[196:228]))), | |
| validUntil: uint48(uint256(bytes32(execData[228:260]))), | |
| tokenData: td, | |
| nonce: validatorStorage[scw].nonce | |
| }); | |
| } | |
| revert RLV_OnlyCallTypeSingle(); | |
| } | |
| // Return empty ResourceLock if selector doesn’t match | |
| return emptyLock; | |
| } |
- Decided that backend will call `getNonce` function of the `ResourceLockValidator` and pass that through to the validator in the `UserOperation.calldata`. Validation is then performed that the nonce provided by the backend matches what the `ResourceLockValidator` is expecting. - Added test to confirm that revert happens if mismatch in nonce occurs.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/modules/validators/ResourceLockValidator.sol (4)
78-108: Complex owner extraction logic needs simplificationThe owner extraction logic in
onInstallis overly complex with nested conditions, making it difficult to follow and maintain. This pattern increases the risk of introducing bugs during future modifications.Consider refactoring this method with a clearer, more structured approach as suggested in previous reviews:
function onInstall(bytes calldata _data) external override { address owner; - // Check if data starts with a function selector (bootstrap method) - if (_data.length >= 4 && _data[0] != 0) { - if (_data.length >= 24) { - // Try to extract owner from the last 20 bytes - owner = address(bytes20(_data[_data.length - 20:])); - if (owner == address(0)) { - bytes calldata actualData = _data[4:]; - if (actualData.length >= 32) { - owner = abi.decode(actualData, (address)); - } - } - } - } else { - // Direct method - extract from the end or decode directly - if (_data.length >= 20) { - owner = address(bytes20(_data[_data.length - 20:])); - } else if (_data.length == 32) { - owner = abi.decode(_data, (address)); - } - } + // Extract owner from different potential formats + if (_data.length == 0) { + revert RLV_InvalidOwner(); + } + + // Handle bootstrap method (starts with function selector) + if (_data.length >= 4 && _data[0] != 0) { + bytes calldata actualData = _data[4:]; + if (actualData.length >= 32) { + owner = abi.decode(actualData, (address)); + } + } + // Handle direct methods + else if (_data.length == 32) { + owner = abi.decode(_data, (address)); + } + // Extract from last 20 bytes as fallback + else if (_data.length >= 20) { + owner = address(bytes20(_data[_data.length - 20:])); + }
171-201: Duplicate signature validation logic between functionsThere's significant duplication between
validateUserOpandisValidSignatureWithSender. Both functions have almost identical signature verification logic but different return types.Extract the common validation logic into a shared internal function to improve maintainability and reduce the risk of inconsistencies:
+ /// @notice Internal function to validate signatures with or without Merkle proofs + /// @dev Used by both validateUserOp and isValidSignatureWithSender + /// @param _owner The owner address to verify against + /// @param _hash The hash to verify the signature for + /// @param _signature The signature data (with optional Merkle proof) + /// @param _verifyMerkleProof Whether to verify against a Merkle root (true) or directly (false) + /// @return bool True if signature is valid, false otherwise + function _validateSignature( + address _owner, + bytes32 _hash, + bytes memory _signature, + bool _verifyMerkleProof + ) internal view returns (bool) { + if (_signature.length == 65) { + if (_owner == ECDSA.recover(_hash, _signature)) { + return true; + } + bytes32 sigHash = ECDSA.toEthSignedMessageHash(_hash); + return _owner == ECDSA.recover(sigHash, _signature); + } + + bytes memory ecdsaSig = _signature[0:65]; + bytes32 root = bytes32(_signature[65:97]); + bytes32[] memory proof = abi.decode(_signature[97:], (bytes32[])); + + if (_verifyMerkleProof && !MerkleProofLib.verify(proof, root, _hash)) { + revert RLV_ResourceLockHashNotInProof(); + } + + if (_owner == ECDSA.recover(root, ecdsaSig)) { + return true; + } + bytes32 sigRoot = ECDSA.toEthSignedMessageHash(root); + return _owner == ECDSA.recover(sigRoot, ecdsaSig); + }Then modify both functions to use this shared logic.
240-243: Hardcoded array offsets in _getArrayInfo are fragileThe function uses hardcoded offsets (260, 292, 100, 132) without clear explanation of how these values were determined, making the code fragile and difficult to maintain.
Add detailed comments explaining these magic numbers or consider using a more robust approach to parse calldata:
function _getArrayInfo(bytes calldata _data) internal pure returns (uint256 offset, uint256 length) { - offset = uint256(bytes32(_data[260:292])); - length = uint256(bytes32(_data[100 + offset:132 + offset])); + // Retrieves the array offset from a specific position in the calldata + // These offsets correspond to: + // - 260: Position where the array offset is stored (bytes from start of the calldata) + // - 292: End position of the array offset (260 + 32 bytes) + offset = uint256(bytes32(_data[260:292])); + + // The first 32 bytes at the array position contain the array length + // - 100: Base position in the calldata for array data + // - 132: End position of the array length (100 + 32 bytes) + length = uint256(bytes32(_data[100 + offset:132 + offset])); + + // NOTE: These hardcoded offsets are based on the exact calldata structure of the resource lock. + // Any changes to the calldata structure will require updating these values. }
262-290:⚠️ Potential issue_getResourceLock function lacks sufficient error handling
The _getResourceLock function has several robustness issues:
- It doesn't return any value or revert when the selector check fails
- It doesn't validate array bounds when accessing calldata
- Complex nested calldata parsing lacks appropriate error handling
Enhance the function with proper error handling and bounds checking:
function _getResourceLock(bytes calldata _callData) internal view returns (ResourceLock memory) { + ResourceLock memory emptyLock; + + // Ensure calldata has enough length for selector + if (_callData.length < 4) { + revert("Insufficient calldata length"); + } + if (bytes4(_callData[:4]) == IERC7579Account.execute.selector) { + // Ensure calldata has enough length for mode code + if (_callData.length < 36) { + revert("Insufficient calldata length for mode code"); + } + (CallType calltype,,,) = ModeLib.decode(ModeCode.wrap(bytes32(_callData[4:36]))); if (calltype == CALLTYPE_SINGLE) { + // Ensure calldata has enough length for execution data + if (_callData.length < 100) { + revert("Insufficient calldata length for execution data"); + } + (,, bytes calldata execData) = ExecutionLib.decodeSingle(_callData[100:]); + + // Validate minimum execution data length + if (execData.length < 324) { // Minimum size required for all fixed fields + revert("Insufficient execution data length"); + } + (uint256 arrayOffset, uint256 arrayLength) = _getArrayInfo(execData); + + // Validate array data is within bounds + uint256 expectedMinLength = 132 + arrayOffset + (arrayLength * 64); + if (execData.length < expectedMinLength) { + revert("Token data array exceeds calldata bounds"); + } + TokenData[] memory td = new TokenData[](arrayLength); for (uint256 i; i < arrayLength; ++i) { td[i] = _getSingleTokenData(execData, 132 + arrayOffset + (i * 64)); } address scw = address(uint160(uint256(bytes32(execData[132:164])))); uint256 expectedNonce = validatorStorage[scw].nonce; uint256 receivedNonce = uint256(bytes32(execData[292:324])); if (receivedNonce != expectedNonce) { revert RLV_InvalidNonce(expectedNonce, receivedNonce); } return ResourceLock({ chainId: uint256(bytes32(execData[100:132])), smartWallet: scw, sessionKey: address(uint160(uint256(bytes32(execData[164:196])))), validAfter: uint48(uint256(bytes32(execData[196:228]))), validUntil: uint48(uint256(bytes32(execData[228:260]))), tokenData: td, nonce: validatorStorage[scw].nonce }); } revert RLV_OnlyCallTypeSingle(); } + // Revert with clear error when selector doesn't match + revert("Invalid calldata selector"); }
🧹 Nitpick comments (1)
src/modules/validators/ResourceLockValidator.sol (1)
292-308: Simplify the _buildResourceLockHash functionThe function is correct but can be simplified for readability and reduced gas costs.
Consider simplifying the function to reduce nesting:
function _buildResourceLockHash(ResourceLock memory _lock) internal pure returns (bytes32) { - return keccak256( - abi.encode( - _lock.chainId, - _lock.smartWallet, - _lock.sessionKey, - _lock.validAfter, - _lock.validUntil, - _hashTokenData(_lock.tokenData), - _lock.nonce - ) - ); + return keccak256(abi.encode( + _lock.chainId, + _lock.smartWallet, + _lock.sessionKey, + _lock.validAfter, + _lock.validUntil, + _hashTokenData(_lock.tokenData), + _lock.nonce + )); }
| function _incrementNonce(address _smartAccount) internal returns (uint256) { | ||
| uint256 newNonce = validatorStorage[_smartAccount].nonce + 1; | ||
| validatorStorage[_smartAccount].nonce = newNonce; | ||
| emit RLV_NonceUpdated(_smartAccount, newNonce); | ||
| return newNonce; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
_incrementNonce function doesn't validate smart account input
The function directly uses the smart account address without verifying it, which could lead to unexpected behavior if an invalid address is provided.
Add validation for the smart account address:
function _incrementNonce(address _smartAccount) internal returns (uint256) {
+ if (_smartAccount == address(0)) {
+ revert("Invalid smart account address");
+ }
+
+ if (!_isInitialized(_smartAccount)) {
+ revert RLV_NotInstalled(_smartAccount);
+ }
+
uint256 newNonce = validatorStorage[_smartAccount].nonce + 1;
validatorStorage[_smartAccount].nonce = newNonce;
emit RLV_NonceUpdated(_smartAccount, newNonce);
return newNonce;
}Committable suggestion skipped: line range outside the PR's diff.
| ResourceLock memory rl = _getResourceLock(userOp.callData); | ||
| bytes memory ecdsaSignature = signature[0:65]; | ||
| bytes32 root = bytes32(signature[65:97]); // 32 bytes | ||
| bytes32[] memory proof = abi.decode(signature[97:], (bytes32[])); // Rest of bytes in signature | ||
| if (!MerkleProofLib.verify(proof, root, _buildResourceLockHash(rl))) { | ||
| revert RLV_ResourceLockHashNotInProof(); | ||
| } | ||
| // check proof is signed | ||
| if (walletOwner == ECDSA.recover(root, ecdsaSignature)) { | ||
| _incrementNonce(msg.sender); | ||
| return SIG_VALIDATION_SUCCESS; | ||
| } | ||
| bytes32 sigRoot = ECDSA.toEthSignedMessageHash(root); | ||
| address recoveredMSigner = ECDSA.recover(sigRoot, ecdsaSignature); | ||
| if (walletOwner != recoveredMSigner) return SIG_VALIDATION_FAILED; | ||
| return SIG_VALIDATION_SUCCESS; | ||
| } |
There was a problem hiding this comment.
validateUserOp function missing nonce increment in some paths
The function increments the nonce in one successful validation path but not in another, which could lead to nonce inconsistencies.
Ensure nonce is incremented for all successful validation paths:
// Standard signature length - no proof packing
if (signature.length == 65) {
// standard ECDSA recover
if (walletOwner == ECDSA.recover(userOpHash, signature)) {
+ _incrementNonce(msg.sender);
return SIG_VALIDATION_SUCCESS;
}
bytes32 sigHash = ECDSA.toEthSignedMessageHash(userOpHash);
address recoveredSigner = ECDSA.recover(sigHash, signature);
if (walletOwner != recoveredSigner) return SIG_VALIDATION_FAILED;
+ _incrementNonce(msg.sender);
return SIG_VALIDATION_SUCCESS;
}Committable suggestion skipped: line range outside the PR's diff.
- Should now unpack single and multiple proofs that are packed in signature. - When no proof is passed by signature and root is provided it will now fail gracefully. - Added helper for packing the proof in the signature (now is simple concatenation rather than packing `bytes32[]`). - Fixed all tests and added new one that can be used to directly check signature when unpacked (if console logs added to contract - currently skipped in suite).
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/modules/validators/ResourceLockValidator.sol(1 hunks)test/ModularTestBase.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/validators/ResourceLockValidator.sol
| if (valid) { | ||
| // Create a larger tree with 8 leaves | ||
| proof = new bytes32[](3); | ||
| // Level 1 proofs | ||
| proof[0] = bytes32("b"); | ||
| // Level 2 proofs | ||
| proof[1] = _hashPair(bytes32("c"), bytes32("d")); | ||
| // Level 3 proofs | ||
| proof[2] = _hashPair(_hashPair(bytes32("e"), bytes32("f")), _hashPair(bytes32("g"), bytes32("h"))); | ||
| // Build root from bottom up | ||
| bytes32 level1Hash = _hashPair(_leaf, proof[0]); | ||
| bytes32 level2Hash = _hashPair(level1Hash, proof[1]); | ||
| root = _hashPair(level2Hash, proof[2]); | ||
| } else { |
There was a problem hiding this comment.
Returned leaf is unintentionally zero for valid proofs
In getTestProof, the leaf output variable is only set in the else branch. For the valid == true branch it defaults to zero, which is misleading to callers that rely on the returned leaf.
bytes32 level2Hash = _hashPair(level1Hash, proof[1]);
- root = _hashPair(level2Hash, proof[2]);
+ root = _hashPair(level2Hash, proof[2]);
+ leaf = _leaf; // propagate the requested leafWithout this, any assertion comparing the returned leaf with the input will fail even though the proof is valid.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (valid) { | |
| // Create a larger tree with 8 leaves | |
| proof = new bytes32[](3); | |
| // Level 1 proofs | |
| proof[0] = bytes32("b"); | |
| // Level 2 proofs | |
| proof[1] = _hashPair(bytes32("c"), bytes32("d")); | |
| // Level 3 proofs | |
| proof[2] = _hashPair(_hashPair(bytes32("e"), bytes32("f")), _hashPair(bytes32("g"), bytes32("h"))); | |
| // Build root from bottom up | |
| bytes32 level1Hash = _hashPair(_leaf, proof[0]); | |
| bytes32 level2Hash = _hashPair(level1Hash, proof[1]); | |
| root = _hashPair(level2Hash, proof[2]); | |
| } else { | |
| if (valid) { | |
| // Create a larger tree with 8 leaves | |
| proof = new bytes32[](3); | |
| // Level 1 proofs | |
| proof[0] = bytes32("b"); | |
| // Level 2 proofs | |
| proof[1] = _hashPair(bytes32("c"), bytes32("d")); | |
| // Level 3 proofs | |
| proof[2] = _hashPair(_hashPair(bytes32("e"), bytes32("f")), _hashPair(bytes32("g"), bytes32("h"))); | |
| // Build root from bottom up | |
| bytes32 level1Hash = _hashPair(_leaf, proof[0]); | |
| bytes32 level2Hash = _hashPair(level1Hash, proof[1]); | |
| root = _hashPair(level2Hash, proof[2]); | |
| leaf = _leaf; // propagate the requested leaf | |
| } else { |
| IMPLEMENTATION = new ModularEtherspotWallet(ENTRYPOINT); | ||
| FACTORY = new ModularEtherspotWalletFactory(address(IMPLEMENTATION), eoa.pub); | ||
| BOOTSTRAP = new Bootstrap(); | ||
| ECDSA_VALIDATOR = new ECDSAValidator(); | ||
| MULTIPLE_OWNER_ECDSA_VALIDATOR = new MultipleOwnerECDSAValidator(); | ||
| GUARDIAN_RECOVERY_VALIDATOR = new GuardianRecoveryValidator(); |
There was a problem hiding this comment.
eoa is used before it is initialized – factory gets a zero address
eoa is instantiated only at line 184, but its .pub field is already passed to ModularEtherspotWalletFactory at line 141.
This silently injects the zero address as the authorised factory owner, which can break access-control paths and render the factory unusable in downstream tests.
- // Setup Deployer
- deployer = _createDeployer("Deployer");
+ // Setup Deployer & default EOA before any contract that depends on them
+ deployer = _createDeployer("Deployer");
+ eoa = _createUser("EOA"); // must precede FACTORY creation
...
- FACTORY = new ModularEtherspotWalletFactory(address(IMPLEMENTATION), eoa.pub);
+ FACTORY = new ModularEtherspotWalletFactory(address(IMPLEMENTATION), eoa.pub);Move the eoa creation right after the deployer (or pass another valid address) so that FACTORY receives a non-zero, intended owner.
Committable suggestion skipped: line range outside the PR's diff.
| abi.encodeWithSelector(_scw.installModule.selector, _moduleType, _module, _initData) | ||
| ); | ||
| if (_moduleType == MODULE_TYPE_FALLBACK) { | ||
| bytes4 selector = bytes4(bytes32(_initData)); | ||
| return _scw.isModuleInstalled(_moduleType, _module, abi.encode(selector)); | ||
| } | ||
| vm.stopPrank(); |
There was a problem hiding this comment.
Early return leaves vm.startPrank open – prank context leaks
Inside _installModule, the fallback branch returns without calling vm.stopPrank, keeping the prank active for the rest of the test and causing extremely confusing side-effects.
if (_moduleType == MODULE_TYPE_FALLBACK) {
bytes4 selector = bytes4(bytes32(_initData));
- return _scw.isModuleInstalled(_moduleType, _module, abi.encode(selector));
+ bool installed = _scw.isModuleInstalled(_moduleType, _module, abi.encode(selector));
+ vm.stopPrank();
+ return installed;
}Always terminate the prank before returning.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| abi.encodeWithSelector(_scw.installModule.selector, _moduleType, _module, _initData) | |
| ); | |
| if (_moduleType == MODULE_TYPE_FALLBACK) { | |
| bytes4 selector = bytes4(bytes32(_initData)); | |
| return _scw.isModuleInstalled(_moduleType, _module, abi.encode(selector)); | |
| } | |
| vm.stopPrank(); | |
| abi.encodeWithSelector(_scw.installModule.selector, _moduleType, _module, _initData) | |
| ); | |
| if (_moduleType == MODULE_TYPE_FALLBACK) { | |
| bytes4 selector = bytes4(bytes32(_initData)); | |
| bool installed = _scw.isModuleInstalled(_moduleType, _module, abi.encode(selector)); | |
| vm.stopPrank(); | |
| return installed; | |
| } | |
| vm.stopPrank(); |
| vm.startPrank(_owner); | ||
| if (_moduleType == MODULE_TYPE_FALLBACK) { | ||
| MOCK_EXECUTOR.executeViaAccount( | ||
| IERC7579Account(_scw), | ||
| address(_scw), | ||
| 0, | ||
| abi.encodeWithSelector(_scw.uninstallModule.selector, _moduleType, _module, _deInitData) | ||
| ); | ||
| return _scw.isModuleInstalled(_moduleType, _module, _deInitData); | ||
| } | ||
| address prevValidator = _getPrevValidator(_scw, _module); | ||
| // Execute the module installation | ||
| MOCK_EXECUTOR.executeViaAccount( | ||
| IERC7579Account(_scw), | ||
| address(_scw), | ||
| 0, | ||
| abi.encodeWithSelector( | ||
| _scw.uninstallModule.selector, _moduleType, _module, abi.encode(prevValidator, _deInitData) | ||
| ) | ||
| ); | ||
| // Verify that the module is installed | ||
| return _scw.isModuleInstalled(_moduleType, _module, ""); | ||
| vm.stopPrank(); |
There was a problem hiding this comment.
Unreachable vm.stopPrank() and duplicated early-return path
_uninstallModule also returns twice before hitting vm.stopPrank, again leaking the prank. Consolidate the logic and ensure the prank is stopped in every path.
- if (_moduleType == MODULE_TYPE_FALLBACK) {
- ...
- return _scw.isModuleInstalled(_moduleType, _module, _deInitData);
- }
- ...
- return _scw.isModuleInstalled(_moduleType, _module, "");
- vm.stopPrank();
+ bool installed;
+ if (_moduleType == MODULE_TYPE_FALLBACK) {
+ ...
+ installed = _scw.isModuleInstalled(_moduleType, _module, _deInitData);
+ } else {
+ ...
+ installed = _scw.isModuleInstalled(_moduleType, _module, "");
+ }
+ vm.stopPrank();
+ return installed;This prevents hidden state bleed-through and makes the helper deterministic.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vm.startPrank(_owner); | |
| if (_moduleType == MODULE_TYPE_FALLBACK) { | |
| MOCK_EXECUTOR.executeViaAccount( | |
| IERC7579Account(_scw), | |
| address(_scw), | |
| 0, | |
| abi.encodeWithSelector(_scw.uninstallModule.selector, _moduleType, _module, _deInitData) | |
| ); | |
| return _scw.isModuleInstalled(_moduleType, _module, _deInitData); | |
| } | |
| address prevValidator = _getPrevValidator(_scw, _module); | |
| // Execute the module installation | |
| MOCK_EXECUTOR.executeViaAccount( | |
| IERC7579Account(_scw), | |
| address(_scw), | |
| 0, | |
| abi.encodeWithSelector( | |
| _scw.uninstallModule.selector, _moduleType, _module, abi.encode(prevValidator, _deInitData) | |
| ) | |
| ); | |
| // Verify that the module is installed | |
| return _scw.isModuleInstalled(_moduleType, _module, ""); | |
| vm.stopPrank(); | |
| vm.startPrank(_owner); | |
| bool installed; | |
| if (_moduleType == MODULE_TYPE_FALLBACK) { | |
| MOCK_EXECUTOR.executeViaAccount( | |
| IERC7579Account(_scw), | |
| address(_scw), | |
| 0, | |
| abi.encodeWithSelector( | |
| _scw.uninstallModule.selector, | |
| _moduleType, | |
| _module, | |
| _deInitData | |
| ) | |
| ); | |
| installed = _scw.isModuleInstalled(_moduleType, _module, _deInitData); | |
| } else { | |
| address prevValidator = _getPrevValidator(_scw, _module); | |
| // Execute the module uninstallation | |
| MOCK_EXECUTOR.executeViaAccount( | |
| IERC7579Account(_scw), | |
| address(_scw), | |
| 0, | |
| abi.encodeWithSelector( | |
| _scw.uninstallModule.selector, | |
| _moduleType, | |
| _module, | |
| abi.encode(prevValidator, _deInitData) | |
| ) | |
| ); | |
| // Verify that the module is installed | |
| installed = _scw.isModuleInstalled(_moduleType, _module, ""); | |
| } | |
| vm.stopPrank(); | |
| return installed; |
Description
AccessControllerand storing wallet owner inside wallet directly.ECDSAValidator,GuardianRecoveryValidatormodules. TheGuardianRecoveryValidatormodule encapsulates theAccessControllerguardianship mechanism. These modules allow for installing guardianship retroactively and also now allow for multiple owners of a wallet to not be the default if not required (can useECDSAValidatorinstead).CredibleAccountModuleto split into two distinctive modules (CredibleAccountValidatorandCredibleAccountHook). This is due to how we need to clear modules when callingonRedelegation()which is required forEIP-7702.CredibleAccountValidatornow contains the storage whichCredibleAccountHookcan read from. Now need toinitialize()CredibleAccountValidatorwith theCredibleAccountHookpost deployment for checking installation.CredibleAccountValidatorstorage of token lock data. Added extra functionality to pull data from contract.EIP-7702delegation.EIP-7702.HookMultiPlexerso we now no longer need to useERC7579HookBase. Reworked data we pass to Hooks when installing.ModuleManager. It now integratesHookManager. Reworked thefallbackto supportonRecievedforERC7579 andERC1155`.Typesfolder for storage of different type data.ERC7201namespaced storage andERC7779.Initializableto manage initialization of the wallet implementation.IHookLensas no longer required.Motivation and Context
EIP-7702.How Has This Been Tested?
Screenshots (if appropriate)
Types of changes
Summary by CodeRabbit
New Features
AccountStorage,BaseAccount,ExecutionHelper,ModuleManager,RegistryAdapter,FactoryStaker, andModularEtherspotWalletFactorycore contracts.CredibleAccountValidator,GuardianRecoveryValidator,MultipleOwnerECDSAValidator,ResourceLockValidator,ERC20SessionKeyValidator, andECDSAValidator.CredibleAccountHookandHookMultiPlexerwith enhanced hook management.BootstrapLib,ExecutionLib,HashLib,HookMultiPlexerLib,Initializable, andArrayLib.MockDelegateTarget.Improvements
Bug Fixes
Chores
dependenciesfolder..env.exampleand.gitignoreto reflect new project structure.Documentation
ResourceLockValidatormodule including integration and security guidelines.