From e607ce9a90c5e1473f49dedd0fa8abb728030a6e Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Mon, 1 Jul 2024 21:18:45 +0200 Subject: [PATCH 1/2] Modify check for no guardians & add tests --- src/EmailRecoveryManager.sol | 9 +- src/libraries/GuardianUtils.sol | 12 ++- .../EmailRecoveryManager/addGuardian.t.sol | 1 + .../configureRecovery.t.sol | 85 +++++++++++++++++++ .../deInitRecoveryFromModule.t.sol | 1 + .../getGuardianConfig.t.sol | 1 - .../GuardianUtils/setupGuardians.t.sol | 1 + 7 files changed, 103 insertions(+), 7 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index ca5471c8..fe435f92 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -227,12 +227,17 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco revert SetupAlreadyCalled(); } - setupGuardians(account, guardians, weights, threshold); - if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToRecover(account)) { revert RecoveryModuleNotAuthorized(); } + // Allow recovery configuration without configuring guardians + if (guardians.length == 0 && weights.length == 0 && threshold == 0) { + guardianConfigs[account].initialized = true; + } else { + setupGuardians(account, guardians, weights, threshold); + } + RecoveryConfig memory recoveryConfig = RecoveryConfig(delay, expiry); updateRecoveryConfig(recoveryConfig); diff --git a/src/libraries/GuardianUtils.sol b/src/libraries/GuardianUtils.sol index 1d49814f..e09e6c7a 100644 --- a/src/libraries/GuardianUtils.sol +++ b/src/libraries/GuardianUtils.sol @@ -49,7 +49,7 @@ library GuardianUtils { revert IncorrectNumberOfWeights(); } - if (threshold == 0 && guardianCount > 0) { + if (threshold == 0) { revert ThresholdCannotBeZero(); } @@ -85,8 +85,12 @@ library GuardianUtils { revert ThresholdCannotExceedTotalWeight(); } - guardianConfigs[account] = - IEmailRecoveryManager.GuardianConfig(guardianCount, totalWeight, threshold, true); + guardianConfigs[account] = IEmailRecoveryManager.GuardianConfig({ + guardianCount: guardianCount, + totalWeight: totalWeight, + threshold: threshold, + initialized: true + }); } function updateGuardianStatus( @@ -202,7 +206,7 @@ library GuardianUtils { revert ThresholdCannotExceedTotalWeight(); } - // There has to be at least one Account guardian. + // Guardian weight should be at least 1 if (threshold == 0) { revert ThresholdCannotBeZero(); } diff --git a/test/unit/EmailRecoveryManager/addGuardian.t.sol b/test/unit/EmailRecoveryManager/addGuardian.t.sol index cd208a2a..d20063e8 100644 --- a/test/unit/EmailRecoveryManager/addGuardian.t.sol +++ b/test/unit/EmailRecoveryManager/addGuardian.t.sol @@ -45,5 +45,6 @@ contract EmailRecoveryManager_addGuardian_Test is UnitBase { assertEq(guardianConfig.guardianCount, expectedGuardianCount); assertEq(guardianConfig.totalWeight, expectedTotalWeight); assertEq(guardianConfig.threshold, expectedThreshold); + assertEq(guardianConfig.initialized, true); } } diff --git a/test/unit/EmailRecoveryManager/configureRecovery.t.sol b/test/unit/EmailRecoveryManager/configureRecovery.t.sol index ff73a910..06a08207 100644 --- a/test/unit/EmailRecoveryManager/configureRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/configureRecovery.t.sol @@ -6,6 +6,7 @@ import { ModuleKitHelpers } from "modulekit/ModuleKit.sol"; import { MODULE_TYPE_EXECUTOR } from "modulekit/external/ERC7579.sol"; import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol"; import { GuardianStorage, GuardianStatus } from "src/libraries/EnumerableGuardianMap.sol"; +import { GuardianUtils } from "src/libraries/GuardianUtils.sol"; import { UnitBase } from "../UnitBase.t.sol"; import { IModule } from "erc7579/interfaces/IERC7579Module.sol"; @@ -73,10 +74,94 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { assertEq(guardianConfig.guardianCount, guardians.length); assertEq(guardianConfig.totalWeight, totalWeight); assertEq(guardianConfig.threshold, threshold); + assertEq(guardianConfig.initialized, true); GuardianStorage memory guardian = emailRecoveryManager.getGuardian(accountAddress, guardians[0]); assertEq(uint256(guardian.status), uint256(GuardianStatus.REQUESTED)); assertEq(guardian.weight, guardianWeights[0]); } + + function test_ConfigureRecovery_RevertWhen_ZeroGuardians() public { + instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); + vm.startPrank(accountAddress); + emailRecoveryModule.allowValidatorRecovery( + validatorAddress, isInstalledContext, functionSelector + ); + address[] memory zeroGuardians; + + vm.expectRevert(GuardianUtils.IncorrectNumberOfWeights.selector); + emailRecoveryManager.configureRecovery( + zeroGuardians, guardianWeights, threshold, delay, expiry + ); + } + + function test_ConfigureRecovery_RevertWhen_ZeroGuardianWeights() public { + instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); + vm.startPrank(accountAddress); + emailRecoveryModule.allowValidatorRecovery( + validatorAddress, isInstalledContext, functionSelector + ); + uint256[] memory zeroGuardianWeights; + + vm.expectRevert(GuardianUtils.IncorrectNumberOfWeights.selector); + emailRecoveryManager.configureRecovery( + guardians, zeroGuardianWeights, threshold, delay, expiry + ); + } + + function test_ConfigureRecovery_RevertWhen_ZeroThreshold() public { + instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); + vm.startPrank(accountAddress); + emailRecoveryModule.allowValidatorRecovery( + validatorAddress, isInstalledContext, functionSelector + ); + uint256 zeroThreshold = 0; + + vm.expectRevert(GuardianUtils.ThresholdCannotBeZero.selector); + emailRecoveryManager.configureRecovery( + guardians, guardianWeights, zeroThreshold, delay, expiry + ); + } + + function test_ConfigureRecovery_SucceedsWithNoGuardians() public { + instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); + + address[] memory zeroGuardians; + uint256[] memory zeroGuardianWeights; + uint256 zeroThreshold = 0; + + // Install recovery module - configureRecovery is called on `onInstall` + instance.installModule({ + moduleTypeId: MODULE_TYPE_EXECUTOR, + module: recoveryModuleAddress, + data: abi.encode( + validatorAddress, + isInstalledContext, + functionSelector, + zeroGuardians, + zeroGuardianWeights, + zeroThreshold, + delay, + expiry + ) + }); + + IEmailRecoveryManager.RecoveryConfig memory recoveryConfig = + emailRecoveryManager.getRecoveryConfig(accountAddress); + assertEq(recoveryConfig.delay, delay); + assertEq(recoveryConfig.expiry, expiry); + + IEmailRecoveryManager.GuardianConfig memory guardianConfig = + emailRecoveryManager.getGuardianConfig(accountAddress); + assertEq(guardianConfig.guardianCount, zeroGuardians.length); + assertEq(guardianConfig.totalWeight, 0); + assertEq(guardianConfig.threshold, zeroThreshold); + assertEq(guardianConfig.initialized, true); + + GuardianStorage memory guardian = + emailRecoveryManager.getGuardian(accountAddress, guardians[0]); + assertEq(uint256(guardian.status), uint256(GuardianStatus.NONE)); + assertEq(guardian.weight, 0); + } } diff --git a/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol b/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol index c07b96d0..b04b56bc 100644 --- a/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol +++ b/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol @@ -67,5 +67,6 @@ contract EmailRecoveryManager_deInitRecoveryFromModule_Test is UnitBase { assertEq(guardianConfig.guardianCount, 0); assertEq(guardianConfig.totalWeight, 0); assertEq(guardianConfig.threshold, 0); + assertEq(guardianConfig.threshold, false); } } diff --git a/test/unit/EmailRecoveryManager/getGuardianConfig.t.sol b/test/unit/EmailRecoveryManager/getGuardianConfig.t.sol index 942f4410..89371012 100644 --- a/test/unit/EmailRecoveryManager/getGuardianConfig.t.sol +++ b/test/unit/EmailRecoveryManager/getGuardianConfig.t.sol @@ -29,7 +29,6 @@ contract EmailRecoveryManager_getGuardianConfig_Test is UnitBase { function test_GetGuardianConfig_Succeeds() public { IEmailRecoveryManager.GuardianConfig memory guardianConfig = emailRecoveryManager.getGuardianConfig(accountAddress); - console2.log(expectedGuardianCount); assertEq(guardianConfig.guardianCount, expectedGuardianCount); assertEq(guardianConfig.totalWeight, expectedTotalWeight); assertEq(guardianConfig.threshold, expectedThreshold); diff --git a/test/unit/libraries/GuardianUtils/setupGuardians.t.sol b/test/unit/libraries/GuardianUtils/setupGuardians.t.sol index 476cd60a..d8a58fe4 100644 --- a/test/unit/libraries/GuardianUtils/setupGuardians.t.sol +++ b/test/unit/libraries/GuardianUtils/setupGuardians.t.sol @@ -118,5 +118,6 @@ contract GuardianUtils_setupGuardians_Test is UnitBase { assertEq(guardianConfig.guardianCount, expectedGuardianCount); assertEq(guardianConfig.totalWeight, expectedTotalWeight); assertEq(guardianConfig.threshold, expectedThreshold); + assertEq(guardianConfig.initialized, true); } } From 1f03ce34c7322d917e7fc8118981140f46a22c0a Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Mon, 1 Jul 2024 21:26:11 +0200 Subject: [PATCH 2/2] Fix assertion typo --- test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol b/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol index b04b56bc..5c8d8dfe 100644 --- a/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol +++ b/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol @@ -67,6 +67,6 @@ contract EmailRecoveryManager_deInitRecoveryFromModule_Test is UnitBase { assertEq(guardianConfig.guardianCount, 0); assertEq(guardianConfig.totalWeight, 0); assertEq(guardianConfig.threshold, 0); - assertEq(guardianConfig.threshold, false); + assertEq(guardianConfig.initialized, false); } }