Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/cantina fixes #139

Merged
merged 31 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
c7b3d0f
#26 Remove unnecessary module type check in uninstallModule function
Aboudjem Aug 15, 2024
6f86af4
#25 Update K1Validator to use isValidSignatureNowCalldata method for …
Aboudjem Aug 15, 2024
0747d3d
#24 Update RegistryFactory constructor to validate threshold against …
Aboudjem Aug 15, 2024
7d2cbb7
chore: Update RegistryFactory constructor to validate threshold again…
Aboudjem Aug 15, 2024
7f237b8
#15: Fix validator check and revert error in Nexus initialization
Aboudjem Aug 15, 2024
07162ce
remove empty line
Aboudjem Aug 15, 2024
b8fe78c
#14 refactor _uninstallValidator (gas optimization)
Aboudjem Aug 15, 2024
e1ddd0e
#10 Typos & Documentation errors and updates
Aboudjem Aug 15, 2024
6a54eb4
#15 Update Nexus initialization to include validator check
Aboudjem Aug 15, 2024
d199ff4
refactor: introduce _hasValidators validator check
Aboudjem Aug 15, 2024
aacd37d
refactor: update error CannotRemoveLastValidator -> MissingValidator…
Aboudjem Aug 15, 2024
2115a82
#7: Update ModuleType from uint256 to uint8
Aboudjem Aug 15, 2024
39bc78b
refactor: Add test for reverting account initialization without valid…
Aboudjem Aug 15, 2024
95dfc94
refactor: Update RegistryFactory constructor to validate threshold ag…
Aboudjem Aug 15, 2024
970d808
refactor: Update error message from CannotRemoveLastValidator to Miss…
Aboudjem Aug 15, 2024
8b0b6d2
refactor: Update error message from CannotRemoveLastValidator to Miss…
Aboudjem Aug 15, 2024
db5a697
refactor: Update error message from CannotRemoveLastValidator to Miss…
Aboudjem Aug 15, 2024
a47720b
refactor: Update error message from CannotRemoveLastValidator to Miss…
Aboudjem Aug 15, 2024
fe2df72
#9: Update NonceLib to use byte function instead of shifting and mask…
Aboudjem Aug 15, 2024
ea3c7e2
#4 Missing return parameters in natspec
Aboudjem Aug 15, 2024
f8fa433
#5: Add factoryOwner check in K1ValidatorFactory constructor + test
Aboudjem Aug 15, 2024
94a0afc
#40 add memory-safe assembly annotation
Aboudjem Aug 15, 2024
dbe28dd
#11 add natspec for parsenablemodedata
Aboudjem Aug 15, 2024
884b652
#3 Document that RegistryFactory.createAccount is only compatible wit…
Aboudjem Aug 15, 2024
84f0948
refactor: Improve natspec for validateUserop
Aboudjem Aug 16, 2024
d8696cc
refactor: Update error for removing last validator
Aboudjem Aug 16, 2024
0a12d75
refactor: Update error for removing last validator
Aboudjem Aug 16, 2024
a2730fc
refactor: Update error for removing last validator
Aboudjem Aug 16, 2024
1ec9599
Merge branch 'remediations/cantina-spearbit' into fix/cantina-fixes
Aboudjem Aug 18, 2024
b1f51b2
Merge branch 'remediations/cantina-spearbit' into fix/cantina-fixes
Aboudjem Aug 19, 2024
fb1e85b
respond to PR comments
livingrockrises Aug 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions contracts/Nexus.sol
Original file line number Diff line number Diff line change
Expand Up @@ -77,23 +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.
/// The entryPoint calls this only if validation succeeds. Fails by returning `VALIDATION_FAILED` for invalid signatures.
Aboudjem marked this conversation as resolved.
Show resolved Hide resolved
/// 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.
/// @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,
Expand Down Expand Up @@ -200,9 +201,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) {
Expand All @@ -220,7 +219,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);

require(success, NexusInitializationFailed());
require(_hasValidators(), MissingValidator());
Aboudjem marked this conversation as resolved.
Show resolved Hide resolved
}

function setRegistry(IERC7484 newRegistry, address[] calldata attesters, uint8 threshold) external payable onlyEntryPointOrSelf {
Expand Down Expand Up @@ -325,7 +326,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(
Expand Down
16 changes: 11 additions & 5 deletions contracts/base/ExecutionHelper.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
35 changes: 18 additions & 17 deletions contracts/base/ModuleManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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());
livingrockrises marked this conversation as resolved.
Show resolved Hide resolved

// Perform the removal first
validators.pop(prev, validator);

// Sentinel pointing to itself means the list is empty, so check this after removal
require(_hasValidators(), 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.
Expand Down Expand Up @@ -328,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
Expand Down Expand Up @@ -374,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]);
Expand Down Expand Up @@ -463,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);
Aboudjem marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked the SentinelListLib again. There's a bug in this lib when you call popAll() it leaves the list in an uninitialized state instead of an initialized but empty state.

You're not using popAll() but it's better to fix it and change this check to also ensure the next is not the zero address (uninitialized):

return _getAccountStorage().validators.getNext(address(0x01)) != address(0x01) && _getAccountStorage().validators.getNext(address(0x01)) != address(0x00);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this comment be appropriate then? @MrToph

// Sentinel pointing to itself means the list is empty, so check this after removal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just double check above comment. actual fix is done @MrToph

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can write this instead

// Sentinel pointing to itself / zero means the list is empty / uninitialized, so check this after removal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok changing this

}

/// @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.
Expand Down
30 changes: 17 additions & 13 deletions contracts/base/RegistryAdapter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -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);
}
}
Expand Down
2 changes: 1 addition & 1 deletion contracts/factory/K1ValidatorFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
livingrockrises marked this conversation as resolved.
Show resolved Hide resolved
ZeroAddressNotAllowed()
);
ACCOUNT_IMPLEMENTATION = implementation;
Expand Down
Loading
Loading