From fa8d1cec4208287f99c5eed4ca24b190bc7d995f Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Wed, 18 Dec 2024 18:23:04 +0300 Subject: [PATCH 1/6] draft 7739 support --- contracts/Nexus.sol | 3 +- contracts/base/ERC7779Adapter.sol | 60 +++++++++++++++++++ contracts/mocks/MockERC7779.sol | 12 ++++ .../unit/fuzz/TestFuzz_ERC7779Adapter.t.sol | 35 +++++++++++ 4 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 contracts/base/ERC7779Adapter.sol create mode 100644 contracts/mocks/MockERC7779.sol create mode 100644 test/foundry/unit/fuzz/TestFuzz_ERC7779Adapter.t.sol diff --git a/contracts/Nexus.sol b/contracts/Nexus.sol index 265dc1de..8928ba28 100644 --- a/contracts/Nexus.sol +++ b/contracts/Nexus.sol @@ -25,6 +25,7 @@ import { MODULE_TYPE_VALIDATOR, MODULE_TYPE_EXECUTOR, MODULE_TYPE_FALLBACK, MODU import { ModeLib, ExecutionMode, ExecType, CallType, CALLTYPE_BATCH, CALLTYPE_SINGLE, CALLTYPE_DELEGATECALL, EXECTYPE_DEFAULT, EXECTYPE_TRY } from "./lib/ModeLib.sol"; import { NonceLib } from "./lib/NonceLib.sol"; import { SentinelListLib, SENTINEL, ZERO_ADDRESS } from "sentinellist/SentinelList.sol"; +import { ERC7779Adapter } from "./base/ERC7779Adapter.sol"; /// @title Nexus - Smart Account /// @notice This contract integrates various functionalities to handle modular smart accounts compliant with ERC-7579 and ERC-4337 standards. @@ -34,7 +35,7 @@ import { SentinelListLib, SENTINEL, ZERO_ADDRESS } from "sentinellist/SentinelLi /// @author @filmakarov | Biconomy | filipp.makarov@biconomy.io /// @author @zeroknots | Rhinestone.wtf | zeroknots.eth /// Special thanks to the Solady team for foundational contributions: https://github.com/Vectorized/solady -contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgradeable { +contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgradeable, ERC7779Adapter { using ModeLib for ExecutionMode; using ExecLib for bytes; using NonceLib for uint256; diff --git a/contracts/base/ERC7779Adapter.sol b/contracts/base/ERC7779Adapter.sol new file mode 100644 index 00000000..b91ba35a --- /dev/null +++ b/contracts/base/ERC7779Adapter.sol @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.27; + +abstract contract ERC7779Adapter { + error NonAuthorizedOnRedelegationCaller(); + + // keccak256(abi.encode(uint256(keccak256(bytes("InteroperableDelegatedAccount.ERC.Storage"))) - 1)) & ~bytes32(uint256(0xff)); + bytes32 internal constant ERC7779_STORAGE_BASE = 0xc473de86d0138e06e4d4918a106463a7cc005258d2e21915272bcb4594c18900; + + struct ERC7779Storage { + bytes32[] storageBases; + } + + /* + * @dev Externally shares the storage bases that has been used throughout the account. + * Majority of 7702 accounts will have their distinctive storage base to reduce the + chance of storage collision. + * This allows the external entities to know what the storage base is of the account. + * Wallets willing to redelegate already-delegated accounts should call + accountStorageBase() to check if it confirms with the account it plans to redelegate. + * + * The bytes32 array should be stored at the storage slot: + keccak(keccak('InteroperableDelegatedAccount.ERC.Storage')-1) & ~0xff + * This is an append-only array so newly redelegated accounts should not overwrite the + storage at this slot, but just append their base to the array. + * This append operation should be done during the initialization of the account. + */ + function accountStorageBases() external view returns (bytes32[] memory) { + ERC7779Storage storage $; + assembly { + $.slot := ERC7779_STORAGE_BASE + } + return $.storageBases; + } + + function _addStorageBase(bytes32 storageBase) internal { + ERC7779Storage storage $; + assembly { + $.slot := ERC7779_STORAGE_BASE + } + $.storageBases.push(storageBase); + } + + /* + * @dev Function called before redelegation. + * This function should prepare the account for a delegation to a different + implementation. + * This function could be triggered by the new wallet that wants to redelegate an already + delegated EOA. + * It should uninitialize storages if needed and execute wallet-specific logic to prepare + for redelegation. + * msg.sender should be the owner of the account. + */ + function onRedelegation() external returns (bool) { + require(msg.sender == address(this), NonAuthorizedOnRedelegationCaller()); + // this is not implemented at the moment so that the account can preserve state across + // delegations + return true; + } +} \ No newline at end of file diff --git a/contracts/mocks/MockERC7779.sol b/contracts/mocks/MockERC7779.sol new file mode 100644 index 00000000..4ee62a2f --- /dev/null +++ b/contracts/mocks/MockERC7779.sol @@ -0,0 +1,12 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.27; + +import { ERC7779Adapter } from "../base/ERC7779Adapter.sol"; + +contract MockERC7779 is ERC7779Adapter { + + function addStorageBase(bytes32 storageBase) external { + _addStorageBase(storageBase); + } + +} diff --git a/test/foundry/unit/fuzz/TestFuzz_ERC7779Adapter.t.sol b/test/foundry/unit/fuzz/TestFuzz_ERC7779Adapter.t.sol new file mode 100644 index 00000000..f36c713e --- /dev/null +++ b/test/foundry/unit/fuzz/TestFuzz_ERC7779Adapter.t.sol @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.27; + +import { MockERC7779 } from "contracts/mocks/MockERC7779.sol"; +import "forge-std/Test.sol"; + +/// @title TestFuzz_ERC7779Adapter +/// @notice Tests the ERC7779Adapter contract +contract TestFuzz_ERC7779Adapter is Test { + MockERC7779 private mockERC7779; + + function setUp() public { + mockERC7779 = new MockERC7779(); + //bytes32 erc7779StorageBase = keccak256(abi.encode(uint256(keccak256(bytes("InteroperableDelegatedAccount.ERC.Storage"))) - 1)) & ~bytes32(uint256(0xff)); + //console.logBytes32(erc7779StorageBase); + } + + function test_Fuzz_ERC7779Adapter_AddStorageBases(uint256 amountOfBases) public { + vm.assume(amountOfBases > 0 && amountOfBases < 100); + bytes32[] memory expectedStorageBases = new bytes32[](amountOfBases); + + for (uint256 i = 0; i < amountOfBases; i++) { + bytes32 storageBase = bytes32(uint256(i)); + expectedStorageBases[i] = storageBase; + mockERC7779.addStorageBase(storageBase); + } + + bytes32[] memory retrievedStorageBases = mockERC7779.accountStorageBases(); + assertEq(retrievedStorageBases.length, amountOfBases); + for (uint256 i = 0; i < amountOfBases; i++) { + assertEq(retrievedStorageBases[i], expectedStorageBases[i]); + } + } +} + From 40b0d2d9a2ff448d8530d9f48aa6e6e78cf59699 Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Wed, 18 Dec 2024 18:25:16 +0300 Subject: [PATCH 2/6] add base at init --- contracts/Nexus.sol | 1 + contracts/base/Storage.sol | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/Nexus.sol b/contracts/Nexus.sol index 8928ba28..8c15b4d4 100644 --- a/contracts/Nexus.sol +++ b/contracts/Nexus.sol @@ -211,6 +211,7 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra _initModuleManager(); (address bootstrap, bytes memory bootstrapCall) = abi.decode(initData, (address, bytes)); (bool success, ) = bootstrap.delegatecall(bootstrapCall); + _addStorageBase(_STORAGE_LOCATION); require(success, NexusInitializationFailed()); require(_hasValidators(), NoValidatorInstalled()); diff --git a/contracts/base/Storage.sol b/contracts/base/Storage.sol index ddfd8073..59f6ebc0 100644 --- a/contracts/base/Storage.sol +++ b/contracts/base/Storage.sol @@ -25,7 +25,7 @@ import { IStorage } from "../interfaces/base/IStorage.sol"; contract Storage is IStorage { /// @custom:storage-location erc7201:biconomy.storage.Nexus /// ERC-7201 namespaced via `keccak256(abi.encode(uint256(keccak256(bytes("biconomy.storage.Nexus"))) - 1)) & ~bytes32(uint256(0xff));` - bytes32 private constant _STORAGE_LOCATION = 0x0bb70095b32b9671358306b0339b4c06e7cbd8cb82505941fba30d1eb5b82f00; + bytes32 internal constant _STORAGE_LOCATION = 0x0bb70095b32b9671358306b0339b4c06e7cbd8cb82505941fba30d1eb5b82f00; /// @dev Utilizes ERC-7201's namespaced storage pattern for isolated storage access. This method computes /// the storage slot based on a predetermined location, ensuring collision-resistant storage for contract states. From a968100c23059d8a797b6ce9ae35adcd33716910 Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Thu, 19 Dec 2024 16:25:59 +0300 Subject: [PATCH 3/6] rm excess sage of addBase --- contracts/Nexus.sol | 4 ++-- contracts/base/Storage.sol | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/Nexus.sol b/contracts/Nexus.sol index 5ee496f0..5bd9ae26 100644 --- a/contracts/Nexus.sol +++ b/contracts/Nexus.sol @@ -123,7 +123,6 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra if (!_isAlreadyInitialized()) { if (ECDSA.recover(userOpHash.toEthSignedMessageHash(), op.signature) == address(this)) { // add 7739 storage base - _addStorageBase(_STORAGE_LOCATION); validationData = VALIDATION_SUCCESS; } else { validationData = VALIDATION_FAILED; @@ -270,7 +269,8 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra _initModuleManager(); (address bootstrap, bytes memory bootstrapCall) = abi.decode(initData, (address, bytes)); (bool success, ) = bootstrap.delegatecall(bootstrapCall); - _addStorageBase(_STORAGE_LOCATION); + // TODO: DO IT FOR 7702 ONLY + _addStorageBase(_NEXUS_STORAGE_LOCATION); require(success, NexusInitializationFailed()); require(_hasValidators(), NoValidatorInstalled()); diff --git a/contracts/base/Storage.sol b/contracts/base/Storage.sol index 59f6ebc0..c1ea7039 100644 --- a/contracts/base/Storage.sol +++ b/contracts/base/Storage.sol @@ -25,7 +25,7 @@ import { IStorage } from "../interfaces/base/IStorage.sol"; contract Storage is IStorage { /// @custom:storage-location erc7201:biconomy.storage.Nexus /// ERC-7201 namespaced via `keccak256(abi.encode(uint256(keccak256(bytes("biconomy.storage.Nexus"))) - 1)) & ~bytes32(uint256(0xff));` - bytes32 internal constant _STORAGE_LOCATION = 0x0bb70095b32b9671358306b0339b4c06e7cbd8cb82505941fba30d1eb5b82f00; + bytes32 internal constant _NEXUS_STORAGE_LOCATION = 0x0bb70095b32b9671358306b0339b4c06e7cbd8cb82505941fba30d1eb5b82f00; /// @dev Utilizes ERC-7201's namespaced storage pattern for isolated storage access. This method computes /// the storage slot based on a predetermined location, ensuring collision-resistant storage for contract states. @@ -34,7 +34,7 @@ contract Storage is IStorage { /// @return $ The proxy to the `AccountStorage` struct, providing a reference to the namespaced storage slot. function _getAccountStorage() internal pure returns (AccountStorage storage $) { assembly { - $.slot := _STORAGE_LOCATION + $.slot := _NEXUS_STORAGE_LOCATION } } } From 24a08a21413b0b2d9123e717d669713381959594 Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Mon, 23 Dec 2024 12:05:47 +0300 Subject: [PATCH 4/6] add base only when Nexus is erc7702 --- contracts/Nexus.sol | 5 +++-- contracts/base/BaseAccount.sol | 10 ++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/contracts/Nexus.sol b/contracts/Nexus.sol index 5bd9ae26..7cc66a9b 100644 --- a/contracts/Nexus.sol +++ b/contracts/Nexus.sol @@ -269,8 +269,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); - // TODO: DO IT FOR 7702 ONLY - _addStorageBase(_NEXUS_STORAGE_LOCATION); + if (_amIERC7702()) { + _addStorageBase(_NEXUS_STORAGE_LOCATION); + } require(success, NexusInitializationFailed()); require(_hasValidators(), NoValidatorInstalled()); diff --git a/contracts/base/BaseAccount.sol b/contracts/base/BaseAccount.sol index 1f15e989..84e0c473 100644 --- a/contracts/base/BaseAccount.sol +++ b/contracts/base/BaseAccount.sol @@ -127,4 +127,14 @@ contract BaseAccount is IBaseAccount { function entryPoint() external view returns (address) { return _ENTRYPOINT; } + + function _amIERC7702() internal view returns (bool res) { + assembly { + res := + eq( + extcodehash(address()), + 0xeadcdba66a79ab5dce91622d1d75c8cff5cff0b96944c3bf1072cd08ce018329 // (keccak256(0xef01)) + ) + } + } } From 45d738a9afbcd75eade3aa579b7cd177f9d9feba Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Mon, 23 Dec 2024 12:52:47 +0300 Subject: [PATCH 5/6] implement onRedelegate --- contracts/Nexus.sol | 13 ++++++++ contracts/base/ERC7779Adapter.sol | 22 +++++++------- contracts/interfaces/IERC7779.sol | 26 ++++++++++++++++ contracts/interfaces/INexus.sol | 4 +-- contracts/mocks/MockERC7779.sol | 4 +++ .../unit/concrete/eip7702/TestEIP7702.t.sol | 30 +++++++++++++++++++ 6 files changed, 87 insertions(+), 12 deletions(-) create mode 100644 contracts/interfaces/IERC7779.sol diff --git a/contracts/Nexus.sol b/contracts/Nexus.sol index 7cc66a9b..96c89b30 100644 --- a/contracts/Nexus.sol +++ b/contracts/Nexus.sol @@ -21,6 +21,7 @@ import { IERC7484 } from "./interfaces/IERC7484.sol"; import { ModuleManager } from "./base/ModuleManager.sol"; import { ExecutionHelper } from "./base/ExecutionHelper.sol"; import { IValidator } from "./interfaces/modules/IValidator.sol"; +import { IHook } from "./interfaces/modules/IHook.sol"; import { MODULE_TYPE_VALIDATOR, MODULE_TYPE_EXECUTOR, @@ -269,6 +270,7 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra _initModuleManager(); (address bootstrap, bytes memory bootstrapCall) = abi.decode(initData, (address, bytes)); (bool success, ) = bootstrap.delegatecall(bootstrapCall); + if (_amIERC7702()) { _addStorageBase(_NEXUS_STORAGE_LOCATION); } @@ -421,6 +423,17 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra /// @param newImplementation The address of the new implementation to upgrade to. function _authorizeUpgrade(address newImplementation) internal virtual override(UUPSUpgradeable) onlyEntryPointOrSelf { } + /// @dev This function is called when the account is redelegated. + function _onRedelegation() internal virtual override { + AccountStorage storage $ = _getAccountStorage(); + $.validators.popAll(); + $.executors.popAll(); + $.emergencyUninstallTimelock[address($.hook)] = 0; + $.hook = IHook(ZERO_ADDRESS); + // reinitialize the module manager + _initModuleManager(); + } + /// @dev EIP712 domain name and version. function _domainNameAndVersion() internal pure override returns (string memory name, string memory version) { name = "Nexus"; diff --git a/contracts/base/ERC7779Adapter.sol b/contracts/base/ERC7779Adapter.sol index b91ba35a..382d07ab 100644 --- a/contracts/base/ERC7779Adapter.sol +++ b/contracts/base/ERC7779Adapter.sol @@ -1,7 +1,9 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.27; -abstract contract ERC7779Adapter { +import { IERC7779 } from "../interfaces/IERC7779.sol"; + +abstract contract ERC7779Adapter is IERC7779 { error NonAuthorizedOnRedelegationCaller(); // keccak256(abi.encode(uint256(keccak256(bytes("InteroperableDelegatedAccount.ERC.Storage"))) - 1)) & ~bytes32(uint256(0xff)); @@ -43,18 +45,18 @@ abstract contract ERC7779Adapter { /* * @dev Function called before redelegation. - * This function should prepare the account for a delegation to a different - implementation. - * This function could be triggered by the new wallet that wants to redelegate an already - delegated EOA. - * It should uninitialize storages if needed and execute wallet-specific logic to prepare - for redelegation. + * This function should prepare the account for a delegation to a different implementation. + * This function could be triggered by the new wallet that wants to redelegate an already delegated EOA. + * It should uninitialize storages if needed and execute wallet-specific logic to prepare for redelegation. * msg.sender should be the owner of the account. */ function onRedelegation() external returns (bool) { require(msg.sender == address(this), NonAuthorizedOnRedelegationCaller()); - // this is not implemented at the moment so that the account can preserve state across - // delegations + _onRedelegation(); return true; } -} \ No newline at end of file + + /// @dev This function is called when the account is redelegated. + /// @dev This function should be overridden by the account to implement wallet-specific logic. + function _onRedelegation() internal virtual; +} diff --git a/contracts/interfaces/IERC7779.sol b/contracts/interfaces/IERC7779.sol new file mode 100644 index 00000000..1b0a467b --- /dev/null +++ b/contracts/interfaces/IERC7779.sol @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.27; + +interface IERC7779 { + /* + * @dev Externally shares the storage bases that has been used throughout the account. + * Majority of 7702 accounts will have their distinctive storage base to reduce the chance of storage collision. + * This allows the external entities to know what the storage base is of the account. + * Wallets willing to redelegate already-delegated accounts should call accountStorageBase() to check if it confirms with the account it plans to redelegate. + * + * The bytes32 array should be stored at the storage slot: keccak(keccak('InteroperableDelegatedAccount.ERC.Storage')-1) & ~0xff + * This is an append-only array so newly redelegated accounts should not overwrite the storage at this slot, but just append their base to the array. + * This append operation should be done during the initialization of the account. + */ + function accountStorageBases() external view returns (bytes32[] memory); + + /* + * @dev Function called before redelegation. + * This function should prepare the account for a delegation to a different implementation. + * This function could be triggered by the new wallet that wants to redelegate an already delegated EOA. + * It should uninitialize storages if needed and execute wallet-specific logic to prepare for redelegation. + * msg.sender should be the owner of the account. + */ + function onRedelegation() external returns (bool); + +} diff --git a/contracts/interfaces/INexus.sol b/contracts/interfaces/INexus.sol index 31b379d2..1308dc33 100644 --- a/contracts/interfaces/INexus.sol +++ b/contracts/interfaces/INexus.sol @@ -15,7 +15,7 @@ pragma solidity ^0.8.27; import { IERC4337Account } from "./IERC4337Account.sol"; import { IERC7579Account } from "./IERC7579Account.sol"; import { INexusEventsAndErrors } from "./INexusEventsAndErrors.sol"; - +import { IERC7779 } from "./IERC7779.sol"; /// @title Nexus - INexus Interface /// @notice Integrates ERC-4337 and ERC-7579 standards to manage smart accounts within the Nexus suite. /// @dev Consolidates ERC-4337 user operations and ERC-7579 configurations into a unified interface for smart account management. @@ -27,7 +27,7 @@ import { INexusEventsAndErrors } from "./INexusEventsAndErrors.sol"; /// @author @filmakarov | Biconomy | filipp.makarov@biconomy.io /// @author @zeroknots | Rhinestone.wtf | zeroknots.eth /// Special thanks to the Solady team for foundational contributions: https://github.com/Vectorized/solady -interface INexus is IERC4337Account, IERC7579Account, INexusEventsAndErrors { +interface INexus is IERC4337Account, IERC7579Account, INexusEventsAndErrors, IERC7779 { /// @notice Initializes the smart account with a validator and custom data. /// @dev This method sets up the account for operation, linking it with a validator and initializing it with specific data. /// Can be called directly or via a factory. diff --git a/contracts/mocks/MockERC7779.sol b/contracts/mocks/MockERC7779.sol index 4ee62a2f..cc5c6483 100644 --- a/contracts/mocks/MockERC7779.sol +++ b/contracts/mocks/MockERC7779.sol @@ -9,4 +9,8 @@ contract MockERC7779 is ERC7779Adapter { _addStorageBase(storageBase); } + function _onRedelegation() internal override { + // do nothing + } + } diff --git a/test/foundry/unit/concrete/eip7702/TestEIP7702.t.sol b/test/foundry/unit/concrete/eip7702/TestEIP7702.t.sol index 07d8a489..6f0e8a0c 100644 --- a/test/foundry/unit/concrete/eip7702/TestEIP7702.t.sol +++ b/test/foundry/unit/concrete/eip7702/TestEIP7702.t.sol @@ -5,6 +5,7 @@ import { NexusTest_Base } from "../../../utils/NexusTest_Base.t.sol"; import "../../../utils/Imports.sol"; import { MockTarget } from "contracts/mocks/MockTarget.sol"; import { IExecutionHelper } from "contracts/interfaces/base/IExecutionHelper.sol"; +import { IHook } from "contracts/interfaces/modules/IHook.sol"; contract TestEIP7702 is NexusTest_Base { using ECDSA for bytes32; @@ -235,4 +236,33 @@ contract TestEIP7702 is NexusTest_Base { // Assert that the value was set ie that execution was successful assertTrue(valueTarget.balance == value); } + + function test_erc7702_redelegate() public { + address account = test_initializeAndExecSingle(); + assertTrue(INexus(account).isModuleInstalled(MODULE_TYPE_VALIDATOR, address(mockValidator), "")); + assertTrue(INexus(account).isModuleInstalled(MODULE_TYPE_EXECUTOR, address(mockExecutor), "")); + + // storage is cleared + vm.prank(address(account)); + INexus(account).onRedelegation(); + assertFalse(INexus(account).isModuleInstalled(MODULE_TYPE_VALIDATOR, address(mockValidator), "")); + assertFalse(INexus(account).isModuleInstalled(MODULE_TYPE_EXECUTOR, address(mockExecutor), "")); + + // account is properly initialized to install modules again + vm.startPrank(address(ENTRYPOINT)); + INexus(account).installModule(MODULE_TYPE_VALIDATOR, address(mockValidator), ""); + INexus(account).installModule(MODULE_TYPE_EXECUTOR, address(mockExecutor), ""); + INexus(account).installModule(MODULE_TYPE_HOOK, address(HOOK_MODULE), ""); + + vm.stopPrank(); + assertTrue(INexus(account).isModuleInstalled(MODULE_TYPE_VALIDATOR, address(mockValidator), "")); + assertTrue(INexus(account).isModuleInstalled(MODULE_TYPE_EXECUTOR, address(mockExecutor), "")); + 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); +} \ No newline at end of file From 779d85d5498bf212e0b304468e20278565667251 Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Mon, 23 Dec 2024 15:12:31 +0300 Subject: [PATCH 6/6] proper uninstalling in onRedelegate --- contracts/Nexus.sol | 8 ++-- contracts/base/ModuleManager.sol | 41 ++++++++++++++++++- .../base/IModuleManagerEventsAndErrors.sol | 4 ++ .../unit/concrete/eip7702/TestEIP7702.t.sol | 5 --- 4 files changed, 49 insertions(+), 9 deletions(-) diff --git a/contracts/Nexus.sol b/contracts/Nexus.sol index 96c89b30..3d1dfdb4 100644 --- a/contracts/Nexus.sol +++ b/contracts/Nexus.sol @@ -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(); } diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index 96d2412b..21082c01 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -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: security@biconomy.io -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"; @@ -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. @@ -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. @@ -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 { diff --git a/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol b/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol index 77ddc271..b3a49cb7 100644 --- a/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol +++ b/contracts/interfaces/base/IModuleManagerEventsAndErrors.sol @@ -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(); diff --git a/test/foundry/unit/concrete/eip7702/TestEIP7702.t.sol b/test/foundry/unit/concrete/eip7702/TestEIP7702.t.sol index 6f0e8a0c..4a5623da 100644 --- a/test/foundry/unit/concrete/eip7702/TestEIP7702.t.sol +++ b/test/foundry/unit/concrete/eip7702/TestEIP7702.t.sol @@ -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); } \ No newline at end of file