ResourceLockValidator, test suite cleanup and Soldeer integration
#6
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.
|
""" WalkthroughThis pull request updates dependency management and configuration files, along with significant restructuring of the project’s module and test architecture. Submodules and commit references have been removed in favor of new dependency directories and remappings. The documentation and configuration (e.g., README, foundry.toml, .env.example) are updated with new instructions and sections. New modules—including the Resource Lock Validator—and interfaces have been introduced, and various modules have been refactored with improved logging, error handling, and standardized import paths. Additionally, many test files have been added, modified, or removed to align with the updated modular design. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant W as Modular Wallet
participant V as ResourceLockValidator
participant C as CryptoLib (ECDSA/MerkleProof)
U->>W: Submit User Operation
W->>V: Call validateUserOp(userOp, userOpHash)
V->>C: Verify ECDSA signature & Merkle proof
C-->>V: Return verification result
V->>W: Return validation status
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 13
🧹 Nitpick comments (66)
script/DeployAllAndSetup.s.sol (2)
59-75: Add comprehensive error handling for deployment failures.While the script handles address mismatches, it could benefit from additional error handling:
- Check contract code size after deployment to verify successful deployment
- Add try-catch blocks for deployment operations
- Add revert messages for deployment failures
Example implementation:
if (EXPECTED_IMPLEMENTATION.code.length == 0) { + try { implementation = new ModularEtherspotWallet{salt: SALT}(); + if (address(implementation).code.length == 0) { + revert("Deployment failed: No code at implementation address"); + } if (address(implementation) != EXPECTED_IMPLEMENTATION) { revert("Unexpected wallet implementation address!!!"); } else { console2.log( "Wallet implementation deployed at address", address(implementation) ); } + } catch Error(string memory reason) { + revert(string.concat("Deployment failed: ", reason)); + } catch { + revert("Deployment failed: Unknown error"); + } }Also applies to: 80-99, 104-119, 124-145
44-44: Consider making stake amount and delay configurable.The current implementation uses hardcoded values:
FACTORY_STAKE = 1e16(0.01 ETH) might be too low for production- Unstake delay of 86400 seconds (1 day) might need adjustment per network
Consider making these values configurable:
-uint256 public constant FACTORY_STAKE = 1e16; +uint256 public immutable FACTORY_STAKE; +uint256 public immutable UNSTAKE_DELAY; + +constructor(uint256 factoryStake, uint256 unstakeDelay) { + FACTORY_STAKE = factoryStake; + UNSTAKE_DELAY = unstakeDelay; +}Then update the staking call:
-factory.addStake{value: FACTORY_STAKE}(address(entryPoint), 86400); +factory.addStake{value: FACTORY_STAKE}(address(entryPoint), UNSTAKE_DELAY);Also applies to: 150-156
src/common/Structs.sol (1)
49-58: Add NatSpec documentation for the ResourceLock struct.While the struct's fields are well-defined, it lacks the comprehensive NatSpec documentation present in other structs. Consider adding documentation to maintain consistency and improve code readability.
Add the following documentation:
// ResourceLockValidator +/// @title ResourceLock +/// @notice Struct containing data for resource locking +/// @dev Used to manage and validate resource locks across chains struct ResourceLock { uint256 chainId; address smartWallet; address sessionKey; uint48 validAfter; uint48 validUntil; TokenData[] tokenData; uint256 nonce; }src/utils/ERC7579HookBase.sol (1)
1-6: Consider pinning or updating the Solidity version.
Currently, the contract specifiespragma solidity ^0.8.23;. Ensure you are on a version that is stable and has no known security issues. If the environment supports a newer patch release (e.g., 0.8.20+), consider upgrading to benefit from available compiler fixes and enhancements.src/interfaces/ICredibleAccountModule.sol (1)
1-3: Review Solidity version pinning.
As with other files, confirm thatpragma solidity 0.8.23;is aligned with the project's overall version strategy and free of known issues.src/modules/hooks/HookMultiPlexer.sol (7)
20-25: Leverage inherited components effectively.By inheriting both
ERC7484RegistryAdapterandTrustedForwarder, this contract benefits from registry checks and meta-transaction support. Verify that you really need both functionalities in this single contract to keep the module lean.
26-28: Consolidate library usage for clarity.The widespread usage of
HookMultiPlexerLib,LibSort, and other libraries is comprehensive. Ensure that developers are aware of each library’s responsibilities to mitigate confusion about which utilities belong to which library.
197-217: Optimize large-scale hook retrieval.
getHooksmerges multiple arrays and sorts them, which can be costly if many hooks exist. Consider caching or more efficient lookups to reduce repeated sorting overhead for large sets.
266-288: Potential duplication in sig hooks.
addSigHookcallspushon the array with no check for duplicates beyond the registry attestation. If the same hook is added repeatedly for the same signature, the array can grow unnecessarily.
342-391: Robust transaction gating viapreCheck.
preCheckmerges subHooks from various arrays, sorts them, and calls each subHook'spreCheckSubHook. The sorting ensures uniqueness but can be expensive if many hooks exist. Consider a less expensive approach if the same subHooks get frequently re-run.
424-435: Private accessor naming convention.
$getConfigis used internally for convenience, but the leading$can be unconventional. If you want to unify naming across your codebase, consider a more standard naming like_getConfig.
464-466: Semantic versioning.
version()returning1.0.0is straightforward, but consider implementing semantic versioning or a dynamic version if multi-release changes are expected, to help external integrators track updates.src/libraries/HookMultiPlexerLib.sol (3)
22-25: Maintain descriptive error messages.
SubHookPreCheckErrorandSubHookPostCheckErrorindicate which hook failed, but not why. If debugging on chain is required, consider including a reason string for clarity.
115-149: Efficient array joining.
join(address[] memory a, address[] memory b)inlines an approach to extend arrays in memory. Ensure no out-of-bounds concerns for large arrays.
396-442: Batch execution hooking.
appendExecutionHookelegantly aggregates multiple sub-hooks from batch calls. Consider whether the appended hooks might get re-sorted excessively in the main contract, and if so, explore ways to unify the approach for performance.test/ModularTestBase.sol (2)
120-186: Comprehensive environment setup in_testInit.The
_testInitfunction properly sets up mocks, tokens, and the primary wallet. The coverage is broad. Ensure that each segment is needed for core tests, or consider splitting the environment build into smaller, more specialized methods.
201-209: Token distribution in_createUser.Assigning each user 100 ether and 100 tokens is suitable for general testing. Confirm that subsequent tests don’t require alternative initial balances.
test/modules/ERC1155FallbackHandler/concrete/ERC1155FallbackHandler.t.sol (3)
29-58: Installation test covers essential event emission
The test checks for event emission and uses_installModulewith pre-encoded arguments. This is good for verifying success paths. Consider also testing intermediate states, such as partial or invalid encoding, if not already covered elsewhere.
303-326:test_receiveERC1155– verifying direct token transfer
Good coverage for checking direct transfers from an external account to the modular wallet. Also, ensure you test any overlap with other fallback handlers or potential concurrency issues if batches are executed in parallel.
482-529: Selecting distinct selectors for separate tokens
Installing fallback handlers with different selectors for each token is a clean approach to modularizing calls. Be mindful that expansions (e.g., additional tokens or advanced usage) might require re-installation or more flexible fallback resolution.src/modules/validators/CredibleAccountModule.sol (1)
241-310: Module type checks and onInstall/onUninstall
The approach to verifying correct installation ensures hooking viahookMultiPlexer. Consider logging or enumerating installed modules to facilitate better debuggability.test/modules/SessionKeyValidator/fuzz/SessionKeyValidator.t.sol (3)
18-24: Room for additional negative tests
The event checks and setup logic are thorough. Consider adding negative test coverage for invalid session keys or missing session data, if not covered otherwise.
209-233: FuzzedtestFuzz_getSessionKeysByWallet
The incremental approach to adding multiple session keys appears robust. Carefully confirm that session key indexing is correct after each enablement, especially if keys are removed or rotated in other tests.
783-804:testFuzz_updateUsesensures decrement logic
Good approach verifying that permission usage is decremented. Also ensure coverage for edge outcomes (e.g., usage left is 1 or 0) to confirm the correct transition.src/interfaces/IHookLens.sol (1)
3-5: Add NatSpec documentation for better code clarity.Consider adding NatSpec documentation to describe the interface's purpose and function behavior:
+/// @title IHookLens +/// @notice Interface for retrieving the currently active hook interface IHookLens { + /// @notice Returns the address of the currently active hook + /// @return hook The address of the active hook function getActiveHook() external view returns (address hook); }src/utils/ProofVerifier.sol (1)
5-6: Fix @inheritdoc usage.The @inheritdoc tag is used incorrectly. Either:
- Add NatSpec documentation in the interface, or
- Remove the @inheritdoc tag and add direct documentation
src/interfaces/IProofVerifier.sol (1)
4-9: Enhance documentation with proof format details.The interface is well-documented, but could benefit from additional details about:
- Expected proof format/structure
- Verification failure conditions
- Example usage
/// @title IProofVerifier /// @author Etherspot /// @notice Interface for the contract with verification function to verify proof interface IProofVerifier { + /// @notice Verifies the provided proof + /// @param proof The proof data in bytes format + /// @return True if the proof is valid, false otherwise + /// @dev The proof should be formatted as [format details here] function verifyProof(bytes calldata proof) external view returns (bool); }src/common/Enums.sol (2)
4-12: Add documentation for ComparisonRule enum values.Consider adding NatSpec documentation to explain each comparison rule's purpose and usage:
-// SessionKeyValidator +/// @notice Comparison rules used by SessionKeyValidator enum ComparisonRule { + /// @notice Less than comparison (<) LESS_THAN, + /// @notice Less than or equal comparison (<=) LESS_THAN_OR_EQUAL, + /// @notice Equality comparison (==) EQUAL, + /// @notice Greater than or equal comparison (>=) GREATER_THAN_OR_EQUAL, + /// @notice Greater than comparison (>) GREATER_THAN, + /// @notice Not equal comparison (!=) NOT_EQUAL }
14-21: Add documentation for HookType enum values.Consider adding NatSpec documentation to explain each hook type's purpose and usage:
-// HookMultiplexer +/// @notice Hook types supported by HookMultiplexer enum HookType { + /// @notice Global hooks that apply to all operations GLOBAL, + /// @notice Hooks specific to delegatecall operations DELEGATECALL, + /// @notice Hooks for operations involving value transfer VALUE, + /// @notice Hooks based on function signatures SIG, + /// @notice Hooks based on target address and function signature TARGET_SIG }src/interfaces/IResourceLockValidator.sol (1)
6-13: Add NatSpec documentation for the interface and events.Consider adding comprehensive NatSpec documentation to improve code readability and maintainability:
+/// @title IResourceLockValidator - Interface for resource lock validation +/// @notice This interface extends IValidator to provide resource locking capabilities interface IResourceLockValidator is IValidator { /*////////////////////////////////////////////////////////////// EVENTS //////////////////////////////////////////////////////////////*/ + /// @notice Emitted when a validator is enabled for a smart contract wallet + /// @param scw The address of the smart contract wallet + /// @param owner The address of the wallet owner event RLV_ValidatorEnabled(address indexed scw, address indexed owner); + /// @notice Emitted when a validator is disabled for a smart contract wallet + /// @param scw The address of the smart contract wallet event RLV_ValidatorDisabled(address indexed scw); }src/utils/ERC7484RegistryAdapter.sol (1)
6-20: Add NatSpec documentation for the contract.Consider adding comprehensive NatSpec documentation to improve code readability and maintainability:
+/// @title ERC7484RegistryAdapter - Abstract contract for ERC7484 registry integration +/// @notice This contract provides base functionality for interacting with an ERC7484 registry +/// @dev Inherit from this contract to implement registry-specific functionality abstract contract ERC7484RegistryAdapter { // registry address IERC7484 public immutable REGISTRY;src/test/mocks/MockModule.sol (1)
6-20: Enhance mock implementation for better testing coverage.Consider these improvements to make the mock more useful for testing:
- Add NatSpec documentation
- Add revert cases
- Add state tracking for function calls
+/// @title MockModule - Mock implementation of IModule for testing +/// @dev This contract provides a basic mock with configurable behaviors contract MockModule is IModule { + // Track function calls for testing + bool public installCalled; + bool public uninstallCalled; + mapping(address => bool) public initializationState; + + // Configurable return values + bool public moduleTypeResponse = true; + function isModuleType(uint256) external view returns (bool) { - return true; + return moduleTypeResponse; } - function onInstall(bytes calldata) external {} + function onInstall(bytes calldata) external { + installCalled = true; + } - function onUninstall(bytes calldata) external {} + function onUninstall(bytes calldata) external { + uninstallCalled = true; + } function isInitialized(address smartAccount) external view returns (bool) { - return false; + return initializationState[smartAccount]; } + // Configuration functions for testing + function setModuleTypeResponse(bool response) external { + moduleTypeResponse = response; + } + + function setInitializationState(address smartAccount, bool state) external { + initializationState[smartAccount] = state; + } + receive() external payable {} }src/interfaces/IHookMultiplexer.sol (1)
6-10: Enhance interface documentation.The NatSpec documentation could be improved by adding
@paramand@returndescriptions for each function.test/wallet/ModularEtherspotWallet/utils/ModularEtherspotWalletTestUtils.sol (1)
28-32: Add input validation to _addGuardians function.The function should validate that the input wallet address is not zero and that guardian public keys are valid.
function _addGuardians(ModularEtherspotWallet _scw) internal { + require(address(_scw) != address(0), "Invalid wallet address"); + require(guardian1.pub != address(0), "Invalid guardian1 address"); + require(guardian2.pub != address(0), "Invalid guardian2 address"); + require(guardian3.pub != address(0), "Invalid guardian3 address"); _scw.addGuardian(guardian1.pub); _scw.addGuardian(guardian2.pub); _scw.addGuardian(guardian3.pub); }src/test/mocks/MockHook.sol (2)
2-2: Update Solidity version for consistency.The file uses ^0.8.21 while other files use ^0.8.23. Consider updating for consistency across the codebase.
-pragma solidity ^0.8.21; +pragma solidity ^0.8.23;
21-32: Extract magic strings into constants.The strings "revert", "revertPost", and "success" should be defined as constants to improve maintainability and reduce the risk of typos.
+ string constant REVERT_MESSAGE = "revert"; + string constant REVERT_POST_MESSAGE = "revertPost"; + string constant SUCCESS_MESSAGE = "success"; if ( - mode == ModeSelector.wrap(bytes4(keccak256(abi.encode("revert")))) + mode == ModeSelector.wrap(bytes4(keccak256(abi.encode(REVERT_MESSAGE)))) ) { - revert("revert"); + revert(REVERT_MESSAGE); } else if ( mode == - ModeSelector.wrap(bytes4(keccak256(abi.encode("revertPost")))) + ModeSelector.wrap(bytes4(keccak256(abi.encode(REVERT_POST_MESSAGE)))) ) { - hookData = abi.encode("revertPost"); + hookData = abi.encode(REVERT_POST_MESSAGE); } else { - hookData = abi.encode("success"); + hookData = abi.encode(SUCCESS_MESSAGE); }src/interfaces/IERC7484.sol (2)
2-2: Specify exact Solidity version.Consider using a specific version instead of ^0.8.0 to ensure consistent behavior across compilations.
-pragma solidity ^0.8.0; +pragma solidity ^0.8.23;
10-22: Add NatSpec documentation for check functions.The check functions lack proper documentation explaining their purpose, parameters, and return values.
src/test/mocks/MockRegistry.sol (2)
11-13: Consider using a custom error with a descriptive message.Replace the basic
revert()with a custom error to provide more context about why the check failed.+ error InvalidModule(address module); function check(address module) external view { if (module == address(0x420)) { - revert(); + revert InvalidModule(module); } }
1-70: Reduce code duplication in check functions.The same validation logic is repeated across multiple check functions. Consider extracting the common logic into a private function.
+ function _checkModule(address module) private pure { + if (module == address(0x420)) { + revert InvalidModule(module); + } + } + function check(address module) external view { - if (module == address(0x420)) { - revert(); - } + _checkModule(module); } // Apply similar changes to other check functionssrc/utils/TrustedForwarder.sol (1)
51-67: Enhance assembly block safety.The assembly block could benefit from additional safety checks and clearer variable naming.
function _getAccount() internal view returns (address account) { account = msg.sender; address _account; address forwarder; - if (msg.data.length >= 40) { + uint256 minDataLength = 40; + if (msg.data.length >= minDataLength) { assembly { + let dataEnd := sub(calldatasize(), 1) + if lt(dataEnd, minDataLength) { revert(0, 0) } _account := shr(96, calldataload(sub(calldatasize(), 20))) forwarder := shr(96, calldataload(sub(calldatasize(), 40))) }test/harnesses/CredibleAccountModuleHarness.sol (1)
9-12: Add constructor parameter validation and documentation.The constructor should validate input parameters and include documentation about test-specific behavior.
+ /// @notice Test harness for CredibleAccountModule + /// @dev Exposes internal functions for testing purposes + /// @param _proofVerifier Address of the proof verifier contract + /// @param _hookMultiPlexer Address of the hook multiplexer contract constructor( address _proofVerifier, address _hookMultiPlexer - ) CredibleAccountModule(_proofVerifier, _hookMultiPlexer) {} + ) CredibleAccountModule(_proofVerifier, _hookMultiPlexer) { + require(_proofVerifier != address(0), "Invalid proof verifier"); + require(_hookMultiPlexer != address(0), "Invalid hook multiplexer"); + }src/ERC7579/core/HookManager.sol (1)
30-34: Document the msg.value addition in preCheck.The addition of msg.value to preCheck parameters should be documented to explain the change.
+ // Pass msg.value to allow hooks to validate transaction value bytes memory hookData = IHook(hook).preCheck( msg.sender, msg.value, msg.data );test/wallet/ModularEtherspotWalletFactory/fuzz/ModularEtherspotWalletFactory.t.sol (1)
27-67: Enhance test coverage with additional assertions and edge cases.While the test verifies basic account creation, consider adding:
- Assertions to verify the initialized state of validators, executors, and hooks
- Negative test cases with invalid configurations
- Tests for boundary conditions in the initialization parameters
test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol (2)
72-86: Make test data generation more flexible.The
_generateResourceLockfunction uses hardcoded values which could make tests brittle. Consider:
- Parameterizing values like chainId, timestamps, and nonce
- Adding helper functions to generate dynamic test data
- Using block.timestamp for time-based values
-function _generateResourceLock(address _scw, address _sk) internal view returns (ResourceLock memory) { +function _generateResourceLock( + address _scw, + address _sk, + uint256 _chainId, + uint256 _validAfter, + uint256 _validUntil, + uint256 _nonce +) internal view returns (ResourceLock memory) { TokenData[] memory td = new TokenData[](2); td[0] = TokenData({token: address(usdt), amount: 100}); td[1] = TokenData({token: address(dai), amount: 200}); ResourceLock memory rl = ResourceLock({ - chainId: 42161, + chainId: _chainId, smartWallet: _scw, sessionKey: _sk, - validAfter: 1732176210, - validUntil: 1732435407, + validAfter: _validAfter, + validUntil: _validUntil, tokenData: td, - nonce: 14 + nonce: _nonce }); return rl; }
18-26: Consider adding validation in withRequiredModules modifier.The modifier installs multiple modules but doesn't verify if the installation was successful. Consider adding assertions to validate the installation state.
test/modules/CredibleAccountModule/utils/CredibleAccountModuleTestUtils.sol (3)
31-36: Consider adding validation for token amounts.The test variables for token amounts use hardcoded decimals which might not match all ERC20 tokens.
Consider making the amounts configurable based on token decimals:
- uint256[3] internal amounts = [100e6, 200e18, 300e18]; + uint256[3] internal amounts; + + function _setAmounts() internal { + amounts[0] = 100 * (10 ** usdc.decimals()); + amounts[1] = 200 * (10 ** dai.decimals()); + amounts[2] = 300 * (10 ** usdt.decimals()); + }
73-88: Consider adding chain ID validation.The
_createResourceLockfunction uses a hardcoded chain ID.Consider making the chain ID configurable:
- chainId: 42161, // Arbitrum + chainId: block.chainid,
125-145: Consider adding gas optimization for batch operations.The
_claimTokensBySolverfunction could be optimized for gas usage.Consider using a more gas-efficient approach for batch operations:
- Execution[] memory batch = new Execution[](3); - batch[0] = Execution({target: address(usdc), value: 0, callData: usdcData}); - batch[1] = Execution({target: address(dai), value: 0, callData: daiData}); - batch[2] = Execution({target: address(usdt), value: 0, callData: usdtData}); + uint256 batchLength = 0; + for (uint256 i; i < 3; i++) { + if (amounts[i] > 0) batchLength++; + } + Execution[] memory batch = new Execution[](batchLength); + uint256 j; + if (amounts[0] > 0) batch[j++] = Execution({target: address(usdc), value: 0, callData: usdcData}); + if (amounts[1] > 0) batch[j++] = Execution({target: address(dai), value: 0, callData: daiData}); + if (amounts[2] > 0) batch[j++] = Execution({target: address(usdt), value: 0, callData: usdtData});test/wallet/ModularEtherspotWalletFactory/concrete/ModularEtherspotWalletFactory.t.sol (2)
33-72: Consider adding revert test cases.The test for account creation could include cases where creation should fail.
Consider adding tests for failure scenarios:
+ function test_createAccount_RevertsOnInvalidInitCode() public { + bytes memory invalidInitCode = hex"deadbeef"; + vm.startPrank(eoa.pub); + vm.expectRevert("Invalid init code"); + factory.createAccount({salt: TEST_SALT, initCode: invalidInitCode}); + vm.stopPrank(); + }
127-177: Consider adding gas usage assertions.The event emission test could include gas usage checks to prevent regressions.
Consider adding gas usage assertions:
+ uint256 gasBefore = gasleft(); scw = ModularEtherspotWallet( payable( factory.createAccount({salt: TEST_SALT, initCode: initCode}) ) ); + uint256 gasUsed = gasBefore - gasleft(); + assertLt(gasUsed, 1000000, "Gas usage too high");test/modules/CredibleAccountModule/fuzz/CredibleAccountModule.t.sol (3)
34-78: Consider adding invariant checks for session key data.The session key enablement test could include additional invariant checks.
Consider adding these invariant checks:
+ // Check invariants + assertGt(retrievedData.validUntil, retrievedData.validAfter, "Invalid time range"); + for (uint256 i; i < lockedTokens.length; ++i) { + assertGt(lockedTokens[i].lockedAmount, 0, "Invalid locked amount"); + assertLt(lockedTokens[i].claimedAmount, lockedTokens[i].lockedAmount, "Invalid claimed amount"); + }
160-161: Remove debug logging in tests.Debug logging should not be present in production test code.
- console2.log("sessionKeyData.validUntil", sessionKeyData.validUntil);
169-187: Strengthen session key validation test.The session key validation test could be more comprehensive.
Consider adding more specific test cases:
+ // Test with expired session + vm.warp(validUntil + 1); + assertFalse(cam.validateSessionKeyParams(sessionKey.pub, op), "Should fail for expired session"); + + // Test with future session + vm.warp(validAfter - 1); + assertFalse(cam.validateSessionKeyParams(sessionKey.pub, op), "Should fail for future session");src/access/AccessController.sol (2)
91-94: Consider adding a more descriptive error message.The error handling for removing an owner could be more informative by including the reason in the error name, similar to how other errors are named (e.g.,
WalletNeedsOwner).- if (ownerCount <= 1) { - revert WalletNeedsOwner(); - } + if (ownerCount <= 1) { + revert RemovingLastOwner(); + }
12-14: Consider adding documentation for state variables.The state variables lack documentation explaining their purpose and constraints.
/// State Variables + /// @dev Multiplier used for percentage calculations uint128 constant MULTIPLY_FACTOR = 1000; + /// @dev Threshold for guardian approval (60%) uint16 constant SIXTY_PERCENT = 600; + /// @dev Default timelock duration for proposals uint24 constant INITIAL_PROPOSAL_TIMELOCK = 24 hours;test/modules/ResourceLockValidator/concrete/ResourceLockValidator.t.sol (1)
1-553: Consider adding test cases for concurrent resource locks.The test suite is comprehensive but could benefit from additional test cases:
- Multiple concurrent resource locks for the same session key
- Resource lock expiration scenarios
- Race conditions in resource lock validation
test/modules/ERC20SessionKeyValidator/concrete/ERC20SessionKeyValidator.t.sol (2)
177-191: Fix inconsistent test function naming.The test function name
test_fail_enableSessionKey_invalidValidUntildoesn't follow the established naming convention. Other similar test functions usetest_enableSessionKey_RevertIf_*.- function test_fail_enableSessionKey_invalidValidUntil() public { + function test_enableSessionKey_RevertIf_InvalidValidUntil() public {
557-558: Fix typo in test function name.The test function name has a typo:
ReveertIfshould beRevertIf.- function test_discardCurrentProposal_ReveertIf_NotOwnerOrGuardianOrSelf() public { + function test_discardCurrentProposal_RevertIf_NotOwnerOrGuardianOrSelf() public {test/modules/CredibleAccountModule/concrete/CredibleAccountModule.t.sol (2)
535-535: Hardcoded session key address in test data.The test uses a hardcoded session key address which might make the test brittle.
Consider using a dynamic address generation or existing test variables for session keys:
-sessionKey: 0xB071527c3721215A46958d172A42E7E3BDd1DF46, +sessionKey: otherSessionKey.pub,
746-777: Test case could be more comprehensive.The test for Uniswap V2 swap only checks for failure when there's insufficient balance.
Consider adding positive test cases to verify successful swaps when sufficient balance is available and proper approvals are set.
README.md (1)
18-27: Consider adding version information for Soldeer.The new Dependencies section introduces Soldeer but doesn't specify the required version.
Consider adding the minimum required version of Soldeer and installation instructions if needed:
## Dependencies Uses Solidity native dependency manager [Soldeer](https://soldeer.xyz/) as package manager. +Requires Soldeer v1.0.0 or higher. To install Soldeer: + +```bash +cargo install soldeer +``` + To install dependencies: ```bash forge soldeer install</blockquote></details> <details> <summary>docs/RESOURCE_LOCK_VALIDATOR.md (2)</summary><blockquote> `35-43`: **Add example signature formats for validation methods.** The validation methods section would benefit from concrete examples of valid signature formats. Consider adding code examples showing both standard signatures and merkle proof formats: ```solidity // Standard 65-byte signature bytes signature = hex"1234..."; // r, s, v components // Merkle proof packed signature bytes signature = abi.encodePacked( bytes32 resourceLockHash, bytes32[] proof, bytes standardSignature );
92-98: Enhance security considerations section.The security section could be more detailed about potential risks and mitigations.
Consider adding:
- Recommendations for merkle tree depth limits
- Guidelines for timestamp validations
- Best practices for nonce management
- Potential front-running considerations
foundry.toml (2)
13-20: Updated Remappings Configuration:
The remappings now explicitly define paths for various dependencies, including:
@openzeppelin/contractspointing todependencies/@openzeppelin-contracts-5.2.0/ERC4337pointing todependencies/eth-infinitism-account-abstraction-0.7/contracts/ERC7579pointing tosrc/ERC7579/forge-stdpointing todependencies/forge-std-1.9.6/src/soladyandsolarraysimilarly defined.Please verify that each remapping correctly reflects the folder structure established by your dependency manager (Soldeer) and that version-specific directory naming (e.g., appending
-5.2.0or-0.7) is handled as expected. Additionally, confirm that the mapping forERC7579(located insrc/ERC7579/) is the intended design for in-project modules versus external dependencies.
34-39: Explicit Dependencies Declaration:
The[dependencies]section now lists the external dependencies along with their versions and, in the case ofsolarray, the git source and revision. This explicit declaration improves clarity over dependency management.A couple of points to check:
- Ensure that the directory names inferred for each dependency (e.g.,
eth-infinitism-account-abstraction-0.7) match what is expected by the remappings defined above.- Verify that any naming conventions between the dependencies and their remapping targets are correctly aligned.
If there’s any ambiguity, consider adding a post-dependency-install verification step to confirm that the generated directory names match your remapping configuration.
📜 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 (67)
.gitignore(1 hunks).gitmodules(0 hunks)README.md(1 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)remappings.txt(0 hunks)script/DeployAllAndSetup.s.sol(1 hunks)src/ERC7579/core/HookManager.sol(1 hunks)src/ERC7579/interfaces/IERC7579Module.sol(1 hunks)src/ERC7579/test/Bootstrap.t.sol(1 hunks)src/access/AccessController.sol(10 hunks)src/common/Constants.sol(1 hunks)src/common/Enums.sol(2 hunks)src/common/Structs.sol(2 hunks)src/interfaces/ICredibleAccountModule.sol(1 hunks)src/interfaces/IERC20SessionKeyValidator.sol(1 hunks)src/interfaces/IERC7484.sol(1 hunks)src/interfaces/IHookLens.sol(1 hunks)src/interfaces/IHookMultiplexer.sol(1 hunks)src/interfaces/IModularEtherspotWallet.sol(1 hunks)src/interfaces/IProofVerifier.sol(1 hunks)src/interfaces/IResourceLockValidator.sol(1 hunks)src/interfaces/ISessionKeyValidator.sol(1 hunks)src/libraries/ErrorsLib.sol(0 hunks)src/libraries/HookMultiPlexerLib.sol(1 hunks)src/modules/fallbacks/ERC1155FallbackHandler.sol(1 hunks)src/modules/hooks/HookMultiPlexer.sol(1 hunks)src/modules/hooks/ModuleIsolationHook.sol(3 hunks)src/modules/validators/CredibleAccountModule.sol(1 hunks)src/modules/validators/ERC20SessionKeyValidator.sol(1 hunks)src/modules/validators/MultipleOwnerECDSAValidator.sol(1 hunks)src/modules/validators/ResourceLockValidator.sol(1 hunks)src/modules/validators/SessionKeyValidator.sol(1 hunks)src/test/TestExecutor.sol(1 hunks)src/test/mocks/MockHook.sol(1 hunks)src/test/mocks/MockModule.sol(1 hunks)src/test/mocks/MockRegistry.sol(1 hunks)src/test/mocks/MockTarget.sol(1 hunks)src/utils/ERC4337Utils.sol(1 hunks)src/utils/ERC7484RegistryAdapter.sol(1 hunks)src/utils/ERC7579HookBase.sol(1 hunks)src/utils/ProofVerifier.sol(1 hunks)src/utils/TrustedForwarder.sol(1 hunks)src/wallet/ModularEtherspotWallet.sol(1 hunks)test/ModularTestBase.sol(1 hunks)test/TestAdvancedUtils.t.sol(0 hunks)test/harnesses/CredibleAccountModuleHarness.sol(1 hunks)test/modules/CredibleAccountModule/concrete/CredibleAccountModule.t.sol(1 hunks)test/modules/CredibleAccountModule/fuzz/CredibleAccountModule.t.sol(1 hunks)test/modules/CredibleAccountModule/utils/CredibleAccountModuleTestUtils.sol(1 hunks)test/modules/ERC1155FallbackHandler/concrete/ERC1155FallbackHandler.t.sol(2 hunks)test/modules/ERC20SessionKeyValidator.t.sol(0 hunks)test/modules/ERC20SessionKeyValidator/concrete/ERC20SessionKeyValidator.t.sol(1 hunks)test/modules/ResourceLockValidator/concrete/ResourceLockValidator.t.sol(1 hunks)test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol(1 hunks)test/modules/SessionKeyValidator/fuzz/SessionKeyValidator.t.sol(30 hunks)test/modules/SessionKeyValidator/utils/SessionKeyTestUtils.sol(4 hunks)test/wallet/ModularEtherspotWallet.t.sol(0 hunks)test/wallet/ModularEtherspotWallet/concrete/ModularEtherspotWallet.t.sol(1 hunks)test/wallet/ModularEtherspotWallet/utils/ModularEtherspotWalletTestUtils.sol(1 hunks)test/wallet/ModularEtherspotWalletFactory.t.sol(0 hunks)test/wallet/ModularEtherspotWalletFactory/concrete/ModularEtherspotWalletFactory.t.sol(1 hunks)test/wallet/ModularEtherspotWalletFactory/fuzz/ModularEtherspotWalletFactory.t.sol(1 hunks)
💤 Files with no reviewable changes (11)
- lib/solady
- lib/openzeppelin-contracts
- .gitmodules
- src/libraries/ErrorsLib.sol
- remappings.txt
- lib/forge-std
- test/wallet/ModularEtherspotWallet.t.sol
- test/TestAdvancedUtils.t.sol
- test/wallet/ModularEtherspotWalletFactory.t.sol
- test/modules/ERC20SessionKeyValidator.t.sol
- lib/account-abstraction
✅ Files skipped from review due to trivial changes (11)
- src/utils/ERC4337Utils.sol
- src/interfaces/IERC20SessionKeyValidator.sol
- src/modules/fallbacks/ERC1155FallbackHandler.sol
- src/interfaces/IModularEtherspotWallet.sol
- src/interfaces/ISessionKeyValidator.sol
- src/modules/validators/MultipleOwnerECDSAValidator.sol
- src/ERC7579/test/Bootstrap.t.sol
- src/common/Constants.sol
- src/modules/validators/ERC20SessionKeyValidator.sol
- src/modules/validators/SessionKeyValidator.sol
- src/wallet/ModularEtherspotWallet.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 (82)
script/DeployAllAndSetup.s.sol (2)
8-8: LGTM! Import path updated for Soldeer integration.The import path change from relative to absolute aligns with the PR's objective of transitioning to the Soldeer package manager.
34-43:❓ Verification inconclusive
Verify the hardcoded expected addresses.
These addresses are crucial for deterministic deployment. Please ensure they are correctly calculated for the target network.
Run the following script to verify the addresses:
🏁 Script executed:
#!/bin/bash # Description: Verify that the expected addresses match the CREATE2 computation. # Search for any existing deployments or address computations rg -A 2 'Expected.*address.*0x[a-fA-F0-9]{40}'Length of output: 48
Address Verification for Deterministic Deployment
The search command did not return any evidence that the expected addresses are computed using CREATE2. Please manually verify that the addresses defined in
script/DeployAllAndSetup.s.solon lines 34–43:
DEPLOYER: 0x09FD4F6088f2025427AB1e89257A44747081Ed59EXPECTED_IMPLEMENTATION: 0x339eAB59e54fE25125AceC3225254a0cBD305A7bEXPECTED_FACTORY: 0x2A40091f044e48DEB5C0FCbc442E443F3341B451EXPECTED_BOOTSTRAP: 0x0D5154d7751b6e2fDaa06F0cC9B400549394C8AAEXPECTED_MULTIPLE_OWNER_ECDSA_VALIDATOR: 0x0740Ed7c11b9da33d9C80Bd76b826e4E90CC1906...match the derivations (e.g., via CREATE2) expected for your target network. If these values are updated elsewhere or need recomputation, please address that accordingly.
test/modules/SessionKeyValidator/utils/SessionKeyTestUtils.sol (11)
5-9: Imports appear consistent with new architecture.
The updated imports from Solady, ERC4337, and ERC7579 libraries reflect the shift toward a more modular approach for user operations and session handling. No immediate issues found.
14-15: New test contract imports are aligned with expanded testing requirements.
The introductions ofTestERC721andTestUniswapV3suggest broader test coverage. Ensure these mocked contracts accurately represent the real-world behavior being tested.
16-16: Adoption ofModularTestBasealigns well with the PR’s overarching changes.
Switching toModularTestBaselikely centralizes common test functionality, fostering a more organized test suite.
18-18: Inheritance fromModularTestBaseadheres to the new modular testing design.
This reflects the larger transition seen in the PR, removing the previousTestAdvancedUtilsin favor of a more general and extensible base.
29-30: Additional internal references for ERC721 and Uniswap test instances.
Declaring these new contract instances helps broaden the test scenarios. Verify they are properly utilized in subsequent tests to maintain full coverage.
38-53: NewvalidatorInstalledmodifier for streamlined module installation flow.
Encapsulating the module installation and environment setup code into a modifier is a good approach. Just be cautious about re-entrancy or side effects if it’s used multiple times in a single test file.Would you like to confirm usage across the suite to ensure this modifier doesn’t inadvertently alter test assumptions?
60-60: Refined_testInit()call and new test contract creations.
The switch to_testInit()and creation ofcryptoPunk/uniswapV3appear consistent with your modular architecture. Ensure these additions are reflected in newly added or updated tests.Also applies to: 64-65
68-69: Refactored_getSessionKeyAndPermissionsto handle aUserstruct.
Migrating to aUserstruct with_sessionKey.pubis a clearer pattern. Confirm thatalice.pubis declared and properly initialized elsewhere to avoid referencing an undefined test user.Also applies to: 82-82
99-108: Helper_getExecutionValidation()is straightforward.
Provides a clean interface for settingvalidAfterandvalidUntilfields without side effects. Looks correct.
111-111:_setupSingleUserOpmethod updated for new user operation structure.
Encoding logic for single operation flows is neatly contained. Ensure thorough test coverage for signature creation and validity checks.Also applies to: 113-113, 116-133
136-152:_setupBatchUserOpmethod mirrors single-op logic for batch calls.
Adopting a parallel approach to single user operations enhances consistency and clarity. Implementation looks aligned with the new user op model.src/common/Structs.sol (2)
4-4: LGTM!The import statement is appropriate for a common file that uses multiple enums.
41-47: LGTM!The TokenData struct is well-documented and appropriately structured for storing token information.
src/utils/ERC7579HookBase.sol (3)
7-24: Well-structured hook interface integration forpreCheck.
The externalpreCheckfunction clearly delegates to the internal_preCheckmethod, promoting extensibility. Documentation comments are concise and align with the parameters.
26-35:postCheckmirrorspreCheckeffectively.
The externalpostCheckfunction provides symmetrical post-execution handling, ensuring that all pre-check data flows consistently into_postCheck.
36-63: Good use of internal virtual methods_preCheckand_postCheck.
Declaring_preCheckand_postCheckasinternal virtualenables child contracts to override them and implement custom logic. This maintains clean separation of concerns within the hook.src/interfaces/ICredibleAccountModule.sol (3)
9-72: Comprehensive event definitions for session key life cycle.
Defining separate events for installation, uninstallation, enabling, disabling, pausing, and unpausing facilitates robust tracking of session key states for off-chain indexers. The clarity in param naming is beneficial.
77-159: Clear function signatures for session key management.
Functions such asenableSessionKey,disableSessionKey, andvalidateSessionKeyParamsare well-documented. Ensure that implementing contracts properly handle all return conditions and edge cases (e.g., repeated enabling, disabling, or validating).
163-195:preCheckandpostCheckmethod definitions maintain compatibility with IHook.
The added methods align with the extended IHook interface. Ensure that the implementing contract consistently encodes and decodeshookDatafor correct usage in post-execution checks.src/modules/validators/ResourceLockValidator.sol (3)
1-39: Initialization mapping and struct usage look sound.
ThevalidatorStoragemapping properly tracks an owner's address and whether a validator is enabled. The use of a dedicatedValidatorStoragestruct helps keep stateful data organized.
53-67: Installation logic is straightforward but verify repeated installation edge cases.
TheonInstallfunction checks if the validator is already enabled and reverts if so, preventing accidental double-installs. When the validator is removed,onUninstallcleans up storage. Confirm your integration tests capture reinstall attempts or partial state updates.
139-217: Internal helper functions enhance clarity but watch for revert messages.
The_getResourceLockfunction reverts withRLV_OnlyCallTypeSingleif thecalltypeis not single, which is appropriate. Verify that revert messages are descriptive enough for debugging. Also, the_buildResourceLockHashapproach methodically captures all relevant fields, providing strong replay protection.src/modules/hooks/HookMultiPlexer.sol (9)
72-79: Validate hook type bounding logic.
onlySupportedHookTypeassumes that hook types up toHookType.TARGET_SIGare valid. If new hook types arise, adjusting this check might be overlooked. Consider more descriptive checks or enumerations.
96-147: Potential double initialization hazard.
onInstallreverts if the module is already initialized, which is good. However, it also invokesonInstallon each global subHook. Confirm that all subHooks can safely handle repeated calls. If not, consider a protective mechanism in subHooks or a stricter check here.
149-174: Ensure robust cleanup on uninstall.
onUninstallclears all hooks, but calls_removeHookonly forGLOBALhooks. Confirm that you intend to silently deleteVALUEorDELEGATECALLhooks without explicitly calling theironUninstallfunction. This can leave the subHook state out of sync if that logic is needed.
184-188: Straightforward initialization check.
isInitializedis well-defined for reading the module’s state. No concerns here.
235-256: Validate hook correctness before adding.
addHookensures the hook is attested in the registry, then callsonInstall. This is a good pattern. Consider also verifying that the hook is not already present, to avoid callingonInstallon duplicates.
300-309: Reentrancy consideration on external call.
_removeHookcallsonUninstallon the hook prior to removing it. If the hook externally calls back into this contract, it could produce unexpected states. Verify that it’s safe for hooks to reenter or that execution halts if reentry occurs.
392-422: Post-execution indexing approach is sound.
postCheckdecodes stored contexts and callspostCheckSubHook. This logic is straightforward. IfpreCheckorpostCheckexpansions are planned, ensure the data structure remains backward-compatible.
444-448: Module type check is concise.
isModuleTypedirectly returnstrueiftypeID == MODULE_TYPE_HOOK. This is simple and effective.
455-457: Name function clarity.
name()returns"HookMultiPlexer". This is standard practice for module introspection.src/libraries/HookMultiPlexerLib.sol (7)
1-3: Precise compiler version and licensing.Declaring
pragma solidity 0.8.23;and AGPL-3.0-only licensing is consistent with the rest of the project. No issues here.
39-58: Sensible subhook pre-check aggregation.Collecting contexts in
preCheckSubHooksis a robust approach. Keep memory usage in mind if subHooks can expand significantly.
70-91: Careful about nested external calls.
preCheckSubHookencodes the originalmsg.senderwithmsgDataand calls into the subHook. Confirm that subHooks are trusted or that re-entrancy cannot exploit intermediate states.
93-113: Post-check calls mirror pre-check.
postCheckSubHookusage is consistent withpreCheckSubHook. Confirm subHook code paths can handle partial failures or potential revert scenarios.
168-177: Sorted uniqueness enforced systematically.
requireSortedAndUniqueensures that hooks are pre-validated. This is important for stable merges. Using an error typeHooksNotSortedis helpful for debugging.
289-338: Inline assembly for savings.
decodeOnInstalluses assembly to decode arrays in a single pass, which is advanced and fosters efficiency. Thoroughly test edge cases (e.g., zero hooks) to confirm no memory misalignment.
340-380: Signature hooks store and cleanup.
storeSelectorHooksanddeleteHookssystematically manage the sig-based hooks. This approach is good, but watch for potential collisions in sorting or edge cases where no sig hooks exist.test/ModularTestBase.sol (7)
1-3: Consistent licensing strategy.Using MIT licensing for test utilities is consistent with open-source norms. No issues here.
48-73: Centralized references to system contracts.This contract references numerous modules (
MultipleOwnerECDSAValidator,ERC20SessionKeyValidator, etc.). The approach is cohesive, but double-check that you only include what’s actually tested to avoid unnecessary dependencies.
95-106: User structure for improved clarity.Defining a
Userstruct for storing addresses and private keys is a good pattern. This is straightforward for testing and debugging.
270-294: Initialize HookMultiPlexer in_createSCW.The contract uses a pre-set array of hooks (
globalHooks,valueHooks, etc.) before installation. The approach is thorough. Validate that newly added hook types in the future won’t break the current initialization procedure.
296-318: Module installation logic.
_installModuleusesmockExec.executeViaAccountto install modules. This is a good approach for testing. Keep in mind to thoroughly test revert scenarios or module collisions.
352-362: Hook management wrapper methods.
_installHookViaMultiplexerand_uninstallHookViaMultiplexerprovide good clarity in tests about how hooks are manipulated.
383-412: Merkle-based testing utilities.
getTestProofand_hashPairhelp you simulate valid/invalid proofs. If other tests or modules also need Merkle proofs, centralizing this logic was a good move.test/modules/ERC1155FallbackHandler/concrete/ERC1155FallbackHandler.t.sol (5)
5-10: Use of new imports for modular fallback handling
These import statements allow for hooking into the module management and fallback logic. Make sure all imported types and constants (e.g.,CALLTYPE_SINGLE) are indeed used throughout the test suite and remain in sync with their definitions in the referenced libraries.
13-24: Switch toModularTestBase
Inheriting fromModularTestBasecentralizes test utilities for modular contract testing. Verify that any replaced or removed helper logic is ported correctly, and confirm that shared setup routines are still valid in this new inheritance structure.
60-79: Multiple allowed callers
Allowing multiple token addresses in the array broadens the fallback handler’s scope. Ensure that other tests confirm no interference occurs between tokens, particularly in edge cases of partial batch transfers or unusual token logic.
100-126: Ensuring revert on repeated selector usage
The test effectively confirms that only one fallback handler can be installed per selector. This prevents collisions but also might block certain multi-scenario fallbacks. Consider whether advanced fallback chaining is needed.
218-244: Mismatch between allowed selector and actual minted calls
Reverting on the wrong selector ensures no accidental call acceptance. The test thoroughly checks the revert scenario, but keep an eye on potential conflicts with other fallback modules.src/modules/validators/CredibleAccountModule.sol (4)
1-19: New contract introducing session key and token locking logic
This contract centralizes session key management and locked token tracking. Ensure all library versions are pinned to prevent unexpected changes, given the reliance on external imports.
20-45: Extensive custom errors for module installation & session checks
Defining descriptive errors aids debugging. Verify error coverage with thorough tests, especially for partial or incomplete data (e.g., zero session keys, zero locked tokens).
46-75: Constructor validations
The constructor reverts if_proofVerifieror_hookMultiPlexeris zero, which is good for preventing misconfiguration. Double-check cross-contract references to ensure hooking logic is fully set upon deployment.
207-231:validateUserOp– primary gate to user operation
This function recovers the session key from the signature and checks the proof. Good layering withvalidateSessionKeyParams. However, ensure that any signature replays are invalidated by additional checks if needed.test/modules/SessionKeyValidator/fuzz/SessionKeyValidator.t.sol (3)
6-16: Integration ofPackedUserOperationwith SessionKey logic
Switching toTestUtilscentralizes repeated test patterns. Ensure that all new references toPackedUserOperationmap consistently to the original test approach (e.g., encoded call data structure).
55-73:setUpmethod andvalidatorInstalled
The newvalidatorInstalledmodifier ensures a base precondition. Confirm that older test cases without installation steps are updated or otherwise excluded so they won’t fail unexpectedly.
451-470: Removing session key permission
This test uses fuzzed indices for removing permissions—a solid strategy to discover off-by-one or memory shifting errors. No concerns noted, but consider verifying partial removal (where multiple permissions exist).src/interfaces/IResourceLockValidator.sol (1)
1-5: LGTM!The pragma version and import statement are correctly specified.
src/utils/ERC7484RegistryAdapter.sol (1)
1-5: LGTM!The pragma version and import statement are correctly specified.
src/test/mocks/MockTarget.sol (2)
1-8: LGTM!The state variables are correctly declared.
9-18: LGTM!The setter functions are implemented correctly.
src/test/mocks/MockRegistry.sol (1)
66-69: Add implementation for trustAttesters function.The empty implementation might not be suitable for all test scenarios. Consider adding basic validation or tracking of attesters.
function trustAttesters( uint8 threshold, address[] calldata attesters - ) external {} + ) external { + require(threshold <= attesters.length, "Invalid threshold"); + // Add logic to track trusted attesters if needed for tests + }src/test/TestExecutor.sol (1)
4-7: LGTM! Import paths have been standardized.The switch to absolute imports from the "ERC7579" directory improves maintainability and reduces the risk of path-related issues.
src/ERC7579/interfaces/IERC7579Module.sol (2)
82-84: LGTM! Added msg.value parameter to preCheck.The addition of
msgValueparameter enables hooks to validate and track value transfers, which is essential for comprehensive transaction validation.
87-87:❓ Verification inconclusive
Verify the impact of removing postCheck's return value.
The removal of
returns (bool success)from postCheck changes how errors are handled. Ensure all implementations are updated to use revert for error handling instead of returning false.
🏁 Script executed:
#!/bin/bash # Search for implementations of postCheck that might still return bool ast-grep --pattern 'function postCheck(bytes calldata $_) $_ returns (bool $_) { $$$ }'Length of output: 91
Verify that postCheck no longer returns a bool
Our automated searches (both ast-grep and an additional rg-based check) did not reveal any implementations ofpostCheckthat still specify areturns (bool). Please double-check that all implementations are updated to use revert for error handling instead of relying on a boolean return value.
🏁 Script executed:
#!/bin/bash # Re-run the search using a regex with rg to look for lingering bool return types in implementations of postCheck rg "function\s+postCheck\([^)]*\)\s+returns\s*\(bool" -t solLength of output: 90
Action Required: Verify Use of Revert for Error Handling
Our previous searches did not conclusively detect implementations ofpostCheckthat still return a bool. Please ensure that all implementations ofpostCheckno longer specify a return type (i.e.,returns (bool)) and instead use revert-based error handling. Manual verification may be needed to confirm that no implementations have been missed.
🏁 Script executed:
#!/bin/bash # Re-run the search for any lingering 'returns (bool ...)' in Solidity files rg -g '*.sol' "function\s+postCheck\([^)]*\)\s+returns\s*\(bool"Length of output: 66
Action Required: Verify the Updated Error Handling for postCheck
Our automated searches did not detect any implementations ofpostCheckthat still specify areturns (bool)signature. Please ensure that all implementations have been updated to remove the boolean return and now use reverts for signaling errors. Double-check any custom implementations or overrides to confirm consistency with the updated interface.src/modules/hooks/ModuleIsolationHook.sol (3)
9-11: LGTM! Import paths updated.The import paths have been updated to use absolute paths from the root, which improves maintainability.
47-51: LGTM! Function signature updated to match interface.The
preCheckfunction signature has been updated to include themsgValueparameter, maintaining consistency with the interface.
118-118: LGTM! Simplified postCheck function.The
postCheckfunction's return type has been removed since it wasn't being used, making the code more concise.src/access/AccessController.sol (1)
47-66: LGTM! Custom error declarations improve gas efficiency and error handling clarity.The custom error declarations are well-organized and provide clear error messages for different scenarios.
test/wallet/ModularEtherspotWallet/concrete/ModularEtherspotWallet.t.sol (1)
1-611: LGTM! Comprehensive test coverage.The test suite thoroughly covers:
- Module installation/uninstallation
- Transaction execution (single, batch, delegate calls)
- Owner and guardian management
- Error cases and edge conditions
test/modules/CredibleAccountModule/concrete/CredibleAccountModule.t.sol (1)
1-883: Well-structured and comprehensive test suite.The test file demonstrates good practices:
- Clear test organization with descriptive names
- Thorough coverage of success and failure cases
- Proper event verification
- Comprehensive validation of state changes
.gitignore (1)
22-23: LGTM! Appropriate gitignore entry for Soldeer.The addition of the
dependenciesdirectory to .gitignore is correct for projects using Soldeer as the dependency manager.README.md (1)
1-90: Well-structured documentation with clear instructions.The README provides clear installation steps and properly documents the new dependency management system.
🧰 Tools
🪛 LanguageTool
[style] ~35-~35: To form a complete sentence, be sure to include a subject.
Context: ...nd in.env.example. ### Deployments Can be found in/scriptfolder. There are...(MISSING_IT_THERE)
[grammar] ~38-~38: It seems that hyphens are missing.
Context: ...ng the wallet factory. There is also an all in one script to deploy all required contracts...(ALL_IN_ONE_HYPHEN)
docs/RESOURCE_LOCK_VALIDATOR.md (1)
1-111: Comprehensive and well-structured documentation.The documentation effectively covers all major aspects of the ResourceLockValidator:
- Clear explanation of functionality
- Detailed method descriptions
- Important security considerations
- Integration guidelines
🧰 Tools
🪛 LanguageTool
[uncategorized] ~87-~87: Loose punctuation mark.
Context: ...ror Conditions -RLV_AlreadyInstalled: Validator already installed for account...(UNLIKELY_OPENING_PUNCTUATION)
foundry.toml (6)
4-4: Dependency Directory Update:
Thelibssetting is now updated to["dependencies"]instead of the old value (likely["lib"]). This change aligns with the new approach for managing external dependencies via the Soldeer package manager. Ensure that all related documentation and scripts reflect this new dependency folder.
9-9: Treat Warnings as Errors:
Enablingdeny_warnings = trueenforces stricter quality control by treating compilation warnings as errors. Verify that all current warnings are resolved to prevent unexpected build failures.
11-11: Optimization Runs Decrease:
Reducing the optimizerrunsfrom a higher value (previously 1000) to256can impact the gas efficiency of deployments. Please ensure that this setting has been validated against your performance and cost requirements.
12-12: Universal Gas Reporting:
Changinggas_reportsto["*"]now enables gas reporting for all contracts. This is useful for comprehensive monitoring but may result in more verbose output. Confirm that the increased report volume is acceptable and meets your needs.
21-21: Manual Remapping Detection Disabled:
Disablingauto_detect_remappingsby setting it tofalseensures that only explicitly defined remappings (as specified above) are used. This strict control can prevent unexpected resolution errors but requires manual updates if dependency paths change.
26-32: New Soldeer Configuration Section:
The[soldeer]section introduces several new parameters (such asremappings_generate,remappings_regenerate,remappings_version, etc.) for managing dependency remappings. These settings are critical for ensuring proper handling of dependencies via Soldeer rather than git submodules. It’s advisable to validate these options with your intended workflow and update internal documentation accordingly to reflect this transition.
src/common/Structs.sol
Outdated
| // HookMultiplexer | ||
| struct SigHookInit { | ||
| bytes4 sig; | ||
| address[] subHooks; | ||
| } | ||
|
|
||
| struct HookAndContext { | ||
| address hook; | ||
| bytes context; | ||
| } | ||
|
|
||
| struct SignatureHooks { | ||
| bytes4[] allSigs; | ||
| mapping(bytes4 => address[]) sigHooks; | ||
| } | ||
|
|
||
| struct Config { | ||
| bool initialized; | ||
| mapping(HookType hookType => address[]) hooks; | ||
| mapping(HookType hookType => SignatureHooks) sigHooks; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add NatSpec documentation for hook-related structs.
The hook-related structs lack NatSpec documentation. Consider adding comprehensive documentation to maintain consistency with other structs in the file.
Add documentation for each struct:
// HookMultiplexer
+/// @title SigHookInit
+/// @notice Struct for initializing signature hooks
+/// @dev Used to configure function-specific hooks
struct SigHookInit {
bytes4 sig;
address[] subHooks;
}
+/// @title HookAndContext
+/// @notice Struct containing a hook address and its context
+/// @dev Used to manage hook execution with associated context data
struct HookAndContext {
address hook;
bytes context;
}
+/// @title SignatureHooks
+/// @notice Struct managing function-specific hooks
+/// @dev Maps function signatures to their associated hooks
struct SignatureHooks {
bytes4[] allSigs;
mapping(bytes4 => address[]) sigHooks;
}
+/// @title Config
+/// @notice Configuration struct for hook management
+/// @dev Manages initialization state and hook mappings by type
struct Config {
bool initialized;
mapping(HookType hookType => address[]) hooks;
mapping(HookType hookType => SignatureHooks) sigHooks;
}Verify Solidity version compatibility for named mapping parameters.
The Config struct uses the new mapping syntax with named parameters. Let's verify compatibility with Solidity 0.8.23.
🌐 Web query:
When was named mapping parameters feature introduced in Solidity?
💡 Result:
The named parameters in mapping types feature was introduced in Solidity 0.8.18, released on February 1, 2023[3][6]. This update allowed developers to specify parameter names for keys in mappings, improving code readability and ABI output clarity.
For example:
mapping(uint256 key => string value) public dataMap;Here, key and value become named parameters in the automatically generated getter function's ABI[1][6]. This change was particularly useful for nested mappings where multiple parameters might otherwise be unnamed[8].
The update was part of Solidity's ongoing improvements to developer experience and contract interoperability[3][6]. Subsequent versions have maintained this syntax while adding other compiler features and optimizations[3].
Citations:
- 1: Obtain an ABI from a contract with named parameters in mapping types with array output ethereum/solidity#14144
- 2: https://www.alchemy.com/overviews/solidity-mapping
- 3: https://github.com/ethereum/solidity/blob/develop/Changelog.md
- 4: https://solidityscan.com/discover/what-is-mapping-in-solidity/
- 5: https://metana.io/blog/solidity-mappings-explained-a-beginner-friendly-guide/
- 6: https://soliditylang.org/blog/2023/02/01/solidity-0.8.18-release-announcement/
- 7: https://docs.soliditylang.org/en/latest/control-structures.html
- 8: Allow specifying parameter name in public mappings ethereum/solidity#11407
- 9: https://docs.soliditylang.org
- 10: https://metaschool.so/articles/understand-mappings-in-solidity/
Documentation and Solidity Version Confirmation
- The hook-related structs in
src/common/Structs.sol(lines 60-80) currently lack NatSpec documentation. Please add comprehensive NatSpec comments to each struct as illustrated below. - The named mapping parameters used in the
Configstruct have been verified to be supported since Solidity 0.8.18. Ensure your project is compiled with Solidity 0.8.18 or later.
Suggested diff snippet for documentation:
// HookMultiplexer
+/// @title SigHookInit
+/// @notice Struct for initializing signature hooks.
+/// @dev Used to configure function-specific hooks.
struct SigHookInit {
bytes4 sig;
address[] subHooks;
}
+/// @title HookAndContext
+/// @notice Struct containing a hook address alongside its context.
+/// @dev Used to manage hook execution with associated context data.
struct HookAndContext {
address hook;
bytes context;
}
+/// @title SignatureHooks
+/// @notice Struct for managing function-specific hooks.
+/// @dev Maps function signatures to their associated hooks.
struct SignatureHooks {
bytes4[] allSigs;
mapping(bytes4 => address[]) sigHooks;
}
+/// @title Config
+/// @notice Configuration struct for hook management.
+/// @dev Manages initialization state and hook mappings by type.
struct Config {
bool initialized;
mapping(HookType hookType => address[]) hooks;
mapping(HookType hookType => SignatureHooks) sigHooks;
}📝 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.
| // HookMultiplexer | |
| struct SigHookInit { | |
| bytes4 sig; | |
| address[] subHooks; | |
| } | |
| struct HookAndContext { | |
| address hook; | |
| bytes context; | |
| } | |
| struct SignatureHooks { | |
| bytes4[] allSigs; | |
| mapping(bytes4 => address[]) sigHooks; | |
| } | |
| struct Config { | |
| bool initialized; | |
| mapping(HookType hookType => address[]) hooks; | |
| mapping(HookType hookType => SignatureHooks) sigHooks; | |
| } | |
| // HookMultiplexer | |
| /// @title SigHookInit | |
| /// @notice Struct for initializing signature hooks. | |
| /// @dev Used to configure function-specific hooks. | |
| struct SigHookInit { | |
| bytes4 sig; | |
| address[] subHooks; | |
| } | |
| /// @title HookAndContext | |
| /// @notice Struct containing a hook address alongside its context. | |
| /// @dev Used to manage hook execution with associated context data. | |
| struct HookAndContext { | |
| address hook; | |
| bytes context; | |
| } | |
| /// @title SignatureHooks | |
| /// @notice Struct for managing function-specific hooks. | |
| /// @dev Maps function signatures to their associated hooks. | |
| struct SignatureHooks { | |
| bytes4[] allSigs; | |
| mapping(bytes4 => address[]) sigHooks; | |
| } | |
| /// @title Config | |
| /// @notice Configuration struct for hook management. | |
| /// @dev Manages initialization state and hook mappings by type. | |
| struct Config { | |
| bool initialized; | |
| mapping(HookType hookType => address[]) hooks; | |
| mapping(HookType hookType => SignatureHooks) sigHooks; | |
| } |
| 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
isValidSignatureWithSender follows the same dual approach but ensure consistent signature checks.
Similar to validateUserOp, ensure the minimum signature length is enforced to prevent slicing errors. Otherwise, the approach of verifying both direct and eth-signed message hashes is beneficial for improved signature compatibility.
| // TODO: Need to figure out how to unpack resource lock data and hash | ||
| // from UserOperation | ||
| function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash) | ||
| external | ||
| override | ||
| returns (uint256) | ||
| { | ||
| bytes calldata signature = userOp.signature; | ||
| address walletOwner = validatorStorage[msg.sender].owner; | ||
| // Standard signature length - no proof packing | ||
| if (signature.length == 65) { | ||
| // standard ECDSA recover | ||
| if (walletOwner == ECDSA.recover(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; | ||
| } | ||
| // 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)) { | ||
| 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.
Signature handling in validateUserOp should check minimum signature length.
When parsing standard ECDSA signatures (65 bytes) vs. extended signatures (≥ 65 bytes), consider an explicit check to avoid out-of-bounds slicing if the signature is shorter than expected. If a malformed signature is passed in, the call could revert unexpectedly.
Below is an example diff to add a length check:
function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash)
external
override
returns (uint256)
{
bytes calldata signature = userOp.signature;
+ require(signature.length >= 65, "Invalid signature length");
address walletOwner = validatorStorage[msg.sender].owner;
...
}📝 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.
| // TODO: Need to figure out how to unpack resource lock data and hash | |
| // from UserOperation | |
| function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash) | |
| external | |
| override | |
| returns (uint256) | |
| { | |
| bytes calldata signature = userOp.signature; | |
| address walletOwner = validatorStorage[msg.sender].owner; | |
| // Standard signature length - no proof packing | |
| if (signature.length == 65) { | |
| // standard ECDSA recover | |
| if (walletOwner == ECDSA.recover(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; | |
| } | |
| // 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)) { | |
| 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; | |
| } | |
| // TODO: Need to figure out how to unpack resource lock data and hash | |
| // from UserOperation | |
| function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash) | |
| external | |
| override | |
| returns (uint256) | |
| { | |
| bytes calldata signature = userOp.signature; | |
| require(signature.length >= 65, "Invalid signature length"); | |
| address walletOwner = validatorStorage[msg.sender].owner; | |
| // Standard signature length - no proof packing | |
| if (signature.length == 65) { | |
| // standard ECDSA recover | |
| if (walletOwner == ECDSA.recover(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; | |
| } | |
| // 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)) { | |
| 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; | |
| } |
| /*////////////////////////////////////////////////////////////// | ||
| VALIDATOR PUBLIC/EXTERNAL | ||
| //////////////////////////////////////////////////////////////*/ | ||
|
|
||
| // @inheritdoc ICredibleAccountModule | ||
| function enableSessionKey(bytes calldata _resourceLock) external { | ||
| ResourceLock memory rl = abi.decode(_resourceLock, (ResourceLock)); | ||
| if (rl.sessionKey == address(0)) | ||
| revert CredibleAccountModule_InvalidSessionKey(); | ||
| if (rl.validAfter == 0) | ||
| revert CredibleAccountModule_InvalidValidAfter(); | ||
| if (rl.validUntil == 0 || rl.validUntil <= rl.validAfter) | ||
| revert CredibleAccountModule_InvalidValidUntil(rl.validUntil); | ||
| sessionData[rl.sessionKey][msg.sender] = SessionData({ | ||
| sessionKey: rl.sessionKey, | ||
| validAfter: rl.validAfter, | ||
| validUntil: rl.validUntil, | ||
| live: true // unused | ||
| }); | ||
| for (uint256 i; i < rl.tokenData.length; ++i) { | ||
| lockedTokens[rl.sessionKey].push( | ||
| LockedToken({ | ||
| token: rl.tokenData[i].token, | ||
| lockedAmount: rl.tokenData[i].amount, | ||
| claimedAmount: 0 | ||
| }) | ||
| ); | ||
| } | ||
| walletSessionKeys[msg.sender].push(rl.sessionKey); | ||
| emit CredibleAccountModule_SessionKeyEnabled(rl.sessionKey, msg.sender); | ||
| } | ||
|
|
||
| // @inheritdoc ICredibleAccountModule | ||
| function disableSessionKey(address _sessionKey) external { | ||
| if (sessionData[_sessionKey][msg.sender].validUntil == 0) | ||
| revert CredibleAccountModule_SessionKeyDoesNotExist(_sessionKey); | ||
| if ( | ||
| sessionData[_sessionKey][msg.sender].validUntil >= | ||
| block.timestamp && | ||
| !isSessionClaimed(_sessionKey) | ||
| ) revert CredibleAccountModule_LockedTokensNotClaimed(_sessionKey); | ||
| delete sessionData[_sessionKey][msg.sender]; | ||
| delete lockedTokens[_sessionKey]; | ||
| address[] storage keys = walletSessionKeys[msg.sender]; | ||
| for (uint256 i; i < keys.length; ++i) { | ||
| if (keys[i] == _sessionKey) { | ||
| keys[i] = keys[keys.length - 1]; | ||
| keys.pop(); | ||
| break; | ||
| } | ||
| } | ||
| emit CredibleAccountModule_SessionKeyDisabled(_sessionKey, msg.sender); | ||
| } | ||
|
|
||
| // @inheritdoc ICredibleAccountModule | ||
| function validateSessionKeyParams( | ||
| address _sessionKey, | ||
| PackedUserOperation calldata userOp | ||
| ) public returns (bool) { | ||
| if (isSessionClaimed(_sessionKey)) return false; | ||
| bytes calldata callData = userOp.callData; | ||
| if (bytes4(callData[:4]) == IERC7579Account.execute.selector) { | ||
| ModeCode mode = ModeCode.wrap(bytes32(callData[4:36])); | ||
| (CallType calltype, , , ) = ModeLib.decode(mode); | ||
| if (calltype == CALLTYPE_SINGLE) { | ||
| return | ||
| _validateSingleCall(callData, _sessionKey, userOp.sender); | ||
| } else if (calltype == CALLTYPE_BATCH) { | ||
| return _validateBatchCall(callData, _sessionKey, userOp.sender); | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Session key creation and disabling
The logic in enableSessionKey and disableSessionKey ensures time-bound validity and mandatory locked token claims. Consider a scenario with partial claims on multiple tokens—double-check that disableSessionKey accounts for partial redemption.
Potential edge case: If some tokens are only partially claimed, the session can’t be disabled. Confirm that this is intentional or provide a partial-claim scenario.
src/utils/ProofVerifier.sol
Outdated
| function verifyProof(bytes calldata proof) external view returns (bool) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Implement actual proof verification logic.
The current implementation always returns true without verifying the proof. This is a critical security risk if used in production.
Please confirm:
- Is this intended to be a placeholder implementation?
- Are there plans to implement the actual verification logic?
- Are there any test cases that rely on this implementation?
Run the following script to check for usages of this contract:
🏁 Script executed:
#!/bin/bash
# Search for imports and contract instantiations
rg -A 5 "ProofVerifier|IProofVerifier"Length of output: 5726
Security Risk: Placeholder Implementation in ProofVerifier
The verifyProof function in src/utils/ProofVerifier.sol currently always returns true, effectively acting as a placeholder. This implementation is used both in testing (as seen in test/ModularTestBase.sol) and by other modules (e.g., in CredibleAccountModule.sol), which means if it's deployed as-is, it presents a serious security risk.
- Please confirm if this placeholder implementation is intentional.
- If it is a temporary stub, are there plans to implement the actual proof verification logic?
- Are there any tests or module behaviors that depend on the current implementation?
src/interfaces/IHookMultiplexer.sol
Outdated
| function hasHook( | ||
| address walletAddress, | ||
| address hookAddress, | ||
| HookType hookType | ||
| ) external returns (bool); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add view modifier to hasHook function.
The hasHook function appears to be a query function that doesn't modify state and should be marked as view.
function hasHook(
address walletAddress,
address hookAddress,
HookType hookType
- ) external returns (bool);
+ ) external view returns (bool);📝 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 hasHook( | |
| address walletAddress, | |
| address hookAddress, | |
| HookType hookType | |
| ) external returns (bool); | |
| } | |
| function hasHook( | |
| address walletAddress, | |
| address hookAddress, | |
| HookType hookType | |
| ) external view returns (bool); | |
| } |
src/interfaces/IERC7484.sol
Outdated
| pragma solidity ^0.8.0; | ||
|
|
||
| interface IERC7484 { | ||
| event NewTrustedAttesters(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameters to NewTrustedAttesters event.
The event should include relevant parameters such as the attesters and threshold for better tracking and filtering.
- event NewTrustedAttesters();
+ event NewTrustedAttesters(
+ address indexed smartAccount,
+ address[] attesters,
+ uint8 threshold
+ );📝 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.
| event NewTrustedAttesters(); | |
| event NewTrustedAttesters( | |
| address indexed smartAccount, | |
| address[] attesters, | |
| uint8 threshold | |
| ); |
src/utils/TrustedForwarder.sol
Outdated
| function setTrustedForwarder(address forwarder) external { | ||
| trustedForwarder[msg.sender] = forwarder; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add event emission and zero address validation.
Track forwarder changes with events and validate the forwarder address.
+ event TrustedForwarderSet(address indexed account, address indexed forwarder);
+ event TrustedForwarderCleared(address indexed account);
function setTrustedForwarder(address forwarder) external {
+ require(forwarder != address(0), "Invalid forwarder address");
trustedForwarder[msg.sender] = forwarder;
+ emit TrustedForwarderSet(msg.sender, forwarder);
}📝 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 setTrustedForwarder(address forwarder) external { | |
| trustedForwarder[msg.sender] = forwarder; | |
| } | |
| event TrustedForwarderSet(address indexed account, address indexed forwarder); | |
| event TrustedForwarderCleared(address indexed account); | |
| function setTrustedForwarder(address forwarder) external { | |
| require(forwarder != address(0), "Invalid forwarder address"); | |
| trustedForwarder[msg.sender] = forwarder; | |
| emit TrustedForwarderSet(msg.sender, forwarder); | |
| } |
src/ERC7579/core/HookManager.sol
Outdated
| bytes memory hookData = IHook(hook).preCheck( | ||
| msg.sender, | ||
| msg.value, | ||
| msg.data | ||
| ); | ||
| _; | ||
| if (!IHook(hook).postCheck(hookData)) revert HookPostCheckFailed(); | ||
| IHook(hook).postCheck(hookData); | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Critical: Verify the removal of postCheck error handling.
The error handling for postCheck has been removed, which could lead to silent failures. This change needs careful consideration.
🏁 Script executed:
#!/bin/bash
# Search for postCheck implementations to understand potential impact
ast-grep --pattern 'function postCheck($$$) {
$$$
return false;
$$$
}'Length of output: 78
Attention: Manual Verification Needed for postCheck Error Handling
The current implementation calls IHook(hook).postCheck(hookData) without any try-catch or explicit error handling. Our initial script did not detect any implementation using a return false pattern for error handling, which makes it unclear whether the removal of error handling was intentional or might lead to silent failures.
- File:
src/ERC7579/core/HookManager.sol(Lines 30-37) - Concern: The absence of error handling around the
postCheckcall could lead to unreported failures ifpostCheckreverts or behaves unexpectedly. - Action: Please manually verify if the removal of error handling in
postCheckis intentional and that any failure in the hook’s execution is appropriately managed elsewhere or is acceptable for the overall design.
🏁 Script executed:
#!/bin/bash
# Print the context around the postCheck call from HookManager.sol to check for error handling mechanisms.
rg -A 5 -B 5 "IHook(hook).postCheck" src/ERC7579/core/HookManager.solLength of output: 71
Critical: The current implementation in src/ERC7579/core/HookManager.sol now calls IHook(hook).postCheck(hookData) without any visible error handling (e.g., try/catch or explicit failure checks). Since our automated searches (for error-handling patterns in postCheck and related contexts) produced no output, please manually verify that this design change is intentional and that any failures in postCheck are either acceptable or appropriately handled upstream.
| function test_enableSessionKey_ViaUserOp() public withRequiredModules { | ||
| // Enable session key | ||
| _enableSessionKey(address(scw)); | ||
| // Enable another session key | ||
| uint256[2] memory newAmounts = [uint256(10e6), uint256(40e18)]; | ||
| usdc.mint(address(scw), newAmounts[0]); | ||
| dai.mint(address(scw), newAmounts[1]); | ||
| vm.deal(address(scw), uint256(113e18)); | ||
| TokenData[] memory newTokenData = new TokenData[](tokens.length + 1); | ||
| for (uint256 i; i < 2; ++i) { | ||
| newTokenData[i] = TokenData(tokens[i], newAmounts[i]); | ||
| } | ||
| bytes memory rl = abi.encode( | ||
| ResourceLock({ | ||
| chainId: 42161, | ||
| smartWallet: address(scw), | ||
| sessionKey: otherSessionKey.pub, | ||
| validAfter: validAfter, | ||
| validUntil: validUntil, | ||
| tokenData: newTokenData, | ||
| nonce: 2 | ||
| }) | ||
| ); | ||
| bytes memory enableSessionKeyData = abi.encodeWithSelector(CAM.enableSessionKey.selector, rl); | ||
| TokenData[] memory newTokenData2 = new TokenData[](tokens.length + 1); | ||
| newTokenData2[0] = TokenData(0xa0Cb889707d426A7A386870A03bc70d1b0697598, uint256(10000000)); // USDC | ||
| newTokenData2[1] = TokenData(0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9, uint256(40000000000000000000)); // DAI | ||
| // Encode the session data | ||
| bytes memory newRl = abi.encode( | ||
| ResourceLock({ | ||
| chainId: 42161, | ||
| smartWallet: address(scw), | ||
| sessionKey: 0xB071527c3721215A46958d172A42E7E3BDd1DF46, | ||
| validAfter: uint48(1729743735), | ||
| validUntil: uint48(1729744025), | ||
| tokenData: newTokenData2, | ||
| nonce: 3 | ||
| }) | ||
| ); | ||
| // Encode the function call data with the function selector and the encoded session data | ||
| bytes memory enableSessionKeyData2 = abi.encodeWithSelector(CAM.enableSessionKey.selector, newRl); | ||
| bytes memory opCalldata = abi.encodeCall( | ||
| IERC7579Account.execute, | ||
| (ModeLib.encodeSimpleSingle(), ExecutionLib.encodeSingle(address(cam), 0, enableSessionKeyData)) | ||
| ); | ||
| (PackedUserOperation memory op,) = _createUserOpWithSignature(eoa, address(scw), address(moecdsav), opCalldata); | ||
| // Execute the user operation | ||
| _executeUserOp(op); | ||
| // Get cumulative locked funds for wallet | ||
| TokenData[] memory data = cam.cumulativeLockedForWallet(); | ||
| // Verify retrieved data matches expected | ||
| assertEq(data[0].token, address(usdc), "First token address does not match expected (expected USDC)"); | ||
| assertEq( | ||
| data[0].amount, amounts[0] + newAmounts[0], "Cumulative USDC locked balance does not match expected amount" | ||
| ); | ||
| assertEq(data[1].token, address(dai), "Second token address does not match expected (expected DAI)"); | ||
| assertEq( | ||
| data[1].amount, amounts[1] + newAmounts[1], "Cumulative DAI locked balance does not match expected amount" | ||
| ); | ||
| vm.stopPrank(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded addresses in test data could lead to test failures.
The test uses hardcoded addresses for tokens which might not exist on all test networks:
newTokenData2[0] = TokenData(0xa0Cb889707d426A7A386870A03bc70d1b0697598, uint256(10000000)); // USDC
newTokenData2[1] = TokenData(0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9, uint256(40000000000000000000)); // DAIConsider using the existing token variables (usdc, dai) instead of hardcoded addresses to make the tests more maintainable and network-agnostic:
-newTokenData2[0] = TokenData(0xa0Cb889707d426A7A386870A03bc70d1b0697598, uint256(10000000)); // USDC
-newTokenData2[1] = TokenData(0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9, uint256(40000000000000000000)); // DAI
+newTokenData2[0] = TokenData(address(usdc), uint256(10000000));
+newTokenData2[1] = TokenData(address(dai), uint256(40000000000000000000));
- Added `FOUNDRY_DISABLE_NIGHTLY_WARNING` param to .example.env. - Removed `deny_warnings` config from foundry.toml. - Fixed import in deploy script.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/modules/validators/CredibleAccountModule.sol (2)
387-403: Remove commented-out code in _digestSignature.The function contains commented-out code that appears to be an alternative implementation. This should be removed to maintain clean, production-ready code.
function _digestSignature(bytes calldata _signature) internal pure returns (bytes memory, bytes memory) { bytes32 r = bytes32(_signature[0:32]); bytes32 s = bytes32(_signature[32:64]); uint8 v = uint8(_signature[64]); bytes memory proof = bytes(_signature[65:]); bytes memory signature = abi.encodePacked(r, s, v); - // r (32 bytes) + s (32 bytes) + v (1 byte) = 65 bytes. - // uint256 proofLength = _signature.length - 65; - // proof = new bytes(proofLength); - // if (proofLength > 0) { - // uint256 proofStart = 65; // 32 byte r + 32 byte s + 1 byte v - // for (uint256 i; i < proofLength; ++i) { - // proof[i] = _signature[proofStart + i]; - // } - // } return (signature, proof); }
347-347: Missing NatSpec documentation.There's an empty line where NatSpec documentation should be for the
_validateTokenDatafunction. Consider adding proper documentation to maintain consistency./** * @notice check if the tokenAddress in calldata of userOp is part of the session data and wallet has sufficient token balance * @dev locked tokenBalance check is done in the CredibleAccountModule * @dev for `transfer` as function-selector, then check for the wallet balance * @dev for `transferFrom` as function-selector, then check for the wallet balance and allowance + * @param _sessionKey The address of the session key + * @param _wallet The address of the wallet + * @param _amount The amount of tokens to transfer + * @param _token The address of the token + * @return bool indicating whether the token data is valid */foundry.toml (2)
13-20: Explicit Remappings ConfigurationThe new remappings array lists explicit paths for various dependencies and contracts. This approach enhances clarity and control over dependency resolution. Please double-check that each remapping (e.g., for
ERC7579,solady,solarray) correctly corresponds to your project’s directory structure and that related changes (such as removal of submodules) are reflected elsewhere in your documentation.
34-40: New Dependencies Section AddedThe
[dependencies]section now explicitly lists dependencies such assolady,@openzeppelin-contracts,eth-infinitism-account-abstraction,forge-std, andsolarray. Consider verifying that:
- The versions specified are correct and compatible.
- The SSH URL for
solarrayis accessible in your deployment environment (switching to HTTPS might avoid SSH key issues if needed).This explicit declaration aids in transparency and simplifies dependency management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.env.example(1 hunks)foundry.toml(1 hunks)script/NonDeterministicDeployAllAndSetup.s.sol(4 hunks)src/modules/validators/CredibleAccountModule.sol(22 hunks)test/modules/CredibleAccountModule/fuzz/CredibleAccountModule.t.sol(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- script/NonDeterministicDeployAllAndSetup.s.sol
🔇 Additional comments (20)
test/modules/CredibleAccountModule/fuzz/CredibleAccountModule.t.sol (8)
6-7: Good addition of standard libraries.Adding ECDSA, IERC20, ExecutionLib, and ModeLib imports provides better type safety and functionality over raw operations.
Also applies to: 11-12
15-16: Note the test utility restructuring.The test inherits from
TestUtils(aliased from CredibleAccountModuleTestUtils), indicating a broader test utilities restructuring that aligns with the PR objective of cleaning up the test suite.
35-42: Improved session key handling with User objects.Converting from raw address to structured User objects with
_createUser(_sessionKey)improves code organization and encapsulates the session key functionality better.
52-57: Updated Resource Lock structure with wallet reference.The resource lock now correctly references
address(scw)as the smart wallet and uses the public key from the User object (sk.pub) as the session key.
87-89: Variable naming standardization to scw.Changed from
mewtoscw(likely SmartContractWallet) for consistency with the rest of the codebase.
108-118: Improved execution creation pattern for token transfers.The code now uses a more structured approach to create token transfer executions and batch them together, which better represents how these operations would occur in production.
40-40: Test functions now use withRequiredModules modifier.All test functions have been updated to include the
withRequiredModulesmodifier, which ensures proper module prerequisites are set up before running tests. This is a good practice for ensuring test isolation and clarity.Also applies to: 81-82, 138-139
26-28:❓ Verification inconclusive
Simplified setup method.
The setUp method no longer calls
super.setUp()and directly invokes_testSetup(). Ensure that all necessary setup from the parent class is still being performed.
🏁 Script executed:
#!/bin/bash # Look for _testSetup implementation to check what initialization is happening rg -A 10 "_testSetup\(\)" test/modules/CredibleAccountModule/utils/CredibleAccountModuleTestUtils.solLength of output: 590
Simplified setup verification – Ensure Complete Initialization is Maintained
The verification script shows that
_testSetup()intest/modules/CredibleAccountModule/utils/CredibleAccountModuleTestUtils.solperforms the full initialization (calling_testInit()and setting up contracts, wallets, and test variables). This indicates that the parent class's setup logic is either encapsulated within_testInit()or has been intentionally replaced by_testSetup(). Please confirm that this change is deliberate and that no essential initialization from the formersuper.setUp()call has been omitted.
- Location:
test/modules/CredibleAccountModule/fuzz/CredibleAccountModule.t.sol(lines 26-28)- Initialization Logic: Refer to
_testSetup()implementation intest/modules/CredibleAccountModule/utils/CredibleAccountModuleTestUtils.solsrc/modules/validators/CredibleAccountModule.sol (7)
5-5: Updated import path follows package manager convention.Changed from "openzeppelin/contracts/token/ERC20/IERC20.sol" to "@openzeppelin/contracts/token/ERC20/IERC20.sol" to align with standard import path conventions when using package managers like Soldeer.
23-23: Added console logging for debugging.The addition of the console2 import enables better debugging capabilities during development and testing. Ensure these are used appropriately and don't impact production gas costs.
Is the console2 import appropriately guarded to prevent inclusion in production builds? Consider checking if there's a convention in the project for development-only imports.
73-78: Improved conditional formatting with explicit braces.Adding explicit braces to conditional statements improves code readability and prevents potential bugs from implicit scoping. This is a good practice for maintaining code clarity.
Also applies to: 90-98, 119-121, 267-271
153-156: Added diagnostic logging.The logging statements provide useful information during execution, which will aid in debugging and understanding the flow of operations during tests and development.
Also applies to: 246-246
57-57: Mapping declaration formatting.The mapping declaration for sessionData has been reformatted to a single line for consistency with other mappings in the contract.
119-121: Session key disabling with locked tokens validation.The check for not allowing session key disabling when tokens are not claimed and the session is still valid is important. Consider a scenario with partial claims on multiple tokens—ensure that
disableSessionKeycorrectly handles partial redemption scenarios.
315-320: Improved calldata extraction in validation methods.The validation methods now use better patterns for extracting and processing calldata, which improves readability and maintainability of the code.
Also applies to: 332-339
.env.example (1)
3-3: New Environment Variable for Foundry Warning SuppressionThe addition of
FOUNDRY_DISABLE_NIGHTLY_WARNING=1provides a clear way to disable Foundry’s nightly warnings. Please ensure that this setting is documented in your project guidelines or README to help new developers understand its purpose.foundry.toml (4)
4-4: Updated Libraries DirectoryThe
libsfield now points to["dependencies"]instead of what may have been a previous value (e.g.,"lib"). This change aligns with the new dependency management approach. Please verify that all documentation and any related tooling references have been updated accordingly.
10-11: Adjusted Optimization and Gas Reporting SettingsThe optimizer
runsvalue has been reduced to256, and thegas_reportsfield is now set to report for all contracts (["*"]). Ensure that 256 optimization runs are sufficient for your deployment targets and that comprehensive gas reporting does not lead to unnecessary clutter or performance issues during builds.
21-21: Disable Auto-Detection of RemappingsSetting
auto_detect_remappingsto false enforces manual control over dependency paths. This can prevent unexpected behavior due to automatic remapping. Just ensure that future dependency updates or re-arrangements are manually updated in this configuration.
26-33: New Soldeer Package Manager ConfigurationThe
[soldeer]block introduces configurations for the Soldeer package manager, disabling automatic remapping generation and version checks while enabling recursive dependency handling. Confirm that these settings integrate well with your overall dependency workflow and that the Soldeer integration has been validated through testing.
- No longer hashing the `TokenData[]` in `ResourceLock` and then rehashing with all of the other data. Only encoding `TokenData[]` and hashing with other data. - Fixed helper util and retested.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/modules/validators/ResourceLockValidator.sol (2)
71-105: Signature handling invalidateUserOpshould check minimum signature length.When parsing standard ECDSA signatures (65 bytes) vs. extended signatures (≥ 65 bytes), consider an explicit check to avoid out-of-bounds slicing if the signature is shorter than expected.
function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash) external override returns (uint256) { bytes calldata signature = userOp.signature; + if (signature.length < 65) return SIG_VALIDATION_FAILED; address walletOwner = validatorStorage[msg.sender].owner; // Rest of function...
107-137:isValidSignatureWithSenderfollows the same dual approach but needs signature length validation.Similar to
validateUserOp, ensure the minimum signature length is enforced to prevent slicing errors. Otherwise, the approach of verifying both direct and eth-signed message hashes is beneficial for improved signature compatibility.function isValidSignatureWithSender(address sender, bytes32 hash, bytes calldata signature) external view override returns (bytes4) { + if (signature.length < 65) return ERC1271_INVALID; address walletOwner = validatorStorage[msg.sender].owner; // Rest of function...
🧹 Nitpick comments (3)
test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol (2)
32-52: Consider adding validation for dynamic test scenarios.The function creates user operations with resource locks, but consider adding parameters to test various edge cases (e.g., expired timestamps, zero token amounts) for more comprehensive testing.
-function _createUserOpWithResourceLock(address _scw, User memory _user, bool _validProof) +function _createUserOpWithResourceLock( + address _scw, + User memory _user, + bool _validProof, + uint48 validAfter, + uint48 validUntil +) internal returns (PackedUserOperation memory op, ResourceLock memory rl, bytes32[] memory proof, bytes32 root) { // Create base UserOp op = _createUserOp(_scw, address(rlv)); // Create ResourceLock and generate proof - rl = _generateResourceLock(_scw, _user.pub); + rl = _generateResourceLock(_scw, _user.pub, validAfter, validUntil); (proof, root,) = getTestProof(_buildResourceLockHash(rl), _validProof); // Rest of function...
68-82: Hardcoded values may limit test flexibility.The function uses hardcoded values for chain ID, timestamps, and nonce, which might make tests brittle. Consider parameterizing these values for more flexible test scenarios.
-function _generateResourceLock(address _scw, address _sk) internal view returns (ResourceLock memory) { +function _generateResourceLock( + address _scw, + address _sk, + uint48 _validAfter, + uint48 _validUntil +) internal view returns (ResourceLock memory) { TokenData[] memory td = new TokenData[](2); td[0] = TokenData({token: address(usdt), amount: 100}); td[1] = TokenData({token: address(dai), amount: 200}); ResourceLock memory rl = ResourceLock({ chainId: 42161, smartWallet: _scw, sessionKey: _sk, - validAfter: 1732176210, - validUntil: 1732435407, + validAfter: _validAfter == 0 ? 1732176210 : _validAfter, + validUntil: _validUntil == 0 ? 1732435407 : _validUntil, tokenData: td, nonce: 14 }); return rl; }src/modules/validators/ResourceLockValidator.sol (1)
69-70: Remove TODO comment before finalizing.The TODO comment indicates incomplete work regarding resource lock data extraction from UserOperation. Either implement the solution or document why it's not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/modules/validators/ResourceLockValidator.sol(1 hunks)test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol(1 hunks)
🔇 Additional comments (3)
test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol (2)
18-26: Good design with the withRequiredModules modifier.The modifier efficiently sets up the test environment by installing all necessary modules and hooks in the correct order. This creates a clean pattern for test setup that should be easy to reuse across test cases.
54-66: LGTM - Consistent hashing implementation.The ResourceLock hash implementation is correct and matches the implementation in the core contract.
src/modules/validators/ResourceLockValidator.sol (1)
195-207: LGTM - Resource lock hash generation is consistent.The implementation of
_buildResourceLockHashcorrectly encodes all relevant fields of the ResourceLock struct, maintaining consistency with the test utilities implementation.
| 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.
💡 Verification agent
🧩 Analysis chain
Hardcoded offsets in _getArrayInfo are brittle.
The function uses hardcoded offsets (260, 292, 100) to extract array information from calldata, which makes the code fragile if the calldata structure changes. Consider using a more robust approach.
🏁 Script executed:
#!/bin/bash
# Verify if the expected calldata format matches what's being used in tests
# Look for tests that could indicate the expected calldata format
echo "Searching for tests that construct calldata for ResourceLockValidator..."
rg -A 5 "ResourceLock" test/ | grep -A 3 "callData ="Length of output: 1499
Action Needed: Refactor Hardcoded CallData Offsets in _getArrayInfo
While the tests (e.g., those in ResourceLockValidatorTestUtils.sol and ResourceLockValidator.t.sol) currently construct calldata using abi.encodeCall and thus adhere to an expected format, the reliance on hardcoded offsets (260, 292, and the base offset of 100) in _getArrayInfo remains inherently brittle. If the calldata structure ever changes, these fixed offsets will no longer align with the actual data layout, potentially breaking the validator logic.
- File:
src/modules/validators/ResourceLockValidator.sol(lines 155–158) - Issue: Extraction of array information relies on fixed offsets, which may not be future-proof.
- Suggestion: Investigate using a dynamic or more descriptive decoding approach (e.g., leveraging
abi.decode) to automatically adjust to any modifications in the calldata format.
Please review and refactor accordingly to enhance long-term maintainability.
| 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)); | ||
| } | ||
| return ResourceLock({ | ||
| chainId: uint256(bytes32(execData[100:132])), | ||
| smartWallet: address(uint160(uint256(bytes32(execData[132:164])))), | ||
| 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: uint256(bytes32(execData[292:324])) | ||
| }); | ||
| } | ||
| revert RLV_OnlyCallTypeSingle(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate extracted ResourceLock data in _getResourceLock.
The function extracts a ResourceLock from calldata without validating that the timestamps make sense (validAfter ≤ validUntil) or that extracted addresses are non-zero. This could lead to acceptance of invalid resource locks.
function _getResourceLock(bytes calldata _callData) internal view returns (ResourceLock memory) {
// ... existing implementation ...
+ // Add validation at the end before returning
+ ResourceLock memory lock = ResourceLock({
chainId: uint256(bytes32(execData[100:132])),
smartWallet: address(uint160(uint256(bytes32(execData[132:164])))),
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: uint256(bytes32(execData[292:324]))
});
+
+ // Basic validation
+ require(lock.smartWallet != address(0), "RLV: Invalid smart wallet");
+ require(lock.sessionKey != address(0), "RLV: Invalid session key");
+ require(lock.validAfter <= lock.validUntil, "RLV: Invalid time range");
+
+ return lock;
}📝 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)); | |
| } | |
| return ResourceLock({ | |
| chainId: uint256(bytes32(execData[100:132])), | |
| smartWallet: address(uint160(uint256(bytes32(execData[132:164])))), | |
| 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: uint256(bytes32(execData[292:324])) | |
| }); | |
| } | |
| revert RLV_OnlyCallTypeSingle(); | |
| } | |
| } | |
| 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)); | |
| } | |
| // Add validation at the end before returning | |
| ResourceLock memory lock = ResourceLock({ | |
| chainId: uint256(bytes32(execData[100:132])), | |
| smartWallet: address(uint160(uint256(bytes32(execData[132:164])))), | |
| 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: uint256(bytes32(execData[292:324])) | |
| }); | |
| // Basic validation | |
| require(lock.smartWallet != address(0), "RLV: Invalid smart wallet"); | |
| require(lock.sessionKey != address(0), "RLV: Invalid session key"); | |
| require(lock.validAfter <= lock.validUntil, "RLV: Invalid time range"); | |
| return lock; | |
| } | |
| revert RLV_OnlyCallTypeSingle(); | |
| } | |
| } |
- Tracking nonce per SCW for signatures with Merkle proofs appended (ResourceLock signatures). - Adjusted test utils to take into account new changes. - Tested successful increment of nonce on valid signature. - Added EntryPoint test dependency as was previously overridden by .gitignore as Soldeer uses dependencies folder and both were being selected.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
src/modules/validators/ResourceLockValidator.sol (2)
70-78: Add a global minimum-length guard for signaturesBoth execution branches assume
signature.length >= 65.
When a malformed (e.g. 60-byte) signature is supplied the slices below silently truncate and will revert with an out-of-bounds panic.bytes calldata signature = userOp.signature; -require(signature.length == 65 || signature.length >= 97, "RLV: bad sig length"); +require(signature.length == 65 || signature.length >= 97, "RLV: bad sig length");(The
97constant is65 /*sig*/ + 32 /*root*/— the minimum before any proof bytes.)
110-140: Replicate the length guard inisValidSignatureWithSenderIdentical unsafe slicing occurs here; apply the same ≥ 97 byte guard as above to avoid out-of-bounds panics.
(The earlier comment already covered this pattern.)
🧹 Nitpick comments (7)
test/modules/ResourceLockValidator/concrete/ResourceLockValidator.t.sol (2)
344-352: Suppress “unused-variable” warnings from discarded return valuesSeveral tests capture
rl,proof,merkleRootbut never read them, triggering compiler warnings and obscuring real issues during CI.- (PackedUserOperation memory op,, bytes32[] memory proof, bytes32 merkleRoot) = + (PackedUserOperation memory op,, bytes32[] memory proof, bytes32 merkleRoot) =can be rewritten as
- (PackedUserOperation memory op,, bytes32[] memory proof, bytes32 merkleRoot) = + (PackedUserOperation memory op,, bytes32[] memory proof, bytes32 merkleRoot) =→
(PackedUserOperation memory op,, bytes32[] memory proof, bytes32 merkleRoot) = …becomes
(PackedUserOperation memory op,, bytes32[] memory proof, bytes32 merkleRoot) = …Instead, drop the unused identifiers:
(PackedUserOperation memory op,, bytes32[] memory proof, bytes32 merkleRoot) = …→
(PackedUserOperation memory op,, bytes32[] memory proof, bytes32 merkleRoot) = …(or replace with
_placeholders).
This keeps the bytecode identical but silences warnings.Also applies to: 370-378, 398-405
413-441: Un-checked loop increment saves gas in test runsThe
for (uint256 i; i < 10; ++i)loop is executed on-chain during tests; using anunchecked { ++i; }block avoids the redundant overflow check:for (uint256 i; i < 10; ++i) { - tokens[i] = TokenData({token: vm.randomAddress(), amount: vm.randomUint()}); + unchecked { ++i; } + tokens[i] = TokenData({token: vm.randomAddress(), amount: vm.randomUint()}); }While minor, this is a free optimisation and keeps gas profiles consistent with production contracts that may borrow this pattern.
Also applies to: 492-520
test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol (1)
68-82: Use runtimeblock.chainidinstead of hard-coding Arbitrum (42161)Hard-coding the chain id makes the helper brittle; tests will fail when run on a local fork or an L1 simulation.
- chainId: 42161, + chainId: block.chainid,src/test/dependencies/EntryPoint.sol (2)
8-27: Consider making_entrypointAddrimmutableIf the contract is always initialised via
init(), storing_entrypointAddronce asimmutable(constructor) removes an SLOAD every time it’s read:address immutable _entrypointAddr; constructor(address ep) { _entrypointAddr = ep; initSenderCreator(); }This requires minor refactoring of
etchEntrypoint()but yields cheaper reads in every simulation.
39-74: Helper free-functions risk name collisionsGlobal-scope functions (
sign,label,prank, …) are convenient but collide if another utility file defines the same signature. Wrapping them in alibrary Cheatsavoids shadowing and keeps the public namespace clean.library Cheats { function getAddr(uint256 pk) internal pure returns (address) { … } … }Call sites then import with
using Cheats for uint256;orCheats.getAddr(pk).src/modules/validators/ResourceLockValidator.sol (2)
174-197: Ensure call-type is enforced at entry
CALLTYPE_SINGLEis validated, but the earlier selector check is silent.
Move theCALLTYPE_SINGLEguard outside and revert early to save gas.
218-223: Use unchecked increment for minor gas saving
noncecannot overflow under realistic conditions. Wrapping the addition inunchecked {}saves ~5 gas.function _incrementNonce(address smartAccount) internal returns (uint256) { - uint256 newNonce = validatorStorage[smartAccount].nonce + 1; + uint256 newNonce; + unchecked { newNonce = validatorStorage[smartAccount].nonce + 1; } validatorStorage[smartAccount].nonce = newNonce; emit RLV_NonceUpdated(smartAccount, newNonce); return newNonce; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)src/interfaces/IResourceLockValidator.sol(1 hunks)src/modules/validators/ResourceLockValidator.sol(1 hunks)src/test/dependencies/EntryPoint.sol(1 hunks)test/modules/ResourceLockValidator/concrete/ResourceLockValidator.t.sol(1 hunks)test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- src/interfaces/IResourceLockValidator.sol
🔇 Additional comments (2)
test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol (1)
18-26:withRequiredModulesleaves the test inscwcontext
vm.startPrank(address(scw))is opened inside the modifier but the caller might need to perform assertions from the default test signer later.
Accidentally leaking the prank across internal calls can mask authZ bugs.Consider moving the prank inside each helper or balancing it with
vm.stopPrank()in atry…finallystyleafterhook:modifier withRequiredModules() { … - vm.startPrank(address(scw)); - _; - vm.stopPrank(); + { + vm.startPrank(address(scw)); + _; + vm.stopPrank(); + } }src/modules/validators/ResourceLockValidator.sol (1)
199-215: Hash construction may be non-canonical
abi.encode(_lock.tokenData)encodes a dynamic array, including its length and offset, which changes the hash if token ordering or padding varies.
If ordering is not semantically significant, sorttokenDataor hash each element individually usingabi.encodePacked.
test/modules/ResourceLockValidator/concrete/ResourceLockValidator.t.sol
Outdated
Show resolved
Hide resolved
| function _buildResourceLockHash(ResourceLock memory _lock) internal pure returns (bytes32) { | ||
| return keccak256( | ||
| abi.encode( | ||
| _lock.chainId, | ||
| _lock.smartWallet, | ||
| _lock.sessionKey, | ||
| _lock.validAfter, | ||
| _lock.validUntil, | ||
| abi.encode(_lock.tokenData), | ||
| _lock.nonce | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
_buildResourceLockHash double-encodes the dynamic array
abi.encode(_lock.tokenData) embeds a length prefix plus the entire in-memory layout; two semantically equal arrays with different non-canonical in-memory representation hash to different values.
Safer pattern:
- abi.encode(_lock.tokenData),
+ keccak256(abi.encode(_lock.tokenData)),so the outer hash depends only on the logical content of tokenData, not its memory encoding.
📝 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 _buildResourceLockHash(ResourceLock memory _lock) internal pure returns (bytes32) { | |
| return keccak256( | |
| abi.encode( | |
| _lock.chainId, | |
| _lock.smartWallet, | |
| _lock.sessionKey, | |
| _lock.validAfter, | |
| _lock.validUntil, | |
| abi.encode(_lock.tokenData), | |
| _lock.nonce | |
| ) | |
| ); | |
| } | |
| function _buildResourceLockHash(ResourceLock memory _lock) internal pure returns (bytes32) { | |
| return keccak256( | |
| abi.encode( | |
| _lock.chainId, | |
| _lock.smartWallet, | |
| _lock.sessionKey, | |
| _lock.validAfter, | |
| _lock.validUntil, | |
| - abi.encode(_lock.tokenData), | |
| + keccak256(abi.encode(_lock.tokenData)), | |
| _lock.nonce | |
| ) | |
| ); | |
| } |
| function etchEntrypoint() returns (IEntryPoint) { | ||
| address payable entryPoint = payable(address(new EntryPointSimulationsPatch())); | ||
| etch(ENTRYPOINT_ADDR, entryPoint.code); | ||
| EntryPointSimulationsPatch(payable(ENTRYPOINT_ADDR)).init(entryPoint); | ||
|
|
There was a problem hiding this comment.
Wrong address passed to init – breaks deterministic SenderCreator
etchEntrypoint() calls init(entryPoint) where entryPoint is the temporary address of the freshly-deployed patch, not the final ENTRYPOINT_ADDR.
Consequently _entrypointAddr inside the contract differs from its on-chain address and the RLP-based calculation in initSenderCreator() produces an invalid SenderCreator.
- EntryPointSimulationsPatch(payable(ENTRYPOINT_ADDR)).init(entryPoint);
+ EntryPointSimulationsPatch(payable(ENTRYPOINT_ADDR)).init(ENTRYPOINT_ADDR);Without this fix any CREATE-based simulations relying on the sender creator may revert or emit unexpected addresses.
📝 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 etchEntrypoint() returns (IEntryPoint) { | |
| address payable entryPoint = payable(address(new EntryPointSimulationsPatch())); | |
| etch(ENTRYPOINT_ADDR, entryPoint.code); | |
| EntryPointSimulationsPatch(payable(ENTRYPOINT_ADDR)).init(entryPoint); | |
| function etchEntrypoint() returns (IEntryPoint) { | |
| address payable entryPoint = payable(address(new EntryPointSimulationsPatch())); | |
| etch(ENTRYPOINT_ADDR, entryPoint.code); | |
| - EntryPointSimulationsPatch(payable(ENTRYPOINT_ADDR)).init(entryPoint); | |
| + EntryPointSimulationsPatch(payable(ENTRYPOINT_ADDR)).init(ENTRYPOINT_ADDR); | |
| } |
| 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.
🛠️ Refactor suggestion
Hard-coded calldata offsets are brittle
_getArrayInfo relies on magic numbers (260, 292, 100). Any abi-encoding change will silently break decoding.
Consider packing the array length immediately after the array pointer or, preferably, switch to abi.decode on a typed struct:
struct ExecHeader {
uint256 chainId;
address smartWallet;
...
TokenData[] tokenData;
}
ExecHeader memory h = abi.decode(execData, (ExecHeader));| function onInstall(bytes calldata _data) external override { | ||
| address owner = address(bytes20(_data[_data.length - 20:])); | ||
| if (validatorStorage[msg.sender].enabled) { | ||
| revert RLV_AlreadyInstalled(msg.sender, validatorStorage[msg.sender].owner); | ||
| } | ||
| validatorStorage[msg.sender].owner = owner; | ||
| validatorStorage[msg.sender].enabled = true; | ||
| emit RLV_ValidatorEnabled(msg.sender, owner); | ||
| } |
There was a problem hiding this comment.
Validate _data length and zero-address owner on install
onInstall blindly slices the last 20 bytes of _data and stores them as the owner.
If _data.length < 20, or if those 20 bytes decode to address(0), the validator either reverts with a low-level slice panic or leaves the contract in an unusable state.
function onInstall(bytes calldata _data) external override {
- address owner = address(bytes20(_data[_data.length - 20:]));
+ require(_data.length >= 20, "RLV: install data too short");
+ address owner = address(bytes20(_data[_data.length - 20:]));
+ require(owner != address(0), "RLV: zero owner");📝 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 onInstall(bytes calldata _data) external override { | |
| address owner = address(bytes20(_data[_data.length - 20:])); | |
| if (validatorStorage[msg.sender].enabled) { | |
| revert RLV_AlreadyInstalled(msg.sender, validatorStorage[msg.sender].owner); | |
| } | |
| validatorStorage[msg.sender].owner = owner; | |
| validatorStorage[msg.sender].enabled = true; | |
| emit RLV_ValidatorEnabled(msg.sender, owner); | |
| } | |
| function onInstall(bytes calldata _data) external override { | |
| require(_data.length >= 20, "RLV: install data too short"); | |
| address owner = address(bytes20(_data[_data.length - 20:])); | |
| require(owner != address(0), "RLV: zero owner"); | |
| if (validatorStorage[msg.sender].enabled) { | |
| revert RLV_AlreadyInstalled(msg.sender, validatorStorage[msg.sender].owner); | |
| } | |
| validatorStorage[msg.sender].owner = owner; | |
| validatorStorage[msg.sender].enabled = true; | |
| emit RLV_ValidatorEnabled(msg.sender, owner); | |
| } |
| // or if signature.length >= 65 (standard signature length + proof packing) | ||
| ResourceLock memory rl = _getResourceLock(userOp.callData); | ||
| // Nonce validation | ||
| 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.
🛠️ Refactor suggestion
Nonce and chain-context are unchecked; lock validity window ignored
rl.nonceis taken fromvalidatorStorage[scw].nonce; an attacker could craft calldata with an arbitraryscwthat is notmsg.sender, thus bypassing the correct nonce.validAfter/validUntilare never compared toblock.timestamp; an expired lock can still pass.rl.chainIdis not compared toblock.chainid.
ResourceLock memory rl = _getResourceLock(userOp.callData);
+require(rl.smartWallet == msg.sender, "RLV: scw mismatch");
+require(rl.chainId == block.chainid, "RLV: wrong chain");
+require(
+ rl.validAfter <= block.timestamp && block.timestamp <= rl.validUntil,
+ "RLV: lock expired/too early"
+);📝 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.
| // or if signature.length >= 65 (standard signature length + proof packing) | |
| ResourceLock memory rl = _getResourceLock(userOp.callData); | |
| // Nonce validation | |
| 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; | |
| } | |
| // or if signature.length >= 65 (standard signature length + proof packing) | |
| ResourceLock memory rl = _getResourceLock(userOp.callData); | |
| // Ensure the lock was actually issued for this wallet | |
| require(rl.smartWallet == msg.sender, "RLV: scw mismatch"); | |
| // Prevent replay across chains | |
| require(rl.chainId == block.chainid, "RLV: wrong chain"); | |
| // Enforce the lock’s valid time window | |
| require( | |
| rl.validAfter <= block.timestamp && block.timestamp <= rl.validUntil, | |
| "RLV: lock expired/too early" | |
| ); | |
| // Nonce validation | |
| 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; |
| 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.
Require extended signature ≥ 97 bytes before slicing
signature[65:97] and signature[97:] will revert if signature.length is 66–96.
-bytes32 root = bytes32(signature[65:97]); // 32 bytes
-bytes32[] memory proof = abi.decode(signature[97:], (bytes32[])); // Rest of bytes
+require(signature.length >= 97, "RLV: short extended sig");
+bytes32 root = bytes32(signature[65:97]);
+bytes32[] memory proof = abi.decode(signature[97:], (bytes32[]));📝 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 (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; | |
| } | |
| // Ensure extended signature has at least 97 bytes before slicing | |
| require(signature.length >= 97, "RLV: short extended sig"); | |
| bytes32 root = bytes32(signature[65:97]); // 32 bytes | |
| bytes32[] memory proof = abi.decode(signature[97:], (bytes32[])); // Rest of bytes | |
| 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; |
- Proof will now unpack correctly for non-existent proof, single proof and multiple proofs. - Non-existent proof will now fail gracefully rather than just via `EVMError`. - Removed `nonce` from ResourceLock and added `bidHash`. - Fixed test helper functions to correctly apply proof packing. - Fixed test suite and added new tests for checking mismatched nonces and correct signature unpacking.
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
script/CredibleAccountScript.s.sol (1)
1-96: 🛠️ Refactor suggestionAdd error handling to the deployment script
The script lacks error handling for deployment failures. Consider adding try/catch blocks to handle potential deployment issues gracefully.
function run() external { HookMultiPlexer hookMultiPlexer; CredibleAccountModule credibleAccountModule; ResourceLockValidator resourceLockValidator; ProofVerifier proofVerifier; uint256 deployerPrivateKey = vm.envUint("DEPLOYER_PRIVATE_KEY"); + + // Track deployment success + bool success = true; + string memory failureReason = ""; + vm.startBroadcast(deployerPrivateKey); /*////////////////////////////////////////////////////////////// Starting Deployment //////////////////////////////////////////////////////////////*/ console2.log("Starting deployment sequence..."); /*////////////////////////////////////////////////////////////// Deploy HookMultiPlexer //////////////////////////////////////////////////////////////*/ console2.log("Deploying HookMultiPlexer..."); + try new HookMultiPlexer() returns (HookMultiPlexer _hookMultiPlexer) { + hookMultiPlexer = _hookMultiPlexer; + console2.log("HookMultiPlexer deployed at address", address(hookMultiPlexer)); + } catch Error(string memory reason) { + console2.log("Failed to deploy HookMultiPlexer:", reason); + success = false; + failureReason = reason; + } catch { + console2.log("Failed to deploy HookMultiPlexer: Unknown error"); + success = false; + failureReason = "Unknown error during HookMultiPlexer deployment"; + } + + // Only continue if previous deployment succeeded + if (!success) { + console2.log("Deployment sequence failed:", failureReason); + vm.stopBroadcast(); + return; + } - hookMultiPlexer = new HookMultiPlexer(); - console2.log("HookMultiPlexer deployed at address", address(hookMultiPlexer)); // Apply similar try/catch pattern to other contract deploymentssrc/modules/hooks/HookMultiPlexer.sol (1)
124-133:⚠️ Potential issueLogic bug – hooks are deleted before they are iterated.
delete $config.hooks[HookType.GLOBAL];zeroes the array, so the subsequent loop iterates over an empty list and never callsonUninstallon the sub-hooks.Move the deletion after the loop:
- delete $config.hooks[HookType.GLOBAL]; uint256 length = $config.hooks[HookType.GLOBAL].length; for (uint256 i = 0; i < length; i++) { address hookAddress = $config.hooks[HookType.GLOBAL][i]; _removeHook(hookAddress, HookType.GLOBAL); } + delete $config.hooks[HookType.GLOBAL];
♻️ Duplicate comments (7)
src/common/Structs.sol (1)
60-80: Add NatSpec documentation for hook-related structsThe hook-related structs still lack NatSpec documentation. Please add comprehensive documentation to maintain consistency with other structs in the file.
// HookMultiplexer +/// @title SigHookInit +/// @notice Struct for initializing signature hooks. +/// @dev Used to configure function-specific hooks. struct SigHookInit { bytes4 sig; address[] subHooks; } +/// @title HookAndContext +/// @notice Struct containing a hook address alongside its context. +/// @dev Used to manage hook execution with associated context data. struct HookAndContext { address hook; bytes context; } +/// @title SignatureHooks +/// @notice Struct for managing function-specific hooks. +/// @dev Maps function signatures to their associated hooks. struct SignatureHooks { bytes4[] allSigs; mapping(bytes4 => address[]) sigHooks; } +/// @title Config +/// @notice Configuration struct for hook management. +/// @dev Manages initialization state and hook mappings by type. struct Config { bool initialized; mapping(HookType hookType => address[]) hooks; mapping(HookType hookType => SignatureHooks) sigHooks; }test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol (1)
54-66:⚠️ Potential issueFix double-encoding of dynamic array in
_buildResourceLockHash
abi.encode(_lock.tokenData)embeds a length prefix plus the entire in-memory layout. Two semantically equal arrays with different non-canonical in-memory representation will hash to different values.Apply this safer pattern:
function _buildResourceLockHash(ResourceLock memory _lock) internal pure returns (bytes32) { return keccak256( abi.encode( _lock.chainId, _lock.smartWallet, _lock.sessionKey, _lock.validAfter, _lock.validUntil, _lock.bidHash, - abi.encode(_lock.tokenData) + keccak256(abi.encode(_lock.tokenData)) ) ); }This ensures the outer hash depends only on the logical content of
tokenData, not its memory encoding.src/modules/validators/ResourceLockValidator.sol (4)
53-61: No length / zero-address checks inonInstall.An empty
_dataslice reverts with a slicing panic, or worse, installs a validator withowner == address(0).- address owner = address(bytes20(_data[_data.length - 20:])); + require(_data.length >= 20, "RLV: install data too short"); + address owner = address(bytes20(_data[_data.length - 20:])); + require(owner != address(0), "RLV: zero owner");
76-88:validateUserOpmisses a minimal signature length guard.If
signature.length < 65, the slice operations revert.
Insert a defensive check:+ require(signature.length >= 65, "RLV: short signature");
184-187: Hard-coded calldata offsets are brittle and undocumented.
_getArrayInfoassumes fixed positions (292, 324, 100). Any change to the encoded struct breaks decoding.Refactor to an
abi.decodeapproach or at least centralise the constants with comments describing the layout.
196-219:⚠️ Potential issueFunction can fall through without returning a value → compilation error.
If
bytes4(_callData)is notexecute.selector, the function reaches the end withoutreturn/revert, violating its declared return type.Add an explicit revert:
- } + } + revert("RLV: unsupported calldata");test/modules/CredibleAccountModule/concrete/CredibleAccountModule.t.sol (1)
528-530: Hard-coded main-net token addresses – same concern as earlier reviewPrevious feedback about replacing magic addresses with the existing in-memory
usdc/daivariables still applies.
🧹 Nitpick comments (13)
test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol (2)
18-26: Add explanatory comments to thewithRequiredModulesmodifierThe modifier installs multiple modules and hooks without explaining their purpose or relationships.
modifier withRequiredModules() { + // Install ECDSA validator for basic signature verification _installModule(eoa.pub, scw, MODULE_TYPE_VALIDATOR, address(moecdsav), hex""); + // Install Credible Account Module as a global hook for session key management _installHookViaMultiplexer(scw, address(cam), HookType.GLOBAL); + // Install Credible Account Module as a validator _installModule(eoa.pub, scw, MODULE_TYPE_VALIDATOR, address(cam), abi.encode(MODULE_TYPE_VALIDATOR)); + // Install Resource Lock Validator with EOA as the trusted validator _installModule(eoa.pub, scw, MODULE_TYPE_VALIDATOR, address(rlv), abi.encode(eoa.pub)); vm.startPrank(address(scw)); _; vm.stopPrank(); }
76-79: Use constants instead of magic numbers for time valuesThe hardcoded values
1732176210and1732435407forvalidAfterandvalidUntilshould be declared as constants with explanatory names.+// Define time constants at contract level +uint48 private constant VALID_AFTER_TIME = 1732176210; // May 21, 2024 +uint48 private constant VALID_UNTIL_TIME = 1732435407; // May 24, 2024 function _generateResourceLock(address _scw, address _sk) internal view returns (ResourceLock memory) { TokenData[] memory td = new TokenData[](2); td[0] = TokenData({token: address(usdt), amount: 100}); td[1] = TokenData({token: address(dai), amount: 200}); ResourceLock memory rl = ResourceLock({ chainId: 42161, smartWallet: _scw, sessionKey: _sk, - validAfter: 1732176210, - validUntil: 1732435407, + validAfter: VALID_AFTER_TIME, + validUntil: VALID_UNTIL_TIME, bidHash: DUMMY_BID_HASH, tokenData: td }); return rl; }test/modules/ResourceLockValidator/concrete/ResourceLockValidator.t.sol (3)
555-567: Document the purpose and usage of the skipped testThe
test_signature_unpackingtest is skipped by default and lacks clear documentation on how to use it for debugging.- // NOTE: This test is for testing specific signatures to check correct unpacking - // Replace the op.signature with your own, add logs and run test - // Test will fail with RLV_ResourceLockHashNotInProof() + /// @notice Test for debugging signature unpacking mechanism + /// @dev This test is skipped by default and intended for manual debugging + /// How to use: + /// 1. Replace op.signature with your own signature to debug + /// 2. Add console.log statements to inspect unpacked components + /// 3. Run with `forge test --match-test test_signature_unpacking -v` + /// 4. Expected failure: RLV_ResourceLockHashNotInProof() function test_signature_unpacking() public withRequiredModules { vm.skip(true); // Create UserOp with ResourceLock (PackedUserOperation memory op,, bytes32[] memory proof, bytes32 merkleRoot) = _createUserOpWithResourceLock(address(scw), sessionKey, true); // Use predefined sig op.signature = hex"137ad66810b0325f2820c1f9160c2076a1607e5fd7010c4b02368b3905bccef1222086c638e9d828464dcc6330517430cd93516969b23612e3e41199f65950621b4a2c9276c86b3c670b424ab981c89c53f858e870f31a2999cf52353837897362bc36789e7a1e281436464229828f817d6612f7b477d66591ff96a9e064bcc98a"; _executeUserOp(op); }
414-445: Extract token setup to a helper functionMultiple test functions (
test_validateUserOp_DirectMerkleSignature_LargeTokenDataandtest_validateUserOp_EthSignedMerkleSignature_LargeTokenData) use nearly identical code for setting up token data and verification.Extract the common logic to a helper function to reduce duplication:
+ /// @notice Helper function to create and validate a user operation with large token data + /// @param signType Type of signature to use (0 = direct, 1 = ethSigned) + /// @return op The executed user operation + function _createAndValidateUserOpWithLargeTokenData(uint8 signType) + internal + returns (PackedUserOperation memory op) + { + // Create UserOp + op = _createUserOp(address(scw), address(rlv)); + // Create TokenData array with 10 entries + TokenData[] memory tokens = new TokenData[](10); + for (uint256 i; i < 10; ++i) { + tokens[i] = TokenData({token: vm.randomAddress(), amount: vm.randomUint()}); + } + // Create ResourceLock with large TokenData array + ResourceLock memory rl = _generateResourceLock(address(scw), sessionKey.pub); + rl.tokenData = tokens; + // Generate proof and merkle root + (bytes32[] memory proof, bytes32 merkleRoot,) = getTestProof(_buildResourceLockHash(rl), true); + // Create UserOp calldata + op.callData = abi.encodeCall( + IERC7579Account.execute, + ( + ModeLib.encodeSimpleSingle(), + ExecutionLib.encodeSingle( + address(cam), 0, abi.encodeWithSelector(cam.enableSessionKey.selector, abi.encode(rl)) + ) + ) + ); + // Sign merkle root + bytes memory sig; + if (signType == 0) { + sig = _sign(merkleRoot, eoa); + } else { + sig = _ethSign(merkleRoot, eoa); + } + op.signature = bytes.concat(sig, abi.encodePacked(merkleRoot), _packProofForSignature(proof)); + _executeUserOp(op); + + // Check locked tokens + ICredibleAccountModule.LockedToken[] memory locked = cam.getLockedTokensForSessionKey(sessionKey.pub); + assertEq(locked.length, 10); + for (uint256 i; i < 10; ++i) { + assertEq(locked[i].token, tokens[i].token); + assertEq(locked[i].lockedAmount, tokens[i].amount); + } + + return op; + } function test_validateUserOp_DirectMerkleSignature_LargeTokenData() public withRequiredModules { - // Create UserOp - PackedUserOperation memory op = _createUserOp(address(scw), address(rlv)); - // Create TokenData array with 10 entries - TokenData[] memory tokens = new TokenData[](10); - for (uint256 i; i < 10; ++i) { - tokens[i] = TokenData({token: vm.randomAddress(), amount: vm.randomUint()}); - } - // Create ResourceLock with large TokenData array - ResourceLock memory rl = _generateResourceLock(address(scw), sessionKey.pub); - rl.tokenData = tokens; - // Generate proof and merkle root - (bytes32[] memory proof, bytes32 merkleRoot,) = getTestProof(_buildResourceLockHash(rl), true); - // Create UserOp calldata - op.callData = abi.encodeCall( - IERC7579Account.execute, - ( - ModeLib.encodeSimpleSingle(), - ExecutionLib.encodeSingle( - address(cam), 0, abi.encodeWithSelector(cam.enableSessionKey.selector, abi.encode(rl)) - ) - ) - ); - // Sign merkle root with eth prefix - bytes memory sig = _sign(merkleRoot, eoa); - op.signature = bytes.concat(sig, abi.encodePacked(merkleRoot), _packProofForSignature(proof)); - _executeUserOp(op); - // Check locked tokens - ICredibleAccountModule.LockedToken[] memory locked = cam.getLockedTokensForSessionKey(sessionKey.pub); - assertEq(locked.length, 10); - for (uint256 i; i < 10; ++i) { - assertEq(locked[i].token, tokens[i].token); - assertEq(locked[i].lockedAmount, tokens[i].amount); - } + _createAndValidateUserOpWithLargeTokenData(0); }And similarly update the corresponding eth-sign test function.
262-262: Use specific error selectors in revert expectationsThe test uses
bytes4(0)as the expected error selector in several places. This seems to indicate a general low-level Solidity error rather than a specific error from the contract.Consider checking for specific error selectors that better represent what should happen, or at least add a comment explaining why
bytes4(0)is expected.Also applies to: 274-274, 286-286
script/CredibleAccountScript.s.sol (2)
12-16: Unused constants and placeholdersThe script contains several constants that are either unused or have placeholder values (
0x0000...). These should either be properly set or removed to prevent confusion.- bytes32 public immutable SALT = bytes32(abi.encodePacked("ModularEtherspotWallet:Create2:salt")); - address public constant DEPLOYER = 0x09FD4F6088f2025427AB1e89257A44747081Ed59; - address public constant EXPECTED_MULTIPLEXER_ADDRESS = 0x0000000000000000000000000000000000000000; - address public constant EXPECTED_CA_MODULE_ADDRESS = 0x0000000000000000000000000000000000000000; - address public constant EXPECTED_RESOURCE_LOCK_VALIDATOR_ADDRESS = 0x0000000000000000000000000000000000000000; + // TODO: Set up actual expected addresses for deterministic deployment verification + // or remove if not needed for the deployment flow
37-46: Clean up commented deployment verification codeThe script contains three sections of commented code that are intended for verifying deployment addresses. Since this functionality isn't being used, consider either implementing it fully or removing the comments to maintain a cleaner codebase.
Instead of keeping the commented verification code in three places, implement a helper function for verification:
+ /** + * @notice Helper to verify deployed address matches expected address + * @param deployed The actual deployed contract address + * @param expected The expected contract address + * @param contractName The name of the contract for logging + */ + function _verifyDeployment(address deployed, address expected, string memory contractName) internal { + if (expected != address(0) && deployed != expected) { + revert(string.concat("Unexpected ", contractName, " address!!!")); + } + console2.log(string.concat(contractName, " deployed at address"), deployed); + } function run() external { // ...existing code... console2.log("Deploying HookMultiPlexer..."); - // if (EXPECTED_MULTIPLEXER_ADDRESS.code.length == 0) { hookMultiPlexer = new HookMultiPlexer(); - // 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); - // } + _verifyDeployment(address(hookMultiPlexer), EXPECTED_MULTIPLEXER_ADDRESS, "HookMultiPlexer"); // ... apply similar changes to other deployment sections ...Then uncomment this function once you have actual expected addresses to verify against.
Also applies to: 61-70, 77-86
src/modules/hooks/HookMultiPlexer.sol (2)
166-184:getHooksomits SIG/TARGET_SIG uniqueness across nested mappings.
SigHookentries are added viastoreSelectorHooks, butgetHooksonly flattensallSigsvalues, not the actual addresses per selector.
Consumers calling this view may get incomplete data.Consider iterating over
sigHooks.allSigsand joining each array of hooks per selector, or expose a dedicated getter for sig hooks.
198-208:addHookallows silent duplication.Hooks are blindly
push-ed; multiple identical hooks of the same type will accumulate and waste gas inpreCheck().Add an existence check before pushing, or use
pushUniqueas done elsewhere:- $getConfig({account: msg.sender}).hooks[hookType].push(hook); + $getConfig({account: msg.sender}).hooks[hookType].pushUnique(hook);src/modules/validators/ResourceLockValidator.sol (1)
225-236: Docstring/outcome mismatch – nonce omitted from hash.The Natspec says the nonce is included, but
_buildResourceLockHashleaves it out (bidHashis included instead).
Either update the docs or add the nonce field to prevent ambiguous lock hashes.test/modules/CredibleAccountModule/fuzz/CredibleAccountModule.t.sol (1)
128-129: Remove debugconsole2.login production-grade tests
console2.logslows fuzzing and pollutes the output; keep it only while developing.- console2.log("sessionKeyData.validUntil", sessionKeyData.validUntil);test/modules/CredibleAccountModule/utils/CredibleAccountModuleTestUtils.sol (1)
125-145: Unused_userparameter in_claimTokensBySolver– dead code & API noise
_useris never referenced, making the signature misleading and hiding potential mistakes at call sites.-function _claimTokensBySolver( - User memory _user, +function _claimTokensBySolver((also drop the argument from all invocations)
test/modules/CredibleAccountModule/concrete/CredibleAccountModule.t.sol (1)
544-547: Dead variables increase cognitive load
enableSessionKeyData2/newRlare prepared but never used, so the test does the same work twice and risks diverging expectations.- bytes memory enableSessionKeyData2 = abi.encodeWithSelector(CAM.enableSessionKey.selector, newRl);(If the second payload was meant to be executed, add it to the batch; otherwise delete the unused code.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
broadcast/CredibleAccountScript.s.sol/11155111/run-1746104233.json(1 hunks)broadcast/CredibleAccountScript.s.sol/11155111/run-latest.json(1 hunks)script/CredibleAccountScript.s.sol(1 hunks)src/common/Structs.sol(1 hunks)src/modules/hooks/HookMultiPlexer.sol(14 hunks)src/modules/validators/ResourceLockValidator.sol(1 hunks)test/ModularTestBase.sol(1 hunks)test/modules/CredibleAccountModule/concrete/CredibleAccountModule.t.sol(5 hunks)test/modules/CredibleAccountModule/fuzz/CredibleAccountModule.t.sol(4 hunks)test/modules/CredibleAccountModule/utils/CredibleAccountModuleTestUtils.sol(1 hunks)test/modules/ResourceLockValidator/concrete/ResourceLockValidator.t.sol(1 hunks)test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- broadcast/CredibleAccountScript.s.sol/11155111/run-latest.json
- broadcast/CredibleAccountScript.s.sol/11155111/run-1746104233.json
🔇 Additional comments (2)
src/common/Structs.sol (1)
56-56: Field type changed fromuint256 noncetobytes32 bidHashThe
ResourceLockstruct has been updated to usebytes32 bidHashinstead of the previousuint256 noncefield. This change aligns with the newResourceLockValidatormodule implementation that uses the bidHash for signature verification and Merkle proof validation.test/modules/ResourceLockValidator/concrete/ResourceLockValidator.t.sol (1)
292-304:❌ Incorrect review comment
Check for specific revert reason in empty proof test
While the test correctly expects a revert with
RLV_ResourceLockHashNotInProof, it might be more accurate to check for a specific error related to empty proof arrays.
🏁 Script executed:
#!/bin/bash # Check if ResourceLockValidator has a specific error for empty proofs # Look for specific error constant in the ResourceLockValidator contract rg -A 1 "error RLV_" --include "*.sol" src/modules/validators/ResourceLockValidator.solLength of output: 531
#!/bin/bash # Correctly search for ResourceLockValidator error constants in .sol files rg -A1 "error RLV_" -g '*.sol' src/modules/validators
#!/bin/bash # List all RLV_ error definitions in validators rg -n "error RLV_" -g "*.sol" src/modules/validators # List all error declarations in ResourceLockValidator.sol rg -n "error " src/modules/validators/ResourceLockValidator.sol
#!/bin/bash # Search for ResourceLockValidator error definitions related to empty proofs rg -n "error RLV_" -g "*.sol" src/modules/validators
No specific empty-proof error exists—test is correct as written
TheResourceLockValidatorcontract only definesRLV_ResourceLockHashNotInProoffor proof-related failures; there isn’t a distinct error for empty proof arrays. Your test already asserts the correct revert reason, so no change is needed.Likely an incorrect or invalid review comment.
| vm.startPrank(_owner); | ||
| // Execute the module installation | ||
| mockExec.executeViaAccount( | ||
| IERC7579Account(_scw), | ||
| address(_scw), | ||
| 0, | ||
| 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(); | ||
| // Verify that the module is installed |
There was a problem hiding this comment.
vm.stopPrank() is skipped on early return → prank leaks into subsequent code.
When installing a fallback module the function returns inside the if, leaving the prank active.
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;
}📝 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); | |
| // Execute the module installation | |
| mockExec.executeViaAccount( | |
| IERC7579Account(_scw), | |
| address(_scw), | |
| 0, | |
| 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(); | |
| // Verify that the module is installed | |
| vm.startPrank(_owner); | |
| // Execute the module installation | |
| mockExec.executeViaAccount( | |
| IERC7579Account(_scw), | |
| address(_scw), | |
| 0, | |
| 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(); | |
| // Verify that the module is installed |
| vm.startPrank(_owner); | ||
| if (_moduleType == MODULE_TYPE_FALLBACK) { | ||
| mockExec.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 | ||
| mockExec.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.
🛠️ Refactor suggestion
Unreachable vm.stopPrank() and missing stop on early return.
The return statements above line 349 prevent the final vm.stopPrank() from executing, again keeping the prank open.
Move vm.stopPrank() before each return, or use a try…finally pattern:
- return _scw.isModuleInstalled(_moduleType, _module, "");
- vm.stopPrank();
+ bool uninstalled = _scw.isModuleInstalled(_moduleType, _module, "");
+ vm.stopPrank();
+ return uninstalled;Committable suggestion skipped: line range outside the PR's diff.
| BootstrapConfig[] memory validators = new BootstrapConfig[](1); | ||
| validators[0] = _makeBootstrapConfig(address(mockVal), ""); | ||
| BootstrapConfig[] memory executors = makeBootstrapConfig(address(mockExec), ""); | ||
| BootstrapConfig memory hook = _makeBootstrapConfig(address(hmp), hmpData); | ||
| BootstrapConfig[] memory fallbacks = makeBootstrapConfig(address(0), ""); | ||
| bytes memory _initCode = abi.encode( | ||
| _owner, |
There was a problem hiding this comment.
makeBootstrapConfig call is undefined / wrong overload.
makeBootstrapConfig is not declared in this contract and the imported BootstrapUtil exposes _makeBootstrapConfig (note the leading underscore) that returns a single BootstrapConfig.
Compiling will fail here.
- BootstrapConfig[] memory executors = makeBootstrapConfig(address(mockExec), "");
+ BootstrapConfig[] memory executors = new BootstrapConfig[](1);
+ executors[0] = _makeBootstrapConfig(address(mockExec), "");📝 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.
| BootstrapConfig[] memory validators = new BootstrapConfig[](1); | |
| validators[0] = _makeBootstrapConfig(address(mockVal), ""); | |
| BootstrapConfig[] memory executors = makeBootstrapConfig(address(mockExec), ""); | |
| BootstrapConfig memory hook = _makeBootstrapConfig(address(hmp), hmpData); | |
| BootstrapConfig[] memory fallbacks = makeBootstrapConfig(address(0), ""); | |
| bytes memory _initCode = abi.encode( | |
| _owner, | |
| BootstrapConfig[] memory validators = new BootstrapConfig[](1); | |
| validators[0] = _makeBootstrapConfig(address(mockVal), ""); | |
| BootstrapConfig[] memory executors = new BootstrapConfig[](1); | |
| executors[0] = _makeBootstrapConfig(address(mockExec), ""); | |
| BootstrapConfig memory hook = _makeBootstrapConfig(address(hmp), hmpData); | |
| BootstrapConfig[] memory fallbacks = makeBootstrapConfig(address(0), ""); | |
| bytes memory _initCode = abi.encode( | |
| _owner, |
| impl = new ModularEtherspotWallet(); | ||
| factory = new ModularEtherspotWalletFactory(address(impl), eoa.pub); | ||
| moecdsav = new MultipleOwnerECDSAValidator(); |
There was a problem hiding this comment.
Factory uses an un-initialised owner (likely address(0)).
eoa is populated later in _testInit, so at this point eoa.pub is the default zero address.
This means every wallet produced by the factory in the test run will be owned by address(0), which can mask bugs or cause silent failures in owner-checks.
- factory = new ModularEtherspotWalletFactory(address(impl), eoa.pub);
+ // Initialise `eoa` first or pass the intended owner explicitly
+ eoa = _createUser("EOA");
+ factory = new ModularEtherspotWalletFactory(address(impl), eoa.pub);📝 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.
| impl = new ModularEtherspotWallet(); | |
| factory = new ModularEtherspotWalletFactory(address(impl), eoa.pub); | |
| moecdsav = new MultipleOwnerECDSAValidator(); | |
| impl = new ModularEtherspotWallet(); | |
| // Initialise `eoa` first or pass the intended owner explicitly | |
| eoa = _createUser("EOA"); | |
| factory = new ModularEtherspotWalletFactory(address(impl), eoa.pub); | |
| moecdsav = new MultipleOwnerECDSAValidator(); |
| // Nonce validation | ||
| bytes memory ecdsaSignature = signature[0:65]; | ||
| bytes32 root = bytes32(signature[65:97]); // 32 bytes | ||
| bytes32[] memory proof; | ||
| if (signature.length > 97) { | ||
| // Calculate how many proof elements we have | ||
| uint256 proofCount = (signature.length - 97) / 32; | ||
| // Create an array of the right size | ||
| proof = new bytes32[](proofCount); | ||
| // Extract each proof element | ||
| for (uint256 i; i < proofCount; ++i) { | ||
| uint256 startPos = 97 + (i * 32); | ||
| proof[i] = bytes32(signature[startPos:startPos + 32]); | ||
| } | ||
| } else { | ||
| // Empty proof | ||
| proof = new bytes32[](0); | ||
| } | ||
| if (!MerkleProofLib.verify(proof, root, _buildResourceLockHash(rl))) { | ||
| revert RLV_ResourceLockHashNotInProof(); | ||
| } | ||
| // check proof is signed |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Resource-lock specific verification ignores chain, wallet, and time window.
rl is parsed but never validated:
require(rl.smartWallet == msg.sender, "RLV: SCW mismatch");
require(rl.chainId == block.chainid, "RLV: wrong chain");
require(rl.validAfter <= block.timestamp && block.timestamp <= rl.validUntil,
"RLV: lock expired / not yet valid");Without these, an attacker can replay a lock on another chain, another wallet, or outside the allowed window.
| if (_sessionKey == sessionKey.pub) { | ||
| // Additional checks based on _callData content could be added here | ||
| assertTrue(isValid || !isValid); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ineffective assertion renders the fuzz test meaningless
assertTrue(isValid || !isValid); is a tautology that can never fail, so the branch where _sessionKey == sessionKey.pub provides no verification value.
Either assert the expected relation between _callData and isValid (preferred) or drop the branch entirely.
- // Additional checks based on _callData content could be added here
- assertTrue(isValid || !isValid);
+ // Expect the validator result to be `true` for the wallet’s own session key
+ assertTrue(isValid, "validateSessionKeyParams should return true for the active session key");📝 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 (_sessionKey == sessionKey.pub) { | |
| // Additional checks based on _callData content could be added here | |
| assertTrue(isValid || !isValid); | |
| } else { | |
| if (_sessionKey == sessionKey.pub) { | |
| // Expect the validator result to be `true` for the active session key | |
| assertTrue(isValid, "validateSessionKeyParams should return true for the active session key"); | |
| } else { |
| modifier withRequiredModules() { | ||
| _installModule(eoa.pub, scw, MODULE_TYPE_VALIDATOR, address(moecdsav), hex""); | ||
| _installHookViaMultiplexer(scw, address(cam), HookType.GLOBAL); | ||
| _installModule(eoa.pub, scw, MODULE_TYPE_VALIDATOR, address(cam), abi.encode(MODULE_TYPE_VALIDATOR)); | ||
| vm.startPrank(address(scw)); | ||
| _; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
withRequiredModules never stops its prank – risk of leaking msg.sender across tests
The modifier starts a prank but relies on every test to remember vm.stopPrank(). This is error-prone and already duplicated in every call-site.
- _installModule(eoa.pub, scw, MODULE_TYPE_VALIDATOR, address(cam), abi.encode(MODULE_TYPE_VALIDATOR));
- vm.startPrank(address(scw));
- _;
+ _installModule(eoa.pub, scw, MODULE_TYPE_VALIDATOR, address(cam), abi.encode(MODULE_TYPE_VALIDATOR));
+ vm.startPrank(address(scw));
+ _;
+ vm.stopPrank(); // guarantees cleanup even when a test reverts early📝 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.
| modifier withRequiredModules() { | |
| _installModule(eoa.pub, scw, MODULE_TYPE_VALIDATOR, address(moecdsav), hex""); | |
| _installHookViaMultiplexer(scw, address(cam), HookType.GLOBAL); | |
| _installModule(eoa.pub, scw, MODULE_TYPE_VALIDATOR, address(cam), abi.encode(MODULE_TYPE_VALIDATOR)); | |
| vm.startPrank(address(scw)); | |
| _; | |
| } | |
| modifier withRequiredModules() { | |
| _installModule(eoa.pub, scw, MODULE_TYPE_VALIDATOR, address(moecdsav), hex""); | |
| _installHookViaMultiplexer(scw, address(cam), HookType.GLOBAL); | |
| _installModule(eoa.pub, scw, MODULE_TYPE_VALIDATOR, address(cam), abi.encode(MODULE_TYPE_VALIDATOR)); | |
| vm.startPrank(address(scw)); | |
| _; | |
| vm.stopPrank(); // guarantees cleanup even when a test reverts early | |
| } |
- Added batch `UserOperation` support. - Added tests for batch with direct Merkle signature and Eth signed Merkle signature. - Added utility function for batch `UserOperation` in `ResourceLockValidatorTestUtils`.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
test/ModularTestBase.sol (4)
280-285:makeBootstrapConfigis still undefined – compilation will failPrevious review already highlighted that only
_makeBootstrapConfigexists inBootstrapUtil.
Replace the calls or add a wrapper to avoid a hard compiler error.- BootstrapConfig[] memory executors = makeBootstrapConfig(address(mockExec), ""); + BootstrapConfig[] memory executors = new BootstrapConfig[](1); + executors[0] = _makeBootstrapConfig(address(mockExec), ""); - BootstrapConfig[] memory fallbacks = makeBootstrapConfig(address(0), ""); + BootstrapConfig[] memory fallbacks = new BootstrapConfig[](1); + fallbacks[0] = _makeBootstrapConfig(address(0), "");
304-316: Early-return path leaksvm.prankcontextIn the fallback-module branch the function
returns before callingvm.stopPrank(), so the prank
remains active for the rest of the test – exactly the issue flagged in the previous review.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; }
328-351: Same prank-leak & unreachable code in_uninstallModule
vm.stopPrank()is never hit (there is areturnabove it) and the fallback branch again omits the stop call entirely.
Apply the same fix as for_installModuleto every early-return.
140-143:⚠️ Potential issueFactory is deployed with
owner = address(0)– this breaks all owner-checks downstream
eoais instantiated after these lines, so its default value is the zero address when passed toModularEtherspotWalletFactory.
Every wallet produced by the factory will therefore be owned byaddress(0), masking bugs and allowing anyone to control the wallets.- factory = new ModularEtherspotWalletFactory(address(impl), eoa.pub); + // Initialise the EOA first, then pass the correct owner + eoa = _createUser("EOA"); + factory = new ModularEtherspotWalletFactory(address(impl), eoa.pub);test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol (1)
70-80:tokenDatais double-encoded – identical logical arrays can hash differentlyThe outer
abi.encodeembeds the full in-memory layout of the dynamic array, making the hash
depend on memory representation instead of pure content. Use an innerkeccak256as previously suggested.- abi.encode(_lock.tokenData) + keccak256(abi.encode(_lock.tokenData))
🧹 Nitpick comments (3)
test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol (1)
55-66: Shadowed / unused named return variable hurts readabilityYou declare
userOpas a named return variable but build a localopand return that instead.
Either reuseuserOpor drop the name to avoid confusion.- PackedUserOperation memory op = _createUserOp(_scw, address(rlv)); + userOp = _createUserOp(_scw, address(rlv)); rl = _generateResourceLock(_scw, _user.pub); ... - return (op, rl, proof, root); + return (userOp, rl, proof, root);test/modules/ResourceLockValidator/concrete/ResourceLockValidator.t.sol (2)
226-233: Docstring says “eth-signed” but code uses raw_sign– keep comments and code in syncThe test is still valid (it is meant to fail on wrong proof), yet the mismatch may confuse future readers.
- // Sign merkle root with eth prefix - bytes memory sig = _sign(merkleRoot, eoa); + // Direct signature (not eth-signed), the proof itself is intentionally invalid + bytes memory sig = _sign(merkleRoot, eoa);
582-590: Dead-code helper left behind – consider deleting or gating withforge test --match
vm.skip(true);already disables the body, but the placeholder test bloats the suite.
If it is only for ad-hoc debugging, move it to a separate scratch file or guard it behind
if (false)so it never ships to CI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/modules/validators/ResourceLockValidator.sol(1 hunks)test/ModularTestBase.sol(1 hunks)test/modules/ResourceLockValidator/concrete/ResourceLockValidator.t.sol(1 hunks)test/modules/ResourceLockValidator/utils/ResourceLockValidatorTestUtils.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/validators/ResourceLockValidator.sol
Description
ResourceLockValidatormodule and test suite.TestAdvancedUtilstoModularTestBasethat moves test specific utils into their own contract util files and make utils inModularTestBaseextensible and generic.Motivation and Context
CredibleAccountModulefor validation of resource locks for enabling credible account sessions.How Has This Been Tested?
Screenshots (if appropriate)
Types of changes
Summary by CodeRabbit