From c7b3d0fa2ad719d85aec5f2943f15bb20847f5c0 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 16:03:11 +0200 Subject: [PATCH 01/29] #26 Remove unnecessary module type check in uninstallModule function --- contracts/Nexus.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/contracts/Nexus.sol b/contracts/Nexus.sol index fe01666d..70f4d214 100644 --- a/contracts/Nexus.sol +++ b/contracts/Nexus.sol @@ -200,9 +200,7 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra /// @param deInitData De-initialization data for the module. /// @dev Ensures that the operation is authorized and valid before proceeding with the uninstallation. function uninstallModule(uint256 moduleTypeId, address module, bytes calldata deInitData) external payable onlyEntryPointOrSelf withHook { - require(IModule(module).isModuleType(moduleTypeId), MismatchModuleTypeId(moduleTypeId)); require(_isModuleInstalled(moduleTypeId, module, deInitData), ModuleNotInstalled(moduleTypeId, module)); - emit ModuleUninstalled(moduleTypeId, module); if (moduleTypeId == MODULE_TYPE_VALIDATOR) { From 6f86af4c0222d37e74e4603782af0ffa26740101 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 16:04:40 +0200 Subject: [PATCH 02/29] #25 Update K1Validator to use isValidSignatureNowCalldata method for signature validation --- contracts/modules/validators/K1Validator.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/modules/validators/K1Validator.sol b/contracts/modules/validators/K1Validator.sol index 3bf303c8..5d0f21d4 100644 --- a/contracts/modules/validators/K1Validator.sol +++ b/contracts/modules/validators/K1Validator.sol @@ -84,8 +84,8 @@ contract K1Validator is IValidator { function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash) external view returns (uint256) { address owner = smartAccountOwners[userOp.sender]; if ( - owner.isValidSignatureNow(ECDSA.toEthSignedMessageHash(userOpHash), userOp.signature) || - owner.isValidSignatureNow(userOpHash, userOp.signature) + owner.isValidSignatureNowCalldata(ECDSA.toEthSignedMessageHash(userOpHash), userOp.signature) || + owner.isValidSignatureNowCalldata(userOpHash, userOp.signature) ) { return VALIDATION_SUCCESS; } From 0747d3dc7dd7516700795d019fbda8d694faae39 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 16:12:07 +0200 Subject: [PATCH 03/29] #24 Update RegistryFactory constructor to validate threshold against attesters length --- contracts/factory/RegistryFactory.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/contracts/factory/RegistryFactory.sol b/contracts/factory/RegistryFactory.sol index 9e060e41..116d92e2 100644 --- a/contracts/factory/RegistryFactory.sol +++ b/contracts/factory/RegistryFactory.sol @@ -42,12 +42,18 @@ contract RegistryFactory is Stakeable, INexusFactory { /// @param module The module address that is not whitelisted. error ModuleNotWhitelisted(address module); + /// @notice Error thrown when the threshold exceeds the number of attesters. + /// @param threshold The provided threshold value. + /// @param attestersLength The number of attesters provided. + error InvalidThreshold(uint8 threshold, uint256 attestersLength); + /// @notice Constructor to set the smart account implementation address and owner. /// @param implementation_ The address of the Nexus implementation to be used for all deployments. /// @param owner_ The address of the owner of the factory. constructor(address implementation_, address owner_, IERC7484 registry_, address[] memory attesters_, uint8 threshold_) Stakeable(owner_) { require(implementation_ != address(0), ImplementationAddressCanNotBeZero()); require(owner_ != address(0), ZeroAddressNotAllowed()); + require(threshold_ <= attesters_.length, InvalidThreshold(threshold_, attesters_.length)); REGISTRY = registry_; attesters = attesters_; threshold = threshold_; From 7d2cbb7de8f076d37cf105600224ea52726a131e Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 16:12:20 +0200 Subject: [PATCH 04/29] chore: Update RegistryFactory constructor to validate threshold against attesters length --- .../factory/TestRegistryFactory_Deployments.t.sol | 12 ++++++++++++ .../factory/TestRegistryFactory_Deployments.tree | 3 ++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol b/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol index 9f6c55b0..73637e5c 100644 --- a/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol +++ b/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol @@ -49,6 +49,18 @@ contract TestRegistryFactory_Deployments is NexusTest_Base { new RegistryFactory(address(0), address(this), registry, attestersArray, 1); } + /// @notice Tests that the constructor reverts if the threshold is greater than the length of the attesters array. + function test_Constructor_RevertIf_ThresholdExceedsAttestersLength() public { + address implementation = address(0x123); + address; + attestersArray[0] = address(0x789); + + // Expect the constructor to revert because the threshold (2) is greater than the number of attesters (1) + vm.expectRevert(abi.encodeWithSelector(InvalidThreshold.selector, 2, attestersArray.length)); + new RegistryFactory(implementation, address(this), registry, attestersArray, 2); + } + + /// @notice Tests adding and removing attesters from the registry. function test_AddRemoveAttester() public { address attester = address(0x456); diff --git a/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.tree b/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.tree index 676840b8..69d5d9b7 100644 --- a/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.tree +++ b/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.tree @@ -3,7 +3,8 @@ TestRegistryFactory_Deployments ├── when the constructor is called │ ├── it should set the implementation address correctly │ ├── it should revert if the owner address is zero - │ └── it should revert if the implementation address is zero + │ ├── it should revert if the implementation address is zero + │ └── it should revert if the threshold exceeds the length of attesters ├── when managing the attesters │ ├── it should add an attester │ ├── it should add multiple attesters From 7f237b8ac8516bed8649b702abce5e84718aba39 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 16:21:43 +0200 Subject: [PATCH 05/29] #15: Fix validator check and revert error in Nexus initialization --- contracts/Nexus.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contracts/Nexus.sol b/contracts/Nexus.sol index 70f4d214..f7960853 100644 --- a/contracts/Nexus.sol +++ b/contracts/Nexus.sol @@ -218,6 +218,9 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra _initModuleManager(); (address bootstrap, bytes memory bootstrapCall) = abi.decode(initData, (address, bytes)); (bool success, ) = bootstrap.delegatecall(bootstrapCall); + + SentinelListLib.SentinelList storage validators = _getAccountStorage().validators; + require(!(prev == address(0x01) && validators.getNext(validator) == address(0x01)), CannotRemoveLastValidator()); require(success, NexusInitializationFailed()); } From 07162cecdc20f1c2715387136875cf13b52b4e7c Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 16:22:01 +0200 Subject: [PATCH 06/29] remove empty line --- .../unit/concrete/factory/TestRegistryFactory_Deployments.t.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol b/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol index 73637e5c..61e76a4c 100644 --- a/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol +++ b/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol @@ -60,7 +60,6 @@ contract TestRegistryFactory_Deployments is NexusTest_Base { new RegistryFactory(implementation, address(this), registry, attestersArray, 2); } - /// @notice Tests adding and removing attesters from the registry. function test_AddRemoveAttester() public { address attester = address(0x456); From b8fe78ce9113e05f9cddab2344036717e55a947a Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 16:22:35 +0200 Subject: [PATCH 07/29] #14 refactor _uninstallValidator (gas optimization) --- contracts/base/ModuleManager.sol | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index b8d56f5d..c3f8f4e0 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -232,14 +232,16 @@ abstract contract ModuleManager is Storage, EIP712, IModuleManagerEventsAndError (address prev, bytes memory disableModuleData) = abi.decode(data, (address, bytes)); - // Check if the account has at least one validator installed before proceeding - // Having at least one validator is a requirement for the account to function properly - require(!(prev == address(0x01) && validators.getNext(validator) == address(0x01)), CannotRemoveLastValidator()); - + // Perform the removal first validators.pop(prev, validator); + + // Sentinel pointing to itself means the list is empty, so check this after removal + require(validators.getNext(address(0x01)) != address(0x01), CannotRemoveLastValidator()); + (bool success, ) = validator.call(abi.encodeWithSelector(IModule.onUninstall.selector, disableModuleData)); } + /// @dev Installs a new executor module after checking if it matches the required module type. /// @param executor The address of the executor module to be installed. /// @param data Initialization data to configure the executor upon installation. From e1ddd0edb575b83face09e76f8a2c52aa85d6e0d Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 17:34:29 +0200 Subject: [PATCH 08/29] #10 Typos & Documentation errors and updates --- contracts/Nexus.sol | 4 +-- contracts/base/ModuleManager.sol | 19 ++++-------- contracts/base/RegistryAdapter.sol | 30 +++++++++++-------- contracts/interfaces/base/IStorage.sol | 2 +- .../lib/local/LocalCallDataParserLib.sol | 2 +- contracts/modules/validators/K1Validator.sol | 8 +++-- 6 files changed, 32 insertions(+), 33 deletions(-) diff --git a/contracts/Nexus.sol b/contracts/Nexus.sol index f7960853..c9ebb96a 100644 --- a/contracts/Nexus.sol +++ b/contracts/Nexus.sol @@ -78,8 +78,6 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra } /// Validates a user operation against a specified validator, extracted from the operation's nonce. - /// The entryPoint calls this only if validation succeeds. Fails by returning `VALIDATION_FAILED` for invalid signatures. - /// Other validation failures (e.g., nonce mismatch) should revert. /// @param op The operation to validate, encapsulating all transaction details. /// @param userOpHash Hash of the operation data, used for signature validation. /// @param missingAccountFunds Funds missing from the account's deposit necessary for transaction execution. @@ -326,7 +324,7 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra UUPSUpgradeable.upgradeToAndCall(newImplementation, data); } - /// @notice Wrapper around `_eip712Hash()` to produce a replay-safe hash fron the given `hash`. + /// @notice Wrapper around `_eip712Hash()` to produce a replay-safe hash from the given `hash`. /// /// @dev The returned EIP-712 compliant replay-safe hash is the result of: /// keccak256( diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index c3f8f4e0..a51517ae 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -330,18 +330,11 @@ abstract contract ModuleManager is Storage, EIP712, IModuleManagerEventsAndError (bool success, ) = fallbackHandler.call(abi.encodeWithSelector(IModule.onUninstall.selector, data[4:])); } - /// To make it easier to install multiple modules at once, this function will - /// install multiple modules at once. The init data is expected to be a abi encoded tuple - /// of (uint[] types, bytes[] initDatas) - /// @dev Install multiple modules at once - /// @dev It will call module.onInstall for every initialization so it ensure the flow - /// consistent with the flow of the SA's that do not implement _multiTypeInstall - /// and thus will call the multityped module several times - /// The multityped modules can not expect all the 7579 SA's to implement _multiTypeInstall and - /// thus should account for the flow when they are going to be called with onUnistall - /// for the initialization as every of the module types they declare they are - /// @param module address of the module - /// @param initData initialization data for the module + /// @notice Installs a module with multiple types in a single operation. + /// @dev This function handles installing a multi-type module by iterating through each type and initializing it. + /// The initData should include an ABI-encoded tuple of (uint[] types, bytes[] initDatas). + /// @param module The address of the multi-type module. + /// @param initData Initialization data for each type within the module. function _multiTypeInstall( address module, bytes calldata initData @@ -376,7 +369,7 @@ abstract contract ModuleManager is Storage, EIP712, IModuleManagerEventsAndError _installFallbackHandler(module, initDatas[i]); } /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ - /* INSTALL HOOK (global or sig specific) */ + /* INSTALL HOOK (global only, not sig-specific) */ /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ else if (theType == MODULE_TYPE_HOOK) { _installHook(module, initDatas[i]); diff --git a/contracts/base/RegistryAdapter.sol b/contracts/base/RegistryAdapter.sol index 900c6bac..5fb669b0 100644 --- a/contracts/base/RegistryAdapter.sol +++ b/contracts/base/RegistryAdapter.sol @@ -3,24 +3,28 @@ pragma solidity ^0.8.26; import { IERC7484 } from "../interfaces/IERC7484.sol"; -/** - * IERC7484 Registry adapter. - * this feature is opt-in. The smart account owner can choose to use the registry and which - * attesters to trust - */ +/// @title RegistryAdapter +/// @notice This contract provides an interface for interacting with an ERC-7484 compliant registry. +/// @dev The registry feature is opt-in, allowing the smart account owner to select and trust specific attesters. abstract contract RegistryAdapter { IERC7484 public registry; + /// @notice Emitted when a new ERC-7484 registry is configured for the account. + /// @param registry The configured registry contract. event ERC7484RegistryConfigured(IERC7484 indexed registry); + /// @notice Modifier to check if a module meets the required attestations in the registry. + /// @param module The module to check. + /// @param moduleType The type of the module to verify in the registry. modifier withRegistry(address module, uint256 moduleType) { _checkRegistry(module, moduleType); _; } - /** - * Configure ERC7484 Registry for Account - */ + /// @notice Configures the ERC-7484 registry and sets trusted attesters. + /// @param newRegistry The new registry contract to use. + /// @param attesters The list of attesters to trust. + /// @param threshold The number of attestations required. function _configureRegistry(IERC7484 newRegistry, address[] calldata attesters, uint8 threshold) internal { registry = newRegistry; if (address(newRegistry) != address(0)) { @@ -29,14 +33,14 @@ abstract contract RegistryAdapter { emit ERC7484RegistryConfigured(newRegistry); } - /** - * Check on ERC7484 Registry, if suffcient attestations were made - * This will revert, if not succicient valid attestations are on the registry - */ + /// @notice Checks the registry to ensure sufficient valid attestations for a module. + /// @param module The module to check. + /// @param moduleType The type of the module to verify in the registry. + /// @dev Reverts if the required attestations are not met. function _checkRegistry(address module, uint256 moduleType) internal view { IERC7484 moduleRegistry = registry; if (address(moduleRegistry) != address(0)) { - // this will revert if attestations / threshold are not met + // This will revert if attestations or the threshold are not met. moduleRegistry.check(module, moduleType); } } diff --git a/contracts/interfaces/base/IStorage.sol b/contracts/interfaces/base/IStorage.sol index 65037c64..d3ed7c89 100644 --- a/contracts/interfaces/base/IStorage.sol +++ b/contracts/interfaces/base/IStorage.sol @@ -40,6 +40,6 @@ interface IStorage { /// @notice Defines a fallback handler with an associated handler address and a call type. struct FallbackHandler { address handler; ///< The address of the fallback function handler. - CallType calltype; ///< The type of call this handler supports (e.g., static or delegatecall). + CallType calltype; ///< The type of call this handler supports (e.g., static or call). } } diff --git a/contracts/lib/local/LocalCallDataParserLib.sol b/contracts/lib/local/LocalCallDataParserLib.sol index b0c2945d..c3ef1464 100644 --- a/contracts/lib/local/LocalCallDataParserLib.sol +++ b/contracts/lib/local/LocalCallDataParserLib.sol @@ -36,7 +36,7 @@ library LocalCallDataParserLib { } - /// @dev Parses the data to obtain types and initdata's for Multi Type module isntall mode + /// @dev Parses the data to obtain types and initdata's for Multi Type module install mode /// @param initData Multi Type module init data, abi.encoded function parseMultiTypeInitData(bytes calldata initData) internal diff --git a/contracts/modules/validators/K1Validator.sol b/contracts/modules/validators/K1Validator.sol index 5d0f21d4..8355423d 100644 --- a/contracts/modules/validators/K1Validator.sol +++ b/contracts/modules/validators/K1Validator.sol @@ -21,8 +21,12 @@ import { ERC1271_MAGICVALUE, ERC1271_INVALID } from "../../../contracts/types/Co import { MODULE_TYPE_VALIDATOR, VALIDATION_SUCCESS, VALIDATION_FAILED } from "../../../contracts/types/Constants.sol"; import { MessageHashUtils } from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; -/// @title Nexus - K1Validator -/// @notice This contract is a simple validator for testing purposes, verifying user operation signatures against registered owners. +/// @title Nexus - K1Validator (ECDSA) +/// @notice Validator module for smart accounts, verifying user operation signatures +/// based on the K1 curve (secp256k1), a widely used ECDSA algorithm. +/// @dev Implements secure ownership validation by checking signatures against registered +/// owners. This module supports ERC-7579 and ERC-4337 standards, ensuring only the +/// legitimate owner of a smart account can authorize transactions. /// @author @livingrockrises | Biconomy | chirag@biconomy.io /// @author @aboudjem | Biconomy | adam.boudjemaa@biconomy.io /// @author @filmakarov | Biconomy | filipp.makarov@biconomy.io From 6a54eb4cb6dfd241370981f7ff373c0173dc800e Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 18:19:40 +0200 Subject: [PATCH 09/29] #15 Update Nexus initialization to include validator check --- contracts/Nexus.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/Nexus.sol b/contracts/Nexus.sol index c9ebb96a..165a3338 100644 --- a/contracts/Nexus.sol +++ b/contracts/Nexus.sol @@ -217,9 +217,8 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra (address bootstrap, bytes memory bootstrapCall) = abi.decode(initData, (address, bytes)); (bool success, ) = bootstrap.delegatecall(bootstrapCall); - SentinelListLib.SentinelList storage validators = _getAccountStorage().validators; - require(!(prev == address(0x01) && validators.getNext(validator) == address(0x01)), CannotRemoveLastValidator()); require(success, NexusInitializationFailed()); + require(_hasValidators(), MissingValidator()); } function setRegistry(IERC7484 newRegistry, address[] calldata attesters, uint8 threshold) external payable onlyEntryPointOrSelf { From d199ff495dbf92a1222495f8841f0bff11c4a331 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 18:20:16 +0200 Subject: [PATCH 10/29] refactor: introduce _hasValidators validator check --- contracts/base/ModuleManager.sol | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index a51517ae..58d09aa0 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -236,7 +236,7 @@ abstract contract ModuleManager is Storage, EIP712, IModuleManagerEventsAndError validators.pop(prev, validator); // Sentinel pointing to itself means the list is empty, so check this after removal - require(validators.getNext(address(0x01)) != address(0x01), CannotRemoveLastValidator()); + require(_hasValidators(), MissingValidator()); (bool success, ) = validator.call(abi.encodeWithSelector(IModule.onUninstall.selector, disableModuleData)); } @@ -458,6 +458,12 @@ abstract contract ModuleManager is Storage, EIP712, IModuleManagerEventsAndError return _getAccountStorage().validators.contains(validator); } + /// @dev Checks if there is at least one validator installed. + /// @return True if there is at least one validator, otherwise false. + function _hasValidators() internal view returns (bool) { + return _getAccountStorage().validators.getNext(address(0x01)) != address(0x01); + } + /// @dev Checks if an executor is currently installed. /// @param executor The address of the executor to check. /// @return True if the executor is installed, otherwise false. From aacd37d68fb939675d1458fa42dc0c8022018677 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 18:20:48 +0200 Subject: [PATCH 11/29] refactor: update error CannotRemoveLastValidator -> MissingValidator to be more generic in IModuleManagerEventsAndErrors.sol --- contracts/interfaces/base/IModuleManagerEventsAndErrors.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol b/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol index 45e94da8..3951e0ba 100644 --- a/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol +++ b/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol @@ -31,8 +31,8 @@ interface IModuleManagerEventsAndErrors { /// @param module The address of the uninstalled module. event ModuleUninstalled(uint256 moduleTypeId, address module); - /// @dev Thrown when an attempt is made to uninstall the last validator module, which is prohibited. - error CannotRemoveLastValidator(); + /// @notice Thrown when no validators exist or when attempting to remove the last one. + error MissingValidator(); /// @dev Thrown when the specified module address is not recognized as valid. error InvalidModule(address module); From 2115a822b5b6b47885ab2a6138a0947abd52d49f Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 18:21:13 +0200 Subject: [PATCH 12/29] #7: Update ModuleType from uint256 to uint8 --- contracts/lib/ModuleTypeLib.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/lib/ModuleTypeLib.sol b/contracts/lib/ModuleTypeLib.sol index 20c4c3c2..66fb1cd6 100644 --- a/contracts/lib/ModuleTypeLib.sol +++ b/contracts/lib/ModuleTypeLib.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.26; type EncodedModuleTypes is uint256; -type ModuleType is uint256; +type ModuleType is uint8; /// @title ModuleTypeLib /// @notice A library for handling module types and encoding them as bits From 39bc78b569bc2e78f79e80cda54bd060c59aab1d Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 18:23:54 +0200 Subject: [PATCH 13/29] refactor: Add test for reverting account initialization without validator --- .../TestNexusAccountFactory_Deployments.t.sol | 14 ++++++++++++++ .../TestNexusAccountFactory_Deployments.tree | 2 ++ 2 files changed, 16 insertions(+) diff --git a/test/foundry/unit/concrete/factory/TestNexusAccountFactory_Deployments.t.sol b/test/foundry/unit/concrete/factory/TestNexusAccountFactory_Deployments.t.sol index b8d45d48..48121d45 100644 --- a/test/foundry/unit/concrete/factory/TestNexusAccountFactory_Deployments.t.sol +++ b/test/foundry/unit/concrete/factory/TestNexusAccountFactory_Deployments.t.sol @@ -117,6 +117,20 @@ contract TestNexusAccountFactory_Deployments is NexusTest_Base { INexus(firstAccountAddress).initializeAccount(factoryData); } + /// @notice Tests that account initialization reverts if no validator is installed. + function test_RevertIf_NoValidatorDuringInitialization() public { + BootstrapConfig[] memory emptyValidators; // Empty validators array + BootstrapConfig memory hook = BootstrapLib.createSingleConfig(address(0), ""); + bytes memory saDeploymentIndex = "0"; + bytes32 salt = keccak256(saDeploymentIndex); + + // Create initcode with no validator configuration + bytes memory _initData = BOOTSTRAPPER.getInitNexusScopedCalldata(emptyValidators, hook, REGISTRY, ATTESTERS, THRESHOLD); + + vm.expectRevert(MissingValidator.selector); + FACTORY.createAccount(_initData, salt); + } + /// @notice Tests creating accounts with different indexes. function test_DeployAccount_DifferentIndexes() public { BootstrapConfig[] memory validators = BootstrapLib.createArrayConfig(address(VALIDATOR_MODULE), initData); diff --git a/test/foundry/unit/concrete/factory/TestNexusAccountFactory_Deployments.tree b/test/foundry/unit/concrete/factory/TestNexusAccountFactory_Deployments.tree index e26135c4..37bfbe60 100644 --- a/test/foundry/unit/concrete/factory/TestNexusAccountFactory_Deployments.tree +++ b/test/foundry/unit/concrete/factory/TestNexusAccountFactory_Deployments.tree @@ -17,6 +17,8 @@ TestNexusAccountFactory_Deployments │ └── it should revert ├── when deploying an account with insufficient gas │ └── it should revert +├── when initializing an account without a validator module +│ └── it should revert ├── when testing the constructor of the Nexus contract │ └── it should revert if the entry point address is zero ├── when using BootstrapLib.createArrayConfig function for multiple modules From 95dfc9422c736f2cd18851be74f86d67e29d1045 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 18:24:35 +0200 Subject: [PATCH 14/29] refactor: Update RegistryFactory constructor to validate threshold against attesters length --- .../unit/concrete/factory/TestRegistryFactory_Deployments.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol b/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol index 61e76a4c..66701ab7 100644 --- a/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol +++ b/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol @@ -52,7 +52,7 @@ contract TestRegistryFactory_Deployments is NexusTest_Base { /// @notice Tests that the constructor reverts if the threshold is greater than the length of the attesters array. function test_Constructor_RevertIf_ThresholdExceedsAttestersLength() public { address implementation = address(0x123); - address; + address[] memory attestersArray = new address[](1); attestersArray[0] = address(0x789); // Expect the constructor to revert because the threshold (2) is greater than the number of attesters (1) From 970d80807f9d118951558273bd874f2cb544129d Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 18:24:53 +0200 Subject: [PATCH 15/29] refactor: Update error message from CannotRemoveLastValidator to MissingValidator for more generic handling --- .../modulemanager/TestModuleManager_UninstallModule.t.sol | 6 +++--- .../modulemanager/TestModuleManager_UninstallModule.tree | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.t.sol b/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.t.sol index 4dd7306b..cb4ea4b3 100644 --- a/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.t.sol +++ b/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.t.sol @@ -164,7 +164,7 @@ contract TestModuleManager_UninstallModule is TestModuleManagement_Base { abi.encode(prev, "") ); - bytes memory expectedRevertReason = abi.encodeWithSignature("CannotRemoveLastValidator()"); + bytes memory expectedRevertReason = abi.encodeWithSignature("MissingValidator()"); Execution[] memory execution = new Execution[](1); execution[0] = Execution(address(BOB_ACCOUNT), 0, callData); @@ -205,7 +205,7 @@ contract TestModuleManager_UninstallModule is TestModuleManagement_Base { ); // Define expected revert reason - bytes memory expectedRevertReason = abi.encodeWithSignature("MismatchModuleTypeId(uint256)", MODULE_TYPE_EXECUTOR); + bytes memory expectedRevertReason = abi.encodeWithSelector(ModuleNotInstalled.selector, MODULE_TYPE_EXECUTOR, address(VALIDATOR_MODULE)); Execution[] memory execution = new Execution[](1); execution[0] = Execution(address(BOB_ACCOUNT), 0, callData); @@ -392,7 +392,7 @@ contract TestModuleManager_UninstallModule is TestModuleManagement_Base { bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOps[0]); // Define expected revert reason - bytes memory expectedRevertReason = abi.encodeWithSignature("CannotRemoveLastValidator()"); + bytes memory expectedRevertReason = abi.encodeWithSignature("MissingValidator()"); // Expect the UserOperationRevertReason event vm.expectEmit(true, true, true, true); diff --git a/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.tree b/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.tree index 265d3d2f..12398dd7 100644 --- a/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.tree +++ b/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.tree @@ -10,13 +10,13 @@ TestModuleManager_UninstallModule ├── when uninstalling an executor module successfully │ └── it should uninstall the executor module ├── when uninstalling the last validator module -│ └── it should revert with CannotRemoveLastValidator error +│ └── it should revert with MissingValidator error ├── when uninstalling a module with incorrect module type │ └── it should revert with MismatchModuleTypeId error ├── when uninstalling a module that is not installed │ └── it should revert with ModuleNotInstalled error ├── when uninstalling the last validator module -│ └── it should revert with CannotRemoveLastValidator error +│ └── it should revert with MissingValidator error ├── when uninstalling the fallback handler module successfully │ └── it should uninstall the fallback handler module ├── when uninstalling a fallback handler that is not installed From 8b0b6d2a1afc11807d76d6bb3f41be6ae072dc96 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 18:25:02 +0200 Subject: [PATCH 16/29] refactor: Update error message from CannotRemoveLastValidator to MissingValidator for more generic handling --- test/foundry/unit/fuzz/TestFuzz_ModuleManager.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/foundry/unit/fuzz/TestFuzz_ModuleManager.t.sol b/test/foundry/unit/fuzz/TestFuzz_ModuleManager.t.sol index 35aaf353..ebbdb62e 100644 --- a/test/foundry/unit/fuzz/TestFuzz_ModuleManager.t.sol +++ b/test/foundry/unit/fuzz/TestFuzz_ModuleManager.t.sol @@ -314,7 +314,7 @@ contract TestFuzz_ModuleManager is TestModuleManagement_Base { // If the module type does not match the installation, expect a revert if (!IModule(moduleAddress).isModuleType(moduleTypeId)) { - bytes memory expectedRevertReason = abi.encodeWithSignature("MismatchModuleTypeId(uint256)", moduleTypeId); + bytes memory expectedRevertReason = abi.encodeWithSelector(ModuleNotInstalled.selector, moduleTypeId, moduleAddress); bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOps[0]); vm.expectEmit(true, true, true, true); emit UserOperationRevertReason( From db5a697903170edbba30baf38a05e18e23c6c6fa Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 18:25:10 +0200 Subject: [PATCH 17/29] refactor: Update error message from CannotRemoveLastValidator to MissingValidator for more generic handling --- test/foundry/utils/EventsAndErrors.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/foundry/utils/EventsAndErrors.sol b/test/foundry/utils/EventsAndErrors.sol index 14eb2cf8..3f9b5a3a 100644 --- a/test/foundry/utils/EventsAndErrors.sol +++ b/test/foundry/utils/EventsAndErrors.sol @@ -42,6 +42,8 @@ contract EventsAndErrors { error InnerCallFailed(); error CallToDeployWithFactoryFailed(); error NexusInitializationFailed(); + error InvalidThreshold(uint8 providedThreshold, uint256 attestersCount); + // ========================== // Operation Errors @@ -61,7 +63,7 @@ contract EventsAndErrors { // ========================== // Module Errors // ========================== - error CannotRemoveLastValidator(); + error MissingValidator(); error InvalidModule(address module); error InvalidModuleTypeId(uint256 moduleTypeId); error ModuleAlreadyInstalled(uint256 moduleTypeId, address module); From a47720b60da9065701fb17d7ad9cedfaa9fbbed9 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 18:25:15 +0200 Subject: [PATCH 18/29] refactor: Update error message from CannotRemoveLastValidator to MissingValidator for more generic handling --- test/hardhat/smart-account/Nexus.ModuleManager.specs.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/hardhat/smart-account/Nexus.ModuleManager.specs.ts b/test/hardhat/smart-account/Nexus.ModuleManager.specs.ts index 79a35b01..2c82e84b 100644 --- a/test/hardhat/smart-account/Nexus.ModuleManager.specs.ts +++ b/test/hardhat/smart-account/Nexus.ModuleManager.specs.ts @@ -183,7 +183,7 @@ describe("Nexus Module Management Tests", () => { ), ).to.be.revertedWithCustomError( deployedNexus, - "CannotRemoveLastValidator()", + "MissingValidator()", ); }); From fe2df72b2d8465b787ec895d9ada6d14c9576aa1 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 19:40:15 +0200 Subject: [PATCH 19/29] #9: Update NonceLib to use byte function instead of shifting and masking for vmode calculation --- contracts/lib/NonceLib.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/lib/NonceLib.sol b/contracts/lib/NonceLib.sol index bdc25be8..3c2cc475 100644 --- a/contracts/lib/NonceLib.sol +++ b/contracts/lib/NonceLib.sol @@ -24,7 +24,7 @@ library NonceLib { /// @return res boolean result, true if it is the Module Enable Mode function isModuleEnableMode(uint256 nonce) internal pure returns (bool res) { assembly { - let vmode := shr(248, shl(24, nonce)) + let vmode := byte(3, nonce) res := eq(shl(248, vmode), MODE_MODULE_ENABLE) } } From ea3c7e2e10f49fd87ccf74b712baa97f093323b6 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 20:45:54 +0200 Subject: [PATCH 20/29] #4 Missing return parameters in natspec --- contracts/base/ExecutionHelper.sol | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/contracts/base/ExecutionHelper.sol b/contracts/base/ExecutionHelper.sol index 5512e6dc..71f4dd23 100644 --- a/contracts/base/ExecutionHelper.sol +++ b/contracts/base/ExecutionHelper.sol @@ -34,7 +34,7 @@ contract ExecutionHelper is IExecutionHelperEventsAndErrors { /// @param target The address to execute the call on. /// @param value The amount of wei to send with the call. /// @param callData The calldata to send. - /// @return result The bytes returned from the execution. + /// @return result The bytes returned from the execution, which contains the returned data from the target address. function _execute(address target, uint256 value, bytes calldata callData) internal virtual returns (bytes memory result) { /// @solidity memory-safe-assembly assembly { @@ -58,7 +58,7 @@ contract ExecutionHelper is IExecutionHelperEventsAndErrors { /// @param value The amount of wei to send with the call. /// @param callData The calldata to send. /// @return success True if the execution was successful, false otherwise. - /// @return result The bytes returned from the execution. + /// @return result The bytes returned from the execution, which contains the returned data from the target address. function _tryExecute(address target, uint256 value, bytes calldata callData) internal virtual returns (bool success, bytes memory result) { /// @solidity memory-safe-assembly assembly { @@ -74,7 +74,7 @@ contract ExecutionHelper is IExecutionHelperEventsAndErrors { /// @notice Executes a batch of calls. /// @param executions An array of Execution structs each containing target, value, and calldata. - /// @return result An array of results from each executed call. + /// @return result An array of bytes returned from each executed call, corresponding to the returndata from each target address. function _executeBatch(Execution[] calldata executions) internal returns (bytes[] memory result) { result = new bytes[](executions.length); @@ -87,7 +87,7 @@ contract ExecutionHelper is IExecutionHelperEventsAndErrors { /// @notice Tries to execute a batch of calls and emits an event for each unsuccessful call. /// @param executions An array of Execution structs. - /// @return result An array of results, with unsuccessful calls marked by events. + /// @return result An array of bytes returned from each executed call, with unsuccessful calls marked by events. function _tryExecuteBatch(Execution[] calldata executions) internal returns (bytes[] memory result) { result = new bytes[](executions.length); @@ -101,6 +101,7 @@ contract ExecutionHelper is IExecutionHelperEventsAndErrors { } /// @dev Execute a delegatecall with `delegate` on this account. + /// @return result The bytes returned from the delegatecall, which contains the returned data from the delegate contract. function _executeDelegatecall(address delegate, bytes calldata callData) internal returns (bytes memory result) { /// @solidity memory-safe-assembly assembly { @@ -120,6 +121,8 @@ contract ExecutionHelper is IExecutionHelperEventsAndErrors { } /// @dev Execute a delegatecall with `delegate` on this account and catch reverts. + /// @return success True if the delegatecall was successful, false otherwise. + /// @return result The bytes returned from the delegatecall, which contains the returned data from the delegate contract. function _tryExecuteDelegatecall(address delegate, bytes calldata callData) internal returns (bool success, bytes memory result) { /// @solidity memory-safe-assembly assembly { @@ -167,6 +170,7 @@ contract ExecutionHelper is IExecutionHelperEventsAndErrors { /// @dev Executes a single transaction based on the specified execution type. /// @param executionCalldata The calldata containing the transaction details (target address, value, and data). /// @param execType The execution type, which can be DEFAULT (revert on failure) or TRY (return on failure). + /// @return returnData An array containing the execution result. In the case of a single transaction, the array contains one element. function _handleSingleExecutionAndReturnData(bytes calldata executionCalldata, ExecType execType) internal returns(bytes[] memory returnData) { (address target, uint256 value, bytes calldata callData) = executionCalldata.decodeSingle(); returnData = new bytes[](1); @@ -182,9 +186,10 @@ contract ExecutionHelper is IExecutionHelperEventsAndErrors { } } - /// @dev Executes a batch of transactions based on the specified execution type. + /// @dev Executes a batch of transactions based on the specified execution type. /// @param executionCalldata The calldata for a batch of transactions. /// @param execType The execution type, which can be DEFAULT (revert on failure) or TRY (return on failure). + /// @return returnData An array containing the execution results for each transaction in the batch. function _handleBatchExecutionAndReturnData(bytes calldata executionCalldata, ExecType execType) internal returns(bytes[] memory returnData){ Execution[] calldata executions = executionCalldata.decodeBatch(); if (execType == EXECTYPE_DEFAULT) returnData = _executeBatch(executions); @@ -195,6 +200,7 @@ contract ExecutionHelper is IExecutionHelperEventsAndErrors { /// @dev Executes a single transaction based on the specified execution type. /// @param executionCalldata The calldata containing the transaction details (target address, value, and data). /// @param execType The execution type, which can be DEFAULT (revert on failure) or TRY (return on failure). + /// @return returnData An array containing the result of the delegatecall execution. function _handleDelegateCallExecutionAndReturnData(bytes calldata executionCalldata, ExecType execType) internal returns(bytes[] memory returnData) { (address delegate, bytes calldata callData) = executionCalldata.decodeDelegateCall(); returnData = new bytes[](1); From f8fa4334ae7b77945eb92ed52a155ab507a18fe9 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 20:46:24 +0200 Subject: [PATCH 21/29] #5: Add factoryOwner check in K1ValidatorFactory constructor + test --- contracts/factory/K1ValidatorFactory.sol | 2 +- .../factory/TestK1ValidatorFactory_Deployments.t.sol | 11 +++++++++++ .../factory/TestK1ValidatorFactory_Deployments.tree | 1 + 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/contracts/factory/K1ValidatorFactory.sol b/contracts/factory/K1ValidatorFactory.sol index 42a8f024..43b50f97 100644 --- a/contracts/factory/K1ValidatorFactory.sol +++ b/contracts/factory/K1ValidatorFactory.sol @@ -60,7 +60,7 @@ contract K1ValidatorFactory is Stakeable { IERC7484 registry ) Stakeable(factoryOwner) { require( - !(implementation == address(0) || k1Validator == address(0) || address(bootstrapper) == address(0) || address(registry) == address(0)), + !(implementation == address(0) || k1Validator == address(0) || address(bootstrapper) == address(0) || address(registry) == address(0) || factoryOwner == address(0)), ZeroAddressNotAllowed() ); ACCOUNT_IMPLEMENTATION = implementation; diff --git a/test/foundry/unit/concrete/factory/TestK1ValidatorFactory_Deployments.t.sol b/test/foundry/unit/concrete/factory/TestK1ValidatorFactory_Deployments.t.sol index bd5ab37a..21b81909 100644 --- a/test/foundry/unit/concrete/factory/TestK1ValidatorFactory_Deployments.t.sol +++ b/test/foundry/unit/concrete/factory/TestK1ValidatorFactory_Deployments.t.sol @@ -56,6 +56,17 @@ contract TestK1ValidatorFactory_Deployments is NexusTest_Base { new K1ValidatorFactory(zeroAddress, address(this), address(VALIDATOR_MODULE), bootstrapper, REGISTRY); } + /// @notice Tests that the constructor reverts if the factory owner address is zero. + function test_Constructor_RevertIf_FactoryOwnerIsZero() public { + address zeroAddress = address(0); + + // Expect the contract deployment to revert with the correct error message + vm.expectRevert(ZeroAddressNotAllowed.selector); + + // Try deploying the K1ValidatorFactory with an implementation address of zero + new K1ValidatorFactory(address(this), zeroAddress, address(VALIDATOR_MODULE), bootstrapper, REGISTRY); + } + /// @notice Tests that the constructor reverts if the K1 Validator address is zero. function test_Constructor_RevertIf_K1ValidatorIsZero() public { address zeroAddress = address(0); diff --git a/test/foundry/unit/concrete/factory/TestK1ValidatorFactory_Deployments.tree b/test/foundry/unit/concrete/factory/TestK1ValidatorFactory_Deployments.tree index 4b045401..4ac2f8fb 100644 --- a/test/foundry/unit/concrete/factory/TestK1ValidatorFactory_Deployments.tree +++ b/test/foundry/unit/concrete/factory/TestK1ValidatorFactory_Deployments.tree @@ -3,6 +3,7 @@ TestK1ValidatorFactory_Deployments ├── when the constructor is called │ ├── it should initialize the factory with valid implementation, K1 Validator, and Bootstrapper addresses │ ├── it should revert if the implementation address is zero + │ ├── it should revert if the factory owner address is zero │ ├── it should revert if the K1 Validator address is zero │ └── it should revert if the Bootstrapper address is zero ├── when deploying an account using the factory directly From 94a0afcb8d872f18b7f4c3681f753e4503ae5c9a Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 20:48:24 +0200 Subject: [PATCH 22/29] #40 add memory-safe assembly annotation --- contracts/lib/ExecLib.sol | 2 +- contracts/lib/local/LocalCallDataParserLib.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/lib/ExecLib.sol b/contracts/lib/ExecLib.sol index 45fe0922..e9b4d81d 100644 --- a/contracts/lib/ExecLib.sol +++ b/contracts/lib/ExecLib.sol @@ -16,7 +16,7 @@ library ExecLib { * 0x4 | - | abi.encode(IERC7579Execution.Execution[]) */ - assembly { + assembly ("memory-safe") { let dataPointer := add(callData.offset, calldataload(callData.offset)) // Extract the ERC7579 Executions diff --git a/contracts/lib/local/LocalCallDataParserLib.sol b/contracts/lib/local/LocalCallDataParserLib.sol index c3ef1464..4eb9f782 100644 --- a/contracts/lib/local/LocalCallDataParserLib.sol +++ b/contracts/lib/local/LocalCallDataParserLib.sol @@ -17,7 +17,7 @@ library LocalCallDataParserLib { ) { uint256 p; - assembly { + assembly ("memory-safe") { p := packedData.offset module := shr(96, calldataload(p)) From dbe28ddad5f4829fda0ffcfa0c79a28710fb3782 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 20:49:04 +0200 Subject: [PATCH 23/29] #11 add natspec for parsenablemodedata --- contracts/lib/local/LocalCallDataParserLib.sol | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/contracts/lib/local/LocalCallDataParserLib.sol b/contracts/lib/local/LocalCallDataParserLib.sol index 4eb9f782..83fa58cd 100644 --- a/contracts/lib/local/LocalCallDataParserLib.sol +++ b/contracts/lib/local/LocalCallDataParserLib.sol @@ -3,8 +3,15 @@ pragma solidity 0.8.26; library LocalCallDataParserLib { - /// @dev Parses the data to obtain enable mode specific data - /// @param packedData Packed data. In most cases it will be userOp.signature + /// @dev Parses the `userOp.signature` to extract the module type, module initialization data, + /// enable mode signature, and user operation signature. The `userOp.signature` must be + /// encoded in a specific way to be parsed correctly. + /// @param packedData The packed signature data, typically coming from `userOp.signature`. + /// @return module The address of the module. + /// @return moduleType The type of module as a `uint256`. + /// @return moduleInitData Initialization data specific to the module. + /// @return enableModeSignature Signature used to enable the module mode. + /// @return userOpSignature The remaining user operation signature data. function parseEnableModeData(bytes calldata packedData) internal pure From 884b65210a35c474521d348de761c35d27b21315 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Thu, 15 Aug 2024 20:51:01 +0200 Subject: [PATCH 24/29] #3 Document that RegistryFactory.createAccount is only compatible with an inner Bootstrap.initNexus --- contracts/factory/RegistryFactory.sol | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/contracts/factory/RegistryFactory.sol b/contracts/factory/RegistryFactory.sol index 116d92e2..77b4df1a 100644 --- a/contracts/factory/RegistryFactory.sol +++ b/contracts/factory/RegistryFactory.sol @@ -102,29 +102,27 @@ contract RegistryFactory is Stakeable, INexusFactory { threshold = newThreshold; } + /// @notice Creates a new Nexus account with the provided initialization data. - /// @param initData Initialization data to be called on the new Smart Account. - /// @param salt Unique salt for the Smart Account creation. - /// @return The address of the newly created Nexus. + /// @param initData Initialization data that is expected to be compatible with a `Bootstrap` contract's initialization method. + /// @param salt Unique salt used for deterministic deployment of the Nexus smart account. + /// @return The address of the newly created Nexus account. function createAccount(bytes calldata initData, bytes32 salt) external payable override returns (address payable) { - // Decode the initData to extract the call target and call data + // Decode the initialization data to extract the target bootstrap contract and the data to be used for initialization. (, bytes memory callData) = abi.decode(initData, (address, bytes)); - // Extract the inner data by removing the first 4 bytes (the function selector) + // Ensure that the initData is structured for the expected Bootstrap.initNexus or similar method. + // This step is crucial for ensuring the proper initialization of the Nexus smart account. bytes memory innerData = BytesLib.slice(callData, 4, callData.length - 4); - - // Decode the call data to extract the parameters passed to initNexus ( BootstrapConfig[] memory validators, BootstrapConfig[] memory executors, BootstrapConfig memory hook, BootstrapConfig[] memory fallbacks, - , - , - + , , ) = abi.decode(innerData, (BootstrapConfig[], BootstrapConfig[], BootstrapConfig, BootstrapConfig[], address, address[], uint8)); - // Ensure all modules are whitelisted + // Ensure that all specified modules are whitelisted and allowed for the account. for (uint256 i = 0; i < validators.length; i++) { require(isModuleAllowed(validators[i].module, MODULE_TYPE_VALIDATOR), ModuleNotWhitelisted(validators[i].module)); } @@ -153,12 +151,15 @@ contract RegistryFactory is Stakeable, INexusFactory { (bool alreadyDeployed, address account) = LibClone.createDeterministicERC1967(msg.value, ACCOUNT_IMPLEMENTATION, actualSalt); if (!alreadyDeployed) { + // Initialize the Nexus account using the provided initialization data INexus(account).initializeAccount(initData); emit AccountCreated(account, initData, salt); } + return payable(account); } + /// @notice Computes the expected address of a Nexus contract using the factory's deterministic deployment algorithm. /// @param - Initialization data to be called on the new Smart Account. /// @param - Unique salt for the Smart Account creation. From 84f0948cd801c0bcdd2fd77ba9ff9f39ee4fe984 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Fri, 16 Aug 2024 21:20:16 +0200 Subject: [PATCH 25/29] refactor: Improve natspec for validateUserop --- contracts/Nexus.sol | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/contracts/Nexus.sol b/contracts/Nexus.sol index 165a3338..75b03ed0 100644 --- a/contracts/Nexus.sol +++ b/contracts/Nexus.sol @@ -77,21 +77,24 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra _initModuleManager(); } - /// Validates a user operation against a specified validator, extracted from the operation's nonce. - /// @param op The operation to validate, encapsulating all transaction details. - /// @param userOpHash Hash of the operation data, used for signature validation. + /// @notice Validates a user operation against a specified validator, extracted from the operation's nonce. + /// @param op The user operation to validate, encapsulating all transaction details. + /// @param userOpHash Hash of the user operation data, used for signature validation. /// @param missingAccountFunds Funds missing from the account's deposit necessary for transaction execution. - /// This can be zero if covered by a paymaster or sufficient deposit exists. + /// This can be zero if covered by a paymaster or if sufficient deposit exists. /// @return validationData Encoded validation result or failure, propagated from the validator module. /// - Encoded format in validationData: - /// - First 20 bytes: Validator address, 0x0 for valid or specific failure modes. - /// - `SIG_VALIDATION_FAILED` (1) denotes signature validation failure allowing simulation calls without a valid signature. - /// @dev Expects the validator's address to be encoded in the upper 96 bits of the userOp's nonce. + /// - First 20 bytes: Address of the Validator module, to which the validation task is forwarded. + /// The validator module returns: + /// - `SIG_VALIDATION_SUCCESS` (0) indicates successful validation. + /// - `SIG_VALIDATION_FAILED` (1) indicates signature validation failure. + /// @dev Expects the validator's address to be encoded in the upper 96 bits of the user operation's nonce. /// This method forwards the validation task to the extracted validator module address. + /// @dev The entryPoint calls this function. If validation fails, it returns `VALIDATION_FAILED` (1) otherwise `0`. /// @dev Features Module Enable Mode. - /// This Module Enable Mode flow only makes sense for the module that is used as validator - /// for the userOp that triggers Module Enable Flow. Otherwise, one should just include - /// a call to Nexus.installModule into userOp.callData + /// This Module Enable Mode flow is intended for the module acting as the validator + /// for the user operation that triggers the Module Enable Flow. Otherwise, a call to + /// `Nexus.installModule` should be included in `userOp.callData`. function validateUserOp( PackedUserOperation calldata op, bytes32 userOpHash, From d8696ccf38ad5b83375dbdd6706a58c8bbc6c37e Mon Sep 17 00:00:00 2001 From: aboudjem Date: Fri, 16 Aug 2024 21:20:36 +0200 Subject: [PATCH 26/29] refactor: Update error for removing last validator --- contracts/base/ModuleManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index 58d09aa0..44ed201c 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -236,7 +236,7 @@ abstract contract ModuleManager is Storage, EIP712, IModuleManagerEventsAndError validators.pop(prev, validator); // Sentinel pointing to itself means the list is empty, so check this after removal - require(_hasValidators(), MissingValidator()); + require(_hasValidators(), CanNotRemoveLastValidator()); (bool success, ) = validator.call(abi.encodeWithSelector(IModule.onUninstall.selector, disableModuleData)); } From 0a12d75906eb57a210f9e7de5b48740d032c4a08 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Fri, 16 Aug 2024 21:20:49 +0200 Subject: [PATCH 27/29] refactor: Update error for removing last validator --- contracts/interfaces/base/IModuleManagerEventsAndErrors.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol b/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol index 3951e0ba..949dea34 100644 --- a/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol +++ b/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol @@ -31,9 +31,12 @@ interface IModuleManagerEventsAndErrors { /// @param module The address of the uninstalled module. event ModuleUninstalled(uint256 moduleTypeId, address module); - /// @notice Thrown when no validators exist or when attempting to remove the last one. + /// @notice Thrown when no validators exist on the initialization of a smart account. error MissingValidator(); + /// @notice Thrown when attempting to remove the last validator. + error CanNotRemoveLastValidator(); + /// @dev Thrown when the specified module address is not recognized as valid. error InvalidModule(address module); From a2730fcf2f383d44b4335dce7590de7396e83901 Mon Sep 17 00:00:00 2001 From: aboudjem Date: Fri, 16 Aug 2024 21:21:11 +0200 Subject: [PATCH 28/29] refactor: Update error for removing last validator --- .../modulemanager/TestModuleManager_UninstallModule.t.sol | 4 ++-- .../modulemanager/TestModuleManager_UninstallModule.tree | 4 ++-- test/foundry/utils/EventsAndErrors.sol | 1 + test/hardhat/smart-account/Nexus.ModuleManager.specs.ts | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.t.sol b/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.t.sol index cb4ea4b3..46613fce 100644 --- a/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.t.sol +++ b/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.t.sol @@ -164,7 +164,7 @@ contract TestModuleManager_UninstallModule is TestModuleManagement_Base { abi.encode(prev, "") ); - bytes memory expectedRevertReason = abi.encodeWithSignature("MissingValidator()"); + bytes memory expectedRevertReason = abi.encodeWithSignature("CanNotRemoveLastValidator()"); Execution[] memory execution = new Execution[](1); execution[0] = Execution(address(BOB_ACCOUNT), 0, callData); @@ -392,7 +392,7 @@ contract TestModuleManager_UninstallModule is TestModuleManagement_Base { bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOps[0]); // Define expected revert reason - bytes memory expectedRevertReason = abi.encodeWithSignature("MissingValidator()"); + bytes memory expectedRevertReason = abi.encodeWithSignature("CanNotRemoveLastValidator()"); // Expect the UserOperationRevertReason event vm.expectEmit(true, true, true, true); diff --git a/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.tree b/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.tree index 12398dd7..bd76d695 100644 --- a/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.tree +++ b/test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.tree @@ -10,13 +10,13 @@ TestModuleManager_UninstallModule ├── when uninstalling an executor module successfully │ └── it should uninstall the executor module ├── when uninstalling the last validator module -│ └── it should revert with MissingValidator error +│ └── it should revert with CanNotRemoveLastValidator error ├── when uninstalling a module with incorrect module type │ └── it should revert with MismatchModuleTypeId error ├── when uninstalling a module that is not installed │ └── it should revert with ModuleNotInstalled error ├── when uninstalling the last validator module -│ └── it should revert with MissingValidator error +│ └── it should revert with CanNotRemoveLastValidator error ├── when uninstalling the fallback handler module successfully │ └── it should uninstall the fallback handler module ├── when uninstalling a fallback handler that is not installed diff --git a/test/foundry/utils/EventsAndErrors.sol b/test/foundry/utils/EventsAndErrors.sol index 3f9b5a3a..33a30311 100644 --- a/test/foundry/utils/EventsAndErrors.sol +++ b/test/foundry/utils/EventsAndErrors.sol @@ -64,6 +64,7 @@ contract EventsAndErrors { // Module Errors // ========================== error MissingValidator(); + error CanNotRemoveLastValidator(); error InvalidModule(address module); error InvalidModuleTypeId(uint256 moduleTypeId); error ModuleAlreadyInstalled(uint256 moduleTypeId, address module); diff --git a/test/hardhat/smart-account/Nexus.ModuleManager.specs.ts b/test/hardhat/smart-account/Nexus.ModuleManager.specs.ts index 2c82e84b..6dce7159 100644 --- a/test/hardhat/smart-account/Nexus.ModuleManager.specs.ts +++ b/test/hardhat/smart-account/Nexus.ModuleManager.specs.ts @@ -183,7 +183,7 @@ describe("Nexus Module Management Tests", () => { ), ).to.be.revertedWithCustomError( deployedNexus, - "MissingValidator()", + "CanNotRemoveLastValidator()", ); }); From fb1e85b193fa10700d784ba9e7ba6e5b90e99525 Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Mon, 19 Aug 2024 16:30:42 +0530 Subject: [PATCH 29/29] respond to PR comments --- contracts/base/ModuleManager.sol | 2 +- contracts/interfaces/base/IModuleManagerEventsAndErrors.sol | 3 --- test/foundry/utils/EventsAndErrors.sol | 1 - 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index 200b787c..ae0875f9 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -473,7 +473,7 @@ abstract contract ModuleManager is Storage, EIP712, IModuleManagerEventsAndError /// @dev Checks if there is at least one validator installed. /// @return True if there is at least one validator, otherwise false. function _hasValidators() internal view returns (bool) { - return _getAccountStorage().validators.getNext(address(0x01)) != address(0x01); + return _getAccountStorage().validators.getNext(address(0x01)) != address(0x01) && _getAccountStorage().validators.getNext(address(0x01)) != address(0x00); } /// @dev Checks if an executor is currently installed. diff --git a/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol b/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol index 949dea34..f4966cd4 100644 --- a/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol +++ b/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol @@ -31,9 +31,6 @@ interface IModuleManagerEventsAndErrors { /// @param module The address of the uninstalled module. event ModuleUninstalled(uint256 moduleTypeId, address module); - /// @notice Thrown when no validators exist on the initialization of a smart account. - error MissingValidator(); - /// @notice Thrown when attempting to remove the last validator. error CanNotRemoveLastValidator(); diff --git a/test/foundry/utils/EventsAndErrors.sol b/test/foundry/utils/EventsAndErrors.sol index 08b1a8e0..053891ca 100644 --- a/test/foundry/utils/EventsAndErrors.sol +++ b/test/foundry/utils/EventsAndErrors.sol @@ -64,7 +64,6 @@ contract EventsAndErrors { // ========================== // Module Errors // ========================== - error MissingValidator(); error CanNotRemoveLastValidator(); error InvalidModule(address module); error InvalidModuleTypeId(uint256 moduleTypeId);