From 003123cb35fb26e993a5015c2d4069e8a40d4abd Mon Sep 17 00:00:00 2001 From: SoraSuegami Date: Fri, 30 Aug 2024 21:34:40 +0900 Subject: [PATCH 01/15] Add audit fixes --- src/EmailRecoveryManager.sol | 22 +++-- src/interfaces/ISafe.sol | 6 +- src/modules/EmailRecoveryModule.sol | 23 +++-- src/modules/SafeEmailRecoveryModule.sol | 2 +- src/modules/UniversalEmailRecoveryModule.sol | 25 ++++-- .../EmailRecoveryModule/constructor.t.sol | 90 +++++++++---------- .../allowValidatorRecovery.t.sol | 74 +++++++-------- 7 files changed, 137 insertions(+), 105 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 8207c53..c8d54ae 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -441,17 +441,27 @@ abstract contract EmailRecoveryManager is } /** - * @notice Removes all state related to an account. + * @notice Removes all state related to msg.sender. * @dev In order to prevent unexpected behaviour when reinstalling account modules, the module * should be deinitialized. This should include remove state accociated with an account. */ function deInitRecoveryModule() internal onlyWhenNotRecovering { - delete recoveryConfigs[msg.sender]; - delete recoveryRequests[msg.sender]; + address account = msg.sender; + deInitRecoveryModule(account); + } + + /** + * @notice Removes all state related to an account. + * @dev In order to prevent unexpected behaviour when reinstalling account modules, the module + * should be deinitialized. This should include remove state accociated with an account. + */ + function deInitRecoveryModule(address account) internal onlyWhenNotRecovering { + delete recoveryConfigs[account]; + delete recoveryRequests[account]; - removeAllGuardians(msg.sender); - delete guardianConfigs[msg.sender]; + removeAllGuardians(account); + delete guardianConfigs[account]; - emit RecoveryDeInitialized(msg.sender); + emit RecoveryDeInitialized(account); } } diff --git a/src/interfaces/ISafe.sol b/src/interfaces/ISafe.sol index 6d3c06b..784a836 100644 --- a/src/interfaces/ISafe.sol +++ b/src/interfaces/ISafe.sol @@ -14,5 +14,9 @@ interface ISafe { ) external returns (bool success); - function isModuleEnabled(address module) external view returns (bool); + function isModuleEnabled(address module) external view returns (bool); + function addOwnerWithThreshold(address owner, uint256 _threshold) external; + function removeOwner(address prevOwner, address owner, uint256 _threshold) external; + function swapOwner(address prevOwner, address oldOwner, address newOwner) external; + function changeThreshold(uint256 _threshold) external; } diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index f30c9c3..8f295a6 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -53,13 +53,22 @@ contract EmailRecoveryModule is EmailRecoveryManager, ERC7579ExecutorBase, IEmai if (_validator == address(0)) { revert InvalidValidator(_validator); } - if ( - _selector == IModule.onUninstall.selector || _selector == IModule.onInstall.selector - || _selector == IERC7579Account.execute.selector - || _selector == ISafe.setFallbackHandler.selector - || _selector == ISafe.setGuard.selector || _selector == bytes4(0) - ) { - revert InvalidSelector(_selector); + if(_validator == msg.sender) { + if ( + _selector == ISafe.addOwnerWithThreshold.selector || _selector == ISafe.removeOwner.selector + || _selector == ISafe.swapOwner.selector + || _selector == ISafe.changeThreshold.selector + || _selector == bytes4(0) + ) { + revert InvalidSelector(_selector); + } + } else { + if ( + _selector == IModule.onInstall.selector || _selector == IModule.onUninstall.selector + || _selector == bytes4(0) + ) { + revert InvalidSelector(_selector); + } } validator = _validator; diff --git a/src/modules/SafeEmailRecoveryModule.sol b/src/modules/SafeEmailRecoveryModule.sol index a3a4807..321bbea 100644 --- a/src/modules/SafeEmailRecoveryModule.sol +++ b/src/modules/SafeEmailRecoveryModule.sol @@ -92,6 +92,6 @@ contract SafeEmailRecoveryModule is EmailRecoveryManager { if (ISafe(account).isModuleEnabled(address(this)) == true) { revert ResetFailed(account); } - deInitRecoveryModule(); + deInitRecoveryModule(account); } } diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index c464561..d56463d 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -68,14 +68,23 @@ contract UniversalEmailRecoveryModule is * @notice Modifier to check whether the selector is safe. Reverts if the selector is for * "onInstall" or "onUninstall" */ - modifier withoutUnsafeSelector(bytes4 selector) { - if ( - selector == IModule.onUninstall.selector || selector == IModule.onInstall.selector - || selector == IERC7579Account.execute.selector - || selector == ISafe.setFallbackHandler.selector || selector == ISafe.setGuard.selector + modifier withoutUnsafeSelector(address validator, bytes4 selector) { + if(validator == msg.sender) { + if ( + selector == ISafe.addOwnerWithThreshold.selector || selector == ISafe.removeOwner.selector + || selector == ISafe.swapOwner.selector + || selector == ISafe.changeThreshold.selector || selector == bytes4(0) - ) { - revert InvalidSelector(selector); + ) { + revert InvalidSelector(selector); + } + } else { + if ( + selector == IModule.onInstall.selector || selector == IModule.onUninstall.selector + || selector == bytes4(0) + ) { + revert InvalidSelector(selector); + } } _; } @@ -149,7 +158,7 @@ contract UniversalEmailRecoveryModule is ) public onlyWhenInitialized - withoutUnsafeSelector(recoverySelector) + withoutUnsafeSelector(validator, recoverySelector) { if ( !IERC7579Account(msg.sender).isModuleInstalled( diff --git a/test/unit/modules/EmailRecoveryModule/constructor.t.sol b/test/unit/modules/EmailRecoveryModule/constructor.t.sol index f2d126e..382130b 100644 --- a/test/unit/modules/EmailRecoveryModule/constructor.t.sol +++ b/test/unit/modules/EmailRecoveryModule/constructor.t.sol @@ -60,53 +60,53 @@ contract EmailRecoveryModule_constructor_Test is EmailRecoveryModuleBase { ); } - function test_Constructor_RevertWhen_UnsafeExecuteSelector() public { - vm.expectRevert( - abi.encodeWithSelector( - EmailRecoveryModule.InvalidSelector.selector, IERC7579Account.execute.selector - ) - ); - new EmailRecoveryModule( - address(verifier), - address(dkimRegistry), - address(emailAuthImpl), - address(emailRecoveryHandler), - validatorAddress, - IERC7579Account.execute.selector - ); - } + // function test_Constructor_RevertWhen_UnsafeExecuteSelector() public { + // vm.expectRevert( + // abi.encodeWithSelector( + // EmailRecoveryModule.InvalidSelector.selector, IERC7579Account.execute.selector + // ) + // ); + // new EmailRecoveryModule( + // address(verifier), + // address(dkimRegistry), + // address(emailAuthImpl), + // address(emailRecoveryHandler), + // validatorAddress, + // IERC7579Account.execute.selector + // ); + // } - function test_Constructor_RevertWhen_UnsafeSetFallbackHandlerSelector() public { - vm.expectRevert( - abi.encodeWithSelector( - EmailRecoveryModule.InvalidSelector.selector, ISafe.setFallbackHandler.selector - ) - ); - new EmailRecoveryModule( - address(verifier), - address(dkimRegistry), - address(emailAuthImpl), - address(emailRecoveryHandler), - validatorAddress, - ISafe.setFallbackHandler.selector - ); - } + // function test_Constructor_RevertWhen_UnsafeSetFallbackHandlerSelector() public { + // vm.expectRevert( + // abi.encodeWithSelector( + // EmailRecoveryModule.InvalidSelector.selector, ISafe.setFallbackHandler.selector + // ) + // ); + // new EmailRecoveryModule( + // address(verifier), + // address(dkimRegistry), + // address(emailAuthImpl), + // address(emailRecoveryHandler), + // validatorAddress, + // ISafe.setFallbackHandler.selector + // ); + // } - function test_Constructor_RevertWhen_UnsafeSetGuardSelector() public { - vm.expectRevert( - abi.encodeWithSelector( - EmailRecoveryModule.InvalidSelector.selector, ISafe.setGuard.selector - ) - ); - new EmailRecoveryModule( - address(verifier), - address(dkimRegistry), - address(emailAuthImpl), - address(emailRecoveryHandler), - validatorAddress, - ISafe.setGuard.selector - ); - } + // function test_Constructor_RevertWhen_UnsafeSetGuardSelector() public { + // vm.expectRevert( + // abi.encodeWithSelector( + // EmailRecoveryModule.InvalidSelector.selector, ISafe.setGuard.selector + // ) + // ); + // new EmailRecoveryModule( + // address(verifier), + // address(dkimRegistry), + // address(emailAuthImpl), + // address(emailRecoveryHandler), + // validatorAddress, + // ISafe.setGuard.selector + // ); + // } function test_Constructor_RevertWhen_InvalidSelector() public { vm.expectRevert( diff --git a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol index cb2b737..6fa126e 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol @@ -52,43 +52,43 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { ); } - function test_AllowValidatorRecovery_RevertWhen_UnsafeExecuteSelector() public { - vm.expectRevert( - abi.encodeWithSelector( - UniversalEmailRecoveryModule.InvalidSelector.selector, - IERC7579Account.execute.selector - ) - ); - vm.startPrank(accountAddress); - emailRecoveryModule.allowValidatorRecovery( - validatorAddress, bytes("0"), IERC7579Account.execute.selector - ); - } - - function test_AllowValidatorRecovery_RevertWhen_UnsafeSetFallbackHandlerSelector() public { - vm.expectRevert( - abi.encodeWithSelector( - UniversalEmailRecoveryModule.InvalidSelector.selector, - ISafe.setFallbackHandler.selector - ) - ); - vm.startPrank(accountAddress); - emailRecoveryModule.allowValidatorRecovery( - validatorAddress, bytes("0"), ISafe.setFallbackHandler.selector - ); - } - - function test_AllowValidatorRecovery_RevertWhen_UnsafeSetGuardSelector() public { - vm.expectRevert( - abi.encodeWithSelector( - UniversalEmailRecoveryModule.InvalidSelector.selector, ISafe.setGuard.selector - ) - ); - vm.startPrank(accountAddress); - emailRecoveryModule.allowValidatorRecovery( - validatorAddress, bytes("0"), ISafe.setGuard.selector - ); - } + // function test_AllowValidatorRecovery_RevertWhen_UnsafeExecuteSelector() public { + // vm.expectRevert( + // abi.encodeWithSelector( + // UniversalEmailRecoveryModule.InvalidSelector.selector, + // IERC7579Account.execute.selector + // ) + // ); + // vm.startPrank(accountAddress); + // emailRecoveryModule.allowValidatorRecovery( + // validatorAddress, bytes("0"), IERC7579Account.execute.selector + // ); + // } + + // function test_AllowValidatorRecovery_RevertWhen_UnsafeSetFallbackHandlerSelector() public { + // vm.expectRevert( + // abi.encodeWithSelector( + // UniversalEmailRecoveryModule.InvalidSelector.selector, + // ISafe.setFallbackHandler.selector + // ) + // ); + // vm.startPrank(accountAddress); + // emailRecoveryModule.allowValidatorRecovery( + // validatorAddress, bytes("0"), ISafe.setFallbackHandler.selector + // ); + // } + + // function test_AllowValidatorRecovery_RevertWhen_UnsafeSetGuardSelector() public { + // vm.expectRevert( + // abi.encodeWithSelector( + // UniversalEmailRecoveryModule.InvalidSelector.selector, ISafe.setGuard.selector + // ) + // ); + // vm.startPrank(accountAddress); + // emailRecoveryModule.allowValidatorRecovery( + // validatorAddress, bytes("0"), ISafe.setGuard.selector + // ); + // } function test_AllowValidatorRecovery_RevertWhen_InvalidSelector() public { vm.expectRevert( From 801fc689dae70ae55e3dcec85eb2b48c94e26122 Mon Sep 17 00:00:00 2001 From: SoraSuegami Date: Fri, 30 Aug 2024 22:34:50 +0900 Subject: [PATCH 02/15] FIx selector check when validator == msg.sender --- src/modules/EmailRecoveryModule.sol | 7 +++---- src/modules/UniversalEmailRecoveryModule.sol | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index 8f295a6..8815f8f 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -55,10 +55,9 @@ contract EmailRecoveryModule is EmailRecoveryManager, ERC7579ExecutorBase, IEmai } if(_validator == msg.sender) { if ( - _selector == ISafe.addOwnerWithThreshold.selector || _selector == ISafe.removeOwner.selector - || _selector == ISafe.swapOwner.selector - || _selector == ISafe.changeThreshold.selector - || _selector == bytes4(0) + _selector != ISafe.addOwnerWithThreshold.selector && _selector != ISafe.removeOwner.selector + && _selector != ISafe.swapOwner.selector + && _selector != ISafe.changeThreshold.selector ) { revert InvalidSelector(_selector); } diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index d56463d..c597e42 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -71,10 +71,9 @@ contract UniversalEmailRecoveryModule is modifier withoutUnsafeSelector(address validator, bytes4 selector) { if(validator == msg.sender) { if ( - selector == ISafe.addOwnerWithThreshold.selector || selector == ISafe.removeOwner.selector - || selector == ISafe.swapOwner.selector - || selector == ISafe.changeThreshold.selector - || selector == bytes4(0) + selector != ISafe.addOwnerWithThreshold.selector && selector != ISafe.removeOwner.selector + && selector != ISafe.swapOwner.selector + && selector != ISafe.changeThreshold.selector ) { revert InvalidSelector(selector); } From 4f0d7828f77222500918f72b9ba16bf30a7138cd Mon Sep 17 00:00:00 2001 From: John Guilding <54913924+JohnGuilding@users.noreply.github.com> Date: Wed, 4 Sep 2024 16:06:14 +0100 Subject: [PATCH 03/15] Ignore validator address in EmailRecoveryModule (#43) * Ignore validator address in EmailRecoveryModule * Compile warnings & errors --- .solhint.json | 3 +- src/handlers/EmailRecoverySubjectHandler.sol | 2 +- src/interfaces/IEmailRecoveryManager.sol | 2 +- src/modules/EmailRecoveryModule.sol | 8 +-- src/modules/SafeEmailRecoveryModule.sol | 1 + src/modules/UniversalEmailRecoveryModule.sol | 3 +- .../modules/EmailRecoveryModule/recover.t.sol | 59 +------------------ 7 files changed, 12 insertions(+), 66 deletions(-) diff --git a/.solhint.json b/.solhint.json index 5647c17..15c2326 100644 --- a/.solhint.json +++ b/.solhint.json @@ -13,6 +13,7 @@ "no-empty-blocks": "off", "not-rely-on-time": "off", "one-contract-per-file": "off", - "var-name-mixedcase": "off" + "var-name-mixedcase": "off", + "immutable-vars-naming": "off" } } diff --git a/src/handlers/EmailRecoverySubjectHandler.sol b/src/handlers/EmailRecoverySubjectHandler.sol index 976c7f3..bb81268 100644 --- a/src/handlers/EmailRecoverySubjectHandler.sol +++ b/src/handlers/EmailRecoverySubjectHandler.sol @@ -124,7 +124,7 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { bytes[] calldata subjectParams ) public - view + pure returns (address) { if (templateIdx != 0) { diff --git a/src/interfaces/IEmailRecoveryManager.sol b/src/interfaces/IEmailRecoveryManager.sol index e5def54..9aef2c4 100644 --- a/src/interfaces/IEmailRecoveryManager.sol +++ b/src/interfaces/IEmailRecoveryManager.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.25; -import { GuardianStorage, GuardianStatus } from "../libraries/EnumerableGuardianMap.sol"; +import { GuardianStatus } from "../libraries/EnumerableGuardianMap.sol"; interface IEmailRecoveryManager { /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index 8815f8f..7dde261 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -7,7 +7,6 @@ import { IModule } from "erc7579/interfaces/IERC7579Module.sol"; import { ISafe } from "../interfaces/ISafe.sol"; import { IEmailRecoveryModule } from "../interfaces/IEmailRecoveryModule.sol"; import { EmailRecoveryManager } from "../EmailRecoveryManager.sol"; -import { GuardianManager } from "../GuardianManager.sol"; /** * @title EmailRecoveryModule @@ -148,14 +147,11 @@ contract EmailRecoveryModule is EmailRecoveryManager, ERC7579ExecutorBase, IEmai * being recovered. recoveryData = abi.encode(validator, recoveryFunctionCalldata) */ function recover(address account, bytes calldata recoveryData) internal override { - (address validator, bytes memory recoveryCalldata) = + (, bytes memory recoveryCalldata) = abi.decode(recoveryData, (address, bytes)); - if (validator == address(0)) { - revert InvalidValidator(validator); - } - bytes4 calldataSelector; + // solhint-disable-next-line no-inline-assembly assembly { calldataSelector := mload(add(recoveryCalldata, 32)) } diff --git a/src/modules/SafeEmailRecoveryModule.sol b/src/modules/SafeEmailRecoveryModule.sol index 321bbea..acf579f 100644 --- a/src/modules/SafeEmailRecoveryModule.sol +++ b/src/modules/SafeEmailRecoveryModule.sol @@ -61,6 +61,7 @@ contract SafeEmailRecoveryModule is EmailRecoveryManager { } bytes4 calldataSelector; + // solhint-disable-next-line no-inline-assembly assembly { calldataSelector := mload(add(recoveryCalldata, 32)) } diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index c597e42..da7e7a7 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -5,7 +5,7 @@ import { ERC7579ExecutorBase } from "@rhinestone/modulekit/src/Modules.sol"; import { IERC7579Account } from "erc7579/interfaces/IERC7579Account.sol"; import { IModule } from "erc7579/interfaces/IERC7579Module.sol"; import { ISafe } from "../interfaces/ISafe.sol"; -import { SentinelListLib, SENTINEL, ZERO_ADDRESS } from "sentinellist/SentinelList.sol"; +import { SentinelListLib, SENTINEL } from "sentinellist/SentinelList.sol"; import { IUniversalEmailRecoveryModule } from "../interfaces/IUniversalEmailRecoveryModule.sol"; import { EmailRecoveryManager } from "../EmailRecoveryManager.sol"; @@ -278,6 +278,7 @@ contract UniversalEmailRecoveryModule is } bytes4 selector; + // solhint-disable-next-line no-inline-assembly assembly { selector := mload(add(recoveryCalldata, 32)) } diff --git a/test/unit/modules/EmailRecoveryModule/recover.t.sol b/test/unit/modules/EmailRecoveryModule/recover.t.sol index 694ec5b..559020f 100644 --- a/test/unit/modules/EmailRecoveryModule/recover.t.sol +++ b/test/unit/modules/EmailRecoveryModule/recover.t.sol @@ -2,9 +2,6 @@ pragma solidity ^0.8.25; import { console2 } from "forge-std/console2.sol"; -import { ExecutionHelper } from "safe7579/core/ExecutionHelper.sol"; -import { Kernel } from "kernel/Kernel.sol"; -import { ModuleManager } from "erc7579/core/ModuleManager.sol"; import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; import { EmailRecoveryModule } from "src/modules/EmailRecoveryModule.sol"; import { EmailRecoveryModuleBase } from "./EmailRecoveryModuleBase.t.sol"; @@ -64,62 +61,12 @@ contract EmailRecoveryModule_recover_Test is EmailRecoveryModuleBase { emailRecoveryModule.exposed_recover(accountAddress, invalidData); } - function test_Recover_RevertWhen_ZeroValidatorAddress() public { - address zeroValidator = address(0); + function test_Recover_DoesNotRevertWhen_ZeroAddress() public { bytes memory validCalldata = abi.encodeWithSelector(functionSelector, newOwner); - bytes memory invalidData = abi.encode(zeroValidator, validCalldata); + bytes memory dataWithZeroAddress = abi.encode(address(0), validCalldata); vm.startPrank(recoveryModuleAddress); - vm.expectRevert( - abi.encodeWithSelector(EmailRecoveryModule.InvalidValidator.selector, zeroValidator) - ); - emailRecoveryModule.exposed_recover(accountAddress, invalidData); - } - - function test_Recover_RevertWhen_ValidatorAddressIsAccountAddress() public { - address wrongValidator = accountAddress; - bytes memory validCalldata = abi.encodeWithSelector(functionSelector, newOwner); - bytes memory invalidData = abi.encode(wrongValidator, validCalldata); - - vm.startPrank(recoveryModuleAddress); - - // The error thrown is different depending on what the account type is. - // If it is a Safe, the error should be ExecutionHelper.ExecutionFailed - // If it is a Kernel, the error should be Kernel.InvalidSelector - // If it an MSA account, the error should be ModuleManager.NoFallbackManager - - string memory currentAccountType = vm.envOr("ACCOUNT_TYPE", string("")); - if (Strings.equal(currentAccountType, "SAFE")) { - vm.expectRevert(ExecutionHelper.ExecutionFailed.selector); - } else if (Strings.equal(currentAccountType, "KERNEL")) { - vm.expectRevert(Kernel.InvalidSelector.selector); - } else { - vm.expectRevert( - abi.encodeWithSelector(ModuleManager.NoFallbackHandler.selector, functionSelector) - ); - } - emailRecoveryModule.exposed_recover(accountAddress, invalidData); - } - - function test_Recover_RevertWhen_IncorrectValidatorAddress() public { - address wrongValidator = address(5); - bytes memory validCalldata = abi.encodeWithSelector(functionSelector, newOwner); - bytes memory invalidData = abi.encode(wrongValidator, validCalldata); - - vm.startPrank(recoveryModuleAddress); - - // The error thrown is different depending on what the account type is. - // If it is a Safe, the error should be ExecutionHelper.ExecutionFailed - // If it is a Kernel or an MSA account, the error should be a low level revert - - string memory currentAccountType = vm.envOr("ACCOUNT_TYPE", string("")); - if (Strings.equal(currentAccountType, "SAFE")) { - vm.expectRevert(ExecutionHelper.ExecutionFailed.selector); - } else { - vm.expectRevert(); - } - - emailRecoveryModule.exposed_recover(accountAddress, invalidData); + emailRecoveryModule.exposed_recover(accountAddress, dataWithZeroAddress); } function test_Recover_Succeeds() public { From 2670be988e8252ab4356d4d85a64c9f0ffb11735 Mon Sep 17 00:00:00 2001 From: SoraSuegami Date: Thu, 5 Sep 2024 17:50:56 +0900 Subject: [PATCH 04/15] Add test_AllowValidatorRecovery_When_SafeAddOwnerSelector --- .../allowValidatorRecovery.t.sol | 45 +++---------------- 1 file changed, 7 insertions(+), 38 deletions(-) diff --git a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol index 6fa126e..453ce73 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol @@ -28,6 +28,13 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { emailRecoveryModule.allowValidatorRecovery(validatorAddress, bytes("0"), functionSelector); } + function test_AllowValidatorRecovery_When_SafeAddOwnerSelector() public { + vm.startPrank(accountAddress); + emailRecoveryModule.allowValidatorRecovery( + accountAddress, bytes("0"), ISafe.addOwnerWithThreshold.selector + ); + } + function test_AllowValidatorRecovery_RevertWhen_UnsafeOnInstallSelector() public { vm.expectRevert( abi.encodeWithSelector( @@ -52,44 +59,6 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { ); } - // function test_AllowValidatorRecovery_RevertWhen_UnsafeExecuteSelector() public { - // vm.expectRevert( - // abi.encodeWithSelector( - // UniversalEmailRecoveryModule.InvalidSelector.selector, - // IERC7579Account.execute.selector - // ) - // ); - // vm.startPrank(accountAddress); - // emailRecoveryModule.allowValidatorRecovery( - // validatorAddress, bytes("0"), IERC7579Account.execute.selector - // ); - // } - - // function test_AllowValidatorRecovery_RevertWhen_UnsafeSetFallbackHandlerSelector() public { - // vm.expectRevert( - // abi.encodeWithSelector( - // UniversalEmailRecoveryModule.InvalidSelector.selector, - // ISafe.setFallbackHandler.selector - // ) - // ); - // vm.startPrank(accountAddress); - // emailRecoveryModule.allowValidatorRecovery( - // validatorAddress, bytes("0"), ISafe.setFallbackHandler.selector - // ); - // } - - // function test_AllowValidatorRecovery_RevertWhen_UnsafeSetGuardSelector() public { - // vm.expectRevert( - // abi.encodeWithSelector( - // UniversalEmailRecoveryModule.InvalidSelector.selector, ISafe.setGuard.selector - // ) - // ); - // vm.startPrank(accountAddress); - // emailRecoveryModule.allowValidatorRecovery( - // validatorAddress, bytes("0"), ISafe.setGuard.selector - // ); - // } - function test_AllowValidatorRecovery_RevertWhen_InvalidSelector() public { vm.expectRevert( abi.encodeWithSelector(UniversalEmailRecoveryModule.InvalidSelector.selector, bytes4(0)) From eb668a579d10ecc4bb65bf9847ef548a685525ce Mon Sep 17 00:00:00 2001 From: SoraSuegami Date: Thu, 5 Sep 2024 22:44:08 +0900 Subject: [PATCH 05/15] Fix unit tests for selectors --- test/unit/UnitBase.t.sol | 2 +- .../EmailRecoveryModule/constructor.t.sol | 123 +++++++++++------- .../allowValidatorRecovery.t.sol | 50 ++++++- 3 files changed, 125 insertions(+), 50 deletions(-) diff --git a/test/unit/UnitBase.t.sol b/test/unit/UnitBase.t.sol index 5a9bd9e..778afcc 100644 --- a/test/unit/UnitBase.t.sol +++ b/test/unit/UnitBase.t.sol @@ -28,7 +28,7 @@ import { EmailRecoveryFactory } from "src/factories/EmailRecoveryFactory.sol"; import { EmailRecoveryUniversalFactory } from "src/factories/EmailRecoveryUniversalFactory.sol"; import { OwnableValidator } from "src/test/OwnableValidator.sol"; import { MockGroth16Verifier } from "src/test/MockGroth16Verifier.sol"; -import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; abstract contract UnitBase is RhinestoneModuleKit, Test { using ModuleKitHelpers for *; diff --git a/test/unit/modules/EmailRecoveryModule/constructor.t.sol b/test/unit/modules/EmailRecoveryModule/constructor.t.sol index 382130b..4d86236 100644 --- a/test/unit/modules/EmailRecoveryModule/constructor.t.sol +++ b/test/unit/modules/EmailRecoveryModule/constructor.t.sol @@ -7,6 +7,7 @@ import { IERC7579Account } from "erc7579/interfaces/IERC7579Account.sol"; import { ISafe } from "src/interfaces/ISafe.sol"; import { EmailRecoveryModuleBase } from "./EmailRecoveryModuleBase.t.sol"; import { EmailRecoveryModule } from "src/modules/EmailRecoveryModule.sol"; +import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; contract EmailRecoveryModule_constructor_Test is EmailRecoveryModuleBase { function setUp() public override { @@ -28,6 +29,71 @@ contract EmailRecoveryModule_constructor_Test is EmailRecoveryModuleBase { ); } + function test_Constructor_When_SafeAddOwnerSelector() public { + _skipIfNotSafeAccountType(); + new EmailRecoveryModule( + address(verifier), + address(dkimRegistry), + address(emailAuthImpl), + address(emailRecoveryHandler), + validatorAddress, + ISafe.addOwnerWithThreshold.selector + ); + } + + function test_Constructor_When_SafeRemoveOwnerSelector() public { + _skipIfNotSafeAccountType(); + new EmailRecoveryModule( + address(verifier), + address(dkimRegistry), + address(emailAuthImpl), + address(emailRecoveryHandler), + validatorAddress, + ISafe.removeOwner.selector + ); + } + + function test_Constructor_When_SafeSwapOwnerSelector() public { + _skipIfNotSafeAccountType(); + new EmailRecoveryModule( + address(verifier), + address(dkimRegistry), + address(emailAuthImpl), + address(emailRecoveryHandler), + validatorAddress, + ISafe.swapOwner.selector + ); + } + + function test_Constructor_When_SafeChangeThresholdSelector() public { + _skipIfNotSafeAccountType(); + new EmailRecoveryModule( + address(verifier), + address(dkimRegistry), + address(emailAuthImpl), + address(emailRecoveryHandler), + validatorAddress, + ISafe.changeThreshold.selector + ); + } + + function test_Constructor_RevertWhen_SafeNotAllowedSelector() public { + _skipIfNotSafeAccountType(); + vm.expectRevert( + abi.encodeWithSelector( + EmailRecoveryModule.InvalidSelector.selector, IModule.onInstall.selector + ) + ); + new EmailRecoveryModule( + address(verifier), + address(dkimRegistry), + address(emailAuthImpl), + address(emailRecoveryHandler), + validatorAddress, + IModule.onInstall.selector + ); + } + function test_Constructor_RevertWhen_UnsafeOnInstallSelector() public { vm.expectRevert( abi.encodeWithSelector( @@ -60,54 +126,6 @@ contract EmailRecoveryModule_constructor_Test is EmailRecoveryModuleBase { ); } - // function test_Constructor_RevertWhen_UnsafeExecuteSelector() public { - // vm.expectRevert( - // abi.encodeWithSelector( - // EmailRecoveryModule.InvalidSelector.selector, IERC7579Account.execute.selector - // ) - // ); - // new EmailRecoveryModule( - // address(verifier), - // address(dkimRegistry), - // address(emailAuthImpl), - // address(emailRecoveryHandler), - // validatorAddress, - // IERC7579Account.execute.selector - // ); - // } - - // function test_Constructor_RevertWhen_UnsafeSetFallbackHandlerSelector() public { - // vm.expectRevert( - // abi.encodeWithSelector( - // EmailRecoveryModule.InvalidSelector.selector, ISafe.setFallbackHandler.selector - // ) - // ); - // new EmailRecoveryModule( - // address(verifier), - // address(dkimRegistry), - // address(emailAuthImpl), - // address(emailRecoveryHandler), - // validatorAddress, - // ISafe.setFallbackHandler.selector - // ); - // } - - // function test_Constructor_RevertWhen_UnsafeSetGuardSelector() public { - // vm.expectRevert( - // abi.encodeWithSelector( - // EmailRecoveryModule.InvalidSelector.selector, ISafe.setGuard.selector - // ) - // ); - // new EmailRecoveryModule( - // address(verifier), - // address(dkimRegistry), - // address(emailAuthImpl), - // address(emailRecoveryHandler), - // validatorAddress, - // ISafe.setGuard.selector - // ); - // } - function test_Constructor_RevertWhen_InvalidSelector() public { vm.expectRevert( abi.encodeWithSelector(EmailRecoveryModule.InvalidSelector.selector, bytes4(0)) @@ -135,4 +153,13 @@ contract EmailRecoveryModule_constructor_Test is EmailRecoveryModuleBase { assertEq(validatorAddress, emailRecoveryModule.validator()); assertEq(functionSelector, emailRecoveryModule.selector()); } + + function _skipIfNotSafeAccountType() private { + string memory currentAccountType = vm.envOr("ACCOUNT_TYPE", string("")); + if (Strings.equal(currentAccountType, "SAFE")) { + vm.skip(false); + } else { + vm.skip(true); + } + } } diff --git a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol index 453ce73..428468b 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol @@ -11,6 +11,7 @@ import { SentinelListLib } from "sentinellist/SentinelList.sol"; import { OwnableValidator } from "src/test/OwnableValidator.sol"; import { UniversalEmailRecoveryModule } from "src/modules/UniversalEmailRecoveryModule.sol"; import { UnitBase } from "../../UnitBase.t.sol"; +import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { using ModuleKitHelpers for *; @@ -29,9 +30,47 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { } function test_AllowValidatorRecovery_When_SafeAddOwnerSelector() public { + _skipIfNotSafeAccountType(); vm.startPrank(accountAddress); emailRecoveryModule.allowValidatorRecovery( - accountAddress, bytes("0"), ISafe.addOwnerWithThreshold.selector + validatorAddress, bytes("0"), ISafe.addOwnerWithThreshold.selector + ); + } + + function test_AllowValidatorRecovery_When_SafeRemoveOwnerSelector() public { + _skipIfNotSafeAccountType(); + vm.startPrank(accountAddress); + emailRecoveryModule.allowValidatorRecovery( + validatorAddress, bytes("0"), ISafe.removeOwner.selector + ); + } + + function test_AllowValidatorRecovery_When_SafeSwapOwnerSelector() public { + _skipIfNotSafeAccountType(); + vm.startPrank(accountAddress); + emailRecoveryModule.allowValidatorRecovery( + validatorAddress, bytes("0"), ISafe.swapOwner.selector + ); + } + + function test_AllowValidatorRecovery_When_SafeChangeThresholdSelector() public { + _skipIfNotSafeAccountType(); + vm.startPrank(accountAddress); + emailRecoveryModule.allowValidatorRecovery( + validatorAddress, bytes("0"), ISafe.changeThreshold.selector + ); + } + + function test_AllowValidatorRecovery_RevertWhen_SafeNotAllowedSelector() public { + _skipIfNotSafeAccountType(); + vm.expectRevert( + abi.encodeWithSelector( + UniversalEmailRecoveryModule.InvalidSelector.selector, IModule.onInstall.selector + ) + ); + vm.startPrank(accountAddress); + emailRecoveryModule.allowValidatorRecovery( + validatorAddress, bytes("0"), IModule.onInstall.selector ); } @@ -179,4 +218,13 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { assertEq(allowedSelectors.length, 1); assertEq(allowedSelectors[0], functionSelector); } + + function _skipIfNotSafeAccountType() private { + string memory currentAccountType = vm.envOr("ACCOUNT_TYPE", string("")); + if (Strings.equal(currentAccountType, "SAFE")) { + vm.skip(false); + } else { + vm.skip(true); + } + } } From b6acd3751af237ce59c11331249f32658e615c48 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Fri, 6 Sep 2024 11:37:13 +0100 Subject: [PATCH 06/15] forge fmt & typo fix --- script/DeployEmailRecoveryModule.s.sol | 17 +++++------------ script/DeploySafeNativeRecovery.s.sol | 17 +++++------------ script/DeploySafeRecovery.s.sol | 17 +++++------------ script/DeployUniversalEmailRecoveryModule.s.sol | 17 +++++------------ src/EmailRecoveryManager.sol | 8 +++++--- src/modules/EmailRecoveryModule.sol | 13 ++++++------- src/modules/SafeEmailRecoveryModule.sol | 2 +- src/modules/UniversalEmailRecoveryModule.sol | 10 +++++----- test/integration/IntegrationBase.t.sol | 5 ++--- .../EmailRecoveryModuleBase.t.sol | 2 +- 10 files changed, 40 insertions(+), 68 deletions(-) diff --git a/script/DeployEmailRecoveryModule.s.sol b/script/DeployEmailRecoveryModule.s.sol index b4b2656..8478225 100644 --- a/script/DeployEmailRecoveryModule.s.sol +++ b/script/DeployEmailRecoveryModule.s.sol @@ -10,7 +10,7 @@ import { ECDSAOwnedDKIMRegistry } from import { EmailAuth } from "ether-email-auth/packages/contracts/src/EmailAuth.sol"; import { EmailRecoveryFactory } from "src/factories/EmailRecoveryFactory.sol"; import { OwnableValidator } from "src/test/OwnableValidator.sol"; -import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; contract DeployEmailRecoveryModuleScript is Script { function run() public { @@ -24,14 +24,10 @@ contract DeployEmailRecoveryModuleScript is Script { address initialOwner = vm.addr(vm.envUint("PRIVATE_KEY")); if (verifier == address(0)) { - Verifier verifierImpl = new Verifier(); - console.log( - "Verifier implementation deployed at: %s", - address(verifierImpl) - ); + Verifier verifierImpl = new Verifier(); + console.log("Verifier implementation deployed at: %s", address(verifierImpl)); ERC1967Proxy verifierProxy = new ERC1967Proxy( - address(verifierImpl), - abi.encodeCall(verifierImpl.initialize, (initialOwner)) + address(verifierImpl), abi.encodeCall(verifierImpl.initialize, (initialOwner)) ); verifier = address(Verifier(address(verifierProxy))); vm.setEnv("VERIFIER", vm.toString(address(verifier))); @@ -42,10 +38,7 @@ contract DeployEmailRecoveryModuleScript is Script { require(dkimRegistrySigner != address(0), "DKIM_REGISTRY_SIGNER is required"); ECDSAOwnedDKIMRegistry dkimImpl = new ECDSAOwnedDKIMRegistry(); - console.log( - "ECDSAOwnedDKIMRegistry implementation deployed at: %s", - address(dkimImpl) - ); + console.log("ECDSAOwnedDKIMRegistry implementation deployed at: %s", address(dkimImpl)); ERC1967Proxy dkimProxy = new ERC1967Proxy( address(dkimImpl), abi.encodeCall(dkimImpl.initialize, (initialOwner, dkimRegistrySigner)) diff --git a/script/DeploySafeNativeRecovery.s.sol b/script/DeploySafeNativeRecovery.s.sol index cbf0c96..6af0e46 100644 --- a/script/DeploySafeNativeRecovery.s.sol +++ b/script/DeploySafeNativeRecovery.s.sol @@ -9,7 +9,7 @@ import { ECDSAOwnedDKIMRegistry } from import { EmailAuth } from "ether-email-auth/packages/contracts/src/EmailAuth.sol"; import { SafeRecoverySubjectHandler } from "src/handlers/SafeRecoverySubjectHandler.sol"; import { SafeEmailRecoveryModule } from "src/modules/SafeEmailRecoveryModule.sol"; -import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; contract DeploySafeNativeRecovery_Script is Script { function run() public { @@ -23,14 +23,10 @@ contract DeploySafeNativeRecovery_Script is Script { address initialOwner = vm.addr(vm.envUint("PRIVATE_KEY")); if (verifier == address(0)) { - Verifier verifierImpl = new Verifier(); - console.log( - "Verifier implementation deployed at: %s", - address(verifierImpl) - ); + Verifier verifierImpl = new Verifier(); + console.log("Verifier implementation deployed at: %s", address(verifierImpl)); ERC1967Proxy verifierProxy = new ERC1967Proxy( - address(verifierImpl), - abi.encodeCall(verifierImpl.initialize, (initialOwner)) + address(verifierImpl), abi.encodeCall(verifierImpl.initialize, (initialOwner)) ); verifier = address(Verifier(address(verifierProxy))); vm.setEnv("VERIFIER", vm.toString(address(verifier))); @@ -41,10 +37,7 @@ contract DeploySafeNativeRecovery_Script is Script { require(dkimRegistrySigner != address(0), "DKIM_REGISTRY_SIGNER is required"); ECDSAOwnedDKIMRegistry dkimImpl = new ECDSAOwnedDKIMRegistry(); - console.log( - "ECDSAOwnedDKIMRegistry implementation deployed at: %s", - address(dkimImpl) - ); + console.log("ECDSAOwnedDKIMRegistry implementation deployed at: %s", address(dkimImpl)); ERC1967Proxy dkimProxy = new ERC1967Proxy( address(dkimImpl), abi.encodeCall(dkimImpl.initialize, (initialOwner, dkimRegistrySigner)) diff --git a/script/DeploySafeRecovery.s.sol b/script/DeploySafeRecovery.s.sol index ff257d2..39414b7 100644 --- a/script/DeploySafeRecovery.s.sol +++ b/script/DeploySafeRecovery.s.sol @@ -14,7 +14,7 @@ import { EmailAuth } from "ether-email-auth/packages/contracts/src/EmailAuth.sol import { Safe7579 } from "safe7579/Safe7579.sol"; import { Safe7579Launchpad } from "safe7579/Safe7579Launchpad.sol"; import { IERC7484 } from "safe7579/interfaces/IERC7484.sol"; -import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; // 1. `source .env` // 2. `forge script --chain sepolia script/DeploySafeRecovery.s.sol:DeploySafeRecovery_Script @@ -33,14 +33,10 @@ contract DeploySafeRecovery_Script is Script { address initialOwner = vm.addr(vm.envUint("PRIVATE_KEY")); if (verifier == address(0)) { - Verifier verifierImpl = new Verifier(); - console.log( - "Verifier implementation deployed at: %s", - address(verifierImpl) - ); + Verifier verifierImpl = new Verifier(); + console.log("Verifier implementation deployed at: %s", address(verifierImpl)); ERC1967Proxy verifierProxy = new ERC1967Proxy( - address(verifierImpl), - abi.encodeCall(verifierImpl.initialize, (initialOwner)) + address(verifierImpl), abi.encodeCall(verifierImpl.initialize, (initialOwner)) ); verifier = address(Verifier(address(verifierProxy))); vm.setEnv("VERIFIER", vm.toString(address(verifier))); @@ -51,10 +47,7 @@ contract DeploySafeRecovery_Script is Script { require(dkimRegistrySigner != address(0), "DKIM_REGISTRY_SIGNER is required"); ECDSAOwnedDKIMRegistry dkimImpl = new ECDSAOwnedDKIMRegistry(); - console.log( - "ECDSAOwnedDKIMRegistry implementation deployed at: %s", - address(dkimImpl) - ); + console.log("ECDSAOwnedDKIMRegistry implementation deployed at: %s", address(dkimImpl)); ERC1967Proxy dkimProxy = new ERC1967Proxy( address(dkimImpl), abi.encodeCall(dkimImpl.initialize, (initialOwner, dkimRegistrySigner)) diff --git a/script/DeployUniversalEmailRecoveryModule.s.sol b/script/DeployUniversalEmailRecoveryModule.s.sol index 147cfc9..e5d620d 100644 --- a/script/DeployUniversalEmailRecoveryModule.s.sol +++ b/script/DeployUniversalEmailRecoveryModule.s.sol @@ -9,7 +9,7 @@ import { ECDSAOwnedDKIMRegistry } from "ether-email-auth/packages/contracts/src/utils/ECDSAOwnedDKIMRegistry.sol"; import { EmailAuth } from "ether-email-auth/packages/contracts/src/EmailAuth.sol"; import { EmailRecoveryUniversalFactory } from "src/factories/EmailRecoveryUniversalFactory.sol"; -import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; contract DeployUniversalEmailRecoveryModuleScript is Script { function run() public { @@ -22,14 +22,10 @@ contract DeployUniversalEmailRecoveryModuleScript is Script { address initialOwner = vm.addr(vm.envUint("PRIVATE_KEY")); if (verifier == address(0)) { - Verifier verifierImpl = new Verifier(); - console.log( - "Verifier implementation deployed at: %s", - address(verifierImpl) - ); + Verifier verifierImpl = new Verifier(); + console.log("Verifier implementation deployed at: %s", address(verifierImpl)); ERC1967Proxy verifierProxy = new ERC1967Proxy( - address(verifierImpl), - abi.encodeCall(verifierImpl.initialize, (initialOwner)) + address(verifierImpl), abi.encodeCall(verifierImpl.initialize, (initialOwner)) ); verifier = address(Verifier(address(verifierProxy))); vm.setEnv("VERIFIER", vm.toString(address(verifier))); @@ -40,10 +36,7 @@ contract DeployUniversalEmailRecoveryModuleScript is Script { require(dkimRegistrySigner != address(0), "DKIM_REGISTRY_SIGNER is required"); ECDSAOwnedDKIMRegistry dkimImpl = new ECDSAOwnedDKIMRegistry(); - console.log( - "ECDSAOwnedDKIMRegistry implementation deployed at: %s", - address(dkimImpl) - ); + console.log("ECDSAOwnedDKIMRegistry implementation deployed at: %s", address(dkimImpl)); ERC1967Proxy dkimProxy = new ERC1967Proxy( address(dkimImpl), abi.encodeCall(dkimImpl.initialize, (initialOwner, dkimRegistrySigner)) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index c8d54ae..5cbdb60 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -226,7 +226,9 @@ abstract contract EmailRecoveryManager is * that no recovery is in process. * @param recoveryConfig The new recovery configuration to be set for the caller's account */ - function updateRecoveryConfig(RecoveryConfig memory recoveryConfig) + function updateRecoveryConfig( + RecoveryConfig memory recoveryConfig + ) public onlyWhenNotRecovering { @@ -443,7 +445,7 @@ abstract contract EmailRecoveryManager is /** * @notice Removes all state related to msg.sender. * @dev In order to prevent unexpected behaviour when reinstalling account modules, the module - * should be deinitialized. This should include remove state accociated with an account. + * should be deinitialized. This should include removing state accociated with an account. */ function deInitRecoveryModule() internal onlyWhenNotRecovering { address account = msg.sender; @@ -453,7 +455,7 @@ abstract contract EmailRecoveryManager is /** * @notice Removes all state related to an account. * @dev In order to prevent unexpected behaviour when reinstalling account modules, the module - * should be deinitialized. This should include remove state accociated with an account. + * should be deinitialized. This should include removing state accociated with an account. */ function deInitRecoveryModule(address account) internal onlyWhenNotRecovering { delete recoveryConfigs[account]; diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index 7dde261..9dbcbec 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -52,18 +52,18 @@ contract EmailRecoveryModule is EmailRecoveryManager, ERC7579ExecutorBase, IEmai if (_validator == address(0)) { revert InvalidValidator(_validator); } - if(_validator == msg.sender) { + if (_validator == msg.sender) { if ( - _selector != ISafe.addOwnerWithThreshold.selector && _selector != ISafe.removeOwner.selector - && _selector != ISafe.swapOwner.selector - && _selector != ISafe.changeThreshold.selector + _selector != ISafe.addOwnerWithThreshold.selector + && _selector != ISafe.removeOwner.selector && _selector != ISafe.swapOwner.selector + && _selector != ISafe.changeThreshold.selector ) { revert InvalidSelector(_selector); } } else { if ( _selector == IModule.onInstall.selector || _selector == IModule.onUninstall.selector - || _selector == bytes4(0) + || _selector == bytes4(0) ) { revert InvalidSelector(_selector); } @@ -147,8 +147,7 @@ contract EmailRecoveryModule is EmailRecoveryManager, ERC7579ExecutorBase, IEmai * being recovered. recoveryData = abi.encode(validator, recoveryFunctionCalldata) */ function recover(address account, bytes calldata recoveryData) internal override { - (, bytes memory recoveryCalldata) = - abi.decode(recoveryData, (address, bytes)); + (, bytes memory recoveryCalldata) = abi.decode(recoveryData, (address, bytes)); bytes4 calldataSelector; // solhint-disable-next-line no-inline-assembly diff --git a/src/modules/SafeEmailRecoveryModule.sol b/src/modules/SafeEmailRecoveryModule.sol index acf579f..c1f4b98 100644 --- a/src/modules/SafeEmailRecoveryModule.sol +++ b/src/modules/SafeEmailRecoveryModule.sol @@ -87,7 +87,7 @@ contract SafeEmailRecoveryModule is EmailRecoveryManager { * @param account The account to reset the states for */ function resetWhenDisabled(address account) external { - if (account == address(0) ) { + if (account == address(0)) { revert InvalidAccount(account); } if (ISafe(account).isModuleEnabled(address(this)) == true) { diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index da7e7a7..b13fa58 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -69,18 +69,18 @@ contract UniversalEmailRecoveryModule is * "onInstall" or "onUninstall" */ modifier withoutUnsafeSelector(address validator, bytes4 selector) { - if(validator == msg.sender) { + if (validator == msg.sender) { if ( - selector != ISafe.addOwnerWithThreshold.selector && selector != ISafe.removeOwner.selector - && selector != ISafe.swapOwner.selector - && selector != ISafe.changeThreshold.selector + selector != ISafe.addOwnerWithThreshold.selector + && selector != ISafe.removeOwner.selector && selector != ISafe.swapOwner.selector + && selector != ISafe.changeThreshold.selector ) { revert InvalidSelector(selector); } } else { if ( selector == IModule.onInstall.selector || selector == IModule.onUninstall.selector - || selector == bytes4(0) + || selector == bytes4(0) ) { revert InvalidSelector(selector); } diff --git a/test/integration/IntegrationBase.t.sol b/test/integration/IntegrationBase.t.sol index 5804938..9858c9d 100644 --- a/test/integration/IntegrationBase.t.sol +++ b/test/integration/IntegrationBase.t.sol @@ -11,8 +11,7 @@ import { EmailAuth } from "ether-email-auth/packages/contracts/src/EmailAuth.sol import { ECDSA } from "solady/utils/ECDSA.sol"; import { MockGroth16Verifier } from "src/test/MockGroth16Verifier.sol"; -import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; - +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; abstract contract IntegrationBase is RhinestoneModuleKit, Test { // ZK Email contracts and variables @@ -77,7 +76,7 @@ abstract contract IntegrationBase is RhinestoneModuleKit, Test { bytes memory signature = abi.encodePacked(r, s, v); dkimRegistry.setDKIMPublicKeyHash(selector, domainName, publicKeyHash, signature); - verifier = new MockGroth16Verifier(); + verifier = new MockGroth16Verifier(); emailAuthImpl = new EmailAuth(); vm.stopPrank(); diff --git a/test/unit/modules/EmailRecoveryModule/EmailRecoveryModuleBase.t.sol b/test/unit/modules/EmailRecoveryModule/EmailRecoveryModuleBase.t.sol index c7af4da..262daa6 100644 --- a/test/unit/modules/EmailRecoveryModule/EmailRecoveryModuleBase.t.sol +++ b/test/unit/modules/EmailRecoveryModule/EmailRecoveryModuleBase.t.sol @@ -26,7 +26,7 @@ import { EmailRecoveryModuleHarness } from "../../EmailRecoveryModuleHarness.sol import { EmailRecoveryFactory } from "src/factories/EmailRecoveryFactory.sol"; import { OwnableValidator } from "src/test/OwnableValidator.sol"; import { MockGroth16Verifier } from "src/test/MockGroth16Verifier.sol"; -import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; abstract contract EmailRecoveryModuleBase is RhinestoneModuleKit, Test { using ModuleKitHelpers for *; From 4b387da8b8794d338e6b9474b578f0ad929dc8e7 Mon Sep 17 00:00:00 2001 From: wshino Date: Sun, 8 Sep 2024 22:04:45 +0900 Subject: [PATCH 07/15] Add deInitRecoveryModuleWithAddress.t.sol. --- .../deInitRecoveryModuleWithAddress.t.sol | 76 +++++++++++++++++++ .../UniversalEmailRecoveryModuleHarness.sol | 4 + 2 files changed, 80 insertions(+) create mode 100644 test/unit/EmailRecoveryManager/deInitRecoveryModuleWithAddress.t.sol diff --git a/test/unit/EmailRecoveryManager/deInitRecoveryModuleWithAddress.t.sol b/test/unit/EmailRecoveryManager/deInitRecoveryModuleWithAddress.t.sol new file mode 100644 index 0000000..203c5c5 --- /dev/null +++ b/test/unit/EmailRecoveryManager/deInitRecoveryModuleWithAddress.t.sol @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.25; + +import { console2 } from "forge-std/console2.sol"; +import { UnitBase } from "../UnitBase.t.sol"; +import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol"; +import { GuardianManager } from "src/GuardianManager.sol"; +import { IGuardianManager } from "src/interfaces/IGuardianManager.sol"; +import { GuardianStorage, GuardianStatus } from "src/libraries/EnumerableGuardianMap.sol"; + +contract EmailRecoveryManager_deInitRecoveryModuleWithAddress_Test is UnitBase { + function setUp() public override { + super.setUp(); + } + + function test_DeInitRecoveryModuleWithAddress_RevertWhen_RecoveryInProcess() public { + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + vm.warp(12 seconds); + handleRecovery(recoveryDataHash, accountSalt1); + + vm.prank(accountAddress); + vm.expectRevert(IGuardianManager.RecoveryInProcess.selector); + emailRecoveryModule.exposed_deInitRecoveryModule(accountAddress); + } + + function test_DeInitRecoveryModuleWithAddress_Succeeds() public { + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + + bool isActivated = emailRecoveryModule.isActivated(accountAddress); + assertTrue(isActivated); + + vm.prank(accountAddress); + vm.expectEmit(); + emit IEmailRecoveryManager.RecoveryDeInitialized(accountAddress); + emailRecoveryModule.exposed_deInitRecoveryModule(accountAddress); + + // assert that recovery config has been cleared successfully + IEmailRecoveryManager.RecoveryConfig memory recoveryConfig = + emailRecoveryModule.getRecoveryConfig(accountAddress); + assertEq(recoveryConfig.delay, 0); + assertEq(recoveryConfig.expiry, 0); + + // assert that the recovery request has been cleared successfully + IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = + emailRecoveryModule.getRecoveryRequest(accountAddress); + assertEq(recoveryRequest.executeAfter, 0); + assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.currentWeight, 0); + assertEq(recoveryRequest.recoveryDataHash, ""); + + // assert that guardian storage has been cleared successfully for guardian 1 + GuardianStorage memory guardianStorage1 = + emailRecoveryModule.getGuardian(accountAddress, guardian1); + assertEq(uint256(guardianStorage1.status), uint256(GuardianStatus.NONE)); + assertEq(guardianStorage1.weight, uint256(0)); + + // assert that guardian storage has been cleared successfully for guardian 2 + GuardianStorage memory guardianStorage2 = + emailRecoveryModule.getGuardian(accountAddress, guardian2); + assertEq(uint256(guardianStorage2.status), uint256(GuardianStatus.NONE)); + assertEq(guardianStorage2.weight, uint256(0)); + + // assert that guardian config has been cleared successfully + GuardianManager.GuardianConfig memory guardianConfig = + emailRecoveryModule.getGuardianConfig(accountAddress); + assertEq(guardianConfig.guardianCount, 0); + assertEq(guardianConfig.totalWeight, 0); + assertEq(guardianConfig.acceptedWeight, 0); + assertEq(guardianConfig.threshold, 0); + + isActivated = emailRecoveryModule.isActivated(accountAddress); + assertFalse(isActivated); + } +} diff --git a/test/unit/UniversalEmailRecoveryModuleHarness.sol b/test/unit/UniversalEmailRecoveryModuleHarness.sol index 0bd4ea4..842f816 100644 --- a/test/unit/UniversalEmailRecoveryModuleHarness.sol +++ b/test/unit/UniversalEmailRecoveryModuleHarness.sol @@ -48,6 +48,10 @@ contract UniversalEmailRecoveryModuleHarness is UniversalEmailRecoveryModule { deInitRecoveryModule(); } + function exposed_deInitRecoveryModule(address account) external { + deInitRecoveryModule(account); + } + function exposed_setupGuardians( address account, address[] calldata guardians, From 9760ad436e88b1f11754b381edc0b17c3cf76c15 Mon Sep 17 00:00:00 2001 From: wshino Date: Sun, 8 Sep 2024 22:10:22 +0900 Subject: [PATCH 08/15] Remove test_Constructor_RevertWhen_SafeNotAllowedSelector, it's duplicated. --- .../EmailRecoveryModule/constructor.t.sol | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/test/unit/modules/EmailRecoveryModule/constructor.t.sol b/test/unit/modules/EmailRecoveryModule/constructor.t.sol index 4d86236..cd67448 100644 --- a/test/unit/modules/EmailRecoveryModule/constructor.t.sol +++ b/test/unit/modules/EmailRecoveryModule/constructor.t.sol @@ -77,23 +77,6 @@ contract EmailRecoveryModule_constructor_Test is EmailRecoveryModuleBase { ); } - function test_Constructor_RevertWhen_SafeNotAllowedSelector() public { - _skipIfNotSafeAccountType(); - vm.expectRevert( - abi.encodeWithSelector( - EmailRecoveryModule.InvalidSelector.selector, IModule.onInstall.selector - ) - ); - new EmailRecoveryModule( - address(verifier), - address(dkimRegistry), - address(emailAuthImpl), - address(emailRecoveryHandler), - validatorAddress, - IModule.onInstall.selector - ); - } - function test_Constructor_RevertWhen_UnsafeOnInstallSelector() public { vm.expectRevert( abi.encodeWithSelector( From 630baae28600dee442e1179ceb69637fc692a8b1 Mon Sep 17 00:00:00 2001 From: wshino Date: Sun, 8 Sep 2024 22:11:51 +0900 Subject: [PATCH 09/15] Remove test_AllowValidatorRecovery_RevertWhen_SafeNotAllowedSelector, it's duplicated. --- .../allowValidatorRecovery.t.sol | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol index 428468b..5a3e531 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol @@ -61,19 +61,6 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { ); } - function test_AllowValidatorRecovery_RevertWhen_SafeNotAllowedSelector() public { - _skipIfNotSafeAccountType(); - vm.expectRevert( - abi.encodeWithSelector( - UniversalEmailRecoveryModule.InvalidSelector.selector, IModule.onInstall.selector - ) - ); - vm.startPrank(accountAddress); - emailRecoveryModule.allowValidatorRecovery( - validatorAddress, bytes("0"), IModule.onInstall.selector - ); - } - function test_AllowValidatorRecovery_RevertWhen_UnsafeOnInstallSelector() public { vm.expectRevert( abi.encodeWithSelector( From 3d390501c1aafe89072c87751f6d82571e600490 Mon Sep 17 00:00:00 2001 From: wshino Date: Sun, 8 Sep 2024 23:02:29 +0900 Subject: [PATCH 10/15] Feat/fix test allow validator recovery (#44) * Fix test cases in allow validator recovery. * Add mockCall for isModuleInstalled function. --- .../allowValidatorRecovery.t.sol | 33 ++++++++++++++++--- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol index 5a3e531..ea15a00 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol @@ -16,8 +16,13 @@ import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { using ModuleKitHelpers for *; + address validator2Address; + function setUp() public override { super.setUp(); + // Deploy new validator, it's not installed via installModule function. + OwnableValidator validator2 = new OwnableValidator(); + validator2Address = address(validator2); } function test_AllowValidatorRecovery_RevertWhen_RecoveryModuleNotInitialized() public { @@ -32,32 +37,52 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { function test_AllowValidatorRecovery_When_SafeAddOwnerSelector() public { _skipIfNotSafeAccountType(); vm.startPrank(accountAddress); + vm.mockCall( + address(accountAddress), + abi.encodeWithSelector(IERC7579Account.isModuleInstalled.selector), + abi.encode(true) + ); emailRecoveryModule.allowValidatorRecovery( - validatorAddress, bytes("0"), ISafe.addOwnerWithThreshold.selector + validator2Address, bytes("0"), ISafe.addOwnerWithThreshold.selector ); } function test_AllowValidatorRecovery_When_SafeRemoveOwnerSelector() public { _skipIfNotSafeAccountType(); vm.startPrank(accountAddress); + vm.mockCall( + address(accountAddress), + abi.encodeWithSelector(IERC7579Account.isModuleInstalled.selector), + abi.encode(true) + ); emailRecoveryModule.allowValidatorRecovery( - validatorAddress, bytes("0"), ISafe.removeOwner.selector + validator2Address, bytes("0"), ISafe.removeOwner.selector ); } function test_AllowValidatorRecovery_When_SafeSwapOwnerSelector() public { _skipIfNotSafeAccountType(); vm.startPrank(accountAddress); + vm.mockCall( + address(accountAddress), + abi.encodeWithSelector(IERC7579Account.isModuleInstalled.selector), + abi.encode(true) + ); emailRecoveryModule.allowValidatorRecovery( - validatorAddress, bytes("0"), ISafe.swapOwner.selector + validator2Address, bytes("0"), ISafe.swapOwner.selector ); } function test_AllowValidatorRecovery_When_SafeChangeThresholdSelector() public { _skipIfNotSafeAccountType(); vm.startPrank(accountAddress); + vm.mockCall( + address(accountAddress), + abi.encodeWithSelector(IERC7579Account.isModuleInstalled.selector), + abi.encode(true) + ); emailRecoveryModule.allowValidatorRecovery( - validatorAddress, bytes("0"), ISafe.changeThreshold.selector + validator2Address, bytes("0"), ISafe.changeThreshold.selector ); } From cef236ad4dbd972502b36ac3c7502df7c46e998e Mon Sep 17 00:00:00 2001 From: SoraSuegami Date: Tue, 10 Sep 2024 02:15:34 +0900 Subject: [PATCH 11/15] Add cancelExpiredRecovery function --- src/EmailRecoveryManager.sol | 23 +++- src/interfaces/IEmailRecoveryManager.sol | 1 + .../EmailRecoveryManager.integration.t.sol | 28 ++++ .../cancelExpiredRecovery.t.sol | 123 ++++++++++++++++++ 4 files changed, 172 insertions(+), 3 deletions(-) create mode 100644 test/unit/EmailRecoveryManager/cancelExpiredRecovery.t.sol diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 5cbdb60..a62e511 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -226,9 +226,7 @@ abstract contract EmailRecoveryManager is * that no recovery is in process. * @param recoveryConfig The new recovery configuration to be set for the caller's account */ - function updateRecoveryConfig( - RecoveryConfig memory recoveryConfig - ) + function updateRecoveryConfig(RecoveryConfig memory recoveryConfig) public onlyWhenNotRecovering { @@ -442,6 +440,25 @@ abstract contract EmailRecoveryManager is emit RecoveryCancelled(msg.sender); } + /** + * @notice Cancels the recovery request for a given account if it is expired. + * @dev Deletes the current recovery request associated with the given account if the recovery + * request has expired. + * @param account The address of the account for which the recovery is being cancelled + */ + function cancelExpiredRecovery(address account) external { + if (recoveryRequests[account].currentWeight == 0) { + revert NoRecoveryInProcess(); + } + if (recoveryRequests[account].executeBefore > block.timestamp) { + revert NotCancelUnexpiredRequest( + account, block.timestamp, recoveryRequests[account].executeBefore + ); + } + delete recoveryRequests[account]; + emit RecoveryCancelled(account); + } + /** * @notice Removes all state related to msg.sender. * @dev In order to prevent unexpected behaviour when reinstalling account modules, the module diff --git a/src/interfaces/IEmailRecoveryManager.sol b/src/interfaces/IEmailRecoveryManager.sol index 9aef2c4..cb7a530 100644 --- a/src/interfaces/IEmailRecoveryManager.sol +++ b/src/interfaces/IEmailRecoveryManager.sol @@ -77,6 +77,7 @@ interface IEmailRecoveryManager { error RecoveryRequestExpired(uint256 blockTimestamp, uint256 executeBefore); error InvalidRecoveryDataHash(bytes32 recoveryDataHash, bytes32 expectedRecoveryDataHash); error NoRecoveryInProcess(); + error NotCancelUnexpiredRequest(address account, uint256 blockTimestamp, uint256 executeBefore); error RecoveryIsNotActivated(); /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ diff --git a/test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol b/test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol index 0f22da4..55dea46 100644 --- a/test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol +++ b/test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol @@ -253,4 +253,32 @@ contract EmailRecoveryManager_Integration_Test is assertEq(recoveryRequest.executeBefore, 0); assertEq(recoveryRequest.currentWeight, 0); } + + function test_CancelExpiredRecoveryRequest() public { + acceptGuardian(accountAddress1, guardians1[0]); + acceptGuardian(accountAddress1, guardians1[1]); + vm.warp(12 seconds); + handleRecovery(accountAddress1, guardians1[0], recoveryDataHash1); + handleRecovery(accountAddress1, guardians1[1], recoveryDataHash1); + uint256 executeAfter = block.timestamp + expiry; + + vm.warp(executeAfter); + vm.expectRevert( + abi.encodeWithSelector( + IEmailRecoveryManager.RecoveryRequestExpired.selector, block.timestamp, executeAfter + ) + ); + emailRecoveryModule.completeRecovery(accountAddress1, recoveryData1); + + // Can cancel recovery even when stale + vm.startPrank(vm.addr(1)); + emailRecoveryModule.cancelExpiredRecovery(accountAddress1); + vm.stopPrank(); + + IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = + emailRecoveryModule.getRecoveryRequest(accountAddress1); + assertEq(recoveryRequest.executeAfter, 0); + assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.currentWeight, 0); + } } diff --git a/test/unit/EmailRecoveryManager/cancelExpiredRecovery.t.sol b/test/unit/EmailRecoveryManager/cancelExpiredRecovery.t.sol new file mode 100644 index 0000000..879ae66 --- /dev/null +++ b/test/unit/EmailRecoveryManager/cancelExpiredRecovery.t.sol @@ -0,0 +1,123 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.25; + +import { console2 } from "forge-std/console2.sol"; +import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; +import { UnitBase } from "../UnitBase.t.sol"; +import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol"; + +contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { + using Strings for uint256; + + function setUp() public override { + super.setUp(); + } + + function test_CancelExpiredRecovery_RevertWhen_NoRecoveryInProcess() public { + vm.startPrank(accountAddress); + vm.expectRevert(IEmailRecoveryManager.NoRecoveryInProcess.selector); + emailRecoveryModule.cancelExpiredRecovery(accountAddress); + } + + function test_CancelExpiredRecovery_CannotCancelNotStartedRecoveryRequest() public { + address otherAddress = address(99); + + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + vm.warp(12 seconds); + + IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = + emailRecoveryModule.getRecoveryRequest(accountAddress); + assertEq(recoveryRequest.executeAfter, 0); + assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.currentWeight, 0); + assertEq(recoveryRequest.recoveryDataHash, bytes32(0)); + + vm.startPrank(otherAddress); + vm.expectRevert(IEmailRecoveryManager.NoRecoveryInProcess.selector); + emailRecoveryModule.cancelExpiredRecovery(accountAddress); + } + + function test_CancelExpiredRecovery_CannotCancelNotExpiredRequest() public { + address otherAddress = address(99); + + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + vm.warp(12 seconds); + handleRecovery(recoveryDataHash, accountSalt1); + handleRecovery(recoveryDataHash, accountSalt2); + + IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = + emailRecoveryModule.getRecoveryRequest(accountAddress); + assertEq(recoveryRequest.executeAfter, block.timestamp + delay); + assertEq(recoveryRequest.executeBefore, block.timestamp + expiry); + assertEq(recoveryRequest.currentWeight, 3); + assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); + + vm.startPrank(otherAddress); + vm.expectRevert( + abi.encodeWithSelector( + IEmailRecoveryManager.NotCancelUnexpiredRequest.selector, + accountAddress, + block.timestamp, + block.timestamp + expiry + ) + ); + emailRecoveryModule.cancelExpiredRecovery(accountAddress); + } + + function test_CancelExpiredRecovery_PartialRequest_Succeeds() public { + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + vm.warp(12 seconds + 1 seconds); + handleRecovery(recoveryDataHash, accountSalt1); + + IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = + emailRecoveryModule.getRecoveryRequest(accountAddress); + assertEq(recoveryRequest.executeAfter, 0); + assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.currentWeight, 1); + assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); + + vm.warp(block.timestamp + expiry); + address otherAddress = address(99); + vm.startPrank(otherAddress); + vm.expectEmit(); + emit IEmailRecoveryManager.RecoveryCancelled(accountAddress); + emailRecoveryModule.cancelExpiredRecovery(accountAddress); + + recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress); + assertEq(recoveryRequest.executeAfter, 0); + assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.currentWeight, 0); + assertEq(recoveryRequest.recoveryDataHash, ""); + } + + function test_CancelExpiredRecovery_FullRequest_Succeeds() public { + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + vm.warp(12 seconds + 1 seconds); + handleRecovery(recoveryDataHash, accountSalt1); + handleRecovery(recoveryDataHash, accountSalt2); + + IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = + emailRecoveryModule.getRecoveryRequest(accountAddress); + assertEq(recoveryRequest.executeAfter, block.timestamp + delay); + assertEq(recoveryRequest.executeBefore, block.timestamp + expiry); + assertEq(recoveryRequest.currentWeight, 3); + assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); + + vm.warp(block.timestamp + expiry); + address otherAddress = address(99); + vm.startPrank(otherAddress); + vm.expectEmit(); + emit IEmailRecoveryManager.RecoveryCancelled(accountAddress); + emailRecoveryModule.cancelExpiredRecovery(accountAddress); + + recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress); + assertEq(recoveryRequest.executeAfter, 0); + assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.currentWeight, 0); + assertEq(recoveryRequest.recoveryDataHash, ""); + } +} From 055ed35c2299977a251d7fc190b77eb0679ab0b7 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 10 Sep 2024 12:44:13 +0100 Subject: [PATCH 12/15] Set executeBefore on first guardian approval --- src/EmailRecoveryManager.sol | 14 ++-- src/interfaces/IEmailRecoveryManager.sol | 9 +-- .../EmailRecoveryManager.integration.t.sol | 33 +++++++++ .../EmailRecoveryModule.t.sol | 4 +- .../UniversalEmailRecoveryModule.t.sol | 4 +- .../SafeRecoveryNativeModule.t.sol | 25 ++++++- .../cancelExpiredRecovery.t.sol | 71 +++++++++++++++++-- .../EmailRecoveryManager/cancelRecovery.t.sol | 4 +- .../getRecoveryRequest.t.sol | 2 +- .../processRecovery.t.sol | 2 +- .../allowValidatorRecovery.t.sol | 2 +- 11 files changed, 145 insertions(+), 25 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index a62e511..b1387b5 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -226,7 +226,9 @@ abstract contract EmailRecoveryManager is * that no recovery is in process. * @param recoveryConfig The new recovery configuration to be set for the caller's account */ - function updateRecoveryConfig(RecoveryConfig memory recoveryConfig) + function updateRecoveryConfig( + RecoveryConfig memory recoveryConfig + ) public onlyWhenNotRecovering { @@ -350,6 +352,8 @@ abstract contract EmailRecoveryManager is if (recoveryRequest.recoveryDataHash == bytes32(0)) { recoveryRequest.recoveryDataHash = recoveryDataHash; + uint256 executeBefore = block.timestamp + recoveryConfigs[account].expiry; + recoveryRequest.executeBefore = executeBefore; } if (recoveryRequest.recoveryDataHash != recoveryDataHash) { @@ -360,11 +364,11 @@ abstract contract EmailRecoveryManager is if (recoveryRequest.currentWeight >= guardianConfig.threshold) { uint256 executeAfter = block.timestamp + recoveryConfigs[account].delay; - uint256 executeBefore = block.timestamp + recoveryConfigs[account].expiry; recoveryRequest.executeAfter = executeAfter; - recoveryRequest.executeBefore = executeBefore; - emit RecoveryProcessed(account, guardian, executeAfter, executeBefore, recoveryDataHash); + emit RecoveryProcessed( + account, guardian, executeAfter, recoveryRequest.executeBefore, recoveryDataHash + ); } } @@ -451,7 +455,7 @@ abstract contract EmailRecoveryManager is revert NoRecoveryInProcess(); } if (recoveryRequests[account].executeBefore > block.timestamp) { - revert NotCancelUnexpiredRequest( + revert RecoveryHasNotExpired( account, block.timestamp, recoveryRequests[account].executeBefore ); } diff --git a/src/interfaces/IEmailRecoveryManager.sol b/src/interfaces/IEmailRecoveryManager.sol index cb7a530..00c0bff 100644 --- a/src/interfaces/IEmailRecoveryManager.sol +++ b/src/interfaces/IEmailRecoveryManager.sol @@ -13,12 +13,13 @@ interface IEmailRecoveryManager { * Config should be maintained over subsequent recovery attempts unless explicitly modified */ struct RecoveryConfig { - uint256 delay; // the time from when recovery is started until the recovery request can be - // executed + uint256 delay; // the time from when the threshold for a recovery request has passed (when + // the attempt is successful), until the recovery request can be executed uint256 expiry; // the time from when recovery is started until the recovery request becomes // invalid. The recovery expiry encourages the timely execution of successful recovery // attempts, and reduces the risk of unauthorized access through stale or outdated - // requests. + // requests. After the recovery expiry has passed, anyone can cancel the recovery + // request } /** @@ -77,7 +78,7 @@ interface IEmailRecoveryManager { error RecoveryRequestExpired(uint256 blockTimestamp, uint256 executeBefore); error InvalidRecoveryDataHash(bytes32 recoveryDataHash, bytes32 expectedRecoveryDataHash); error NoRecoveryInProcess(); - error NotCancelUnexpiredRequest(address account, uint256 blockTimestamp, uint256 executeBefore); + error RecoveryHasNotExpired(address account, uint256 blockTimestamp, uint256 executeBefore); error RecoveryIsNotActivated(); /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ diff --git a/test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol b/test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol index 55dea46..31d02d3 100644 --- a/test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol +++ b/test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol @@ -252,6 +252,7 @@ contract EmailRecoveryManager_Integration_Test is assertEq(recoveryRequest.executeAfter, 0); assertEq(recoveryRequest.executeBefore, 0); assertEq(recoveryRequest.currentWeight, 0); + assertEq(recoveryRequest.recoveryDataHash, bytes32(0)); } function test_CancelExpiredRecoveryRequest() public { @@ -280,5 +281,37 @@ contract EmailRecoveryManager_Integration_Test is assertEq(recoveryRequest.executeAfter, 0); assertEq(recoveryRequest.executeBefore, 0); assertEq(recoveryRequest.currentWeight, 0); + assertEq(recoveryRequest.recoveryDataHash, bytes32(0)); + } + + function test_CannotComplete_CancelledExpiredRecoveryRequest() public { + acceptGuardian(accountAddress1, guardians1[0]); + acceptGuardian(accountAddress1, guardians1[1]); + vm.warp(12 seconds); + handleRecovery(accountAddress1, guardians1[0], recoveryDataHash1); + handleRecovery(accountAddress1, guardians1[1], recoveryDataHash1); + uint256 executeAfter = block.timestamp + expiry; + + vm.warp(executeAfter); + // Can cancel recovery even when stale + vm.startPrank(vm.addr(1)); + emailRecoveryModule.cancelExpiredRecovery(accountAddress1); + vm.stopPrank(); + + IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = + emailRecoveryModule.getRecoveryRequest(accountAddress1); + assertEq(recoveryRequest.executeAfter, 0); + assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.currentWeight, 0); + assertEq(recoveryRequest.recoveryDataHash, bytes32(0)); + + vm.expectRevert( + abi.encodeWithSelector( + IEmailRecoveryManager.NotEnoughApprovals.selector, + recoveryRequest.currentWeight, + threshold + ) + ); + emailRecoveryModule.completeRecovery(accountAddress1, recoveryData1); } } diff --git a/test/integration/OwnableValidatorRecovery/EmailRecoveryModule/EmailRecoveryModule.t.sol b/test/integration/OwnableValidatorRecovery/EmailRecoveryModule/EmailRecoveryModule.t.sol index c63804b..1a548c4 100644 --- a/test/integration/OwnableValidatorRecovery/EmailRecoveryModule/EmailRecoveryModule.t.sol +++ b/test/integration/OwnableValidatorRecovery/EmailRecoveryModule/EmailRecoveryModule.t.sol @@ -62,16 +62,16 @@ contract OwnableValidatorRecovery_EmailRecoveryModule_Integration_Test is vm.warp(12 seconds); // handle recovery request for guardian 1 handleRecovery(accountAddress1, guardians1[0], recoveryDataHash1); + uint256 executeBefore = block.timestamp + expiry; IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress1); assertEq(recoveryRequest.executeAfter, 0); - assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.executeBefore, executeBefore); assertEq(recoveryRequest.currentWeight, 1); assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash1); // handle recovery request for guardian 2 uint256 executeAfter = block.timestamp + delay; - uint256 executeBefore = block.timestamp + expiry; handleRecovery(accountAddress1, guardians1[1], recoveryDataHash1); recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress1); assertEq(recoveryRequest.executeAfter, executeAfter); diff --git a/test/integration/OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModule.t.sol b/test/integration/OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModule.t.sol index 6fa9462..5b620d6 100644 --- a/test/integration/OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModule.t.sol +++ b/test/integration/OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModule.t.sol @@ -60,17 +60,17 @@ contract OwnableValidatorRecovery_UniversalEmailRecoveryModule_Integration_Test // Time travel so that EmailAuth timestamp is valid vm.warp(12 seconds); // handle recovery request for guardian 1 + uint256 executeBefore = block.timestamp + expiry; handleRecovery(accountAddress1, guardians1[0], recoveryDataHash1); IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress1); assertEq(recoveryRequest.executeAfter, 0); - assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.executeBefore, executeBefore); assertEq(recoveryRequest.currentWeight, 1); assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash1); // handle recovery request for guardian 2 uint256 executeAfter = block.timestamp + delay; - uint256 executeBefore = block.timestamp + expiry; handleRecovery(accountAddress1, guardians1[1], recoveryDataHash1); recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress1); assertEq(recoveryRequest.executeAfter, executeAfter); diff --git a/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol b/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol index 974e35d..98d71d7 100644 --- a/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol +++ b/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol @@ -60,18 +60,18 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase vm.warp(12 seconds); // handle recovery request for guardian 1 + uint256 executeBefore = block.timestamp + expiry; emailAuthMsg = getRecoveryEmailAuthMessage(safeAddress, recoveryDataHash, guardians1[0]); emailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx); IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = emailRecoveryModule.getRecoveryRequest(safeAddress); assertEq(recoveryRequest.executeAfter, 0); - assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.executeBefore, executeBefore); assertEq(recoveryRequest.currentWeight, 1); assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); // handle recovery request for guardian 2 uint256 executeAfter = block.timestamp + delay; - uint256 executeBefore = block.timestamp + expiry; emailAuthMsg = getRecoveryEmailAuthMessage(safeAddress, recoveryDataHash, guardians1[1]); emailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx); recoveryRequest = emailRecoveryModule.getRecoveryRequest(safeAddress); @@ -98,4 +98,25 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase bool oldOwnerIsOwner = Safe(payable(safeAddress)).isOwner(owner); assertFalse(oldOwnerIsOwner); } + + function testIntegration_AccountRecovery_UninstallsModule() public { + testIntegration_AccountRecovery(); + + bool isModuleEnabled = + Safe(payable(safeAddress)).isModuleEnabled(emailRecoveryModuleAddress); + assertTrue(isModuleEnabled); + + // Uninstall module + // instance1.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); + vm.prank(safeAddress); + Safe(payable(safeAddress)).disableModule(address(1), emailRecoveryModuleAddress); + vm.stopPrank(); + + isModuleEnabled = Safe(payable(safeAddress)).isModuleEnabled(emailRecoveryModuleAddress); + assertFalse(isModuleEnabled); + + vm.prank(safeAddress); + emailRecoveryModule.resetWhenDisabled(safeAddress); + vm.stopPrank(); + } } diff --git a/test/unit/EmailRecoveryManager/cancelExpiredRecovery.t.sol b/test/unit/EmailRecoveryManager/cancelExpiredRecovery.t.sol index 879ae66..6a156fa 100644 --- a/test/unit/EmailRecoveryManager/cancelExpiredRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/cancelExpiredRecovery.t.sol @@ -6,7 +6,7 @@ import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; import { UnitBase } from "../UnitBase.t.sol"; import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol"; -contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { +contract EmailRecoveryManager_cancelExpiredRecovery_Test is UnitBase { using Strings for uint256; function setUp() public override { @@ -38,7 +38,33 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { emailRecoveryModule.cancelExpiredRecovery(accountAddress); } - function test_CancelExpiredRecovery_CannotCancelNotExpiredRequest() public { + function test_CancelExpiredRecovery_RevertWhen_PartialRequest_ExpiryNotPassed() public { + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + vm.warp(12 seconds + 1 seconds); + handleRecovery(recoveryDataHash, accountSalt1); + + IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = + emailRecoveryModule.getRecoveryRequest(accountAddress); + assertEq(recoveryRequest.executeAfter, 0); + assertEq(recoveryRequest.executeBefore, block.timestamp + expiry); + assertEq(recoveryRequest.currentWeight, 1); + assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); + + address otherAddress = address(99); + vm.startPrank(otherAddress); + vm.expectRevert( + abi.encodeWithSelector( + IEmailRecoveryManager.RecoveryHasNotExpired.selector, + accountAddress, + block.timestamp, + block.timestamp + expiry + ) + ); + emailRecoveryModule.cancelExpiredRecovery(accountAddress); + } + + function test_CancelExpiredRecovery_RevertWhen_FullRequest_ExpiryNotPassed() public { address otherAddress = address(99); acceptGuardian(accountSalt1); @@ -54,10 +80,11 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { assertEq(recoveryRequest.currentWeight, 3); assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); + // executeBefore > block.timestamp vm.startPrank(otherAddress); vm.expectRevert( abi.encodeWithSelector( - IEmailRecoveryManager.NotCancelUnexpiredRequest.selector, + IEmailRecoveryManager.RecoveryHasNotExpired.selector, accountAddress, block.timestamp, block.timestamp + expiry @@ -75,7 +102,7 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress); assertEq(recoveryRequest.executeAfter, 0); - assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.executeBefore, block.timestamp + expiry); assertEq(recoveryRequest.currentWeight, 1); assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); @@ -93,7 +120,9 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { assertEq(recoveryRequest.recoveryDataHash, ""); } - function test_CancelExpiredRecovery_FullRequest_Succeeds() public { + function test_CancelExpiredRecovery_FullRequest_SucceedsWhenExecuteBeforeEqualsTimestamp() + public + { acceptGuardian(accountSalt1); acceptGuardian(accountSalt2); vm.warp(12 seconds + 1 seconds); @@ -107,6 +136,7 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { assertEq(recoveryRequest.currentWeight, 3); assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); + // executeBefore == block.timestamp vm.warp(block.timestamp + expiry); address otherAddress = address(99); vm.startPrank(otherAddress); @@ -120,4 +150,35 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { assertEq(recoveryRequest.currentWeight, 0); assertEq(recoveryRequest.recoveryDataHash, ""); } + + function test_CancelExpiredRecovery_FullRequest_SucceedsWhenExecuteBeforeIsLessThanTimestamp() + public + { + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + vm.warp(12 seconds + 1 seconds); + handleRecovery(recoveryDataHash, accountSalt1); + handleRecovery(recoveryDataHash, accountSalt2); + + IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = + emailRecoveryModule.getRecoveryRequest(accountAddress); + assertEq(recoveryRequest.executeAfter, block.timestamp + delay); + assertEq(recoveryRequest.executeBefore, block.timestamp + expiry); + assertEq(recoveryRequest.currentWeight, 3); + assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); + + // executeBefore < block.timestamp + vm.warp(block.timestamp + expiry + 1 seconds); + address otherAddress = address(99); + vm.startPrank(otherAddress); + vm.expectEmit(); + emit IEmailRecoveryManager.RecoveryCancelled(accountAddress); + emailRecoveryModule.cancelExpiredRecovery(accountAddress); + + recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress); + assertEq(recoveryRequest.executeAfter, 0); + assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.currentWeight, 0); + assertEq(recoveryRequest.recoveryDataHash, ""); + } } diff --git a/test/unit/EmailRecoveryManager/cancelRecovery.t.sol b/test/unit/EmailRecoveryManager/cancelRecovery.t.sol index 4aaf6be..d6661e2 100644 --- a/test/unit/EmailRecoveryManager/cancelRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/cancelRecovery.t.sol @@ -30,7 +30,7 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress); assertEq(recoveryRequest.executeAfter, 0); - assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.executeBefore, block.timestamp + expiry); assertEq(recoveryRequest.currentWeight, 1); assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); @@ -48,7 +48,7 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress); assertEq(recoveryRequest.executeAfter, 0); - assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.executeBefore, block.timestamp + expiry); assertEq(recoveryRequest.currentWeight, 1); assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); diff --git a/test/unit/EmailRecoveryManager/getRecoveryRequest.t.sol b/test/unit/EmailRecoveryManager/getRecoveryRequest.t.sol index 69c3852..6344176 100644 --- a/test/unit/EmailRecoveryManager/getRecoveryRequest.t.sol +++ b/test/unit/EmailRecoveryManager/getRecoveryRequest.t.sol @@ -20,7 +20,7 @@ contract EmailRecoveryManager_getRecoveryRequest_Test is UnitBase { IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress); assertEq(recoveryRequest.executeAfter, 0); - assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.executeBefore, block.timestamp + expiry); assertEq(recoveryRequest.currentWeight, 1); assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); } diff --git a/test/unit/EmailRecoveryManager/processRecovery.t.sol b/test/unit/EmailRecoveryManager/processRecovery.t.sol index d934708..5989c5a 100644 --- a/test/unit/EmailRecoveryManager/processRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/processRecovery.t.sol @@ -151,7 +151,7 @@ contract EmailRecoveryManager_processRecovery_Test is UnitBase { IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress); assertEq(recoveryRequest.executeAfter, 0); - assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.executeBefore, block.timestamp + expiry); assertEq(recoveryRequest.currentWeight, guardian1Weight); assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); } diff --git a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol index ea15a00..6672b99 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol @@ -20,7 +20,7 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { function setUp() public override { super.setUp(); - // Deploy new validator, it's not installed via installModule function. + // Deploy new validator, it's not installed via installModule function. OwnableValidator validator2 = new OwnableValidator(); validator2Address = address(validator2); } From c6eb136cc519b40b3f7bdb4b509c4d6d6d4f2947 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 10 Sep 2024 13:17:30 +0100 Subject: [PATCH 13/15] Simplify Safe selector tests & Add update natspec --- src/EmailRecoveryManager.sol | 20 ++++++- .../allowValidatorRecovery.t.sol | 54 +++++-------------- 2 files changed, 32 insertions(+), 42 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index b1387b5..1c5c6b5 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -426,6 +426,19 @@ abstract contract EmailRecoveryManager is emit RecoveryCompleted(account); } + /** + * @notice Called during completeRecovery to finalize recovery. Contains recovery module + * implementation-specific logic to recover an account/module + * @dev this is the only function that must be implemented by consuming contracts to use the + * email recovery manager. This does not encompass other important logic such as module + * installation, that logic is specific to each module and must be implemeted separately + * @param account The address of the account for which the recovery is being completed + * @param recoveryData The data that is passed to recover the validator or account. + * recoveryData = abi.encode(validatorOrAccount, recoveryFunctionCalldata). Although, it is + * possible to design a recovery module using this manager without encoding the validator or + * account, depending on how the handler.parseRecoveryDataHash() and module.recover() functions + * are implemented + */ function recover(address account, bytes calldata recoveryData) internal virtual; /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ @@ -475,8 +488,11 @@ abstract contract EmailRecoveryManager is /** * @notice Removes all state related to an account. - * @dev In order to prevent unexpected behaviour when reinstalling account modules, the module - * should be deinitialized. This should include removing state accociated with an account. + * @dev Although this function is internal, it should be used carefully as it can be called by + * anyone. In order to prevent unexpected behaviour when reinstalling account modules, the + * module should be deinitialized. This should include removing state accociated with an + * account + * @param account The address of the account for which recovery is being deinitialized */ function deInitRecoveryModule(address account) internal onlyWhenNotRecovering { delete recoveryConfigs[account]; diff --git a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol index 6672b99..50f3b06 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol @@ -16,13 +16,18 @@ import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { using ModuleKitHelpers for *; - address validator2Address; + address newValidatorAddress; function setUp() public override { super.setUp(); - // Deploy new validator, it's not installed via installModule function. - OwnableValidator validator2 = new OwnableValidator(); - validator2Address = address(validator2); + // Deploy & install new validator to avoid `LinkedList_EntryAlreadyInList` errors + OwnableValidator newValidator = new OwnableValidator(); + newValidatorAddress = address(newValidator); + instance.installModule({ + moduleTypeId: MODULE_TYPE_VALIDATOR, + module: newValidatorAddress, + data: abi.encode(owner) + }); } function test_AllowValidatorRecovery_RevertWhen_RecoveryModuleNotInitialized() public { @@ -37,52 +42,32 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { function test_AllowValidatorRecovery_When_SafeAddOwnerSelector() public { _skipIfNotSafeAccountType(); vm.startPrank(accountAddress); - vm.mockCall( - address(accountAddress), - abi.encodeWithSelector(IERC7579Account.isModuleInstalled.selector), - abi.encode(true) - ); emailRecoveryModule.allowValidatorRecovery( - validator2Address, bytes("0"), ISafe.addOwnerWithThreshold.selector + newValidatorAddress, bytes("0"), ISafe.addOwnerWithThreshold.selector ); } function test_AllowValidatorRecovery_When_SafeRemoveOwnerSelector() public { _skipIfNotSafeAccountType(); vm.startPrank(accountAddress); - vm.mockCall( - address(accountAddress), - abi.encodeWithSelector(IERC7579Account.isModuleInstalled.selector), - abi.encode(true) - ); emailRecoveryModule.allowValidatorRecovery( - validator2Address, bytes("0"), ISafe.removeOwner.selector + newValidatorAddress, bytes("0"), ISafe.removeOwner.selector ); } function test_AllowValidatorRecovery_When_SafeSwapOwnerSelector() public { _skipIfNotSafeAccountType(); vm.startPrank(accountAddress); - vm.mockCall( - address(accountAddress), - abi.encodeWithSelector(IERC7579Account.isModuleInstalled.selector), - abi.encode(true) - ); emailRecoveryModule.allowValidatorRecovery( - validator2Address, bytes("0"), ISafe.swapOwner.selector + newValidatorAddress, bytes("0"), ISafe.swapOwner.selector ); } function test_AllowValidatorRecovery_When_SafeChangeThresholdSelector() public { _skipIfNotSafeAccountType(); vm.startPrank(accountAddress); - vm.mockCall( - address(accountAddress), - abi.encodeWithSelector(IERC7579Account.isModuleInstalled.selector), - abi.encode(true) - ); emailRecoveryModule.allowValidatorRecovery( - validator2Address, bytes("0"), ISafe.changeThreshold.selector + newValidatorAddress, bytes("0"), ISafe.changeThreshold.selector ); } @@ -119,9 +104,7 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { } function test_AllowValidatorRecovery_RevertWhen_InvalidValidator() public { - OwnableValidator newValidator = new OwnableValidator(); - address newValidatorAddress = address(newValidator); - + instance.uninstallModule(MODULE_TYPE_VALIDATOR, newValidatorAddress, ""); vm.expectRevert( abi.encodeWithSelector( UniversalEmailRecoveryModule.InvalidValidator.selector, newValidatorAddress @@ -172,15 +155,6 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { } function test_AllowValidatorRecovery_SucceedsWhenAlreadyInitialized() public { - // Deplopy and install new validator - OwnableValidator newValidator = new OwnableValidator(); - address newValidatorAddress = address(newValidator); - instance.installModule({ - moduleTypeId: MODULE_TYPE_VALIDATOR, - module: newValidatorAddress, - data: abi.encode(owner) - }); - vm.startPrank(accountAddress); vm.expectEmit(); emit UniversalEmailRecoveryModule.NewValidatorRecovery({ From 374a6f0f70358d143d9d552004717771d4dccc6e Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 10 Sep 2024 15:05:19 +0100 Subject: [PATCH 14/15] fix bug from not checking if module is installed --- src/EmailRecoveryManager.sol | 6 ++-- src/modules/SafeEmailRecoveryModule.sol | 28 ++++++++++++++++++- .../SafeRecoveryNativeModule.t.sol | 4 ++- .../configureRecovery.t.sol | 20 ++++++++----- .../UniversalEmailRecoveryModuleHarness.sol | 12 ++++++++ .../configureSafeRecovery.t.sol | 26 +++++++++++++++++ 6 files changed, 84 insertions(+), 12 deletions(-) create mode 100644 test/unit/modules/SafeEmailRecoveryModule/configureSafeRecovery.t.sol diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 1c5c6b5..c8ba6ce 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -186,8 +186,8 @@ abstract contract EmailRecoveryManager is * @notice Configures recovery for the caller's account. This is the first core function * that must be called during the end-to-end recovery flow * @dev Can only be called once for configuration. Sets up the guardians, and validates config - * parameters, ensuring that no recovery is in process. It is possible to configure guardians at - * a later stage if neccessary + * parameters, ensuring that no recovery is in process. It is possible to update configuration + * at a later stage if neccessary * @param guardians An array of guardian addresses * @param weights An array of weights corresponding to each guardian * @param threshold The threshold weight required for recovery @@ -201,7 +201,7 @@ abstract contract EmailRecoveryManager is uint256 delay, uint256 expiry ) - public + internal { address account = msg.sender; diff --git a/src/modules/SafeEmailRecoveryModule.sol b/src/modules/SafeEmailRecoveryModule.sol index c1f4b98..bd17ade 100644 --- a/src/modules/SafeEmailRecoveryModule.sol +++ b/src/modules/SafeEmailRecoveryModule.sol @@ -20,6 +20,7 @@ contract SafeEmailRecoveryModule is EmailRecoveryManager { event RecoveryExecuted(address indexed account); + error ModuleNotInstalled(address account); error InvalidAccount(address account); error InvalidSelector(bytes4 selector); error RecoveryFailed(address account); @@ -34,6 +35,31 @@ contract SafeEmailRecoveryModule is EmailRecoveryManager { EmailRecoveryManager(verifier, dkimRegistry, emailAuthImpl, subjectHandler) { } + /** + * @notice Configures recovery for the caller's account + * @dev This function ensures that the module is installed before configuring recovery. Calls + * internal configureRecovery function + * @param guardians An array of guardian addresses + * @param weights An array of weights corresponding to each guardian + * @param threshold The threshold weight required for recovery + * @param delay The delay period before recovery can be executed + * @param expiry The expiry time after which the recovery attempt is invalid + */ + function configureSafeRecovery( + address[] memory guardians, + uint256[] memory weights, + uint256 threshold, + uint256 delay, + uint256 expiry + ) + public + { + if (!ISafe(msg.sender).isModuleEnabled(address(this))) { + revert ModuleNotInstalled(msg.sender); + } + configureRecovery(guardians, weights, threshold, delay, expiry); + } + /** * Check if a recovery request can be initiated based on guardian acceptance * @param account The smart account to check @@ -90,7 +116,7 @@ contract SafeEmailRecoveryModule is EmailRecoveryManager { if (account == address(0)) { revert InvalidAccount(account); } - if (ISafe(account).isModuleEnabled(address(this)) == true) { + if (ISafe(account).isModuleEnabled(address(this))) { revert ResetFailed(account); } deInitRecoveryModule(account); diff --git a/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol b/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol index 98d71d7..5d5c06f 100644 --- a/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol +++ b/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol @@ -27,7 +27,9 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase address newOwner = owner2; // Configure recovery vm.startPrank(safeAddress); - emailRecoveryModule.configureRecovery(guardians1, guardianWeights, threshold, delay, expiry); + emailRecoveryModule.configureSafeRecovery( + guardians1, guardianWeights, threshold, delay, expiry + ); vm.stopPrank(); bytes memory recoveryCalldata = abi.encodeWithSignature( diff --git a/test/unit/EmailRecoveryManager/configureRecovery.t.sol b/test/unit/EmailRecoveryManager/configureRecovery.t.sol index 753bb47..06a0ebe 100644 --- a/test/unit/EmailRecoveryManager/configureRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/configureRecovery.t.sol @@ -24,13 +24,17 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { vm.expectRevert(IEmailRecoveryManager.SetupAlreadyCalled.selector); vm.startPrank(accountAddress); - emailRecoveryModule.configureRecovery(guardians, guardianWeights, threshold, delay, expiry); + emailRecoveryModule.exposed_configureRecovery( + guardians, guardianWeights, threshold, delay, expiry + ); } function test_ConfigureRecovery_RevertWhen_ConfigureRecoveryCalledTwice() public { vm.startPrank(accountAddress); vm.expectRevert(IEmailRecoveryManager.SetupAlreadyCalled.selector); - emailRecoveryModule.configureRecovery(guardians, guardianWeights, threshold, delay, expiry); + emailRecoveryModule.exposed_configureRecovery( + guardians, guardianWeights, threshold, delay, expiry + ); } function test_ConfigureRecovery_Succeeds() public { @@ -45,7 +49,9 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { emit IEmailRecoveryManager.RecoveryConfigured( instance.account, guardians.length, totalWeight, threshold ); - emailRecoveryModule.configureRecovery(guardians, guardianWeights, threshold, delay, expiry); + emailRecoveryModule.exposed_configureRecovery( + guardians, guardianWeights, threshold, delay, expiry + ); IEmailRecoveryManager.RecoveryConfig memory recoveryConfig = emailRecoveryModule.getRecoveryConfig(accountAddress); @@ -81,7 +87,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { guardianWeights.length ) ); - emailRecoveryModule.configureRecovery( + emailRecoveryModule.exposed_configureRecovery( zeroGuardians, guardianWeights, threshold, delay, expiry ); } @@ -99,7 +105,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { zeroGuardianWeights.length ) ); - emailRecoveryModule.configureRecovery( + emailRecoveryModule.exposed_configureRecovery( guardians, zeroGuardianWeights, threshold, delay, expiry ); } @@ -111,7 +117,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { uint256 zeroThreshold = 0; vm.expectRevert(IGuardianManager.ThresholdCannotBeZero.selector); - emailRecoveryModule.configureRecovery( + emailRecoveryModule.exposed_configureRecovery( guardians, guardianWeights, zeroThreshold, delay, expiry ); } @@ -129,7 +135,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { IGuardianManager.ThresholdExceedsTotalWeight.selector, threshold, 0 ) ); - emailRecoveryModule.configureRecovery( + emailRecoveryModule.exposed_configureRecovery( zeroGuardians, zeroGuardianWeights, threshold, delay, expiry ); } diff --git a/test/unit/UniversalEmailRecoveryModuleHarness.sol b/test/unit/UniversalEmailRecoveryModuleHarness.sol index 842f816..196ad84 100644 --- a/test/unit/UniversalEmailRecoveryModuleHarness.sol +++ b/test/unit/UniversalEmailRecoveryModuleHarness.sol @@ -18,6 +18,18 @@ contract UniversalEmailRecoveryModuleHarness is UniversalEmailRecoveryModule { UniversalEmailRecoveryModule(verifier, dkimRegistry, emailAuthImpl, subjectHandler) { } + function exposed_configureRecovery( + address[] memory guardians, + uint256[] memory weights, + uint256 threshold, + uint256 delay, + uint256 expiry + ) + external + { + configureRecovery(guardians, weights, threshold, delay, expiry); + } + function exposed_acceptGuardian( address guardian, uint256 templateIdx, diff --git a/test/unit/modules/SafeEmailRecoveryModule/configureSafeRecovery.t.sol b/test/unit/modules/SafeEmailRecoveryModule/configureSafeRecovery.t.sol new file mode 100644 index 0000000..3a383db --- /dev/null +++ b/test/unit/modules/SafeEmailRecoveryModule/configureSafeRecovery.t.sol @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.25; + +import { console2 } from "forge-std/console2.sol"; +import { SafeEmailRecoveryModule } from "src/modules/SafeEmailRecoveryModule.sol"; +import { SafeNativeIntegrationBase } from + "../../../integration/SafeRecovery/SafeNativeIntegrationBase.t.sol"; + +contract SafeEmailRecoveryModule_configureSafeRecovery_Test is SafeNativeIntegrationBase { + function setUp() public override { + super.setUp(); + } + + function test_ConfigureSafeRecovery_RevertWhen_ModuleNotInstalled() public { + skipIfNotSafeAccountType(); + + vm.startPrank(safeAddress); + safe.disableModule(address(1), address(emailRecoveryModule)); + vm.expectRevert( + abi.encodeWithSelector(SafeEmailRecoveryModule.ModuleNotInstalled.selector, safeAddress) + ); + emailRecoveryModule.configureSafeRecovery( + guardians1, guardianWeights, threshold, delay, expiry + ); + } +} From ee2c007ca79dd807c46a65ab9148cc2aea558bb5 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 10 Sep 2024 15:40:38 +0100 Subject: [PATCH 15/15] Clean up SafeEmailRecoveryModule --- src/interfaces/ISafe.sol | 4 ++-- src/modules/SafeEmailRecoveryModule.sol | 13 ++++--------- test/unit/assertErrorSelectors.t.sol | 4 +++- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/interfaces/ISafe.sol b/src/interfaces/ISafe.sol index 784a836..7c11057 100644 --- a/src/interfaces/ISafe.sol +++ b/src/interfaces/ISafe.sol @@ -6,14 +6,14 @@ interface ISafe { function getOwners() external view returns (address[] memory); function setFallbackHandler(address handler) external; function setGuard(address guard) external; - function execTransactionFromModule( + function execTransactionFromModuleReturnData( address to, uint256 value, bytes memory data, uint8 operation ) external - returns (bool success); + returns (bool success, bytes memory returnData); function isModuleEnabled(address module) external view returns (bool); function addOwnerWithThreshold(address owner, uint256 _threshold) external; function removeOwner(address prevOwner, address owner, uint256 _threshold) external; diff --git a/src/modules/SafeEmailRecoveryModule.sol b/src/modules/SafeEmailRecoveryModule.sol index bd17ade..da14a01 100644 --- a/src/modules/SafeEmailRecoveryModule.sol +++ b/src/modules/SafeEmailRecoveryModule.sol @@ -23,7 +23,7 @@ contract SafeEmailRecoveryModule is EmailRecoveryManager { error ModuleNotInstalled(address account); error InvalidAccount(address account); error InvalidSelector(bytes4 selector); - error RecoveryFailed(address account); + error RecoveryFailed(address account, bytes returnData); error ResetFailed(address account); constructor( @@ -79,12 +79,7 @@ contract SafeEmailRecoveryModule is EmailRecoveryManager { * being recovered. recoveryData = abi.encode(safeAccount, recoveryFunctionCalldata) */ function recover(address account, bytes calldata recoveryData) internal override { - (address encodedAccount, bytes memory recoveryCalldata) = - abi.decode(recoveryData, (address, bytes)); - - if (encodedAccount == address(0) || encodedAccount != account) { - revert InvalidAccount(encodedAccount); - } + (, bytes memory recoveryCalldata) = abi.decode(recoveryData, (address, bytes)); bytes4 calldataSelector; // solhint-disable-next-line no-inline-assembly @@ -95,14 +90,14 @@ contract SafeEmailRecoveryModule is EmailRecoveryManager { revert InvalidSelector(calldataSelector); } - bool success = ISafe(account).execTransactionFromModule({ + (bool success, bytes memory returnData) = ISafe(account).execTransactionFromModuleReturnData({ to: account, value: 0, data: recoveryCalldata, operation: uint8(Enum.Operation.Call) }); if (!success) { - revert RecoveryFailed(account); + revert RecoveryFailed(account, returnData); } emit RecoveryExecuted(account); diff --git a/test/unit/assertErrorSelectors.t.sol b/test/unit/assertErrorSelectors.t.sol index 5b92830..f6ba593 100644 --- a/test/unit/assertErrorSelectors.t.sol +++ b/test/unit/assertErrorSelectors.t.sol @@ -65,6 +65,7 @@ contract LogErrorSelectors_Test is Test { assertEq(IEmailRecoveryManager.RecoveryRequestExpired.selector, bytes4(0x566ad75e)); assertEq(IEmailRecoveryManager.InvalidRecoveryDataHash.selector, bytes4(0x9e1a616f)); assertEq(IEmailRecoveryManager.NoRecoveryInProcess.selector, bytes4(0x87434f51)); + assertEq(IEmailRecoveryManager.RecoveryHasNotExpired.selector, bytes4(0x93ae1fc6)); assertEq(IEmailRecoveryManager.RecoveryIsNotActivated.selector, bytes4(0xc737140f)); } @@ -95,9 +96,10 @@ contract LogErrorSelectors_Test is Test { } function test_SafeEmailRecoveryModule_AssertSelectors() public { + assertEq(SafeEmailRecoveryModule.ModuleNotInstalled.selector, bytes4(0x026d9639)); assertEq(SafeEmailRecoveryModule.InvalidAccount.selector, bytes4(0x4b579b22)); assertEq(SafeEmailRecoveryModule.InvalidSelector.selector, bytes4(0x12ba286f)); - assertEq(SafeEmailRecoveryModule.RecoveryFailed.selector, bytes4(0xaa3485bc)); + assertEq(SafeEmailRecoveryModule.RecoveryFailed.selector, bytes4(0xc9dcc2a0)); assertEq(SafeEmailRecoveryModule.ResetFailed.selector, bytes4(0x8078983d)); }