Skip to content

Commit

Permalink
proper uninstalling in onRedelegate
Browse files Browse the repository at this point in the history
  • Loading branch information
Filipp Makarov authored and Filipp Makarov committed Dec 23, 2024
1 parent 45d738a commit 779d85d
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 9 deletions.
8 changes: 5 additions & 3 deletions contracts/Nexus.sol
Original file line number Diff line number Diff line change
Expand Up @@ -426,10 +426,12 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
/// @dev This function is called when the account is redelegated.
function _onRedelegation() internal virtual override {
AccountStorage storage $ = _getAccountStorage();
$.validators.popAll();
$.executors.popAll();

_tryUninstallValidators();
_tryUninstallExecutors();
$.emergencyUninstallTimelock[address($.hook)] = 0;
$.hook = IHook(ZERO_ADDRESS);
_tryUninstallHook();

// reinitialize the module manager
_initModuleManager();
}
Expand Down
41 changes: 40 additions & 1 deletion contracts/base/ModuleManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ pragma solidity ^0.8.27;
// Nexus: A suite of contracts for Modular Smart Accounts compliant with ERC-7579 and ERC-4337, developed by Biconomy.
// Learn more at https://biconomy.io. To report security issues, please contact us at: [email protected]

import { SentinelListLib } from "sentinellist/SentinelList.sol";
import { SentinelListLib, SENTINEL } from "sentinellist/SentinelList.sol";
import { Storage } from "./Storage.sol";
import { IHook } from "../interfaces/modules/IHook.sol";
import { IModule } from "../interfaces/modules/IModule.sol";
Expand Down Expand Up @@ -194,6 +194,21 @@ abstract contract ModuleManager is Storage, EIP712, IModuleManagerEventsAndError
validator.excessivelySafeCall(gasleft(), 0, 0, abi.encodeWithSelector(IModule.onUninstall.selector, disableModuleData));
}

/// @dev Uninstalls all validators and emits an event if any validator fails to uninstall.
function _tryUninstallValidators() internal {
SentinelListLib.SentinelList storage validators = _getAccountStorage().validators;
address validator = validators.getNext(SENTINEL);
// we do not need excessivelySafeCall here as it prevents reversion
// we want to know if there's revert and emit the event
while (validator != SENTINEL) {
try IValidator(validator).onUninstall("") {} catch (bytes memory reason) {
emit ValidatorUninstallFailed(validator, "", reason);
}
validator = validators.getNext(validator);
}
validators.popAll();
}

/// @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 All @@ -212,6 +227,19 @@ abstract contract ModuleManager is Storage, EIP712, IModuleManagerEventsAndError
executor.excessivelySafeCall(gasleft(), 0, 0, abi.encodeWithSelector(IModule.onUninstall.selector, disableModuleData));
}

/// @dev Uninstalls all executors and emits an event if any executor fails to uninstall.
function _tryUninstallExecutors() internal {
SentinelListLib.SentinelList storage executors = _getAccountStorage().executors;
address executor = executors.getNext(SENTINEL);
while (executor != SENTINEL) {
try IExecutor(executor).onUninstall("") {} catch (bytes memory reason) {
emit ExecutorUninstallFailed(executor, "", reason);
}
executor = executors.getNext(executor);
}
executors.popAll();
}

/// @dev Installs a hook module, ensuring no other hooks are installed before proceeding.
/// @param hook The address of the hook to be installed.
/// @param data Initialization data to configure the hook upon installation.
Expand All @@ -231,6 +259,17 @@ abstract contract ModuleManager is Storage, EIP712, IModuleManagerEventsAndError
hook.excessivelySafeCall(gasleft(), 0, 0, abi.encodeWithSelector(IModule.onUninstall.selector, data));
}

/// @dev Uninstalls the hook and emits an event if the hook fails to uninstall.
function _tryUninstallHook() internal {
address hook = _getHook();
if (hook != address(0)) {
try IHook(hook).onUninstall("") {} catch (bytes memory reason) {
emit HookUninstallFailed(hook, "", reason);
}
_setHook(address(0));
}
}

/// @dev Sets the current hook in the storage to the specified address.
/// @param hook The new hook address.
function _setHook(address hook) internal virtual {
Expand Down
4 changes: 4 additions & 0 deletions contracts/interfaces/base/IModuleManagerEventsAndErrors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ interface IModuleManagerEventsAndErrors {
/// @param module The address of the uninstalled module.
event ModuleUninstalled(uint256 moduleTypeId, address module);

event ExecutorUninstallFailed(address executor, bytes data, bytes reason);
event ValidatorUninstallFailed(address validator, bytes data, bytes reason);
event HookUninstallFailed(address hook, bytes data, bytes reason);

/// @notice Thrown when attempting to remove the last validator.
error CanNotRemoveLastValidator();

Expand Down
5 changes: 0 additions & 5 deletions test/foundry/unit/concrete/eip7702/TestEIP7702.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -260,9 +260,4 @@ contract TestEIP7702 is NexusTest_Base {
assertTrue(INexus(account).isModuleInstalled(MODULE_TYPE_HOOK, address(HOOK_MODULE), ""));
}

}

interface IModuleInfo {
function getValidatorsPaginated(address cursor, uint256 maxCount) external view returns (address[] memory validators, address nextValidator);
function getExecutorsPaginated(address cursor, uint256 maxCount) external view returns (address[] memory executors, address nextExecutor);
}

0 comments on commit 779d85d

Please sign in to comment.