From 4fd94bac4be7955eab4cb621995b57f5a0728ee9 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 16 Jul 2024 12:18:22 +0100 Subject: [PATCH 01/30] H1: Multiple vulnerabilities in recovery configuration process --- src/EmailRecoveryManager.sol | 9 +--- src/interfaces/IEmailRecoveryManager.sol | 7 ++- src/libraries/GuardianUtils.sol | 11 +++-- .../EmailRecoveryManager/addGuardian.t.sol | 1 - .../configureRecovery.t.sol | 45 ++++--------------- .../deInitRecoveryFromModule.t.sol | 1 - .../GuardianUtils/setupGuardians.t.sol | 1 - 7 files changed, 19 insertions(+), 56 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index dd606fe..eec12ad 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -245,12 +245,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco 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); - } + setupGuardians(account, guardians, weights, threshold); RecoveryConfig memory recoveryConfig = RecoveryConfig(delay, expiry); updateRecoveryConfig(recoveryConfig); @@ -270,7 +265,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco { address account = msg.sender; - if (!guardianConfigs[account].initialized) { + if (guardianConfigs[account].threshold == 0) { revert AccountNotConfigured(); } if (recoveryConfig.delay > recoveryConfig.expiry) { diff --git a/src/interfaces/IEmailRecoveryManager.sol b/src/interfaces/IEmailRecoveryManager.sol index 450a3c3..5e4938f 100644 --- a/src/interfaces/IEmailRecoveryManager.sol +++ b/src/interfaces/IEmailRecoveryManager.sol @@ -39,10 +39,9 @@ interface IEmailRecoveryManager { * Config should be maintained over subsequent recovery attempts unless explicitly modified */ struct GuardianConfig { - uint256 guardianCount; - uint256 totalWeight; - uint256 threshold; - bool initialized; + uint256 guardianCount; // total count for all guardians + uint256 totalWeight; // combined weight for all guardians + uint256 threshold; // the threshold required to successfully process a recovery attempt } /*////////////////////////////////////////////////////////////////////////// diff --git a/src/libraries/GuardianUtils.sol b/src/libraries/GuardianUtils.sol index 3b5c434..a0cf797 100644 --- a/src/libraries/GuardianUtils.sol +++ b/src/libraries/GuardianUtils.sol @@ -107,8 +107,7 @@ library GuardianUtils { guardianConfigs[account] = IEmailRecoveryManager.GuardianConfig({ guardianCount: guardianCount, totalWeight: totalWeight, - threshold: threshold, - initialized: true + threshold: threshold }); } @@ -153,9 +152,9 @@ library GuardianUtils { ) internal { - // Initialized can only be false at initialization. + // Threshold can only be 0 at initialization. // Check ensures that setup function should be called first - if (!guardianConfigs[account].initialized) { + if (guardianConfigs[account].threshold == 0) { revert SetupNotCalled(); } @@ -242,9 +241,9 @@ library GuardianUtils { ) internal { - // Initialized can only be false at initialization. + // Threshold can only be 0 at initialization. // Check ensures that setup function should be called first - if (!guardianConfigs[account].initialized) { + if (guardianConfigs[account].threshold == 0) { revert SetupNotCalled(); } diff --git a/test/unit/EmailRecoveryManager/addGuardian.t.sol b/test/unit/EmailRecoveryManager/addGuardian.t.sol index d20063e..cd208a2 100644 --- a/test/unit/EmailRecoveryManager/addGuardian.t.sol +++ b/test/unit/EmailRecoveryManager/addGuardian.t.sol @@ -45,6 +45,5 @@ 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 06a0820..8492cef 100644 --- a/test/unit/EmailRecoveryManager/configureRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/configureRecovery.t.sol @@ -8,7 +8,6 @@ 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"; contract EmailRecoveryManager_configureRecovery_Test is UnitBase { using ModuleKitHelpers for *; @@ -74,7 +73,6 @@ 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]); @@ -124,44 +122,19 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { ); } - function test_ConfigureRecovery_SucceedsWithNoGuardians() public { + function test_ConfigureRecovery_RevetrWhen_NoGuardians() public { instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); + vm.startPrank(accountAddress); + emailRecoveryModule.allowValidatorRecovery( + validatorAddress, isInstalledContext, functionSelector + ); 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); + vm.expectRevert(GuardianUtils.ThresholdCannotExceedTotalWeight.selector); + emailRecoveryManager.configureRecovery( + zeroGuardians, zeroGuardianWeights, threshold, delay, expiry + ); } } diff --git a/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol b/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol index 5c8d8df..c07b96d 100644 --- a/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol +++ b/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol @@ -67,6 +67,5 @@ contract EmailRecoveryManager_deInitRecoveryFromModule_Test is UnitBase { assertEq(guardianConfig.guardianCount, 0); assertEq(guardianConfig.totalWeight, 0); assertEq(guardianConfig.threshold, 0); - assertEq(guardianConfig.initialized, false); } } diff --git a/test/unit/libraries/GuardianUtils/setupGuardians.t.sol b/test/unit/libraries/GuardianUtils/setupGuardians.t.sol index d8a58fe..476cd60 100644 --- a/test/unit/libraries/GuardianUtils/setupGuardians.t.sol +++ b/test/unit/libraries/GuardianUtils/setupGuardians.t.sol @@ -118,6 +118,5 @@ contract GuardianUtils_setupGuardians_Test is UnitBase { assertEq(guardianConfig.guardianCount, expectedGuardianCount); assertEq(guardianConfig.totalWeight, expectedTotalWeight); assertEq(guardianConfig.threshold, expectedThreshold); - assertEq(guardianConfig.initialized, true); } } From b31a8442e41397e5975d18e45d11e44c5e9cfda1 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 17 Jul 2024 12:25:57 +0100 Subject: [PATCH 02/30] H2: Premature guardian configuration update in addGuardian function --- src/EmailRecoveryManager.sol | 9 ++++- src/interfaces/IEmailRecoveryManager.sol | 9 ++++- src/libraries/GuardianUtils.sol | 15 +++++-- .../EmailRecoveryManager.integration.t.sol | 4 +- .../EmailRecoveryManager/acceptGuardian.t.sol | 5 +++ .../EmailRecoveryManager/addGuardian.t.sol | 3 ++ .../EmailRecoveryManager/cancelRecovery.t.sol | 2 + .../changeThreshold.t.sol | 1 + .../completeRecovery.t.sol | 1 + .../configureRecovery.t.sol | 4 +- .../deInitRecoveryFromModule.t.sol | 2 + .../getGuardianConfig.t.sol | 3 ++ .../getRecoveryRequest.t.sol | 1 + .../processRecovery.t.sol | 39 +++++++++++++++++++ .../EmailRecoveryManager/removeGuardian.t.sol | 2 + .../EmailRecoveryManager/setupGuardians.t.sol | 1 + test/unit/assertErrorSelectors.t.sol | 4 +- .../libraries/GuardianUtils/addGuardian.t.sol | 2 + .../GuardianUtils/changeThreshold.t.sol | 2 +- .../GuardianUtils/removeGuardian.t.sol | 35 ++++++++++++++++- .../GuardianUtils/setupGuardians.t.sol | 4 +- 21 files changed, 133 insertions(+), 15 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index eec12ad..1d972f3 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -328,6 +328,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco } guardiansStorage.updateGuardianStatus(account, guardian, GuardianStatus.ACCEPTED); + guardianConfigs[account].acceptedWeight += guardianStorage.weight; emit GuardianAccepted(account, guardian); } @@ -364,6 +365,11 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco revert RecoveryModuleNotAuthorized(); } + GuardianConfig memory guardianConfig = guardianConfigs[account]; + if (guardianConfig.threshold > guardianConfig.acceptedWeight) { + revert ThresholdExceedsAcceptedWeight(); + } + // This check ensures GuardianStatus is correct and also implicitly that the // account in email is a valid account GuardianStorage memory guardianStorage = getGuardian(account, guardian); @@ -375,8 +381,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco recoveryRequest.currentWeight += guardianStorage.weight; - uint256 threshold = guardianConfigs[account].threshold; - if (recoveryRequest.currentWeight >= threshold) { + if (recoveryRequest.currentWeight >= guardianConfig.threshold) { uint256 executeAfter = block.timestamp + recoveryConfigs[account].delay; uint256 executeBefore = block.timestamp + recoveryConfigs[account].expiry; diff --git a/src/interfaces/IEmailRecoveryManager.sol b/src/interfaces/IEmailRecoveryManager.sol index 5e4938f..7a835dd 100644 --- a/src/interfaces/IEmailRecoveryManager.sol +++ b/src/interfaces/IEmailRecoveryManager.sol @@ -40,7 +40,13 @@ interface IEmailRecoveryManager { */ struct GuardianConfig { uint256 guardianCount; // total count for all guardians - uint256 totalWeight; // combined weight for all guardians + uint256 totalWeight; // combined weight for all guardians. Important for checking that + // thresholds are valid. + uint256 acceptedWeight; // combined weight for all accepted guardians. This is separated + // from totalWeight as it is important to prevent recovery starting without enough + // accepted guardians to meet the threshold. Storing this in a variable avoids the need + // to loop over accepted guardians whenever checking if a recovery attempt can be + // started without being broken uint256 threshold; // the threshold required to successfully process a recovery attempt } @@ -70,6 +76,7 @@ interface IEmailRecoveryManager { error DelayMoreThanExpiry(); error RecoveryWindowTooShort(); error InvalidTemplateIndex(); + error ThresholdExceedsAcceptedWeight(); error InvalidGuardianStatus( GuardianStatus guardianStatus, GuardianStatus expectedGuardianStatus ); diff --git a/src/libraries/GuardianUtils.sol b/src/libraries/GuardianUtils.sol index a0cf797..4f92954 100644 --- a/src/libraries/GuardianUtils.sol +++ b/src/libraries/GuardianUtils.sol @@ -19,7 +19,7 @@ library GuardianUtils { error InvalidGuardianAddress(); error InvalidGuardianWeight(); error AddressAlreadyGuardian(); - error ThresholdCannotExceedTotalWeight(); + error ThresholdExceedsTotalWeight(); error StatusCannotBeTheSame(); error SetupNotCalled(); error UnauthorizedAccountForGuardian(); @@ -101,12 +101,13 @@ library GuardianUtils { } if (threshold > totalWeight) { - revert ThresholdCannotExceedTotalWeight(); + revert ThresholdExceedsTotalWeight(); } guardianConfigs[account] = IEmailRecoveryManager.GuardianConfig({ guardianCount: guardianCount, totalWeight: totalWeight, + acceptedWeight: 0, threshold: threshold }); } @@ -138,6 +139,7 @@ library GuardianUtils { /** * @notice Adds a guardian for the caller's account with a specified weight + * @dev A guardian is added, but not accepted after this function has been called * @param guardianConfigs The guardian config storage associated with an account * @param account The address of the account associated with the guardian * @param guardian The address of the guardian to be added @@ -206,18 +208,23 @@ library GuardianUtils { // Only allow guardian removal if threshold can still be reached. if (guardianConfig.totalWeight - guardianStorage.weight < guardianConfig.threshold) { - revert ThresholdCannotExceedTotalWeight(); + revert ThresholdExceedsTotalWeight(); } guardiansStorage[account].remove(guardian); guardianConfigs[account].guardianCount--; guardianConfigs[account].totalWeight -= guardianStorage.weight; + if (guardianStorage.status == GuardianStatus.ACCEPTED) { + guardianConfigs[account].acceptedWeight -= guardianStorage.weight; + } emit RemovedGuardian(account, guardian); } /** * @notice Removes all guardians associated with an account + * @dev Does not remove guardian config, this should be modified at the same time as calling + * this function * @param account The address of the account associated with the guardians */ function removeAllGuardians( @@ -249,7 +256,7 @@ library GuardianUtils { // Validate that threshold is smaller than the total weight. if (threshold > guardianConfigs[account].totalWeight) { - revert ThresholdCannotExceedTotalWeight(); + revert ThresholdExceedsTotalWeight(); } // Guardian weight should be at least 1 diff --git a/test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol b/test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol index 91b5ffe..fa369fd 100644 --- a/test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol +++ b/test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol @@ -48,6 +48,7 @@ contract EmailRecoveryManager_Integration_Test is function test_RevertWhen_HandleAcceptanceCalled_DuringRecovery() public { acceptGuardian(accountAddress1, guardians1[0]); + acceptGuardian(accountAddress1, guardians1[1]); vm.warp(12 seconds); handleRecovery(accountAddress1, guardians1[0], calldataHash1); @@ -116,11 +117,12 @@ contract EmailRecoveryManager_Integration_Test is public { acceptGuardian(accountAddress1, guardians1[0]); + acceptGuardian(accountAddress1, guardians1[1]); vm.warp(12 seconds); handleRecovery(accountAddress1, guardians1[0], calldataHash1); EmailAuthMsg memory emailAuthMsg = - getRecoveryEmailAuthMessage(accountAddress1, guardians1[1], calldataHash1); + getRecoveryEmailAuthMessage(accountAddress1, guardians1[2], calldataHash1); vm.expectRevert("guardian is not deployed"); emailRecoveryManager.handleRecovery(emailAuthMsg, templateIdx); diff --git a/test/unit/EmailRecoveryManager/acceptGuardian.t.sol b/test/unit/EmailRecoveryManager/acceptGuardian.t.sol index cb27ec7..546a162 100644 --- a/test/unit/EmailRecoveryManager/acceptGuardian.t.sol +++ b/test/unit/EmailRecoveryManager/acceptGuardian.t.sol @@ -34,6 +34,7 @@ contract EmailRecoveryManager_acceptGuardian_Test is UnitBase { function test_AcceptGuardian_RevertWhen_AlreadyRecovering() public { acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); vm.warp(12 seconds); handleRecovery(recoveryModuleAddress, calldataHash, accountSalt1); @@ -95,5 +96,9 @@ contract EmailRecoveryManager_acceptGuardian_Test is UnitBase { emailRecoveryManager.getGuardian(accountAddress, guardian1); assertEq(uint256(guardianStorage.status), uint256(GuardianStatus.ACCEPTED)); assertEq(guardianStorage.weight, uint256(1)); + + IEmailRecoveryManager.GuardianConfig memory guardianConfig = + emailRecoveryManager.getGuardianConfig(accountAddress); + assertEq(guardianConfig.acceptedWeight, guardianStorage.weight); } } diff --git a/test/unit/EmailRecoveryManager/addGuardian.t.sol b/test/unit/EmailRecoveryManager/addGuardian.t.sol index cd208a2..8581a63 100644 --- a/test/unit/EmailRecoveryManager/addGuardian.t.sol +++ b/test/unit/EmailRecoveryManager/addGuardian.t.sol @@ -14,6 +14,7 @@ contract EmailRecoveryManager_addGuardian_Test is UnitBase { function test_AddGuardian_RevertWhen_AlreadyRecovering() public { acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); vm.warp(12 seconds); handleRecovery(recoveryModuleAddress, calldataHash, accountSalt1); @@ -28,6 +29,7 @@ contract EmailRecoveryManager_addGuardian_Test is UnitBase { uint256 expectedGuardianCount = guardians.length + 1; uint256 expectedTotalWeight = totalWeight + newGuardianWeight; + uint256 expectedAcceptedWeight = 0; // no guardians accepted uint256 expectedThreshold = threshold; // same threshold vm.startPrank(accountAddress); @@ -44,6 +46,7 @@ contract EmailRecoveryManager_addGuardian_Test is UnitBase { emailRecoveryManager.getGuardianConfig(accountAddress); assertEq(guardianConfig.guardianCount, expectedGuardianCount); assertEq(guardianConfig.totalWeight, expectedTotalWeight); + assertEq(guardianConfig.acceptedWeight, expectedAcceptedWeight); assertEq(guardianConfig.threshold, expectedThreshold); } } diff --git a/test/unit/EmailRecoveryManager/cancelRecovery.t.sol b/test/unit/EmailRecoveryManager/cancelRecovery.t.sol index b3b675f..ad96a8f 100644 --- a/test/unit/EmailRecoveryManager/cancelRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/cancelRecovery.t.sol @@ -17,6 +17,7 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { address otherAddress = address(99); acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); vm.warp(12 seconds); handleRecovery(recoveryModuleAddress, calldataHash, accountSalt1); @@ -39,6 +40,7 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { function test_CancelRecovery_PartialRequest_Succeeds() public { acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); vm.warp(12 seconds); handleRecovery(recoveryModuleAddress, calldataHash, accountSalt1); diff --git a/test/unit/EmailRecoveryManager/changeThreshold.t.sol b/test/unit/EmailRecoveryManager/changeThreshold.t.sol index e0db916..efa9cc3 100644 --- a/test/unit/EmailRecoveryManager/changeThreshold.t.sol +++ b/test/unit/EmailRecoveryManager/changeThreshold.t.sol @@ -13,6 +13,7 @@ contract EmailRecoveryManager_changeThreshold_Test is UnitBase { function test_RevertWhen_AlreadyRecovering() public { acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); vm.warp(12 seconds); handleRecovery(recoveryModuleAddress, calldataHash, accountSalt1); diff --git a/test/unit/EmailRecoveryManager/completeRecovery.t.sol b/test/unit/EmailRecoveryManager/completeRecovery.t.sol index 6be11a0..8c65be9 100644 --- a/test/unit/EmailRecoveryManager/completeRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/completeRecovery.t.sol @@ -19,6 +19,7 @@ contract EmailRecoveryManager_completeRecovery_Test is UnitBase { function test_CompleteRecovery_RevertWhen_NotEnoughApprovals() public { acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); vm.warp(12 seconds); handleRecovery(recoveryModuleAddress, calldataHash, accountSalt1); // only one guardian added and one approval diff --git a/test/unit/EmailRecoveryManager/configureRecovery.t.sol b/test/unit/EmailRecoveryManager/configureRecovery.t.sol index 8492cef..f03964f 100644 --- a/test/unit/EmailRecoveryManager/configureRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/configureRecovery.t.sol @@ -18,6 +18,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { function test_ConfigureRecovery_RevertWhen_AlreadyRecovering() public { acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); vm.warp(12 seconds); handleRecovery(recoveryModuleAddress, calldataHash, accountSalt1); @@ -72,6 +73,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { emailRecoveryManager.getGuardianConfig(accountAddress); assertEq(guardianConfig.guardianCount, guardians.length); assertEq(guardianConfig.totalWeight, totalWeight); + assertEq(guardianConfig.acceptedWeight, 0); // no guardians accepted yet assertEq(guardianConfig.threshold, threshold); GuardianStorage memory guardian = @@ -132,7 +134,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { address[] memory zeroGuardians; uint256[] memory zeroGuardianWeights; - vm.expectRevert(GuardianUtils.ThresholdCannotExceedTotalWeight.selector); + vm.expectRevert(GuardianUtils.ThresholdExceedsTotalWeight.selector); emailRecoveryManager.configureRecovery( zeroGuardians, zeroGuardianWeights, threshold, delay, expiry ); diff --git a/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol b/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol index c07b96d..76f2cd4 100644 --- a/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol +++ b/test/unit/EmailRecoveryManager/deInitRecoveryFromModule.t.sol @@ -18,6 +18,7 @@ contract EmailRecoveryManager_deInitRecoveryFromModule_Test is UnitBase { function test_DeInitRecoveryFromModule_RevertWhen_RecoveryInProcess() public { acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); vm.warp(12 seconds); handleRecovery(recoveryModuleAddress, calldataHash, accountSalt1); @@ -66,6 +67,7 @@ contract EmailRecoveryManager_deInitRecoveryFromModule_Test is UnitBase { emailRecoveryManager.getGuardianConfig(accountAddress); assertEq(guardianConfig.guardianCount, 0); assertEq(guardianConfig.totalWeight, 0); + assertEq(guardianConfig.acceptedWeight, 0); assertEq(guardianConfig.threshold, 0); } } diff --git a/test/unit/EmailRecoveryManager/getGuardianConfig.t.sol b/test/unit/EmailRecoveryManager/getGuardianConfig.t.sol index 8937101..ff74a33 100644 --- a/test/unit/EmailRecoveryManager/getGuardianConfig.t.sol +++ b/test/unit/EmailRecoveryManager/getGuardianConfig.t.sol @@ -12,6 +12,7 @@ contract EmailRecoveryManager_getGuardianConfig_Test is UnitBase { uint256 expectedGuardianCount; uint256 expectedTotalWeight; + uint256 expectedAcceptedWeight; uint256 expectedThreshold; function setUp() public override { @@ -19,6 +20,7 @@ contract EmailRecoveryManager_getGuardianConfig_Test is UnitBase { expectedGuardianCount = guardians.length + 1; expectedTotalWeight = totalWeight + newGuardianWeight; + expectedAcceptedWeight = 0; // no guardians accepted expectedThreshold = threshold; vm.startPrank(accountAddress); @@ -31,6 +33,7 @@ contract EmailRecoveryManager_getGuardianConfig_Test is UnitBase { emailRecoveryManager.getGuardianConfig(accountAddress); assertEq(guardianConfig.guardianCount, expectedGuardianCount); assertEq(guardianConfig.totalWeight, expectedTotalWeight); + assertEq(guardianConfig.acceptedWeight, expectedAcceptedWeight); assertEq(guardianConfig.threshold, expectedThreshold); } } diff --git a/test/unit/EmailRecoveryManager/getRecoveryRequest.t.sol b/test/unit/EmailRecoveryManager/getRecoveryRequest.t.sol index 38d9b44..6c594af 100644 --- a/test/unit/EmailRecoveryManager/getRecoveryRequest.t.sol +++ b/test/unit/EmailRecoveryManager/getRecoveryRequest.t.sol @@ -13,6 +13,7 @@ contract EmailRecoveryManager_getRecoveryRequest_Test is UnitBase { function test_GetRecoveryRequest_Succeeds() public { acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); vm.warp(12 seconds); handleRecovery(recoveryModuleAddress, calldataHash, accountSalt1); diff --git a/test/unit/EmailRecoveryManager/processRecovery.t.sol b/test/unit/EmailRecoveryManager/processRecovery.t.sol index 78e8f7f..e2a8ff4 100644 --- a/test/unit/EmailRecoveryManager/processRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/processRecovery.t.sol @@ -41,6 +41,9 @@ contract EmailRecoveryManager_processRecovery_Test is UnitBase { function test_ProcessRecovery_RevertWhen_GuardianStatusIsNONE() public { address invalidGuardian = address(1); + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + // invalidGuardian has not been configured nor accepted, so the guardian status is NONE vm.expectRevert( abi.encodeWithSelector( @@ -55,6 +58,9 @@ contract EmailRecoveryManager_processRecovery_Test is UnitBase { } function test_ProcessRecovery_RevertWhen_GuardianStatusIsREQUESTED() public { + acceptGuardian(accountSalt2); + acceptGuardian(accountSalt3); + // Valid guardian but we haven't called acceptGuardian(), so the guardian // status is still REQUESTED vm.expectRevert( @@ -80,10 +86,43 @@ contract EmailRecoveryManager_processRecovery_Test is UnitBase { ); } + function test_ProcessRecovery_RevertWhen_ThresholdExceedsAcceptedWeight() public { + // total weight = 4 + // threshold = 3 + // useable weight from accepted guardians = 0 + + acceptGuardian(accountSalt1); // weight = 1 + acceptGuardian(accountSalt2); // weight = 2 + + // total weight = 4 + // threshold = 3 + // useable weight from accepted guardians = 3 + + address newGuardian = address(1); + uint256 newWeight = 1; + uint256 newThreshold = 5; + + vm.startPrank(accountAddress); + emailRecoveryManager.addGuardian(newGuardian, newWeight); + emailRecoveryManager.changeThreshold(newThreshold); + vm.stopPrank(); + // total weight = 5 + // threshold = 5 + // useable weight from accepted guardians = 3 + + vm.warp(12 seconds); + + vm.expectRevert(IEmailRecoveryManager.ThresholdExceedsAcceptedWeight.selector); + emailRecoveryManager.exposed_processRecovery( + guardian2, templateIdx, subjectParams, nullifier + ); + } + function test_ProcessRecovery_IncreasesTotalWeight() public { uint256 guardian1Weight = guardianWeights[0]; acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); emailRecoveryManager.exposed_processRecovery( guardian1, templateIdx, subjectParams, nullifier diff --git a/test/unit/EmailRecoveryManager/removeGuardian.t.sol b/test/unit/EmailRecoveryManager/removeGuardian.t.sol index f841808..4be7f60 100644 --- a/test/unit/EmailRecoveryManager/removeGuardian.t.sol +++ b/test/unit/EmailRecoveryManager/removeGuardian.t.sol @@ -17,6 +17,7 @@ contract EmailRecoveryManager_removeGuardian_Test is UnitBase { address guardian = guardian1; acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); vm.warp(12 seconds); handleRecovery(recoveryModuleAddress, calldataHash, accountSalt1); @@ -49,6 +50,7 @@ contract EmailRecoveryManager_removeGuardian_Test is UnitBase { emailRecoveryManager.getGuardianConfig(accountAddress); assertEq(guardianConfig.guardianCount, guardians.length - 1); assertEq(guardianConfig.totalWeight, totalWeight - guardianWeights[0]); + assertEq(guardianConfig.acceptedWeight, 0); // 1 - 1 = 0 assertEq(guardianConfig.threshold, threshold); } } diff --git a/test/unit/EmailRecoveryManager/setupGuardians.t.sol b/test/unit/EmailRecoveryManager/setupGuardians.t.sol index 909ddaa..d06118c 100644 --- a/test/unit/EmailRecoveryManager/setupGuardians.t.sol +++ b/test/unit/EmailRecoveryManager/setupGuardians.t.sol @@ -45,6 +45,7 @@ contract EmailRecoveryManager_setupGuardians_Test is UnitBase { emailRecoveryManager.getGuardianConfig(accountAddress); assertEq(guardianConfig.guardianCount, expectedGuardianCount); assertEq(guardianConfig.totalWeight, expectedTotalWeight); + assertEq(guardianConfig.acceptedWeight, 0); // no guardians accepted assertEq(guardianConfig.threshold, expectedThreshold); } } diff --git a/test/unit/assertErrorSelectors.t.sol b/test/unit/assertErrorSelectors.t.sol index 98de19a..6cbc872 100644 --- a/test/unit/assertErrorSelectors.t.sol +++ b/test/unit/assertErrorSelectors.t.sol @@ -9,7 +9,6 @@ import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol" import { UniversalEmailRecoveryModule } from "src/modules/UniversalEmailRecoveryModule.sol"; import { EnumerableGuardianMap } from "src/libraries/EnumerableGuardianMap.sol"; import { GuardianUtils } from "src/libraries/GuardianUtils.sol"; -import { OwnableValidator } from "src/test/OwnableValidator.sol"; // Helper test to associate custom error bytes with error names. Could just write the selector bytes // in the contracts but this method reduces human error from copying values and also when updating @@ -39,6 +38,7 @@ contract LogErrorSelectors_Test is Test { assertEq(IEmailRecoveryManager.DelayMoreThanExpiry.selector, bytes4(0x655a4874)); assertEq(IEmailRecoveryManager.RecoveryWindowTooShort.selector, bytes4(0x12fa0714)); assertEq(IEmailRecoveryManager.InvalidTemplateIndex.selector, bytes4(0x5abe71c9)); + assertEq(IEmailRecoveryManager.ThresholdExceedsAcceptedWeight.selector, bytes4(0x8f8457ba)); assertEq(IEmailRecoveryManager.InvalidGuardianStatus.selector, bytes4(0x5689b51a)); assertEq(IEmailRecoveryManager.InvalidAccountAddress.selector, bytes4(0x401b6ade)); assertEq(IEmailRecoveryManager.NoRecoveryConfigured.selector, bytes4(0xa66e66b6)); @@ -70,7 +70,7 @@ contract LogErrorSelectors_Test is Test { assertEq(GuardianUtils.InvalidGuardianAddress.selector, bytes4(0x1b081054)); assertEq(GuardianUtils.InvalidGuardianWeight.selector, bytes4(0x148f78e0)); assertEq(GuardianUtils.AddressAlreadyGuardian.selector, bytes4(0xe4e1614e)); - assertEq(GuardianUtils.ThresholdCannotExceedTotalWeight.selector, bytes4(0x717c498a)); + assertEq(GuardianUtils.ThresholdExceedsTotalWeight.selector, bytes4(0x3c7a2aad)); assertEq(GuardianUtils.StatusCannotBeTheSame.selector, bytes4(0x115e823f)); assertEq(GuardianUtils.SetupNotCalled.selector, bytes4(0xae69115b)); assertEq(GuardianUtils.UnauthorizedAccountForGuardian.selector, bytes4(0xe4c3248f)); diff --git a/test/unit/libraries/GuardianUtils/addGuardian.t.sol b/test/unit/libraries/GuardianUtils/addGuardian.t.sol index 143d71f..ce6df84 100644 --- a/test/unit/libraries/GuardianUtils/addGuardian.t.sol +++ b/test/unit/libraries/GuardianUtils/addGuardian.t.sol @@ -63,6 +63,7 @@ contract GuardianUtils_addGuardian_Test is UnitBase { uint256 expectedGuardianCount = guardians.length + 1; uint256 expectedTotalWeight = totalWeight + newGuardianWeight; + uint256 expectedAcceptedWeight = 0; // no guardians accepted uint256 expectedThreshold = threshold; // same threshold vm.startPrank(accountAddress); @@ -79,6 +80,7 @@ contract GuardianUtils_addGuardian_Test is UnitBase { emailRecoveryManager.getGuardianConfig(accountAddress); assertEq(guardianConfig.guardianCount, expectedGuardianCount); assertEq(guardianConfig.totalWeight, expectedTotalWeight); + assertEq(guardianConfig.acceptedWeight, expectedAcceptedWeight); assertEq(guardianConfig.threshold, expectedThreshold); } } diff --git a/test/unit/libraries/GuardianUtils/changeThreshold.t.sol b/test/unit/libraries/GuardianUtils/changeThreshold.t.sol index be21cbc..aa39986 100644 --- a/test/unit/libraries/GuardianUtils/changeThreshold.t.sol +++ b/test/unit/libraries/GuardianUtils/changeThreshold.t.sol @@ -20,7 +20,7 @@ contract GuardianUtils_changeThreshold_Test is UnitBase { uint256 highThreshold = totalWeight + 1; vm.startPrank(accountAddress); - vm.expectRevert(GuardianUtils.ThresholdCannotExceedTotalWeight.selector); + vm.expectRevert(GuardianUtils.ThresholdExceedsTotalWeight.selector); emailRecoveryManager.changeThreshold(highThreshold); } diff --git a/test/unit/libraries/GuardianUtils/removeGuardian.t.sol b/test/unit/libraries/GuardianUtils/removeGuardian.t.sol index ae64fe1..aaefb7e 100644 --- a/test/unit/libraries/GuardianUtils/removeGuardian.t.sol +++ b/test/unit/libraries/GuardianUtils/removeGuardian.t.sol @@ -34,7 +34,7 @@ contract GuardianUtils_removeGuardian_Test is UnitBase { acceptGuardian(accountSalt1); vm.startPrank(accountAddress); - vm.expectRevert(GuardianUtils.ThresholdCannotExceedTotalWeight.selector); + vm.expectRevert(GuardianUtils.ThresholdExceedsTotalWeight.selector); emailRecoveryManager.removeGuardian(guardian); } @@ -48,7 +48,34 @@ contract GuardianUtils_removeGuardian_Test is UnitBase { // (totalWeight - weight == 4 - 1) = 3 // (weight < threshold == 3 < 3) = succeeds - acceptGuardian(accountSalt1); + vm.startPrank(accountAddress); + emailRecoveryManager.removeGuardian(guardian); + + GuardianStorage memory guardianStorage = + emailRecoveryManager.getGuardian(accountAddress, guardian); + assertEq(uint256(guardianStorage.status), uint256(GuardianStatus.NONE)); + assertEq(guardianStorage.weight, 0); + + IEmailRecoveryManager.GuardianConfig memory guardianConfig = + emailRecoveryManager.getGuardianConfig(accountAddress); + assertEq(guardianConfig.guardianCount, guardians.length - 1); + assertEq(guardianConfig.totalWeight, totalWeight - guardianWeights[0]); + assertEq(guardianConfig.acceptedWeight, 0); + assertEq(guardianConfig.threshold, threshold); + } + + function test_RemoveGuardian_SucceedsWithAcceptedGuardian() public { + address guardian = guardian1; // guardian 1 weight is 1 + // threshold = 3 + // totalWeight = 4 + // weight = 1 + + // Fails if totalWeight - weight < threshold + // (totalWeight - weight == 4 - 1) = 3 + // (weight < threshold == 3 < 3) = succeeds + + acceptGuardian(accountSalt1); // weight = 1 + acceptGuardian(accountSalt2); // weight = 2 vm.startPrank(accountAddress); emailRecoveryManager.removeGuardian(guardian); @@ -62,6 +89,10 @@ contract GuardianUtils_removeGuardian_Test is UnitBase { emailRecoveryManager.getGuardianConfig(accountAddress); assertEq(guardianConfig.guardianCount, guardians.length - 1); assertEq(guardianConfig.totalWeight, totalWeight - guardianWeights[0]); + + // Accepted weight before guardian is removed = 3 + // acceptedWeight = 3 - 1 + assertEq(guardianConfig.acceptedWeight, 2); assertEq(guardianConfig.threshold, threshold); } } diff --git a/test/unit/libraries/GuardianUtils/setupGuardians.t.sol b/test/unit/libraries/GuardianUtils/setupGuardians.t.sol index 476cd60..db4ef46 100644 --- a/test/unit/libraries/GuardianUtils/setupGuardians.t.sol +++ b/test/unit/libraries/GuardianUtils/setupGuardians.t.sol @@ -81,7 +81,7 @@ contract GuardianUtils_setupGuardians_Test is UnitBase { instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); vm.stopPrank(); - vm.expectRevert(GuardianUtils.ThresholdCannotExceedTotalWeight.selector); + vm.expectRevert(GuardianUtils.ThresholdExceedsTotalWeight.selector); emailRecoveryManager.exposed_setupGuardians( accountAddress, guardians, guardianWeights, invalidThreshold ); @@ -90,6 +90,7 @@ contract GuardianUtils_setupGuardians_Test is UnitBase { function test_SetupGuardians_Succeeds() public { uint256 expectedGuardianCount = guardians.length; uint256 expectedTotalWeight = totalWeight; + uint256 expectedAcceptedWeight = 0; // no guardians accepted uint256 expectedThreshold = threshold; vm.prank(accountAddress); @@ -117,6 +118,7 @@ contract GuardianUtils_setupGuardians_Test is UnitBase { emailRecoveryManager.getGuardianConfig(accountAddress); assertEq(guardianConfig.guardianCount, expectedGuardianCount); assertEq(guardianConfig.totalWeight, expectedTotalWeight); + assertEq(guardianConfig.acceptedWeight, expectedAcceptedWeight); assertEq(guardianConfig.threshold, expectedThreshold); } } From 06bf75534a9aa944715f82fad1cc549689aa3fbd Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 17 Jul 2024 12:52:37 +0100 Subject: [PATCH 03/30] M1: templateIdx function parameter check is in incorrect place --- src/EmailRecoveryManager.sol | 8 ------- src/handlers/EmailRecoverySubjectHandler.sol | 19 +++++++++++++---- src/handlers/SafeRecoverySubjectHandler.sol | 21 ++++++++++++++----- src/interfaces/IEmailRecoveryManager.sol | 1 - .../EmailRecoveryManager/acceptGuardian.t.sol | 9 -------- .../processRecovery.t.sol | 9 -------- test/unit/assertErrorSelectors.t.sol | 3 ++- .../validateAcceptanceSubject.t.sol | 9 ++++++++ .../validateRecoverySubject.t.sol | 9 ++++++++ .../validateAcceptanceSubject.t.sol | 10 +++++++++ .../validateRecoverySubject.t.sol | 10 +++++++++ 11 files changed, 71 insertions(+), 37 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 1d972f3..5512fd8 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -304,10 +304,6 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco internal override { - if (templateIdx != 0) { - revert InvalidTemplateIndex(); - } - address account = IEmailRecoverySubjectHandler(subjectHandler).validateAcceptanceSubject( templateIdx, subjectParams ); @@ -354,10 +350,6 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco internal override { - if (templateIdx != 0) { - revert InvalidTemplateIndex(); - } - (address account, bytes32 calldataHash) = IEmailRecoverySubjectHandler(subjectHandler) .validateRecoverySubject(templateIdx, subjectParams, address(this)); diff --git a/src/handlers/EmailRecoverySubjectHandler.sol b/src/handlers/EmailRecoverySubjectHandler.sol index c7e0c02..af72d80 100644 --- a/src/handlers/EmailRecoverySubjectHandler.sol +++ b/src/handlers/EmailRecoverySubjectHandler.sol @@ -10,6 +10,7 @@ import { StringUtils } from "../libraries/StringUtils.sol"; * This is the default subject handler that will work with any validator. */ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { + error InvalidTemplateIndex(); error InvalidSubjectParams(); error InvalidAccount(); error InvalidRecoveryModule(); @@ -88,18 +89,24 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { /** * @notice Validates the subject params for an acceptance email + * @param templateIdx The index of the template used for acceptance * @param subjectParams The subject parameters of the recovery email. * @return accountInEmail The account address in the acceptance email */ function validateAcceptanceSubject( - uint256, /* templateIdx */ + uint256 templateIdx, bytes[] calldata subjectParams ) external pure returns (address) { - if (subjectParams.length != 1) revert InvalidSubjectParams(); + if (templateIdx != 0) { + revert InvalidTemplateIndex(); + } + if (subjectParams.length != 1) { + revert InvalidSubjectParams(); + } // The GuardianStatus check in acceptGuardian implicitly // validates the account, so no need to re-validate here @@ -110,14 +117,15 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { /** * @notice Validates the subject params for an acceptance email - * @param subjectParams The subject parameters of the recovery email. + * @param templateIdx The index of the template used for the recovery request + * @param subjectParams The subject parameters of the recovery email * @param recoveryManager The recovery manager address. Used to help with validation * @return accountInEmail The account address in the acceptance email * @return calldataHash The keccak256 hash of the recovery calldata. Verified against later when * recovery is executed */ function validateRecoverySubject( - uint256, /* templateIdx */ + uint256 templateIdx, bytes[] calldata subjectParams, address recoveryManager ) @@ -125,6 +133,9 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { view returns (address, bytes32) { + if (templateIdx != 0) { + revert InvalidTemplateIndex(); + } if (subjectParams.length != 3) { revert InvalidSubjectParams(); } diff --git a/src/handlers/SafeRecoverySubjectHandler.sol b/src/handlers/SafeRecoverySubjectHandler.sol index 9eada9e..8dc55c1 100644 --- a/src/handlers/SafeRecoverySubjectHandler.sol +++ b/src/handlers/SafeRecoverySubjectHandler.sol @@ -14,6 +14,7 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { using Strings for uint256; error InvalidSubjectParams(); + error InvalidTemplateIndex(); error InvalidOldOwner(); error InvalidNewOwner(); error InvalidRecoveryModule(); @@ -96,18 +97,24 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { /** * @notice Validates the subject params for an acceptance email - * @param subjectParams The subject parameters of the recovery email. + * @param templateIdx The index of the template used for acceptance + * @param subjectParams The subject parameters of the recovery email * @return accountInEmail The account address in the acceptance email */ function validateAcceptanceSubject( - uint256, /* templateIdx */ + uint256 templateIdx, bytes[] calldata subjectParams ) external pure returns (address) { - if (subjectParams.length != 1) revert InvalidSubjectParams(); + if (templateIdx != 0) { + revert InvalidTemplateIndex(); + } + if (subjectParams.length != 1) { + revert InvalidSubjectParams(); + } // The GuardianStatus check in acceptGuardian implicitly // validates the account, so no need to re-validate here @@ -118,14 +125,15 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { /** * @notice Validates the subject params for an acceptance email - * @param subjectParams The subject parameters of the recovery email. + * @param templateIdx The index of the template used for the recovery request + * @param subjectParams The subject parameters of the recovery email * @param recoveryManager The recovery manager address. Used to help with validation * @return accountInEmail The account address in the acceptance email * @return calldataHash The keccak256 hash of the recovery calldata. Verified against later when * recovery is executed */ function validateRecoverySubject( - uint256, /* templateIdx */ + uint256 templateIdx, bytes[] calldata subjectParams, address recoveryManager ) @@ -133,6 +141,9 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { view returns (address, bytes32) { + if (templateIdx != 0) { + revert InvalidTemplateIndex(); + } if (subjectParams.length != 4) { revert InvalidSubjectParams(); } diff --git a/src/interfaces/IEmailRecoveryManager.sol b/src/interfaces/IEmailRecoveryManager.sol index 7a835dd..3eab8f4 100644 --- a/src/interfaces/IEmailRecoveryManager.sol +++ b/src/interfaces/IEmailRecoveryManager.sol @@ -75,7 +75,6 @@ interface IEmailRecoveryManager { error RecoveryModuleNotAuthorized(); error DelayMoreThanExpiry(); error RecoveryWindowTooShort(); - error InvalidTemplateIndex(); error ThresholdExceedsAcceptedWeight(); error InvalidGuardianStatus( GuardianStatus guardianStatus, GuardianStatus expectedGuardianStatus diff --git a/test/unit/EmailRecoveryManager/acceptGuardian.t.sol b/test/unit/EmailRecoveryManager/acceptGuardian.t.sol index 546a162..2ca32bb 100644 --- a/test/unit/EmailRecoveryManager/acceptGuardian.t.sol +++ b/test/unit/EmailRecoveryManager/acceptGuardian.t.sol @@ -23,15 +23,6 @@ contract EmailRecoveryManager_acceptGuardian_Test is UnitBase { nullifier = keccak256(abi.encode("nullifier 1")); } - function test_AcceptGuardian_RevertWhen_InvalidTemplateIndex() public { - uint256 invalidTemplateIdx = 1; - - vm.expectRevert(IEmailRecoveryManager.InvalidTemplateIndex.selector); - emailRecoveryManager.exposed_acceptGuardian( - guardian1, invalidTemplateIdx, subjectParams, nullifier - ); - } - function test_AcceptGuardian_RevertWhen_AlreadyRecovering() public { acceptGuardian(accountSalt1); acceptGuardian(accountSalt2); diff --git a/test/unit/EmailRecoveryManager/processRecovery.t.sol b/test/unit/EmailRecoveryManager/processRecovery.t.sol index e2a8ff4..d9b8991 100644 --- a/test/unit/EmailRecoveryManager/processRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/processRecovery.t.sol @@ -29,15 +29,6 @@ contract EmailRecoveryManager_processRecovery_Test is UnitBase { nullifier = keccak256(abi.encode("nullifier 1")); } - function test_ProcessRecovery_RevertWhen_InvalidTemplateIndex() public { - uint256 invalidTemplateIdx = 1; - - vm.expectRevert(IEmailRecoveryManager.InvalidTemplateIndex.selector); - emailRecoveryManager.exposed_processRecovery( - guardian1, invalidTemplateIdx, subjectParams, nullifier - ); - } - function test_ProcessRecovery_RevertWhen_GuardianStatusIsNONE() public { address invalidGuardian = address(1); diff --git a/test/unit/assertErrorSelectors.t.sol b/test/unit/assertErrorSelectors.t.sol index 6cbc872..36bab60 100644 --- a/test/unit/assertErrorSelectors.t.sol +++ b/test/unit/assertErrorSelectors.t.sol @@ -15,12 +15,14 @@ import { GuardianUtils } from "src/libraries/GuardianUtils.sol"; // errors contract LogErrorSelectors_Test is Test { function test_EmailRecoverySubjectHandler_AssertSelectors() public { + assertEq(EmailRecoverySubjectHandler.InvalidTemplateIndex.selector, bytes4(0x5abe71c9)); assertEq(EmailRecoverySubjectHandler.InvalidSubjectParams.selector, bytes4(0xd743ae6c)); assertEq(EmailRecoverySubjectHandler.InvalidAccount.selector, bytes4(0x6d187b28)); assertEq(EmailRecoverySubjectHandler.InvalidRecoveryModule.selector, bytes4(0x7f263111)); } function test_SafeRecoverySubjectHandler_AssertSelectors() public { + assertEq(SafeRecoverySubjectHandler.InvalidTemplateIndex.selector, bytes4(0x5abe71c9)); assertEq(SafeRecoverySubjectHandler.InvalidSubjectParams.selector, bytes4(0xd743ae6c)); assertEq(SafeRecoverySubjectHandler.InvalidOldOwner.selector, bytes4(0xa9ab2692)); assertEq(SafeRecoverySubjectHandler.InvalidNewOwner.selector, bytes4(0x54a56786)); @@ -37,7 +39,6 @@ contract LogErrorSelectors_Test is Test { assertEq(IEmailRecoveryManager.RecoveryModuleNotAuthorized.selector, bytes4(0xbfa7d2a9)); assertEq(IEmailRecoveryManager.DelayMoreThanExpiry.selector, bytes4(0x655a4874)); assertEq(IEmailRecoveryManager.RecoveryWindowTooShort.selector, bytes4(0x12fa0714)); - assertEq(IEmailRecoveryManager.InvalidTemplateIndex.selector, bytes4(0x5abe71c9)); assertEq(IEmailRecoveryManager.ThresholdExceedsAcceptedWeight.selector, bytes4(0x8f8457ba)); assertEq(IEmailRecoveryManager.InvalidGuardianStatus.selector, bytes4(0x5689b51a)); assertEq(IEmailRecoveryManager.InvalidAccountAddress.selector, bytes4(0x401b6ade)); diff --git a/test/unit/handlers/EmailRecoverySubjectHandler/validateAcceptanceSubject.t.sol b/test/unit/handlers/EmailRecoverySubjectHandler/validateAcceptanceSubject.t.sol index 7a85a52..14aa714 100644 --- a/test/unit/handlers/EmailRecoverySubjectHandler/validateAcceptanceSubject.t.sol +++ b/test/unit/handlers/EmailRecoverySubjectHandler/validateAcceptanceSubject.t.sol @@ -10,6 +10,15 @@ contract EmailRecoverySubjectHandler_validateAcceptanceSubject_Test is UnitBase super.setUp(); } + function test_ValidateAcceptanceSubject_RevertWhen_InvalidTemplateIndex() public { + bytes[] memory subjectParams = new bytes[](1); + subjectParams[0] = abi.encode(accountAddress); + uint256 invalidTemplateIdx = 1; + + vm.expectRevert(EmailRecoverySubjectHandler.InvalidTemplateIndex.selector); + emailRecoveryHandler.validateAcceptanceSubject(invalidTemplateIdx, subjectParams); + } + function test_ValidateAcceptanceSubject_RevertWhen_NoSubjectParams() public { bytes[] memory emptySubjectParams; diff --git a/test/unit/handlers/EmailRecoverySubjectHandler/validateRecoverySubject.t.sol b/test/unit/handlers/EmailRecoverySubjectHandler/validateRecoverySubject.t.sol index ff679b1..6d91a40 100644 --- a/test/unit/handlers/EmailRecoverySubjectHandler/validateRecoverySubject.t.sol +++ b/test/unit/handlers/EmailRecoverySubjectHandler/validateRecoverySubject.t.sol @@ -23,6 +23,15 @@ contract EmailRecoverySubjectHandler_validateRecoverySubject_Test is UnitBase { subjectParams[2] = abi.encode(calldataHashString); } + function test_ValidateRecoverySubject_RevertWhen_InvalidTemplateIndex() public { + uint256 invalidTemplateIdx = 1; + + vm.expectRevert(EmailRecoverySubjectHandler.InvalidTemplateIndex.selector); + emailRecoveryHandler.validateRecoverySubject( + invalidTemplateIdx, subjectParams, emailRecoveryManagerAddress + ); + } + function test_ValidateRecoverySubject_RevertWhen_NoSubjectParams() public { bytes[] memory emptySubjectParams; diff --git a/test/unit/handlers/SafeRecoverySubjectHandler/validateAcceptanceSubject.t.sol b/test/unit/handlers/SafeRecoverySubjectHandler/validateAcceptanceSubject.t.sol index b277c6c..d39e380 100644 --- a/test/unit/handlers/SafeRecoverySubjectHandler/validateAcceptanceSubject.t.sol +++ b/test/unit/handlers/SafeRecoverySubjectHandler/validateAcceptanceSubject.t.sol @@ -10,6 +10,16 @@ contract SafeRecoverySubjectHandler_validateAcceptanceSubject_Test is SafeUnitBa super.setUp(); } + function test_ValidateAcceptanceSubject_RevertWhen_InvalidTemplateIndex() public { + skipIfNotSafeAccountType(); + bytes[] memory subjectParams = new bytes[](1); + subjectParams[0] = abi.encode(accountAddress1); + uint256 invalidTemplateIdx = 1; + + vm.expectRevert(SafeRecoverySubjectHandler.InvalidTemplateIndex.selector); + safeRecoverySubjectHandler.validateAcceptanceSubject(invalidTemplateIdx, subjectParams); + } + function test_ValidateAcceptanceSubject_RevertWhen_NoSubjectParams() public { skipIfNotSafeAccountType(); bytes[] memory emptySubjectParams; diff --git a/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol b/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol index 3de7445..cdae48e 100644 --- a/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol +++ b/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol @@ -21,6 +21,16 @@ contract SafeRecoverySubjectHandler_validateRecoverySubject_Test is SafeUnitBase subjectParams[3] = abi.encode(recoveryModuleAddress); } + function test_ValidateRecoverySubject_RevertWhen_InvalidTemplateIndex() public { + skipIfNotSafeAccountType(); + uint256 invalidTemplateIdx = 1; + + vm.expectRevert(SafeRecoverySubjectHandler.InvalidTemplateIndex.selector); + safeRecoverySubjectHandler.validateRecoverySubject( + invalidTemplateIdx, subjectParams, emailRecoveryManagerAddress + ); + } + function test_ValidateAcceptanceSubject_RevertWhen_NoSubjectParams() public { skipIfNotSafeAccountType(); bytes[] memory emptySubjectParams; From ba217b696e75c115f8aa7eb1d6207a8efd5515be Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Thu, 18 Jul 2024 11:40:35 +0100 Subject: [PATCH 04/30] M2: Maximum guardians DoS --- src/libraries/EnumerableGuardianMap.sol | 8 +++--- .../libraries/EnumerableGuardianMap/set.t.sol | 25 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/libraries/EnumerableGuardianMap.sol b/src/libraries/EnumerableGuardianMap.sol index 5f78685..3cf7b28 100644 --- a/src/libraries/EnumerableGuardianMap.sol +++ b/src/libraries/EnumerableGuardianMap.sol @@ -67,12 +67,14 @@ library EnumerableGuardianMap { internal returns (bool) { + map._values[key] = value; + bool success = map._keys.add(key); + uint256 length = map._keys.length(); - if (length >= MAX_NUMBER_OF_GUARDIANS) { + if (success && length > MAX_NUMBER_OF_GUARDIANS) { revert MaxNumberOfGuardiansReached(); } - map._values[key] = value; - return map._keys.add(key); + return success; } /** diff --git a/test/unit/libraries/EnumerableGuardianMap/set.t.sol b/test/unit/libraries/EnumerableGuardianMap/set.t.sol index e6d761a..7ce9797 100644 --- a/test/unit/libraries/EnumerableGuardianMap/set.t.sol +++ b/test/unit/libraries/EnumerableGuardianMap/set.t.sol @@ -106,4 +106,29 @@ contract EnumerableGuardianMap_set_Test is UnitBase { value: GuardianStorage(GuardianStatus.REQUESTED, guardianWeights[1]) }); } + + function test_Set_UpdatesValueWhenMaxNumberOfGuardiansReached() public { + bool result; + for (uint256 i = 1; i <= 32; i++) { + result = guardiansStorage[accountAddress].set({ + key: vm.addr(i), + value: GuardianStorage(GuardianStatus.REQUESTED, guardianWeights[1]) + }); + assertEq(result, true); + } + + bool success = guardiansStorage[accountAddress].set({ + key: vm.addr(1), // update first guardian added in loop + value: GuardianStorage(GuardianStatus.ACCEPTED, guardianWeights[0]) + }); + assertEq(success, false); + require( + guardiansStorage[accountAddress]._values[vm.addr(1)].status == GuardianStatus.ACCEPTED, + "Expected status to be ACCEPTED" + ); + require( + guardiansStorage[accountAddress]._values[vm.addr(1)].weight == guardianWeights[0], + "Expected weight to be 1" + ); + } } From 5f1fc9e9a8a356ba05af465dee921befe4085879 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Thu, 18 Jul 2024 14:42:23 +0100 Subject: [PATCH 05/30] M4: MAX + 1 validators may be configured in UniversalEmailRecoveryModule --- src/modules/UniversalEmailRecoveryModule.sol | 2 +- .../allowValidatorRecovery.t.sol | 30 ++++++++++++++++++- .../disallowValidatorRecovery.t.sol | 4 +-- .../getAllowedSelectors.t.sol | 2 +- .../getAllowedValidators.t.sol | 2 +- 5 files changed, 34 insertions(+), 6 deletions(-) diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index 947c8a6..6d5b061 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -148,7 +148,7 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec revert InvalidValidator(validator); } - if (validatorCount[msg.sender] > MAX_VALIDATORS) { + if (validatorCount[msg.sender] >= MAX_VALIDATORS) { revert MaxValidatorsReached(); } validators[msg.sender].push(validator); diff --git a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol index 9084b4e..e808e4a 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol @@ -66,6 +66,34 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { emailRecoveryModule.allowValidatorRecovery(validatorAddress, "", functionSelector); } + function test_AllowValidatorRecovery_RevertWhen_MaxValidatorsReached() public { + vm.startPrank(accountAddress); + + // One validator is already installed from setup + for (uint256 i = 1; i <= 31; i++) { + address newValidatorAddress = address(new OwnableValidator()); + instance.installModule({ + moduleTypeId: MODULE_TYPE_VALIDATOR, + module: newValidatorAddress, + data: abi.encode(owner) + }); + emailRecoveryModule.allowValidatorRecovery(newValidatorAddress, "", functionSelector); + } + + vm.startPrank(accountAddress); + address newValidatorAddress = address(new OwnableValidator()); + instance.installModule({ + moduleTypeId: MODULE_TYPE_VALIDATOR, + module: newValidatorAddress, + data: abi.encode(owner) + }); + vm.expectRevert(UniversalEmailRecoveryModule.MaxValidatorsReached.selector); + emailRecoveryModule.allowValidatorRecovery(newValidatorAddress, "", functionSelector); + + uint256 validatorCount = emailRecoveryModule.validatorCount(accountAddress); + assertEq(validatorCount, 32); + } + function test_AllowValidatorRecovery_SucceedsWhenAlreadyInitialized() public { // Deplopy and install new validator OwnableValidator newValidator = new OwnableValidator(); @@ -73,7 +101,7 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { instance.installModule({ moduleTypeId: MODULE_TYPE_VALIDATOR, module: newValidatorAddress, - data: abi.encode(owner, recoveryModuleAddress) + data: abi.encode(owner) }); vm.startPrank(accountAddress); diff --git a/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol index 5220bfc..19f89a5 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol @@ -60,7 +60,7 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase instance.installModule({ moduleTypeId: MODULE_TYPE_VALIDATOR, module: newValidatorAddress, - data: abi.encode(owner, recoveryModuleAddress) + data: abi.encode(owner) }); address[] memory allowedValidators = @@ -124,7 +124,7 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase instance.installModule({ moduleTypeId: MODULE_TYPE_VALIDATOR, module: newValidatorAddress, - data: abi.encode(owner, recoveryModuleAddress) + data: abi.encode(owner) }); vm.startPrank(accountAddress); diff --git a/test/unit/modules/UniversalEmailRecoveryModule/getAllowedSelectors.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/getAllowedSelectors.t.sol index 0d45e2c..2922fc0 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/getAllowedSelectors.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/getAllowedSelectors.t.sol @@ -28,7 +28,7 @@ contract UniversalEmailRecoveryModule_getAllowedSelectors_Test is UnitBase { instance.installModule({ moduleTypeId: MODULE_TYPE_VALIDATOR, module: newValidatorAddress, - data: abi.encode(owner, recoveryModuleAddress) + data: abi.encode(owner) }); bytes4 newFunctionSelector = bytes4(keccak256(bytes("rotateOwner(address,address)"))); diff --git a/test/unit/modules/UniversalEmailRecoveryModule/getAllowedValidators.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/getAllowedValidators.t.sol index 01bbdf2..323f22b 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/getAllowedValidators.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/getAllowedValidators.t.sol @@ -47,7 +47,7 @@ contract UniversalEmailRecoveryModule_getAllowedValidators_Test is UnitBase { instance.installModule({ moduleTypeId: MODULE_TYPE_VALIDATOR, module: newValidatorAddress, - data: abi.encode(owner, recoveryModuleAddress) + data: abi.encode(owner) }); bytes4 newFunctionSelector = bytes4(keccak256(bytes("rotateOwner(address,address)"))); From ad4fc63980686bc23f2bb7b2a5721ce2f562bb84 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Fri, 19 Jul 2024 09:25:23 +0100 Subject: [PATCH 06/30] Fix simulate test bug --- .../allowValidatorRecovery.t.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol index e808e4a..4db7ba0 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol @@ -67,8 +67,6 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { } function test_AllowValidatorRecovery_RevertWhen_MaxValidatorsReached() public { - vm.startPrank(accountAddress); - // One validator is already installed from setup for (uint256 i = 1; i <= 31; i++) { address newValidatorAddress = address(new OwnableValidator()); @@ -77,16 +75,18 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { module: newValidatorAddress, data: abi.encode(owner) }); + vm.startPrank(accountAddress); emailRecoveryModule.allowValidatorRecovery(newValidatorAddress, "", functionSelector); + vm.stopPrank(); } - vm.startPrank(accountAddress); address newValidatorAddress = address(new OwnableValidator()); instance.installModule({ moduleTypeId: MODULE_TYPE_VALIDATOR, module: newValidatorAddress, data: abi.encode(owner) }); + vm.startPrank(accountAddress); vm.expectRevert(UniversalEmailRecoveryModule.MaxValidatorsReached.selector); emailRecoveryModule.allowValidatorRecovery(newValidatorAddress, "", functionSelector); From 9b235d8eee7869fcaf42934882491664d5c242f4 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Fri, 19 Jul 2024 09:57:00 +0100 Subject: [PATCH 07/30] L1: Validators can be added/removed before module initialization in UniversalEmailRecovery --- src/modules/UniversalEmailRecoveryModule.sol | 15 ++++++++++- .../configureRecovery.t.sol | 16 +++--------- test/unit/UnitBase.t.sol | 2 +- .../UniversalEmailRecoveryModuleHarness.sol | 4 +++ .../EmailRecoveryModuleBase.t.sol | 2 +- .../allowValidatorRecovery.t.sol | 25 +++++++++++++++++-- .../disallowValidatorRecovery.t.sol | 17 ++++++++++--- 7 files changed, 61 insertions(+), 20 deletions(-) diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index 6d5b061..548ad1c 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -41,6 +41,7 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec event RecoveryExecuted(); error InvalidSelector(bytes4 selector); + error RecoveryModuleNotInitialized(); error InvalidOnInstallData(); error InvalidValidator(address validator); error MaxValidatorsReached(); @@ -85,6 +86,16 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec _; } + /** + * @notice Modifier to check whether the recovery module is initialized + */ + modifier onlyWhenInitialized() { + if (!validators[msg.sender].alreadyInitialized()) { + revert RecoveryModuleNotInitialized(); + } + _; + } + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ /* CONFIG */ /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ @@ -138,6 +149,7 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec bytes4 recoverySelector ) public + onlyWhenInitialized withoutUnsafeSelector(recoverySelector) { if ( @@ -175,6 +187,7 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec bytes4 recoverySelector ) public + onlyWhenInitialized { if ( !IERC7579Account(msg.sender).isModuleInstalled( @@ -224,7 +237,7 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec * @param smartAccount The smart account to check * @return true if the module is initialized, false otherwise */ - function isInitialized(address smartAccount) external view returns (bool) { + function isInitialized(address smartAccount) public view returns (bool) { return IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(smartAccount).threshold != 0; } diff --git a/test/unit/EmailRecoveryManager/configureRecovery.t.sol b/test/unit/EmailRecoveryManager/configureRecovery.t.sol index f03964f..fbce2ed 100644 --- a/test/unit/EmailRecoveryManager/configureRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/configureRecovery.t.sol @@ -85,9 +85,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { function test_ConfigureRecovery_RevertWhen_ZeroGuardians() public { instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); vm.startPrank(accountAddress); - emailRecoveryModule.allowValidatorRecovery( - validatorAddress, isInstalledContext, functionSelector - ); + emailRecoveryModule.workaround_validatorsPush(accountAddress, validatorAddress); address[] memory zeroGuardians; vm.expectRevert(GuardianUtils.IncorrectNumberOfWeights.selector); @@ -99,9 +97,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { function test_ConfigureRecovery_RevertWhen_ZeroGuardianWeights() public { instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); vm.startPrank(accountAddress); - emailRecoveryModule.allowValidatorRecovery( - validatorAddress, isInstalledContext, functionSelector - ); + emailRecoveryModule.workaround_validatorsPush(accountAddress, validatorAddress); uint256[] memory zeroGuardianWeights; vm.expectRevert(GuardianUtils.IncorrectNumberOfWeights.selector); @@ -113,9 +109,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { function test_ConfigureRecovery_RevertWhen_ZeroThreshold() public { instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); vm.startPrank(accountAddress); - emailRecoveryModule.allowValidatorRecovery( - validatorAddress, isInstalledContext, functionSelector - ); + emailRecoveryModule.workaround_validatorsPush(accountAddress, validatorAddress); uint256 zeroThreshold = 0; vm.expectRevert(GuardianUtils.ThresholdCannotBeZero.selector); @@ -127,9 +121,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { function test_ConfigureRecovery_RevetrWhen_NoGuardians() public { instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); vm.startPrank(accountAddress); - emailRecoveryModule.allowValidatorRecovery( - validatorAddress, isInstalledContext, functionSelector - ); + emailRecoveryModule.workaround_validatorsPush(accountAddress, validatorAddress); address[] memory zeroGuardians; uint256[] memory zeroGuardianWeights; diff --git a/test/unit/UnitBase.t.sol b/test/unit/UnitBase.t.sol index ec58a2e..79c64f8 100644 --- a/test/unit/UnitBase.t.sol +++ b/test/unit/UnitBase.t.sol @@ -169,7 +169,7 @@ abstract contract UnitBase is RhinestoneModuleKit, Test { instance.installModule({ moduleTypeId: MODULE_TYPE_VALIDATOR, module: validatorAddress, - data: abi.encode(owner, recoveryModuleAddress) + data: abi.encode(owner) }); instance.installModule({ moduleTypeId: MODULE_TYPE_EXECUTOR, diff --git a/test/unit/UniversalEmailRecoveryModuleHarness.sol b/test/unit/UniversalEmailRecoveryModuleHarness.sol index 01021ad..8e1fe67 100644 --- a/test/unit/UniversalEmailRecoveryModuleHarness.sol +++ b/test/unit/UniversalEmailRecoveryModuleHarness.sol @@ -10,6 +10,10 @@ contract UniversalEmailRecoveryModuleHarness is UniversalEmailRecoveryModule { constructor(address emailRecoveryManager) UniversalEmailRecoveryModule(emailRecoveryManager) { } + function workaround_validatorsPush(address account, address validator) external { + validators[account].push(validator); + } + function exposed_allowedSelectors( address validator, address account diff --git a/test/unit/modules/EmailRecoveryModule/EmailRecoveryModuleBase.t.sol b/test/unit/modules/EmailRecoveryModule/EmailRecoveryModuleBase.t.sol index 996eb8f..7212e2c 100644 --- a/test/unit/modules/EmailRecoveryModule/EmailRecoveryModuleBase.t.sol +++ b/test/unit/modules/EmailRecoveryModule/EmailRecoveryModuleBase.t.sol @@ -166,7 +166,7 @@ abstract contract EmailRecoveryModuleBase is RhinestoneModuleKit, Test { instance.installModule({ moduleTypeId: MODULE_TYPE_VALIDATOR, module: validatorAddress, - data: abi.encode(owner, recoveryModuleAddress) + data: abi.encode(owner) }); instance.installModule({ moduleTypeId: MODULE_TYPE_EXECUTOR, diff --git a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol index 4db7ba0..5724f87 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol @@ -17,6 +17,15 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { super.setUp(); } + function test_AllowValidatorRecovery_RevertWhen_RecoveryModuleNotInitialized() public { + // Uninstall module so module is not initialized + instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); + + vm.startPrank(accountAddress); + vm.expectRevert(UniversalEmailRecoveryModule.RecoveryModuleNotInitialized.selector); + emailRecoveryModule.allowValidatorRecovery(validatorAddress, bytes("0"), functionSelector); + } + function test_AllowValidatorRecovery_RevertWhen_UnsafeOnInstallSelector() public { vm.expectRevert( abi.encodeWithSelector( @@ -123,8 +132,20 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { // Uninstall module so state is reset instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); - vm.startPrank(accountAddress); - emailRecoveryModule.allowValidatorRecovery(validatorAddress, "", functionSelector); + instance.installModule( + MODULE_TYPE_EXECUTOR, + recoveryModuleAddress, + abi.encode( + validatorAddress, + isInstalledContext, + functionSelector, + guardians, + guardianWeights, + threshold, + delay, + expiry + ) + ); address[] memory allowedValidators = emailRecoveryModule.getAllowedValidators(accountAddress); diff --git a/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol index 19f89a5..9c13a2c 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.25; import { console2 } from "forge-std/console2.sol"; import { ModuleKitHelpers } from "modulekit/ModuleKit.sol"; -import { MODULE_TYPE_VALIDATOR } from "modulekit/external/ERC7579.sol"; +import { MODULE_TYPE_VALIDATOR, MODULE_TYPE_EXECUTOR } from "modulekit/external/ERC7579.sol"; import { SentinelListLib } from "sentinellist/SentinelList.sol"; import { SentinelListHelper } from "sentinellist/SentinelListHelper.sol"; import { OwnableValidator } from "src/test/OwnableValidator.sol"; @@ -19,6 +19,17 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase super.setUp(); } + function test_DisallowValidatorRecovery_RevertWhen_RecoveryModuleNotInitialized() public { + // Uninstall module so module is not initialized + instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); + + vm.startPrank(accountAddress); + vm.expectRevert(UniversalEmailRecoveryModule.RecoveryModuleNotInitialized.selector); + emailRecoveryModule.disallowValidatorRecovery( + validatorAddress, address(1), bytes("0"), functionSelector + ); + } + function test_DisallowValidatorRecovery_RevertWhen_InvalidValidator() public { address[] memory allowedValidators = emailRecoveryModule.getAllowedValidators(accountAddress); @@ -40,7 +51,7 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase function test_DisallowValidatorRecovery_RevertWhen_InvalidPreviousValidator() public { OwnableValidator newValidator = new OwnableValidator(); - address invalidPreviousOwner = address(newValidator); + address invalidPreviousValidator = address(newValidator); vm.startPrank(accountAddress); vm.expectRevert( @@ -49,7 +60,7 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase ) ); emailRecoveryModule.disallowValidatorRecovery( - validatorAddress, invalidPreviousOwner, "", functionSelector + validatorAddress, invalidPreviousValidator, "", functionSelector ); } From 184f5e5ff77d9aaf0122ce19bd7122e800680cda Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Fri, 19 Jul 2024 10:24:04 +0100 Subject: [PATCH 08/30] L2: Validators cannot be disallowed after being uninstalled in UniversalEmailRecovery --- src/modules/UniversalEmailRecoveryModule.sol | 8 ---- .../disallowValidatorRecovery.t.sol | 37 +++++++++---------- 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index 548ad1c..78cf994 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -189,14 +189,6 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec public onlyWhenInitialized { - if ( - !IERC7579Account(msg.sender).isModuleInstalled( - TYPE_VALIDATOR, validator, isInstalledContext - ) - ) { - revert InvalidValidator(validator); - } - validators[msg.sender].pop(prevValidator, validator); validatorCount[msg.sender]--; diff --git a/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol index 9c13a2c..a224206 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol @@ -30,25 +30,6 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase ); } - function test_DisallowValidatorRecovery_RevertWhen_InvalidValidator() public { - address[] memory allowedValidators = - emailRecoveryModule.getAllowedValidators(accountAddress); - address prevValidator = allowedValidators.findPrevious(validatorAddress); - - OwnableValidator newValidator = new OwnableValidator(); - address newValidatorAddress = address(newValidator); - - vm.expectRevert( - abi.encodeWithSelector( - UniversalEmailRecoveryModule.InvalidValidator.selector, newValidatorAddress - ) - ); - vm.startPrank(accountAddress); - emailRecoveryModule.disallowValidatorRecovery( - newValidatorAddress, prevValidator, bytes("0"), functionSelector - ); - } - function test_DisallowValidatorRecovery_RevertWhen_InvalidPreviousValidator() public { OwnableValidator newValidator = new OwnableValidator(); address invalidPreviousValidator = address(newValidator); @@ -128,6 +109,24 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase assertEq(allowedSelectors.length, 0); } + function test_DisallowValidatorRecovery_SucceedsWhenValidatorUninstalled() public { + instance.uninstallModule(MODULE_TYPE_VALIDATOR, validatorAddress, ""); + + address[] memory allowedValidators = + emailRecoveryModule.getAllowedValidators(accountAddress); + address prevValidator = allowedValidators.findPrevious(validatorAddress); + + vm.startPrank(accountAddress); + emailRecoveryModule.disallowValidatorRecovery( + validatorAddress, prevValidator, bytes("0"), functionSelector + ); + + allowedValidators = emailRecoveryModule.getAllowedValidators(accountAddress); + bytes4[] memory allowedSelectors = emailRecoveryModule.getAllowedSelectors(accountAddress); + assertEq(allowedValidators.length, 0); + assertEq(allowedSelectors.length, 0); + } + function test_DisallowValidatorRecovery_DisallowsCorrectValidatorOutOfMultiple() public { // Deplopy and install new validator OwnableValidator newValidator = new OwnableValidator(); From b1811d43314a9b377a60f35dce907c5f217bae78 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Fri, 19 Jul 2024 10:26:08 +0100 Subject: [PATCH 09/30] Add missing error selectors from helper test --- test/unit/assertErrorSelectors.t.sol | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/unit/assertErrorSelectors.t.sol b/test/unit/assertErrorSelectors.t.sol index 36bab60..769b92b 100644 --- a/test/unit/assertErrorSelectors.t.sol +++ b/test/unit/assertErrorSelectors.t.sol @@ -6,6 +6,7 @@ import { console2 } from "forge-std/console2.sol"; import { EmailRecoverySubjectHandler } from "src/handlers/EmailRecoverySubjectHandler.sol"; import { SafeRecoverySubjectHandler } from "src/handlers/SafeRecoverySubjectHandler.sol"; import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol"; +import { EmailRecoveryModule } from "src/modules/EmailRecoveryModule.sol"; import { UniversalEmailRecoveryModule } from "src/modules/UniversalEmailRecoveryModule.sol"; import { EnumerableGuardianMap } from "src/libraries/EnumerableGuardianMap.sol"; import { GuardianUtils } from "src/libraries/GuardianUtils.sol"; @@ -51,7 +52,18 @@ contract LogErrorSelectors_Test is Test { } function test_EmailRecoveryModule_AssertSelectors() public { + assertEq(EmailRecoveryModule.InvalidSelector.selector, bytes4(0x12ba286f)); + assertEq(EmailRecoveryModule.InvalidOnInstallData.selector, bytes4(0x5c223882)); + assertEq(EmailRecoveryModule.InvalidValidator.selector, bytes4(0x11d5c560)); + assertEq(EmailRecoveryModule.NotTrustedRecoveryManager.selector, bytes4(0x38f1b648)); + assertEq(EmailRecoveryModule.RecoveryNotAuthorizedForAccount.selector, bytes4(0xba14d9ef)); + } + + function test_UniversalEmailRecoveryModule_AssertSelectors() public { assertEq(UniversalEmailRecoveryModule.InvalidSelector.selector, bytes4(0x12ba286f)); + assertEq( + UniversalEmailRecoveryModule.RecoveryModuleNotInitialized.selector, bytes4(0x0b088c23) + ); assertEq(UniversalEmailRecoveryModule.InvalidOnInstallData.selector, bytes4(0x5c223882)); assertEq(UniversalEmailRecoveryModule.InvalidValidator.selector, bytes4(0x11d5c560)); assertEq(UniversalEmailRecoveryModule.MaxValidatorsReached.selector, bytes4(0xed7948d6)); From 8f3fa2251418d8d1d70f2797bfd0cb9c965cf709 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Fri, 19 Jul 2024 10:43:44 +0100 Subject: [PATCH 10/30] L3: cancelRecovery does not revert when no recovery is in process --- src/EmailRecoveryManager.sol | 3 +++ src/interfaces/IEmailRecoveryManager.sol | 1 + test/unit/EmailRecoveryManager/cancelRecovery.t.sol | 13 +++++++------ test/unit/assertErrorSelectors.t.sol | 1 + 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 5512fd8..3c1bd6c 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -445,6 +445,9 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco * @dev Deletes the current recovery request associated with the caller's account */ function cancelRecovery() external virtual { + if (recoveryRequests[msg.sender].currentWeight == 0) { + revert NoRecoveryInProcess(); + } delete recoveryRequests[msg.sender]; emit RecoveryCancelled(msg.sender); } diff --git a/src/interfaces/IEmailRecoveryManager.sol b/src/interfaces/IEmailRecoveryManager.sol index 3eab8f4..1ae8602 100644 --- a/src/interfaces/IEmailRecoveryManager.sol +++ b/src/interfaces/IEmailRecoveryManager.sol @@ -85,6 +85,7 @@ interface IEmailRecoveryManager { error DelayNotPassed(); error RecoveryRequestExpired(); error InvalidCalldataHash(); + error NoRecoveryInProcess(); error NotRecoveryModule(); /*////////////////////////////////////////////////////////////////////////// diff --git a/test/unit/EmailRecoveryManager/cancelRecovery.t.sol b/test/unit/EmailRecoveryManager/cancelRecovery.t.sol index ad96a8f..8df1224 100644 --- a/test/unit/EmailRecoveryManager/cancelRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/cancelRecovery.t.sol @@ -13,6 +13,12 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { super.setUp(); } + function test_CancelRecovery_RevertWhen_NoRecoveryInProcess() public { + vm.startPrank(accountAddress); + vm.expectRevert(IEmailRecoveryManager.NoRecoveryInProcess.selector); + emailRecoveryManager.cancelRecovery(); + } + function test_CancelRecovery_CannotCancelWrongRecoveryRequest() public { address otherAddress = address(99); @@ -29,13 +35,8 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { assertEq(recoveryRequest.calldataHash, ""); vm.startPrank(otherAddress); + vm.expectRevert(IEmailRecoveryManager.NoRecoveryInProcess.selector); emailRecoveryManager.cancelRecovery(); - - recoveryRequest = emailRecoveryManager.getRecoveryRequest(accountAddress); - assertEq(recoveryRequest.executeAfter, 0); - assertEq(recoveryRequest.executeBefore, 0); - assertEq(recoveryRequest.currentWeight, 1); - assertEq(recoveryRequest.calldataHash, ""); } function test_CancelRecovery_PartialRequest_Succeeds() public { diff --git a/test/unit/assertErrorSelectors.t.sol b/test/unit/assertErrorSelectors.t.sol index 769b92b..a864777 100644 --- a/test/unit/assertErrorSelectors.t.sol +++ b/test/unit/assertErrorSelectors.t.sol @@ -48,6 +48,7 @@ contract LogErrorSelectors_Test is Test { assertEq(IEmailRecoveryManager.DelayNotPassed.selector, bytes4(0xc806ff6e)); assertEq(IEmailRecoveryManager.RecoveryRequestExpired.selector, bytes4(0x4c2babb1)); assertEq(IEmailRecoveryManager.InvalidCalldataHash.selector, bytes4(0xf05609de)); + assertEq(IEmailRecoveryManager.NoRecoveryInProcess.selector, bytes4(0x87434f51)); assertEq(IEmailRecoveryManager.NotRecoveryModule.selector, bytes4(0x2f6ef3d6)); } From 0e4234c5f30243e151d56ada7b8ca3f018f08ec7 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Fri, 19 Jul 2024 17:41:00 +0100 Subject: [PATCH 11/30] W1: isInitialized function returns false if initialized without guardians --- src/modules/EmailRecoveryModule.sol | 12 +++ src/modules/UniversalEmailRecoveryModule.sol | 21 +++++ .../UniversalEmailRecoveryModuleHarness.sol | 10 ++ .../canStartRecoveryRequest.t.sol | 56 +++++++++++ .../canStartRecoveryRequest.t.sol | 94 +++++++++++++++++++ 5 files changed, 193 insertions(+) create mode 100644 test/unit/modules/EmailRecoveryModule/canStartRecoveryRequest.t.sol create mode 100644 test/unit/modules/UniversalEmailRecoveryModule/canStartRecoveryRequest.t.sol diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index b855355..a300090 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -129,6 +129,18 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IEmailRecoveryModule { return authorized[smartAccount]; } + /** + * Check if a recovery request can be initiated based on guardian acceptance + * @param smartAccount The smart account to check + * @return true if the recovery request can be started, false otherwise + */ + function canStartRecoveryRequest(address smartAccount) external view returns (bool) { + IEmailRecoveryManager.GuardianConfig memory guardianConfig = + IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(smartAccount); + + return guardianConfig.acceptedWeight >= guardianConfig.threshold; + } + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ /* MODULE LOGIC */ /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index 78cf994..752ecf8 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -243,6 +243,27 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec return getAllowedValidators(smartAccount).length > 0; } + /** + * Check if a recovery request can be initiated based on guardian acceptance + * @param smartAccount The smart account to check + * @param validator The validator to check + * @return true if the recovery request can be started, false otherwise + */ + function canStartRecoveryRequest( + address smartAccount, + address validator + ) + external + view + returns (bool) + { + IEmailRecoveryManager.GuardianConfig memory guardianConfig = + IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(smartAccount); + + return guardianConfig.acceptedWeight >= guardianConfig.threshold + && validators[smartAccount].contains(validator); + } + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ /* MODULE LOGIC */ /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ diff --git a/test/unit/UniversalEmailRecoveryModuleHarness.sol b/test/unit/UniversalEmailRecoveryModuleHarness.sol index 8e1fe67..67da770 100644 --- a/test/unit/UniversalEmailRecoveryModuleHarness.sol +++ b/test/unit/UniversalEmailRecoveryModuleHarness.sol @@ -14,6 +14,16 @@ contract UniversalEmailRecoveryModuleHarness is UniversalEmailRecoveryModule { validators[account].push(validator); } + function workaround_validatorsContains( + address account, + address validator + ) + external + returns (bool) + { + return validators[account].contains(validator); + } + function exposed_allowedSelectors( address validator, address account diff --git a/test/unit/modules/EmailRecoveryModule/canStartRecoveryRequest.t.sol b/test/unit/modules/EmailRecoveryModule/canStartRecoveryRequest.t.sol new file mode 100644 index 0000000..02070aa --- /dev/null +++ b/test/unit/modules/EmailRecoveryModule/canStartRecoveryRequest.t.sol @@ -0,0 +1,56 @@ +// 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 { EmailRecoveryModuleBase } from "./EmailRecoveryModuleBase.t.sol"; + +contract EmailRecoveryModule_canStartRecoveryRequest_Test is EmailRecoveryModuleBase { + function setUp() public override { + super.setUp(); + } + + function test_CanStartRecoveryRequest_ReturnsFalse_WhenThresholdCannotBeMet() public { + bool canStartRecoveryRequest = emailRecoveryModule.canStartRecoveryRequest(accountAddress); + + // Checking accepted weight is what we expect for this test case + IEmailRecoveryManager.GuardianConfig memory guardianConfig = + IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(accountAddress); + + // No guardians have accepted + assertFalse(canStartRecoveryRequest); + assertFalse(guardianConfig.acceptedWeight >= guardianConfig.threshold); + } + + function test_CanStartRecoveryRequest_ReturnsTrue_WhenThresholdIsHigherThanWeight() public { + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + acceptGuardian(accountSalt3); + + bool canStartRecoveryRequest = emailRecoveryModule.canStartRecoveryRequest(accountAddress); + + // Checking accepted weight is what we expect for this test case + IEmailRecoveryManager.GuardianConfig memory guardianConfig = + IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(accountAddress); + + // Enough guardians have accepted so that accepted weight is higher than the threshold + assertTrue(canStartRecoveryRequest); + assertTrue(guardianConfig.acceptedWeight > guardianConfig.threshold); + } + + function test_CanStartRecoveryRequest_ReturnsTrue_WhenThresholdIsEqualToWeight() public { + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + + bool canStartRecoveryRequest = emailRecoveryModule.canStartRecoveryRequest(accountAddress); + + // Checking accepted weight is what we expect for this test case + IEmailRecoveryManager.GuardianConfig memory guardianConfig = + IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(accountAddress); + + // Enough guardians have accepted so that accepted weight is equal to the threshold + assertTrue(canStartRecoveryRequest); + assertTrue(guardianConfig.acceptedWeight == guardianConfig.threshold); + } +} diff --git a/test/unit/modules/UniversalEmailRecoveryModule/canStartRecoveryRequest.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/canStartRecoveryRequest.t.sol new file mode 100644 index 0000000..ef00322 --- /dev/null +++ b/test/unit/modules/UniversalEmailRecoveryModule/canStartRecoveryRequest.t.sol @@ -0,0 +1,94 @@ +// 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"; + +contract UniversalEmailRecoveryModule_canStartRecoveryRequest_Test is UnitBase { + function setUp() public override { + super.setUp(); + } + + function test_CanStartRecoveryRequest_ReturnsFalse_WhenThresholdCannotBeMet() public { + bool canStartRecoveryRequest = + emailRecoveryModule.canStartRecoveryRequest(accountAddress, validatorAddress); + + // Checking accepted weight and sentinel list storage are what we expect for this test case + IEmailRecoveryManager.GuardianConfig memory guardianConfig = + IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(accountAddress); + bool contains = + emailRecoveryModule.workaround_validatorsContains(accountAddress, validatorAddress); + + // No guardians have accepted + assertFalse(canStartRecoveryRequest); + assertFalse(guardianConfig.acceptedWeight >= guardianConfig.threshold); + assertTrue(contains); + } + + function test_CanStartRecoveryRequest_ReturnsFalse_WhenValidatorNotAdded() public { + address invalidValidatorAddress = address(1); + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + + bool canStartRecoveryRequest = + emailRecoveryModule.canStartRecoveryRequest(accountAddress, invalidValidatorAddress); + + // Checking accepted weight and sentinel list storage are what we expect for this test case + IEmailRecoveryManager.GuardianConfig memory guardianConfig = + IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(accountAddress); + bool contains = emailRecoveryModule.workaround_validatorsContains( + accountAddress, invalidValidatorAddress + ); + + // Enough guardians have accepted but invalid guardian address + assertFalse(canStartRecoveryRequest); + assertTrue(guardianConfig.acceptedWeight >= guardianConfig.threshold); + assertFalse(contains); + } + + function test_CanStartRecoveryRequest_ReturnsTrue_WhenThresholdIsHigherThanWeightAndValidatorAdded( + ) + public + { + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + acceptGuardian(accountSalt3); + + bool canStartRecoveryRequest = + emailRecoveryModule.canStartRecoveryRequest(accountAddress, validatorAddress); + + // Checking accepted weight and sentinel list storage are what we expect for this test case + IEmailRecoveryManager.GuardianConfig memory guardianConfig = + IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(accountAddress); + bool contains = + emailRecoveryModule.workaround_validatorsContains(accountAddress, validatorAddress); + + // Enough guardians have accepted so that accepted weight is higher than the threshold + assertTrue(canStartRecoveryRequest); + assertTrue(guardianConfig.acceptedWeight > guardianConfig.threshold); + assertTrue(contains); + } + + function test_CanStartRecoveryRequest_ReturnsTrue_WhenThresholdIsEqualToWeightAndValidatorAdded( + ) + public + { + acceptGuardian(accountSalt1); + acceptGuardian(accountSalt2); + + bool canStartRecoveryRequest = + emailRecoveryModule.canStartRecoveryRequest(accountAddress, validatorAddress); + + // Checking accepted weight and sentinel list storage are what we expect for this test case + IEmailRecoveryManager.GuardianConfig memory guardianConfig = + IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(accountAddress); + bool contains = + emailRecoveryModule.workaround_validatorsContains(accountAddress, validatorAddress); + + // Enough guardians have accepted so that accepted weight is equal to the threshold + assertTrue(canStartRecoveryRequest); + assertTrue(guardianConfig.acceptedWeight == guardianConfig.threshold); + assertTrue(contains); + } +} From c9028df42d5d0aeaf678ef7de95b4903d14c3d68 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Mon, 22 Jul 2024 20:40:06 +0100 Subject: [PATCH 12/30] W2: Unused bytes32 function parameter in EmailRecoveryManager --- src/EmailRecoveryManager.sol | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 3c1bd6c..0a7be37 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -294,12 +294,14 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco * @param guardian The address of the guardian to be accepted * @param templateIdx The index of the template used for acceptance * @param subjectParams An array of bytes containing the subject parameters + * @param {nullifier} Unused parameter. The nullifier acts as a unique identifier for an email, + * but it is not required in this implementation */ function acceptGuardian( address guardian, uint256 templateIdx, bytes[] memory subjectParams, - bytes32 + bytes32 /* nullifier */ ) internal override @@ -340,12 +342,14 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco * @param guardian The address of the guardian initiating the recovery * @param templateIdx The index of the template used for the recovery request * @param subjectParams An array of bytes containing the subject parameters + * @param {nullifier} Unused parameter. The nullifier acts as a unique identifier for an email, + * but it is not required in this implementation */ function processRecovery( address guardian, uint256 templateIdx, bytes[] memory subjectParams, - bytes32 + bytes32 /* nullifier */ ) internal override From f80edd715aa14f275b7379e3376993f99d187642 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 23 Jul 2024 11:38:17 +0100 Subject: [PATCH 13/30] W3: Unnecessary computation of calldataHash value in validateRecoverySubject function --- src/EmailRecoveryManager.sol | 8 +++- src/handlers/EmailRecoverySubjectHandler.sol | 28 +++++++++++-- src/handlers/SafeRecoverySubjectHandler.sol | 33 ++++++++++++--- .../IEmailRecoverySubjectHandler.sol | 9 ++++- .../parseRecoveryCalldataHash.t.sol | 40 +++++++++++++++++++ .../validateRecoverySubject.t.sol | 24 +++++++++-- .../parseRecoveryCalldataHash.t.sol | 38 ++++++++++++++++++ .../validateRecoverySubject.t.sol | 6 +-- 8 files changed, 167 insertions(+), 19 deletions(-) create mode 100644 test/unit/handlers/EmailRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol create mode 100644 test/unit/handlers/SafeRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 0a7be37..82aeb66 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -354,8 +354,9 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco internal override { - (address account, bytes32 calldataHash) = IEmailRecoverySubjectHandler(subjectHandler) - .validateRecoverySubject(templateIdx, subjectParams, address(this)); + address account = IEmailRecoverySubjectHandler(subjectHandler).validateRecoverySubject( + templateIdx, subjectParams, address(this) + ); if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToRecover(account)) { revert RecoveryModuleNotAuthorized(); @@ -378,6 +379,9 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco recoveryRequest.currentWeight += guardianStorage.weight; if (recoveryRequest.currentWeight >= guardianConfig.threshold) { + bytes32 calldataHash = IEmailRecoverySubjectHandler(subjectHandler) + .parseRecoveryCalldataHash(templateIdx, subjectParams); + uint256 executeAfter = block.timestamp + recoveryConfigs[account].delay; uint256 executeBefore = block.timestamp + recoveryConfigs[account].expiry; diff --git a/src/handlers/EmailRecoverySubjectHandler.sol b/src/handlers/EmailRecoverySubjectHandler.sol index af72d80..09fc797 100644 --- a/src/handlers/EmailRecoverySubjectHandler.sol +++ b/src/handlers/EmailRecoverySubjectHandler.sol @@ -121,8 +121,6 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { * @param subjectParams The subject parameters of the recovery email * @param recoveryManager The recovery manager address. Used to help with validation * @return accountInEmail The account address in the acceptance email - * @return calldataHash The keccak256 hash of the recovery calldata. Verified against later when - * recovery is executed */ function validateRecoverySubject( uint256 templateIdx, @@ -131,7 +129,7 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { ) public view - returns (address, bytes32) + returns (address) { if (templateIdx != 0) { revert InvalidTemplateIndex(); @@ -159,6 +157,28 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { revert InvalidRecoveryModule(); } - return (accountInEmail, calldataHash); + return accountInEmail; + } + + /** + * @notice parses the recovery calldata hash from the subject params. The calldata hash is + * verified against later when recovery is executed + * @param templateIdx The index of the template used for the recovery request + * @param subjectParams The subject parameters of the recovery email + * @return calldataHash The keccak256 hash of the recovery calldata + */ + function parseRecoveryCalldataHash( + uint256 templateIdx, + bytes[] calldata subjectParams + ) + external + returns (bytes32) + { + if (templateIdx != 0) { + revert InvalidTemplateIndex(); + } + + string memory calldataHashInEmail = abi.decode(subjectParams[2], (string)); + return StringUtils.hexToBytes32(calldataHashInEmail); } } diff --git a/src/handlers/SafeRecoverySubjectHandler.sol b/src/handlers/SafeRecoverySubjectHandler.sol index 8dc55c1..6a5e811 100644 --- a/src/handlers/SafeRecoverySubjectHandler.sol +++ b/src/handlers/SafeRecoverySubjectHandler.sol @@ -129,8 +129,6 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { * @param subjectParams The subject parameters of the recovery email * @param recoveryManager The recovery manager address. Used to help with validation * @return accountInEmail The account address in the acceptance email - * @return calldataHash The keccak256 hash of the recovery calldata. Verified against later when - * recovery is executed */ function validateRecoverySubject( uint256 templateIdx, @@ -139,7 +137,7 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { ) public view - returns (address, bytes32) + returns (address) { if (templateIdx != 0) { revert InvalidTemplateIndex(); @@ -172,15 +170,38 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { revert InvalidRecoveryModule(); } + return accountInEmail; + } + + /** + * @notice parses the recovery calldata hash from the subject params. The calldata hash is + * verified against later when recovery is executed + * @param templateIdx The index of the template used for the recovery request + * @param subjectParams The subject parameters of the recovery email + * @return calldataHash The keccak256 hash of the recovery calldata + */ + function parseRecoveryCalldataHash( + uint256 templateIdx, + bytes[] calldata subjectParams + ) + external + returns (bytes32) + { + if (templateIdx != 0) { + revert InvalidTemplateIndex(); + } + + address accountInEmail = abi.decode(subjectParams[0], (address)); + address oldOwnerInEmail = abi.decode(subjectParams[1], (address)); + address newOwnerInEmail = abi.decode(subjectParams[2], (address)); + address previousOwnerInLinkedList = getPreviousOwnerInLinkedList(accountInEmail, oldOwnerInEmail); string memory functionSignature = "swapOwner(address,address,address)"; bytes memory recoveryCallData = abi.encodeWithSignature( functionSignature, previousOwnerInLinkedList, oldOwnerInEmail, newOwnerInEmail ); - bytes32 calldataHash = keccak256(recoveryCallData); - - return (accountInEmail, calldataHash); + return keccak256(recoveryCallData); } /** diff --git a/src/interfaces/IEmailRecoverySubjectHandler.sol b/src/interfaces/IEmailRecoverySubjectHandler.sol index 10f3eb3..a8f2b80 100644 --- a/src/interfaces/IEmailRecoverySubjectHandler.sol +++ b/src/interfaces/IEmailRecoverySubjectHandler.sol @@ -36,5 +36,12 @@ interface IEmailRecoverySubjectHandler { ) external view - returns (address, bytes32); + returns (address); + + function parseRecoveryCalldataHash( + uint256 templateIdx, + bytes[] calldata subjectParams + ) + external + returns (bytes32); } diff --git a/test/unit/handlers/EmailRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol b/test/unit/handlers/EmailRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol new file mode 100644 index 0000000..0b66024 --- /dev/null +++ b/test/unit/handlers/EmailRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol @@ -0,0 +1,40 @@ +// 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 { EmailRecoverySubjectHandler } from "src/handlers/EmailRecoverySubjectHandler.sol"; + +contract EmailRecoverySubjectHandler_parseRecoveryCalldataHash_Test is UnitBase { + using Strings for uint256; + + string calldataHashString; + bytes[] subjectParams; + + function setUp() public override { + super.setUp(); + + calldataHashString = uint256(calldataHash).toHexString(32); + + subjectParams = new bytes[](3); + subjectParams[0] = abi.encode(accountAddress); + subjectParams[1] = abi.encode(recoveryModuleAddress); + subjectParams[2] = abi.encode(calldataHashString); + } + + function test_ParseRecoveryCalldataHash_RevertWhen_InvalidTemplateIndex() public { + uint256 invalidTemplateIdx = 1; + vm.expectRevert(EmailRecoverySubjectHandler.InvalidTemplateIndex.selector); + emailRecoveryHandler.parseRecoveryCalldataHash(invalidTemplateIdx, subjectParams); + } + + function test_ParseRecoveryCalldataHash_Succeeds() public { + bytes32 expectedCalldataHash = keccak256(recoveryCalldata); + + bytes32 calldataHash = + emailRecoveryHandler.parseRecoveryCalldataHash(templateIdx, subjectParams); + + assertEq(calldataHash, expectedCalldataHash); + } +} diff --git a/test/unit/handlers/EmailRecoverySubjectHandler/validateRecoverySubject.t.sol b/test/unit/handlers/EmailRecoverySubjectHandler/validateRecoverySubject.t.sol index 6d91a40..e523038 100644 --- a/test/unit/handlers/EmailRecoverySubjectHandler/validateRecoverySubject.t.sol +++ b/test/unit/handlers/EmailRecoverySubjectHandler/validateRecoverySubject.t.sol @@ -83,10 +83,28 @@ contract EmailRecoverySubjectHandler_validateRecoverySubject_Test is UnitBase { ); } + function test_ValidateRecoverySubject_RevertWhen_ZeroCalldataHash() public { + subjectParams[2] = abi.encode(bytes32(0)); + + vm.expectRevert("invalid hex prefix"); + emailRecoveryHandler.validateRecoverySubject( + templateIdx, subjectParams, emailRecoveryManagerAddress + ); + } + + function test_ValidateRecoverySubject_RevertWhen_InvalidHashLength() public { + subjectParams[2] = abi.encode(uint256(calldataHash).toHexString(33)); + + vm.expectRevert("invalid hex string length"); + emailRecoveryHandler.validateRecoverySubject( + templateIdx, subjectParams, emailRecoveryManagerAddress + ); + } + function test_ValidateRecoverySubject_Succeeds() public view { - (address accountFromEmail, bytes32 calldataHashFromEmail) = emailRecoveryHandler - .validateRecoverySubject(templateIdx, subjectParams, emailRecoveryManagerAddress); + address accountFromEmail = emailRecoveryHandler.validateRecoverySubject( + templateIdx, subjectParams, emailRecoveryManagerAddress + ); assertEq(accountFromEmail, accountAddress); - assertEq(calldataHashFromEmail, calldataHash); } } diff --git a/test/unit/handlers/SafeRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol b/test/unit/handlers/SafeRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol new file mode 100644 index 0000000..9e910e7 --- /dev/null +++ b/test/unit/handlers/SafeRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.25; + +import { console2 } from "forge-std/console2.sol"; +import { SafeUnitBase } from "../../SafeUnitBase.t.sol"; +import { SafeRecoverySubjectHandler } from "src/handlers/SafeRecoverySubjectHandler.sol"; + +contract SafeRecoverySubjectHandler_parseRecoveryCalldataHash_Test is SafeUnitBase { + bytes[] subjectParams; + + function setUp() public override { + super.setUp(); + + subjectParams = new bytes[](4); + subjectParams[0] = abi.encode(accountAddress1); + subjectParams[1] = abi.encode(owner1); + subjectParams[2] = abi.encode(newOwner1); + subjectParams[3] = abi.encode(recoveryModuleAddress); + } + + function test_ParseRecoveryCalldataHash_RevertWhen_InvalidTemplateIndex() public { + skipIfNotSafeAccountType(); + + uint256 invalidTemplateIdx = 1; + vm.expectRevert(SafeRecoverySubjectHandler.InvalidTemplateIndex.selector); + safeRecoverySubjectHandler.parseRecoveryCalldataHash(invalidTemplateIdx, subjectParams); + } + + function test_ParseRecoveryCalldataHash_Succeeds() public { + skipIfNotSafeAccountType(); + bytes32 expectedCalldataHash = keccak256(recoveryCalldata); + + bytes32 calldataHash = + safeRecoverySubjectHandler.parseRecoveryCalldataHash(templateIdx, subjectParams); + + assertEq(calldataHash, expectedCalldataHash); + } +} diff --git a/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol b/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol index cdae48e..e6d14a8 100644 --- a/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol +++ b/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol @@ -100,9 +100,9 @@ contract SafeRecoverySubjectHandler_validateRecoverySubject_Test is SafeUnitBase function test_ValidateRecoverySubject_Succeeds() public { skipIfNotSafeAccountType(); - (address accountFromEmail, bytes32 calldataHashFromEmail) = safeRecoverySubjectHandler - .validateRecoverySubject(templateIdx, subjectParams, emailRecoveryManagerAddress); + address accountFromEmail = safeRecoverySubjectHandler.validateRecoverySubject( + templateIdx, subjectParams, emailRecoveryManagerAddress + ); assertEq(accountFromEmail, accountAddress1); - assertEq(calldataHashFromEmail, calldataHash); } } From bc1410a068f4199e4ed9a327a4e8e5993794ffd9 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 23 Jul 2024 14:32:47 +0100 Subject: [PATCH 14/30] W5: Events missing parameters --- src/EmailRecoveryManager.sol | 11 ++++--- src/factories/EmailRecoveryFactory.sol | 10 +++++-- .../EmailRecoveryUniversalFactory.sol | 6 ++-- src/interfaces/IEmailRecoveryManager.sol | 12 ++++++-- src/libraries/GuardianUtils.sol | 30 +++++++++++-------- src/modules/EmailRecoveryModule.sol | 4 +-- src/modules/UniversalEmailRecoveryModule.sol | 22 +++++++++----- .../deployEmailRecoveryModule.t.sol | 8 +++++ .../EmailRecoveryManager/acceptGuardian.t.sol | 2 ++ .../EmailRecoveryManager/addGuardian.t.sol | 2 +- .../EmailRecoveryManager/cancelRecovery.t.sol | 2 ++ .../configureRecovery.t.sol | 24 +++++---------- .../processRecovery.t.sol | 8 +++++ .../EmailRecoveryManager/removeGuardian.t.sol | 2 ++ .../EmailRecoveryManager/setupGuardians.t.sol | 5 +++- .../updateRecoveryConfig.t.sol | 4 +++ test/unit/EmailRecoveryManagerHarness.sol | 3 +- .../deployUniversalEmailRecoveryModule.t.sol | 6 +++- .../libraries/GuardianUtils/addGuardian.t.sol | 2 +- .../GuardianUtils/removeGuardian.t.sol | 2 ++ .../GuardianUtils/setupGuardians.t.sol | 5 +++- .../GuardianUtils/updateGuardianStatus.t.sol | 2 ++ .../modules/EmailRecoveryModule/recover.t.sol | 2 ++ .../allowValidatorRecovery.t.sol | 6 ++++ .../disallowValidatorRecovery.t.sol | 6 ++++ .../recover.t.sol | 2 ++ 26 files changed, 133 insertions(+), 55 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 82aeb66..46b1626 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -245,12 +245,13 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco revert RecoveryModuleNotAuthorized(); } - setupGuardians(account, guardians, weights, threshold); + (uint256 guardianCount, uint256 totalWeight) = + setupGuardians(account, guardians, weights, threshold); RecoveryConfig memory recoveryConfig = RecoveryConfig(delay, expiry); updateRecoveryConfig(recoveryConfig); - emit RecoveryConfigured(account, guardians.length); + emit RecoveryConfigured(account, guardianCount, totalWeight, threshold); } /** @@ -389,7 +390,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco recoveryRequest.executeBefore = executeBefore; recoveryRequest.calldataHash = calldataHash; - emit RecoveryProcessed(account, executeAfter, executeBefore); + emit RecoveryProcessed(account, guardian, executeAfter, executeBefore, calldataHash); } } @@ -532,8 +533,10 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco uint256 threshold ) internal + returns (uint256, uint256) { - guardianConfigs.setupGuardians(guardiansStorage, account, guardians, weights, threshold); + return + guardianConfigs.setupGuardians(guardiansStorage, account, guardians, weights, threshold); } /** diff --git a/src/factories/EmailRecoveryFactory.sol b/src/factories/EmailRecoveryFactory.sol index 8a7989a..de187f0 100644 --- a/src/factories/EmailRecoveryFactory.sol +++ b/src/factories/EmailRecoveryFactory.sol @@ -24,7 +24,11 @@ contract EmailRecoveryFactory { address public immutable emailAuthImpl; event EmailRecoveryModuleDeployed( - address emailRecoveryModule, address emailRecoveryManager, address subjectHandler + address emailRecoveryModule, + address emailRecoveryManager, + address subjectHandler, + address validator, + bytes4 functionSelector ); constructor(address _verifier, address _emailAuthImpl) { @@ -84,7 +88,9 @@ contract EmailRecoveryFactory { // Initialize recovery manager with module address EmailRecoveryManager(emailRecoveryManager).initialize(emailRecoveryModule); - emit EmailRecoveryModuleDeployed(emailRecoveryModule, emailRecoveryManager, subjectHandler); + emit EmailRecoveryModuleDeployed( + emailRecoveryModule, emailRecoveryManager, subjectHandler, validator, functionSelector + ); return (emailRecoveryModule, emailRecoveryManager, subjectHandler); } diff --git a/src/factories/EmailRecoveryUniversalFactory.sol b/src/factories/EmailRecoveryUniversalFactory.sol index 2f704c0..33994c9 100644 --- a/src/factories/EmailRecoveryUniversalFactory.sol +++ b/src/factories/EmailRecoveryUniversalFactory.sol @@ -16,7 +16,7 @@ contract EmailRecoveryUniversalFactory { address public immutable verifier; address public immutable emailAuthImpl; - event EmailRecoveryModuleDeployed( + event UniversalEmailRecoveryModuleDeployed( address emailRecoveryModule, address emailRecoveryManager, address subjectHandler ); @@ -75,7 +75,9 @@ contract EmailRecoveryUniversalFactory { // Initialize recovery manager with module address EmailRecoveryManager(emailRecoveryManager).initialize(emailRecoveryModule); - emit EmailRecoveryModuleDeployed(emailRecoveryModule, emailRecoveryManager, subjectHandler); + emit UniversalEmailRecoveryModuleDeployed( + emailRecoveryModule, emailRecoveryManager, subjectHandler + ); return (emailRecoveryModule, emailRecoveryManager, subjectHandler); } diff --git a/src/interfaces/IEmailRecoveryManager.sol b/src/interfaces/IEmailRecoveryManager.sol index 1ae8602..1b93acc 100644 --- a/src/interfaces/IEmailRecoveryManager.sol +++ b/src/interfaces/IEmailRecoveryManager.sol @@ -54,10 +54,18 @@ interface IEmailRecoveryManager { EVENTS //////////////////////////////////////////////////////////////////////////*/ - event RecoveryConfigured(address indexed account, uint256 guardianCount); + event RecoveryConfigured( + address indexed account, uint256 guardianCount, uint256 totalWeight, uint256 threshold + ); event RecoveryConfigUpdated(address indexed account, uint256 delay, uint256 expiry); event GuardianAccepted(address indexed account, address indexed guardian); - event RecoveryProcessed(address indexed account, uint256 executeAfter, uint256 executeBefore); + event RecoveryProcessed( + address indexed account, + address indexed guardian, + uint256 executeAfter, + uint256 executeBefore, + bytes32 calldataHash + ); event RecoveryCompleted(address indexed account); event RecoveryCancelled(address indexed account); event RecoveryDeInitialized(address indexed account); diff --git a/src/libraries/GuardianUtils.sol b/src/libraries/GuardianUtils.sol index 4f92954..1b7737d 100644 --- a/src/libraries/GuardianUtils.sol +++ b/src/libraries/GuardianUtils.sol @@ -10,8 +10,11 @@ import { IEmailRecoveryManager } from "../interfaces/IEmailRecoveryManager.sol"; library GuardianUtils { using EnumerableGuardianMap for EnumerableGuardianMap.AddressToGuardianMap; - event AddedGuardian(address indexed account, address indexed guardian); - event RemovedGuardian(address indexed account, address indexed guardian); + event AddedGuardian(address indexed account, address indexed guardian, uint256 weight); + event GuardianStatusUpdated( + address indexed account, address indexed guardian, GuardianStatus newStatus + ); + event RemovedGuardian(address indexed account, address indexed guardian, uint256 weight); event ChangedThreshold(address indexed account, uint256 threshold); error IncorrectNumberOfWeights(); @@ -61,6 +64,7 @@ library GuardianUtils { uint256 threshold ) internal + returns (uint256, uint256) { uint256 guardianCount = guardians.length; @@ -74,30 +78,27 @@ library GuardianUtils { uint256 totalWeight = 0; for (uint256 i = 0; i < guardianCount; i++) { - address guardian = guardians[i]; - uint256 weight = weights[i]; - - if (guardian == address(0) || guardian == account) { + if (guardians[i] == address(0) || guardians[i] == account) { revert InvalidGuardianAddress(); } // As long as weights are 1 or above, there will be enough total weight to reach the // required threshold. This is because we check the guardian count cannot be less // than the threshold and there is an equal amount of guardians to weights. - if (weight == 0) { + if (weights[i] == 0) { revert InvalidGuardianWeight(); } - GuardianStorage memory guardianStorage = guardiansStorage[account].get(guardian); + GuardianStorage memory guardianStorage = guardiansStorage[account].get(guardians[i]); if (guardianStorage.status != GuardianStatus.NONE) { revert AddressAlreadyGuardian(); } guardiansStorage[account].set({ - key: guardian, - value: GuardianStorage(GuardianStatus.REQUESTED, weight) + key: guardians[i], + value: GuardianStorage(GuardianStatus.REQUESTED, weights[i]) }); - totalWeight += weight; + totalWeight += weights[i]; } if (threshold > totalWeight) { @@ -110,6 +111,8 @@ library GuardianUtils { acceptedWeight: 0, threshold: threshold }); + + return (guardianCount, totalWeight); } /** @@ -135,6 +138,7 @@ library GuardianUtils { key: guardian, value: GuardianStorage(newStatus, guardianStorage.weight) }); + emit GuardianStatusUpdated(account, guardian, newStatus); } /** @@ -181,7 +185,7 @@ library GuardianUtils { guardianConfigs[account].guardianCount++; guardianConfigs[account].totalWeight += weight; - emit AddedGuardian(account, guardian); + emit AddedGuardian(account, guardian, weight); } /** @@ -218,7 +222,7 @@ library GuardianUtils { guardianConfigs[account].acceptedWeight -= guardianStorage.weight; } - emit RemovedGuardian(account, guardian); + emit RemovedGuardian(account, guardian, guardianStorage.weight); } /** diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index a300090..5429ee0 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -43,7 +43,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IEmailRecoveryModule { */ mapping(address account => bool isAuthorized) internal authorized; - event RecoveryExecuted(); + event RecoveryExecuted(address indexed account, address indexed validator); error InvalidSelector(bytes4 selector); error InvalidOnInstallData(); @@ -167,7 +167,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IEmailRecoveryModule { _execute({ account: account, to: validator, value: 0, data: recoveryCalldata }); - emit RecoveryExecuted(); + emit RecoveryExecuted(account, validator); } /** diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index 752ecf8..bf45eef 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -36,9 +36,13 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec */ address public immutable emailRecoveryManager; - event NewValidatorRecovery(address indexed validatorModule, bytes4 recoverySelector); - event RemovedValidatorRecovery(address indexed validatorModule, bytes4 recoverySelector); - event RecoveryExecuted(); + event NewValidatorRecovery( + address indexed account, address indexed validator, bytes4 recoverySelector + ); + event RemovedValidatorRecovery( + address indexed account, address indexed validator, bytes4 recoverySelector + ); + event RecoveryExecuted(address indexed account, address indexed validator); error InvalidSelector(bytes4 selector); error RecoveryModuleNotInitialized(); @@ -168,8 +172,11 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec allowedSelectors[validator][msg.sender] = recoverySelector; selectorToValidator[recoverySelector][msg.sender] = validator; - - emit NewValidatorRecovery({ validatorModule: validator, recoverySelector: recoverySelector }); + emit NewValidatorRecovery({ + account: msg.sender, + validator: validator, + recoverySelector: recoverySelector + }); } /** @@ -200,7 +207,8 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec delete selectorToValidator[recoverySelector][msg.sender]; emit RemovedValidatorRecovery({ - validatorModule: validator, + account: msg.sender, + validator: validator, recoverySelector: recoverySelector }); } @@ -289,7 +297,7 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec _execute({ account: account, to: validator, value: 0, data: recoveryCalldata }); - emit RecoveryExecuted(); + emit RecoveryExecuted(account, validator); } /** diff --git a/test/unit/EmailRecoveryFactory/deployEmailRecoveryModule.t.sol b/test/unit/EmailRecoveryFactory/deployEmailRecoveryModule.t.sol index 3f3594a..c4f5a3b 100644 --- a/test/unit/EmailRecoveryFactory/deployEmailRecoveryModule.t.sol +++ b/test/unit/EmailRecoveryFactory/deployEmailRecoveryModule.t.sol @@ -46,6 +46,14 @@ contract EmailRecoveryFactory_deployAll_Test is UnitBase { recoveryModuleSalt, keccak256(recoveryModuleBytecode), address(emailRecoveryFactory) ); + vm.expectEmit(); + emit EmailRecoveryFactory.EmailRecoveryModuleDeployed( + expectedModule, + expectedManager, + expectedSubjectHandler, + validatorAddress, + functionSelector + ); (address emailRecoveryModule, address emailRecoveryManager, address subjectHandler) = emailRecoveryFactory.deployEmailRecoveryModule( subjectHandlerSalt, diff --git a/test/unit/EmailRecoveryManager/acceptGuardian.t.sol b/test/unit/EmailRecoveryManager/acceptGuardian.t.sol index 2ca32bb..6008dea 100644 --- a/test/unit/EmailRecoveryManager/acceptGuardian.t.sol +++ b/test/unit/EmailRecoveryManager/acceptGuardian.t.sol @@ -79,6 +79,8 @@ contract EmailRecoveryManager_acceptGuardian_Test is UnitBase { } function test_AcceptGuardian_Succeeds() public { + vm.expectEmit(); + emit IEmailRecoveryManager.GuardianAccepted(accountAddress, guardian1); emailRecoveryManager.exposed_acceptGuardian( guardian1, templateIdx, subjectParams, nullifier ); diff --git a/test/unit/EmailRecoveryManager/addGuardian.t.sol b/test/unit/EmailRecoveryManager/addGuardian.t.sol index 8581a63..77a15ee 100644 --- a/test/unit/EmailRecoveryManager/addGuardian.t.sol +++ b/test/unit/EmailRecoveryManager/addGuardian.t.sol @@ -34,7 +34,7 @@ contract EmailRecoveryManager_addGuardian_Test is UnitBase { vm.startPrank(accountAddress); vm.expectEmit(); - emit GuardianUtils.AddedGuardian(accountAddress, newGuardian); + emit GuardianUtils.AddedGuardian(accountAddress, newGuardian, newGuardianWeight); emailRecoveryManager.addGuardian(newGuardian, newGuardianWeight); GuardianStorage memory guardianStorage = diff --git a/test/unit/EmailRecoveryManager/cancelRecovery.t.sol b/test/unit/EmailRecoveryManager/cancelRecovery.t.sol index 8df1224..b7c4914 100644 --- a/test/unit/EmailRecoveryManager/cancelRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/cancelRecovery.t.sol @@ -77,6 +77,8 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase { assertEq(recoveryRequest.calldataHash, calldataHash); vm.startPrank(accountAddress); + vm.expectEmit(); + emit IEmailRecoveryManager.RecoveryCancelled(accountAddress); emailRecoveryManager.cancelRecovery(); recoveryRequest = emailRecoveryManager.getRecoveryRequest(accountAddress); diff --git a/test/unit/EmailRecoveryManager/configureRecovery.t.sol b/test/unit/EmailRecoveryManager/configureRecovery.t.sol index fbce2ed..7a77c3e 100644 --- a/test/unit/EmailRecoveryManager/configureRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/configureRecovery.t.sol @@ -45,24 +45,14 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { function test_ConfigureRecovery_Succeeds() public { instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); + vm.startPrank(accountAddress); + emailRecoveryModule.workaround_validatorsPush(accountAddress, validatorAddress); - // Install recovery module - configureRecovery is called on `onInstall` - // vm.expectEmit(); - // emit IEmailRecoveryManager.RecoveryConfigured(instance.account, guardians.length); - instance.installModule({ - moduleTypeId: MODULE_TYPE_EXECUTOR, - module: recoveryModuleAddress, - data: abi.encode( - validatorAddress, - isInstalledContext, - functionSelector, - guardians, - guardianWeights, - threshold, - delay, - expiry - ) - }); + vm.expectEmit(); + emit IEmailRecoveryManager.RecoveryConfigured( + instance.account, guardians.length, totalWeight, threshold + ); + emailRecoveryManager.configureRecovery(guardians, guardianWeights, threshold, delay, expiry); IEmailRecoveryManager.RecoveryConfig memory recoveryConfig = emailRecoveryManager.getRecoveryConfig(accountAddress); diff --git a/test/unit/EmailRecoveryManager/processRecovery.t.sol b/test/unit/EmailRecoveryManager/processRecovery.t.sol index d9b8991..138bfa1 100644 --- a/test/unit/EmailRecoveryManager/processRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/processRecovery.t.sol @@ -138,6 +138,14 @@ contract EmailRecoveryManager_processRecovery_Test is UnitBase { handleRecovery(recoveryModuleAddress, calldataHash, accountSalt1); // Call processRecovery with guardian2 which increases currentWeight to >= threshold + vm.expectEmit(); + emit IEmailRecoveryManager.RecoveryProcessed( + accountAddress, + guardian2, + block.timestamp + delay, + block.timestamp + expiry, + calldataHash + ); emailRecoveryManager.exposed_processRecovery( guardian2, templateIdx, subjectParams, nullifier ); diff --git a/test/unit/EmailRecoveryManager/removeGuardian.t.sol b/test/unit/EmailRecoveryManager/removeGuardian.t.sol index 4be7f60..23e623b 100644 --- a/test/unit/EmailRecoveryManager/removeGuardian.t.sol +++ b/test/unit/EmailRecoveryManager/removeGuardian.t.sol @@ -39,6 +39,8 @@ contract EmailRecoveryManager_removeGuardian_Test is UnitBase { acceptGuardian(accountSalt1); vm.startPrank(accountAddress); + vm.expectEmit(); + emit GuardianUtils.RemovedGuardian(accountAddress, guardian, guardianWeights[0]); emailRecoveryManager.removeGuardian(guardian); GuardianStorage memory guardianStorage = diff --git a/test/unit/EmailRecoveryManager/setupGuardians.t.sol b/test/unit/EmailRecoveryManager/setupGuardians.t.sol index d06118c..6be1199 100644 --- a/test/unit/EmailRecoveryManager/setupGuardians.t.sol +++ b/test/unit/EmailRecoveryManager/setupGuardians.t.sol @@ -24,7 +24,7 @@ contract EmailRecoveryManager_setupGuardians_Test is UnitBase { instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); vm.stopPrank(); - emailRecoveryManager.exposed_setupGuardians( + (uint256 guardianCount, uint256 totalWeight) = emailRecoveryManager.exposed_setupGuardians( accountAddress, guardians, guardianWeights, threshold ); @@ -41,6 +41,9 @@ contract EmailRecoveryManager_setupGuardians_Test is UnitBase { assertEq(uint256(guardianStorage3.status), uint256(GuardianStatus.REQUESTED)); assertEq(guardianStorage3.weight, guardianWeights[2]); + assertEq(guardianCount, expectedGuardianCount); + assertEq(totalWeight, expectedTotalWeight); + IEmailRecoveryManager.GuardianConfig memory guardianConfig = emailRecoveryManager.getGuardianConfig(accountAddress); assertEq(guardianConfig.guardianCount, expectedGuardianCount); diff --git a/test/unit/EmailRecoveryManager/updateRecoveryConfig.t.sol b/test/unit/EmailRecoveryManager/updateRecoveryConfig.t.sol index 833afec..670edfc 100644 --- a/test/unit/EmailRecoveryManager/updateRecoveryConfig.t.sol +++ b/test/unit/EmailRecoveryManager/updateRecoveryConfig.t.sol @@ -95,6 +95,10 @@ contract EmailRecoveryManager_updateRecoveryConfig_Test is UnitBase { IEmailRecoveryManager.RecoveryConfig(newDelay, newExpiry); vm.startPrank(accountAddress); + vm.expectEmit(); + emit IEmailRecoveryManager.RecoveryConfigUpdated( + accountAddress, recoveryConfig.delay, recoveryConfig.expiry + ); emailRecoveryManager.updateRecoveryConfig(recoveryConfig); recoveryConfig = emailRecoveryManager.getRecoveryConfig(accountAddress); diff --git a/test/unit/EmailRecoveryManagerHarness.sol b/test/unit/EmailRecoveryManagerHarness.sol index 3523937..c3a73c6 100644 --- a/test/unit/EmailRecoveryManagerHarness.sol +++ b/test/unit/EmailRecoveryManagerHarness.sol @@ -47,8 +47,9 @@ contract EmailRecoveryManagerHarness is EmailRecoveryManager { uint256 threshold ) external + returns (uint256, uint256) { - setupGuardians(account, guardians, weights, threshold); + return setupGuardians(account, guardians, weights, threshold); } function exposed_updateGuardianStatus( diff --git a/test/unit/EmailRecoveryUniversalFactory/deployUniversalEmailRecoveryModule.t.sol b/test/unit/EmailRecoveryUniversalFactory/deployUniversalEmailRecoveryModule.t.sol index 686f43a..27e259d 100644 --- a/test/unit/EmailRecoveryUniversalFactory/deployUniversalEmailRecoveryModule.t.sol +++ b/test/unit/EmailRecoveryUniversalFactory/deployUniversalEmailRecoveryModule.t.sol @@ -10,7 +10,7 @@ import { EmailRecoverySubjectHandler } from "src/handlers/EmailRecoverySubjectHa import { EmailRecoveryManager } from "src/EmailRecoveryManager.sol"; import { UniversalEmailRecoveryModule } from "src/modules/UniversalEmailRecoveryModule.sol"; -contract EmailRecoveryFactory_deployUniversalEmailRecoveryModule_Test is UnitBase { +contract EmailRecoveryUniversalFactory_deployUniversalEmailRecoveryModule_Test is UnitBase { function setUp() public override { super.setUp(); } @@ -51,6 +51,10 @@ contract EmailRecoveryFactory_deployUniversalEmailRecoveryModule_Test is UnitBas address(emailRecoveryUniversalFactory) ); + vm.expectEmit(); + emit EmailRecoveryUniversalFactory.UniversalEmailRecoveryModuleDeployed( + expectedModule, expectedManager, expectedSubjectHandler + ); (address emailRecoveryModule, address emailRecoveryManager, address subjectHandler) = emailRecoveryUniversalFactory.deployUniversalEmailRecoveryModule( subjectHandlerSalt, diff --git a/test/unit/libraries/GuardianUtils/addGuardian.t.sol b/test/unit/libraries/GuardianUtils/addGuardian.t.sol index ce6df84..40fd3d7 100644 --- a/test/unit/libraries/GuardianUtils/addGuardian.t.sol +++ b/test/unit/libraries/GuardianUtils/addGuardian.t.sol @@ -68,7 +68,7 @@ contract GuardianUtils_addGuardian_Test is UnitBase { vm.startPrank(accountAddress); vm.expectEmit(); - emit GuardianUtils.AddedGuardian(accountAddress, newGuardian); + emit GuardianUtils.AddedGuardian(accountAddress, newGuardian, newGuardianWeight); emailRecoveryManager.addGuardian(newGuardian, newGuardianWeight); GuardianStorage memory guardianStorage = diff --git a/test/unit/libraries/GuardianUtils/removeGuardian.t.sol b/test/unit/libraries/GuardianUtils/removeGuardian.t.sol index aaefb7e..995c3d3 100644 --- a/test/unit/libraries/GuardianUtils/removeGuardian.t.sol +++ b/test/unit/libraries/GuardianUtils/removeGuardian.t.sol @@ -78,6 +78,8 @@ contract GuardianUtils_removeGuardian_Test is UnitBase { acceptGuardian(accountSalt2); // weight = 2 vm.startPrank(accountAddress); + vm.expectEmit(); + emit GuardianUtils.RemovedGuardian(accountAddress, guardian, guardianWeights[0]); emailRecoveryManager.removeGuardian(guardian); GuardianStorage memory guardianStorage = diff --git a/test/unit/libraries/GuardianUtils/setupGuardians.t.sol b/test/unit/libraries/GuardianUtils/setupGuardians.t.sol index db4ef46..14afcde 100644 --- a/test/unit/libraries/GuardianUtils/setupGuardians.t.sol +++ b/test/unit/libraries/GuardianUtils/setupGuardians.t.sol @@ -97,7 +97,7 @@ contract GuardianUtils_setupGuardians_Test is UnitBase { instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); vm.stopPrank(); - emailRecoveryManager.exposed_setupGuardians( + (uint256 guardianCount, uint256 totalWeight) = emailRecoveryManager.exposed_setupGuardians( accountAddress, guardians, guardianWeights, threshold ); @@ -114,6 +114,9 @@ contract GuardianUtils_setupGuardians_Test is UnitBase { assertEq(uint256(guardianStorage3.status), uint256(GuardianStatus.REQUESTED)); assertEq(guardianStorage3.weight, guardianWeights[2]); + assertEq(guardianCount, expectedGuardianCount); + assertEq(totalWeight, expectedTotalWeight); + IEmailRecoveryManager.GuardianConfig memory guardianConfig = emailRecoveryManager.getGuardianConfig(accountAddress); assertEq(guardianConfig.guardianCount, expectedGuardianCount); diff --git a/test/unit/libraries/GuardianUtils/updateGuardianStatus.t.sol b/test/unit/libraries/GuardianUtils/updateGuardianStatus.t.sol index 5620c90..5a90a59 100644 --- a/test/unit/libraries/GuardianUtils/updateGuardianStatus.t.sol +++ b/test/unit/libraries/GuardianUtils/updateGuardianStatus.t.sol @@ -74,6 +74,8 @@ contract GuardianUtils_updateGuardianStatus_Test is UnitBase { function test_UpdateGuardianStatus_UpdatesStatusToACCEPTED() public { GuardianStatus newStatus = GuardianStatus.ACCEPTED; + vm.expectEmit(); + emit GuardianUtils.GuardianStatusUpdated(accountAddress, guardian1, newStatus); emailRecoveryManager.exposed_updateGuardianStatus(accountAddress, guardian1, newStatus); GuardianStorage memory guardianStorage = diff --git a/test/unit/modules/EmailRecoveryModule/recover.t.sol b/test/unit/modules/EmailRecoveryModule/recover.t.sol index 69254c8..6e32dec 100644 --- a/test/unit/modules/EmailRecoveryModule/recover.t.sol +++ b/test/unit/modules/EmailRecoveryModule/recover.t.sol @@ -37,6 +37,8 @@ contract EmailRecoveryModule_recover_Test is EmailRecoveryModuleBase { function test_Recover_Succeeds() public { vm.startPrank(emailRecoveryManagerAddress); + vm.expectEmit(); + emit EmailRecoveryModule.RecoveryExecuted(accountAddress, validatorAddress); emailRecoveryModule.recover(accountAddress, recoveryCalldata); address updatedOwner = validator.owners(accountAddress); diff --git a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol index 5724f87..a6d8335 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol @@ -114,6 +114,12 @@ contract UniversalEmailRecoveryModule_allowValidatorRecovery_Test is UnitBase { }); vm.startPrank(accountAddress); + vm.expectEmit(); + emit UniversalEmailRecoveryModule.NewValidatorRecovery({ + account: accountAddress, + validator: newValidatorAddress, + recoverySelector: functionSelector + }); emailRecoveryModule.allowValidatorRecovery(newValidatorAddress, "", functionSelector); address[] memory allowedValidators = diff --git a/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol index a224206..ad80fda 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol @@ -145,6 +145,12 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase address prevValidator = allowedValidators.findPrevious(validatorAddress); vm.startPrank(accountAddress); + vm.expectEmit(); + emit UniversalEmailRecoveryModule.RemovedValidatorRecovery({ + account: accountAddress, + validator: validatorAddress, + recoverySelector: functionSelector + }); emailRecoveryModule.disallowValidatorRecovery( validatorAddress, prevValidator, "", functionSelector ); diff --git a/test/unit/modules/UniversalEmailRecoveryModule/recover.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/recover.t.sol index 27d689d..eac36ca 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/recover.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/recover.t.sol @@ -43,6 +43,8 @@ contract UniversalEmailRecoveryModule_recover_Test is UnitBase { function test_Recover_Succeeds() public { vm.startPrank(emailRecoveryManagerAddress); + vm.expectEmit(); + emit UniversalEmailRecoveryModule.RecoveryExecuted(accountAddress, validatorAddress); emailRecoveryModule.recover(accountAddress, recoveryCalldata); address updatedOwner = validator.owners(accountAddress); From 8a4afd8f07afa600d39cf2b398cf6e30a069e9f1 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 23 Jul 2024 16:23:09 +0100 Subject: [PATCH 15/30] W6: Missing AddedGuardian event emission in setupGuardians function --- src/EmailRecoveryManager.sol | 6 +++ src/interfaces/IEmailRecoveryManager.sol | 1 + src/libraries/GuardianUtils.sol | 37 ++----------------- .../EmailRecoveryManager/addGuardian.t.sol | 14 +++++++ test/unit/assertErrorSelectors.t.sol | 1 + .../libraries/GuardianUtils/addGuardian.t.sol | 10 ----- .../GuardianUtils/setupGuardians.t.sol | 4 ++ 7 files changed, 29 insertions(+), 44 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 46b1626..adaab23 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -547,6 +547,12 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco * @param weight The weight assigned to the guardian */ function addGuardian(address guardian, uint256 weight) external onlyWhenNotRecovering { + // Threshold can only be 0 at initialization. + // Check ensures that setup function should be called first + if (guardianConfigs[msg.sender].threshold == 0) { + revert SetupNotCalled(); + } + guardiansStorage.addGuardian(guardianConfigs, msg.sender, guardian, weight); } diff --git a/src/interfaces/IEmailRecoveryManager.sol b/src/interfaces/IEmailRecoveryManager.sol index 1b93acc..c493ceb 100644 --- a/src/interfaces/IEmailRecoveryManager.sol +++ b/src/interfaces/IEmailRecoveryManager.sol @@ -95,6 +95,7 @@ interface IEmailRecoveryManager { error InvalidCalldataHash(); error NoRecoveryInProcess(); error NotRecoveryModule(); + error SetupNotCalled(); /*////////////////////////////////////////////////////////////////////////// FUNCTIONS diff --git a/src/libraries/GuardianUtils.sol b/src/libraries/GuardianUtils.sol index 1b7737d..5bdb24a 100644 --- a/src/libraries/GuardianUtils.sol +++ b/src/libraries/GuardianUtils.sol @@ -76,41 +76,16 @@ library GuardianUtils { revert ThresholdCannotBeZero(); } - uint256 totalWeight = 0; for (uint256 i = 0; i < guardianCount; i++) { - if (guardians[i] == address(0) || guardians[i] == account) { - revert InvalidGuardianAddress(); - } - - // As long as weights are 1 or above, there will be enough total weight to reach the - // required threshold. This is because we check the guardian count cannot be less - // than the threshold and there is an equal amount of guardians to weights. - if (weights[i] == 0) { - revert InvalidGuardianWeight(); - } - - GuardianStorage memory guardianStorage = guardiansStorage[account].get(guardians[i]); - if (guardianStorage.status != GuardianStatus.NONE) { - revert AddressAlreadyGuardian(); - } - - guardiansStorage[account].set({ - key: guardians[i], - value: GuardianStorage(GuardianStatus.REQUESTED, weights[i]) - }); - totalWeight += weights[i]; + addGuardian(guardiansStorage, guardianConfigs, account, guardians[i], weights[i]); } + uint256 totalWeight = guardianConfigs[account].totalWeight; if (threshold > totalWeight) { revert ThresholdExceedsTotalWeight(); } - guardianConfigs[account] = IEmailRecoveryManager.GuardianConfig({ - guardianCount: guardianCount, - totalWeight: totalWeight, - acceptedWeight: 0, - threshold: threshold - }); + guardianConfigs[account].threshold = threshold; return (guardianCount, totalWeight); } @@ -158,12 +133,6 @@ library GuardianUtils { ) internal { - // Threshold can only be 0 at initialization. - // Check ensures that setup function should be called first - if (guardianConfigs[account].threshold == 0) { - revert SetupNotCalled(); - } - if (guardian == address(0) || guardian == account) { revert InvalidGuardianAddress(); } diff --git a/test/unit/EmailRecoveryManager/addGuardian.t.sol b/test/unit/EmailRecoveryManager/addGuardian.t.sol index 77a15ee..63ffc14 100644 --- a/test/unit/EmailRecoveryManager/addGuardian.t.sol +++ b/test/unit/EmailRecoveryManager/addGuardian.t.sol @@ -2,12 +2,16 @@ pragma solidity ^0.8.25; import { console2 } from "forge-std/console2.sol"; +import { ModuleKitHelpers } from "modulekit/ModuleKit.sol"; +import { MODULE_TYPE_EXECUTOR } from "modulekit/external/ERC7579.sol"; import { UnitBase } from "../UnitBase.t.sol"; import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol"; import { GuardianStorage, GuardianStatus } from "src/libraries/EnumerableGuardianMap.sol"; import { GuardianUtils } from "src/libraries/GuardianUtils.sol"; contract EmailRecoveryManager_addGuardian_Test is UnitBase { + using ModuleKitHelpers for *; + function setUp() public override { super.setUp(); } @@ -23,6 +27,16 @@ contract EmailRecoveryManager_addGuardian_Test is UnitBase { emailRecoveryManager.addGuardian(guardians[0], guardianWeights[0]); } + function test_AddGuardian_RevertWhen_SetupNotCalled() public { + vm.prank(accountAddress); + instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); + vm.stopPrank(); + + vm.startPrank(accountAddress); + vm.expectRevert(IEmailRecoveryManager.SetupNotCalled.selector); + emailRecoveryManager.addGuardian(guardians[0], guardianWeights[0]); + } + function test_AddGuardian_AddGuardian_Succeeds() public { address newGuardian = address(1); uint256 newGuardianWeight = 1; diff --git a/test/unit/assertErrorSelectors.t.sol b/test/unit/assertErrorSelectors.t.sol index a864777..bc77e85 100644 --- a/test/unit/assertErrorSelectors.t.sol +++ b/test/unit/assertErrorSelectors.t.sol @@ -50,6 +50,7 @@ contract LogErrorSelectors_Test is Test { assertEq(IEmailRecoveryManager.InvalidCalldataHash.selector, bytes4(0xf05609de)); assertEq(IEmailRecoveryManager.NoRecoveryInProcess.selector, bytes4(0x87434f51)); assertEq(IEmailRecoveryManager.NotRecoveryModule.selector, bytes4(0x2f6ef3d6)); + assertEq(IEmailRecoveryManager.SetupNotCalled.selector, bytes4(0xae69115b)); } function test_EmailRecoveryModule_AssertSelectors() public { diff --git a/test/unit/libraries/GuardianUtils/addGuardian.t.sol b/test/unit/libraries/GuardianUtils/addGuardian.t.sol index 40fd3d7..bd1478c 100644 --- a/test/unit/libraries/GuardianUtils/addGuardian.t.sol +++ b/test/unit/libraries/GuardianUtils/addGuardian.t.sol @@ -16,16 +16,6 @@ contract GuardianUtils_addGuardian_Test is UnitBase { super.setUp(); } - function test_AddGuardian_RevertWhen_SetupNotCalled() public { - vm.prank(accountAddress); - instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); - vm.stopPrank(); - - vm.startPrank(accountAddress); - vm.expectRevert(GuardianUtils.SetupNotCalled.selector); - emailRecoveryManager.addGuardian(guardians[0], guardianWeights[0]); - } - function test_AddGuardian_RevertWhen_InvalidGuardianAddress() public { address invalidGuardianAddress = address(0); diff --git a/test/unit/libraries/GuardianUtils/setupGuardians.t.sol b/test/unit/libraries/GuardianUtils/setupGuardians.t.sol index 14afcde..cfff2d5 100644 --- a/test/unit/libraries/GuardianUtils/setupGuardians.t.sol +++ b/test/unit/libraries/GuardianUtils/setupGuardians.t.sol @@ -57,6 +57,10 @@ contract GuardianUtils_setupGuardians_Test is UnitBase { } function test_SetupGuardians_RevertWhen_InvalidGuardianWeight() public { + vm.prank(accountAddress); + instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); + vm.stopPrank(); + guardianWeights[0] = 0; vm.expectRevert(GuardianUtils.InvalidGuardianWeight.selector); From c8d8686b44c2b15f2882a53b1f2cbedd3c4d9c85 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 23 Jul 2024 16:44:49 +0100 Subject: [PATCH 16/30] W7: ERC-4337 violation in onInstall --- README.md | 4 ++++ src/modules/EmailRecoveryModule.sol | 8 +++++--- src/modules/UniversalEmailRecoveryModule.sol | 6 ++++-- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 604e0a7..8384129 100644 --- a/README.md +++ b/README.md @@ -189,6 +189,8 @@ The `recover()` function on the module is the key entry point where recovery is When writing a custom subject handler, an account developer would likely chose to deploy a `EmailRecoveryModule` instance rather than a `UniversalEmailRecoveryModule` instance. This is because a custom subject handler would likely be specific to an validator implementation, so using the recovery module for specific validators is more appropriate than the generic recovery module. +**Note:** This module is an executor and does not abide by the 4337 validation rules. The `onInstall` function breaks the validation rules and it is possible for it to be called during account deployment in the first userOp. So you cannot install this module during account deployment as onInstall will be called as part of the validation phase. Supporting executor initialization during account deployment is not mandated by ERC7579 - if required, install this module after the account has been setup. + ### UniversalEmailRecoveryModule.sol A recovery module that recovers any validator. @@ -198,6 +200,8 @@ The `recover()` function on the module is the key entry point where recovery is `completeRecovery()` calls into the account specific recovery module and can call executeFromExecutor to execute the account specific recovery logic. The call from the executor retains the context of the account so the `msg.sender` of the next call is the account itself. This simplifies access control in the validator being recovered as it can just do a `msg.sender` check. +**Note:** This module is an executor and does not abide by the 4337 validation rules. The `onInstall` function breaks the validation rules and it is possible for it to be called during account deployment in the first userOp. So you cannot install this module during account deployment as `onInstall` will be called as part of the validation phase. Supporting executor initialization during account deployment is not mandated by ERC7579 - if required, install this module after the account has been setup. + ### EmailRecoveryFactory.sol The factory for deploying new instances of `EmailRecoveryModule.sol` and associated managers and subject handlers. Because the relationship between the recovery module and manager is security critical, The factory ensures there is a tight coupling between a deployed module, and associated manager and subject handler. diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index 5429ee0..2936247 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -67,9 +67,11 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IEmailRecoveryModule { /** * Initializes the module with the threshold and guardians - * @dev data is encoded as follows: abi.encode(isInstalledContext, - * guardians, weights, threshold, delay, expiry) - * + * @dev You cannot install this module during account deployment as it breaks the 4337 + * validation rules. ERC7579 does not mandate that executors abide by the validation rules + * during account setup - if required, install this module after the account has been setup. The + * data is encoded as follows: abi.encode(isInstalledContext, guardians, weights, threshold, + * delay, expiry) * @param data encoded data for recovery configuration */ function onInstall(bytes calldata data) external { diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index bf45eef..c7d7552 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -106,9 +106,11 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec /** * Initializes the module with the threshold and guardians - * @dev data is encoded as follows: abi.encode(validator, isInstalledContext, initialSelector, + * @dev You cannot install this module during account deployment as it breaks the 4337 + * validation rules. ERC7579 does not mandate that executors abide by the validation rules + * during account setup - if required, install this module after the account has been setup. The + * data is encoded as follows: abi.encode(validator, isInstalledContext, initialSelector, * guardians, weights, threshold, delay, expiry) - * * @param data encoded data for recovery configuration */ function onInstall(bytes calldata data) external { From 7dff8fc577330864ac105c4a3f8c7f24a95265d8 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 24 Jul 2024 11:12:04 +0100 Subject: [PATCH 17/30] I1: getTrustedRecoveryManager function returns public variable emailRecoveryManager --- src/interfaces/IEmailRecoveryModule.sol | 2 +- src/interfaces/IUniversalEmailRecoveryModule.sol | 8 +++++++- src/modules/EmailRecoveryModule.sol | 8 -------- src/modules/UniversalEmailRecoveryModule.sol | 8 -------- 4 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/interfaces/IEmailRecoveryModule.sol b/src/interfaces/IEmailRecoveryModule.sol index aaebab9..29b5263 100644 --- a/src/interfaces/IEmailRecoveryModule.sol +++ b/src/interfaces/IEmailRecoveryModule.sol @@ -3,6 +3,6 @@ pragma solidity ^0.8.25; interface IEmailRecoveryModule { function isAuthorizedToRecover(address account) external returns (bool); + function canStartRecoveryRequest(address smartAccount) external view returns (bool); function recover(address account, bytes memory recoveryCalldata) external; - function getTrustedRecoveryManager() external returns (address); } diff --git a/src/interfaces/IUniversalEmailRecoveryModule.sol b/src/interfaces/IUniversalEmailRecoveryModule.sol index 2fdc8f6..5736759 100644 --- a/src/interfaces/IUniversalEmailRecoveryModule.sol +++ b/src/interfaces/IUniversalEmailRecoveryModule.sol @@ -3,8 +3,14 @@ pragma solidity ^0.8.25; interface IUniversalEmailRecoveryModule { function isAuthorizedToRecover(address account) external returns (bool); + function canStartRecoveryRequest( + address account, + address validator + ) + external + view + returns (bool); function recover(address account, bytes memory recoveryCalldata) external; - function getTrustedRecoveryManager() external returns (address); /** * Returns validators in reverse order that they were added */ diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index 2936247..2c64340 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -172,14 +172,6 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IEmailRecoveryModule { emit RecoveryExecuted(account, validator); } - /** - * @notice Returns the address of the trusted recovery manager. - * @return address The address of the email recovery manager. - */ - function getTrustedRecoveryManager() external view returns (address) { - return emailRecoveryManager; - } - /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ /* METADATA */ /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index c7d7552..192d465 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -302,14 +302,6 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec emit RecoveryExecuted(account, validator); } - /** - * @notice Returns the address of the trusted recovery manager. - * @return address The address of the email recovery manager. - */ - function getTrustedRecoveryManager() external view returns (address) { - return emailRecoveryManager; - } - /** * @notice Retrieves the list of allowed validators for a given account. * @param account The address of the account. From c7507199610320eb7042459cdce941937f1eae0a Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 24 Jul 2024 11:24:46 +0100 Subject: [PATCH 18/30] I2: Non-immutable state variables in EmailRecoveryManager --- src/EmailRecoveryManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index adaab23..9f0af7a 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -59,7 +59,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco /** * Deployer address stored to prevent frontrunning at initialization */ - address private deployer; + address private immutable deployer; /** * Account address to recovery config From 51755ee7873b534bca72ec40ad4ca474a0917db7 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 24 Jul 2024 11:45:49 +0100 Subject: [PATCH 19/30] I3: Misleading naming --- src/EmailRecoveryManager.sol | 6 +++--- src/interfaces/IEmailRecoveryModule.sol | 2 +- src/interfaces/IUniversalEmailRecoveryModule.sol | 2 +- src/modules/EmailRecoveryModule.sol | 2 +- src/modules/UniversalEmailRecoveryModule.sol | 2 +- ...rizedToRecover.t.sol => isAuthorizedToBeRecovered.t.sol} | 6 +++--- test/unit/modules/EmailRecoveryModule/onInstall.t.sol | 4 ++-- test/unit/modules/EmailRecoveryModule/onUninstall.t.sol | 4 ++-- ...rizedToRecover.t.sol => isAuthorizedToBeRecovered.t.sol} | 6 +++--- 9 files changed, 17 insertions(+), 17 deletions(-) rename test/unit/modules/EmailRecoveryModule/{isAuthorizedToRecover.t.sol => isAuthorizedToBeRecovered.t.sol} (53%) rename test/unit/modules/UniversalEmailRecoveryModule/{isAuthorizedToRecover.t.sol => isAuthorizedToBeRecovered.t.sol} (51%) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 9f0af7a..131d8cb 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -241,7 +241,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco revert SetupAlreadyCalled(); } - if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToRecover(account)) { + if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToBeRecovered(account)) { revert RecoveryModuleNotAuthorized(); } @@ -315,7 +315,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco revert RecoveryInProcess(); } - if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToRecover(account)) { + if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToBeRecovered(account)) { revert RecoveryModuleNotAuthorized(); } @@ -359,7 +359,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco templateIdx, subjectParams, address(this) ); - if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToRecover(account)) { + if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToBeRecovered(account)) { revert RecoveryModuleNotAuthorized(); } diff --git a/src/interfaces/IEmailRecoveryModule.sol b/src/interfaces/IEmailRecoveryModule.sol index 29b5263..f5e27be 100644 --- a/src/interfaces/IEmailRecoveryModule.sol +++ b/src/interfaces/IEmailRecoveryModule.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.25; interface IEmailRecoveryModule { - function isAuthorizedToRecover(address account) external returns (bool); + function isAuthorizedToBeRecovered(address account) external returns (bool); function canStartRecoveryRequest(address smartAccount) external view returns (bool); function recover(address account, bytes memory recoveryCalldata) external; } diff --git a/src/interfaces/IUniversalEmailRecoveryModule.sol b/src/interfaces/IUniversalEmailRecoveryModule.sol index 5736759..fb6c141 100644 --- a/src/interfaces/IUniversalEmailRecoveryModule.sol +++ b/src/interfaces/IUniversalEmailRecoveryModule.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.25; interface IUniversalEmailRecoveryModule { - function isAuthorizedToRecover(address account) external returns (bool); + function isAuthorizedToBeRecovered(address account) external returns (bool); function canStartRecoveryRequest( address account, address validator diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index 2c64340..23b3721 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -127,7 +127,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IEmailRecoveryModule { * @param smartAccount The smart account to check * @return true if the module is authorized, false otherwise */ - function isAuthorizedToRecover(address smartAccount) external view returns (bool) { + function isAuthorizedToBeRecovered(address smartAccount) external view returns (bool) { return authorized[smartAccount]; } diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index 192d465..67197c1 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -249,7 +249,7 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec * @param smartAccount The smart account to check * @return true if the module is authorized, false otherwise */ - function isAuthorizedToRecover(address smartAccount) external view returns (bool) { + function isAuthorizedToBeRecovered(address smartAccount) external view returns (bool) { return getAllowedValidators(smartAccount).length > 0; } diff --git a/test/unit/modules/EmailRecoveryModule/isAuthorizedToRecover.t.sol b/test/unit/modules/EmailRecoveryModule/isAuthorizedToBeRecovered.t.sol similarity index 53% rename from test/unit/modules/EmailRecoveryModule/isAuthorizedToRecover.t.sol rename to test/unit/modules/EmailRecoveryModule/isAuthorizedToBeRecovered.t.sol index 4e3031d..78e72f7 100644 --- a/test/unit/modules/EmailRecoveryModule/isAuthorizedToRecover.t.sol +++ b/test/unit/modules/EmailRecoveryModule/isAuthorizedToBeRecovered.t.sol @@ -4,11 +4,11 @@ pragma solidity ^0.8.25; import { console2 } from "forge-std/console2.sol"; import { EmailRecoveryModuleBase } from "./EmailRecoveryModuleBase.t.sol"; -contract EmailRecoveryModule_isAuthorizedToRecover_Test is EmailRecoveryModuleBase { +contract EmailRecoveryModule_isAuthorizedToBeRecovered_Test is EmailRecoveryModuleBase { function setUp() public override { super.setUp(); } - function test_IsAuthorizedToRecover_ReturnsTrueWhenAuthorized() public view { } - function test_IsAuthorizedToRecover_ReturnsFalseWhenNotAuthorized() public view { } + function test_IsAuthorizedToBeRecovered_ReturnsTrueWhenAuthorized() public view { } + function test_IsAuthorizedToBeRecovered_ReturnsFalseWhenNotAuthorized() public view { } } diff --git a/test/unit/modules/EmailRecoveryModule/onInstall.t.sol b/test/unit/modules/EmailRecoveryModule/onInstall.t.sol index 3cc6304..028e132 100644 --- a/test/unit/modules/EmailRecoveryModule/onInstall.t.sol +++ b/test/unit/modules/EmailRecoveryModule/onInstall.t.sol @@ -59,8 +59,8 @@ contract EmailRecoveryModule_onInstall_Test is EmailRecoveryModuleBase { data: abi.encode(isInstalledContext, guardians, guardianWeights, threshold, delay, expiry) }); - bool isAuthorizedToRecover = emailRecoveryModule.isAuthorizedToRecover(accountAddress); + bool isAuthorizedToBeRecovered = emailRecoveryModule.isAuthorizedToBeRecovered(accountAddress); - assertTrue(isAuthorizedToRecover); + assertTrue(isAuthorizedToBeRecovered); } } diff --git a/test/unit/modules/EmailRecoveryModule/onUninstall.t.sol b/test/unit/modules/EmailRecoveryModule/onUninstall.t.sol index 0771f77..cf9a53f 100644 --- a/test/unit/modules/EmailRecoveryModule/onUninstall.t.sol +++ b/test/unit/modules/EmailRecoveryModule/onUninstall.t.sol @@ -20,7 +20,7 @@ contract EmailRecoveryModule_onUninstall_Test is EmailRecoveryModuleBase { instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); vm.stopPrank(); - bool isAuthorizedToRecover = emailRecoveryModule.isAuthorizedToRecover(accountAddress); - assertFalse(isAuthorizedToRecover); + bool isAuthorizedToBeRecovered = emailRecoveryModule.isAuthorizedToBeRecovered(accountAddress); + assertFalse(isAuthorizedToBeRecovered); } } diff --git a/test/unit/modules/UniversalEmailRecoveryModule/isAuthorizedToRecover.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/isAuthorizedToBeRecovered.t.sol similarity index 51% rename from test/unit/modules/UniversalEmailRecoveryModule/isAuthorizedToRecover.t.sol rename to test/unit/modules/UniversalEmailRecoveryModule/isAuthorizedToBeRecovered.t.sol index f8e7c8c..9049eef 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/isAuthorizedToRecover.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/isAuthorizedToBeRecovered.t.sol @@ -4,11 +4,11 @@ pragma solidity ^0.8.25; import { console2 } from "forge-std/console2.sol"; import { UnitBase } from "../../UnitBase.t.sol"; -contract UniversalEmailRecoveryModule_isAuthorizedToRecover_Test is UnitBase { +contract UniversalEmailRecoveryModule_isAuthorizedToBeRecovered_Test is UnitBase { function setUp() public override { super.setUp(); } - function test_IsAuthorizedToRecover_ReturnsTrueWhenAuthorized() public view { } - function test_IsAuthorizedToRecover_ReturnsFalseWhenNotAuthorized() public view { } + function test_IsAuthorizedToBeRecovered_ReturnsTrueWhenAuthorized() public view { } + function test_IsAuthorizedToBeRecovered_ReturnsFalseWhenNotAuthorized() public view { } } From 4c3a5bbc0f3ae816886535da99b39004f8d11ebd Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 24 Jul 2024 12:35:57 +0100 Subject: [PATCH 20/30] I4: Unchecked return values in EnumerableGuardianMap library --- src/libraries/GuardianUtils.sol | 23 +++++++++---------- test/unit/assertErrorSelectors.t.sol | 2 +- .../GuardianUtils/removeGuardian.t.sol | 4 ++-- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/libraries/GuardianUtils.sol b/src/libraries/GuardianUtils.sol index 5bdb24a..8bc9e4a 100644 --- a/src/libraries/GuardianUtils.sol +++ b/src/libraries/GuardianUtils.sol @@ -25,7 +25,7 @@ library GuardianUtils { error ThresholdExceedsTotalWeight(); error StatusCannotBeTheSame(); error SetupNotCalled(); - error UnauthorizedAccountForGuardian(); + error AddressNotGuardianForAccount(); /** * @notice Retrieves the guardian storage details for a given guardian and account @@ -137,20 +137,18 @@ library GuardianUtils { revert InvalidGuardianAddress(); } - GuardianStorage memory guardianStorage = guardiansStorage[account].get(guardian); - - if (guardianStorage.status != GuardianStatus.NONE) { - revert AddressAlreadyGuardian(); - } - if (weight == 0) { revert InvalidGuardianWeight(); } - guardiansStorage[account].set({ + bool success = guardiansStorage[account].set({ key: guardian, value: GuardianStorage(GuardianStatus.REQUESTED, weight) }); + if (!success) { + revert AddressAlreadyGuardian(); + } + guardianConfigs[account].guardianCount++; guardianConfigs[account].totalWeight += weight; @@ -174,9 +172,11 @@ library GuardianUtils { IEmailRecoveryManager.GuardianConfig memory guardianConfig = guardianConfigs[account]; GuardianStorage memory guardianStorage = guardiansStorage[account].get(guardian); - bool isGuardian = guardianStorage.status != GuardianStatus.NONE; - if (!isGuardian) { - revert UnauthorizedAccountForGuardian(); + bool success = guardiansStorage[account].remove(guardian); + if (!success) { + // false means that the guardian was not present in the map. This serves as a proxy that + // the account is not authorized to remove this guardian + revert AddressNotGuardianForAccount(); } // Only allow guardian removal if threshold can still be reached. @@ -184,7 +184,6 @@ library GuardianUtils { revert ThresholdExceedsTotalWeight(); } - guardiansStorage[account].remove(guardian); guardianConfigs[account].guardianCount--; guardianConfigs[account].totalWeight -= guardianStorage.weight; if (guardianStorage.status == GuardianStatus.ACCEPTED) { diff --git a/test/unit/assertErrorSelectors.t.sol b/test/unit/assertErrorSelectors.t.sol index bc77e85..02bc372 100644 --- a/test/unit/assertErrorSelectors.t.sol +++ b/test/unit/assertErrorSelectors.t.sol @@ -88,6 +88,6 @@ contract LogErrorSelectors_Test is Test { assertEq(GuardianUtils.ThresholdExceedsTotalWeight.selector, bytes4(0x3c7a2aad)); assertEq(GuardianUtils.StatusCannotBeTheSame.selector, bytes4(0x115e823f)); assertEq(GuardianUtils.SetupNotCalled.selector, bytes4(0xae69115b)); - assertEq(GuardianUtils.UnauthorizedAccountForGuardian.selector, bytes4(0xe4c3248f)); + assertEq(GuardianUtils.AddressNotGuardianForAccount.selector, bytes4(0xf3f77749)); } } diff --git a/test/unit/libraries/GuardianUtils/removeGuardian.t.sol b/test/unit/libraries/GuardianUtils/removeGuardian.t.sol index 995c3d3..eaa96b5 100644 --- a/test/unit/libraries/GuardianUtils/removeGuardian.t.sol +++ b/test/unit/libraries/GuardianUtils/removeGuardian.t.sol @@ -13,11 +13,11 @@ contract GuardianUtils_removeGuardian_Test is UnitBase { super.setUp(); } - function test_RemoveGuardian_RevertWhen_UnauthorizedAccountForGuardian() public { + function test_RemoveGuardian_RevertWhen_AddressNotGuardianForAccount() public { address unauthorizedAccount = guardian1; vm.startPrank(unauthorizedAccount); - vm.expectRevert(GuardianUtils.UnauthorizedAccountForGuardian.selector); + vm.expectRevert(GuardianUtils.AddressNotGuardianForAccount.selector); emailRecoveryManager.removeGuardian(guardian1); } From cd22e1f30a4e762332779eaf732e7edfc5890ac0 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 24 Jul 2024 18:22:25 +0100 Subject: [PATCH 21/30] I5: Use calldata in favor of memory in function parameters --- src/EmailRecoveryManager.sol | 10 +++++----- src/factories/EmailRecoveryFactory.sol | 2 +- src/factories/EmailRecoveryUniversalFactory.sol | 2 +- src/handlers/EmailRecoverySubjectHandler.sol | 4 ++-- src/handlers/SafeRecoverySubjectHandler.sol | 4 ++-- src/interfaces/IEmailRecoverySubjectHandler.sol | 2 +- src/libraries/GuardianUtils.sol | 4 ++-- src/modules/UniversalEmailRecoveryModule.sol | 2 +- test/unit/EmailRecoveryManagerHarness.sol | 4 ++-- 9 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 131d8cb..396fe91 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -225,8 +225,8 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco * @param expiry The expiry time after which the recovery attempt is invalid */ function configureRecovery( - address[] memory guardians, - uint256[] memory weights, + address[] calldata guardians, + uint256[] calldata weights, uint256 threshold, uint256 delay, uint256 expiry @@ -410,7 +410,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco * @param account The address of the account for which the recovery is being completed * @param recoveryCalldata The calldata that is passed to recover the validator */ - function completeRecovery(address account, bytes memory recoveryCalldata) public override { + function completeRecovery(address account, bytes calldata recoveryCalldata) public override { if (account == address(0)) { revert InvalidAccountAddress(); } @@ -528,8 +528,8 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco */ function setupGuardians( address account, - address[] memory guardians, - uint256[] memory weights, + address[] calldata guardians, + uint256[] calldata weights, uint256 threshold ) internal diff --git a/src/factories/EmailRecoveryFactory.sol b/src/factories/EmailRecoveryFactory.sol index de187f0..f7719c3 100644 --- a/src/factories/EmailRecoveryFactory.sol +++ b/src/factories/EmailRecoveryFactory.sol @@ -61,7 +61,7 @@ contract EmailRecoveryFactory { bytes32 subjectHandlerSalt, bytes32 recoveryManagerSalt, bytes32 recoveryModuleSalt, - bytes memory subjectHandlerBytecode, + bytes calldata subjectHandlerBytecode, address dkimRegistry, address validator, bytes4 functionSelector diff --git a/src/factories/EmailRecoveryUniversalFactory.sol b/src/factories/EmailRecoveryUniversalFactory.sol index 33994c9..057785a 100644 --- a/src/factories/EmailRecoveryUniversalFactory.sol +++ b/src/factories/EmailRecoveryUniversalFactory.sol @@ -52,7 +52,7 @@ contract EmailRecoveryUniversalFactory { bytes32 subjectHandlerSalt, bytes32 recoveryManagerSalt, bytes32 recoveryModuleSalt, - bytes memory subjectHandlerBytecode, + bytes calldata subjectHandlerBytecode, address dkimRegistry ) external diff --git a/src/handlers/EmailRecoverySubjectHandler.sol b/src/handlers/EmailRecoverySubjectHandler.sol index 09fc797..8ce1eae 100644 --- a/src/handlers/EmailRecoverySubjectHandler.sol +++ b/src/handlers/EmailRecoverySubjectHandler.sol @@ -61,7 +61,7 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { * @param subjectParams The subject parameters of the acceptance email. */ function extractRecoveredAccountFromAcceptanceSubject( - bytes[] memory subjectParams, + bytes[] calldata subjectParams, uint256 /* templateIdx */ ) public @@ -77,7 +77,7 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { * @param subjectParams The subject parameters of the recovery email. */ function extractRecoveredAccountFromRecoverySubject( - bytes[] memory subjectParams, + bytes[] calldata subjectParams, uint256 /* templateIdx */ ) public diff --git a/src/handlers/SafeRecoverySubjectHandler.sol b/src/handlers/SafeRecoverySubjectHandler.sol index 6a5e811..91e0f1d 100644 --- a/src/handlers/SafeRecoverySubjectHandler.sol +++ b/src/handlers/SafeRecoverySubjectHandler.sol @@ -69,7 +69,7 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { * @param subjectParams The subject parameters of the acceptance email. */ function extractRecoveredAccountFromAcceptanceSubject( - bytes[] memory subjectParams, + bytes[] calldata subjectParams, uint256 /* templateIdx */ ) public @@ -85,7 +85,7 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { * @param subjectParams The subject parameters of the recovery email. */ function extractRecoveredAccountFromRecoverySubject( - bytes[] memory subjectParams, + bytes[] calldata subjectParams, uint256 /* templateIdx */ ) public diff --git a/src/interfaces/IEmailRecoverySubjectHandler.sol b/src/interfaces/IEmailRecoverySubjectHandler.sol index a8f2b80..ee6c5ea 100644 --- a/src/interfaces/IEmailRecoverySubjectHandler.sol +++ b/src/interfaces/IEmailRecoverySubjectHandler.sol @@ -40,7 +40,7 @@ interface IEmailRecoverySubjectHandler { function parseRecoveryCalldataHash( uint256 templateIdx, - bytes[] calldata subjectParams + bytes[] memory subjectParams ) external returns (bytes32); diff --git a/src/libraries/GuardianUtils.sol b/src/libraries/GuardianUtils.sol index 8bc9e4a..5779721 100644 --- a/src/libraries/GuardianUtils.sol +++ b/src/libraries/GuardianUtils.sol @@ -59,8 +59,8 @@ library GuardianUtils { mapping(address => IEmailRecoveryManager.GuardianConfig) storage guardianConfigs, mapping(address => EnumerableGuardianMap.AddressToGuardianMap) storage guardiansStorage, address account, - address[] memory guardians, - uint256[] memory weights, + address[] calldata guardians, + uint256[] calldata weights, uint256 threshold ) internal diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index 67197c1..373092c 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -192,7 +192,7 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec function disallowValidatorRecovery( address validator, address prevValidator, - bytes memory isInstalledContext, + bytes calldata isInstalledContext, bytes4 recoverySelector ) public diff --git a/test/unit/EmailRecoveryManagerHarness.sol b/test/unit/EmailRecoveryManagerHarness.sol index c3a73c6..1d8b3dc 100644 --- a/test/unit/EmailRecoveryManagerHarness.sol +++ b/test/unit/EmailRecoveryManagerHarness.sol @@ -42,8 +42,8 @@ contract EmailRecoveryManagerHarness is EmailRecoveryManager { function exposed_setupGuardians( address account, - address[] memory guardians, - uint256[] memory weights, + address[] calldata guardians, + uint256[] calldata weights, uint256 threshold ) external From 142452a0e41a31ee555558b73a9dc05c2898117c Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 24 Jul 2024 19:20:32 +0100 Subject: [PATCH 22/30] I7: Missing zero-address validation in constructors --- src/EmailRecoveryManager.sol | 15 +++++++-- src/factories/EmailRecoveryFactory.sol | 9 ++++++ .../EmailRecoveryUniversalFactory.sol | 9 ++++++ src/interfaces/IEmailRecoveryManager.sol | 3 ++ src/modules/EmailRecoveryModule.sol | 12 ++++++- src/modules/UniversalEmailRecoveryModule.sol | 4 +++ .../EmailRecoveryFactory/constructor.t.sol | 32 +++++++++++++++++++ .../EmailRecoveryManager/constructor.t.sol | 30 +++++++++++++++++ .../constructor.t.sol | 32 +++++++++++++++++++ test/unit/assertErrorSelectors.t.sol | 17 ++++++++++ .../EmailRecoveryModule/constructor.t.sol | 21 ++++++++++++ .../constructor.t.sol | 6 ++++ 12 files changed, 186 insertions(+), 4 deletions(-) create mode 100644 test/unit/EmailRecoveryFactory/constructor.t.sol create mode 100644 test/unit/EmailRecoveryUniversalFactory/constructor.t.sol diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 396fe91..24ffaf4 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -88,13 +88,22 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco address _emailAuthImpl, address _subjectHandler ) { + if (_verifier == address(0)) { + revert InvalidVerifier(); + } + if (_dkimRegistry == address(0)) { + revert InvalidDkimRegistry(); + } + if (_emailAuthImpl == address(0)) { + revert InvalidEmailAuthImpl(); + } + if (_subjectHandler == address(0)) { + revert InvalidSubjectHandler(); + } verifierAddr = _verifier; dkimAddr = _dkimRegistry; emailAuthImplementationAddr = _emailAuthImpl; subjectHandler = _subjectHandler; - if (_subjectHandler == address(0)) { - revert InvalidSubjectHandler(); - } deployer = msg.sender; } diff --git a/src/factories/EmailRecoveryFactory.sol b/src/factories/EmailRecoveryFactory.sol index f7719c3..40b2f37 100644 --- a/src/factories/EmailRecoveryFactory.sol +++ b/src/factories/EmailRecoveryFactory.sol @@ -23,6 +23,9 @@ contract EmailRecoveryFactory { */ address public immutable emailAuthImpl; + error InvalidVerifier(); + error InvalidEmailAuthImpl(); + event EmailRecoveryModuleDeployed( address emailRecoveryModule, address emailRecoveryManager, @@ -32,6 +35,12 @@ contract EmailRecoveryFactory { ); constructor(address _verifier, address _emailAuthImpl) { + if (_verifier == address(0)) { + revert InvalidVerifier(); + } + if (_emailAuthImpl == address(0)) { + revert InvalidEmailAuthImpl(); + } verifier = _verifier; emailAuthImpl = _emailAuthImpl; } diff --git a/src/factories/EmailRecoveryUniversalFactory.sol b/src/factories/EmailRecoveryUniversalFactory.sol index 057785a..a7d2da4 100644 --- a/src/factories/EmailRecoveryUniversalFactory.sol +++ b/src/factories/EmailRecoveryUniversalFactory.sol @@ -20,7 +20,16 @@ contract EmailRecoveryUniversalFactory { address emailRecoveryModule, address emailRecoveryManager, address subjectHandler ); + error InvalidVerifier(); + error InvalidEmailAuthImpl(); + constructor(address _verifier, address _emailAuthImpl) { + if (_verifier == address(0)) { + revert InvalidVerifier(); + } + if (_emailAuthImpl == address(0)) { + revert InvalidEmailAuthImpl(); + } verifier = _verifier; emailAuthImpl = _emailAuthImpl; } diff --git a/src/interfaces/IEmailRecoveryManager.sol b/src/interfaces/IEmailRecoveryManager.sol index c493ceb..6ee0afb 100644 --- a/src/interfaces/IEmailRecoveryManager.sol +++ b/src/interfaces/IEmailRecoveryManager.sol @@ -74,6 +74,9 @@ interface IEmailRecoveryManager { ERRORS //////////////////////////////////////////////////////////////////////////*/ + error InvalidVerifier(); + error InvalidDkimRegistry(); + error InvalidEmailAuthImpl(); error InvalidSubjectHandler(); error InitializerNotDeployer(); error InvalidRecoveryModule(); diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index 23b3721..8293557 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -46,13 +46,23 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IEmailRecoveryModule { event RecoveryExecuted(address indexed account, address indexed validator); error InvalidSelector(bytes4 selector); + error InvalidManager(); error InvalidOnInstallData(); error InvalidValidator(address validator); error NotTrustedRecoveryManager(); error RecoveryNotAuthorizedForAccount(); constructor(address _emailRecoveryManager, address _validator, bytes4 _selector) { - if (_selector == IModule.onUninstall.selector || _selector == IModule.onInstall.selector) { + if (_emailRecoveryManager == address(0)) { + revert InvalidManager(); + } + if (_validator == address(0)) { + revert InvalidValidator(_validator); + } + if ( + _selector == IModule.onUninstall.selector || _selector == IModule.onInstall.selector + || _selector == bytes4(0) + ) { revert InvalidSelector(_selector); } diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index 373092c..c7647b0 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -44,6 +44,7 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec ); event RecoveryExecuted(address indexed account, address indexed validator); + error InvalidManager(); error InvalidSelector(bytes4 selector); error RecoveryModuleNotInitialized(); error InvalidOnInstallData(); @@ -72,6 +73,9 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec selectorToValidator; constructor(address _emailRecoveryManager) { + if (_emailRecoveryManager == address(0)) { + revert InvalidManager(); + } emailRecoveryManager = _emailRecoveryManager; } diff --git a/test/unit/EmailRecoveryFactory/constructor.t.sol b/test/unit/EmailRecoveryFactory/constructor.t.sol new file mode 100644 index 0000000..59dd094 --- /dev/null +++ b/test/unit/EmailRecoveryFactory/constructor.t.sol @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.25; + +import { console2 } from "forge-std/console2.sol"; +import { UnitBase } from "../UnitBase.t.sol"; +import { EmailRecoveryFactory } from "src/factories/EmailRecoveryFactory.sol"; + +contract EmailRecoveryFactory_constructor_Test is UnitBase { + function setUp() public override { + super.setUp(); + } + + function test_Constructor_RevertWhen_InvalidVerifier() public { + address invalidVerifier = address(0); + vm.expectRevert(EmailRecoveryFactory.InvalidVerifier.selector); + new EmailRecoveryFactory(invalidVerifier, address(emailAuthImpl)); + } + + function test_Constructor_RevertWhen_InvalidEmailAuthImpl() public { + address invalidEmailAuth = address(0); + vm.expectRevert(EmailRecoveryFactory.InvalidEmailAuthImpl.selector); + new EmailRecoveryFactory(address(verifier), invalidEmailAuth); + } + + function test_Constructor() public { + EmailRecoveryFactory emailRecoveryFactory = + new EmailRecoveryFactory(address(verifier), address(emailAuthImpl)); + + assertEq(address(verifier), emailRecoveryFactory.verifier()); + assertEq(address(emailAuthImpl), emailRecoveryFactory.emailAuthImpl()); + } +} diff --git a/test/unit/EmailRecoveryManager/constructor.t.sol b/test/unit/EmailRecoveryManager/constructor.t.sol index aeec04b..d1eae6c 100644 --- a/test/unit/EmailRecoveryManager/constructor.t.sol +++ b/test/unit/EmailRecoveryManager/constructor.t.sol @@ -11,6 +11,36 @@ contract EmailRecoveryManager_constructor_Test is UnitBase { super.setUp(); } + function test_Constructor_RevertWhen_InvalidVerifier() public { + address invalidVerifier = address(0); + vm.expectRevert(IEmailRecoveryManager.InvalidVerifier.selector); + new EmailRecoveryManager( + invalidVerifier, + address(dkimRegistry), + address(emailAuthImpl), + address(emailRecoveryHandler) + ); + } + + function test_Constructor_RevertWhen_InvalidDkimRegistry() public { + address invalidDkim = address(0); + vm.expectRevert(IEmailRecoveryManager.InvalidDkimRegistry.selector); + new EmailRecoveryManager( + address(verifier), invalidDkim, address(emailAuthImpl), address(emailRecoveryHandler) + ); + } + + function test_Constructor_RevertWhen_InvalidEmailAuthImpl() public { + address invalidEmailAuth = address(0); + vm.expectRevert(IEmailRecoveryManager.InvalidEmailAuthImpl.selector); + new EmailRecoveryManager( + address(verifier), + address(dkimRegistry), + invalidEmailAuth, + address(emailRecoveryHandler) + ); + } + function test_Constructor_RevertWhen_InvalidSubjectHandler() public { address invalidHandler = address(0); vm.expectRevert(IEmailRecoveryManager.InvalidSubjectHandler.selector); diff --git a/test/unit/EmailRecoveryUniversalFactory/constructor.t.sol b/test/unit/EmailRecoveryUniversalFactory/constructor.t.sol new file mode 100644 index 0000000..93396fe --- /dev/null +++ b/test/unit/EmailRecoveryUniversalFactory/constructor.t.sol @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.25; + +import { console2 } from "forge-std/console2.sol"; +import { UnitBase } from "../UnitBase.t.sol"; +import { EmailRecoveryUniversalFactory } from "src/factories/EmailRecoveryUniversalFactory.sol"; + +contract EmailRecoveryUniversalFactory_constructor_Test is UnitBase { + function setUp() public override { + super.setUp(); + } + + function test_Constructor_RevertWhen_InvalidVerifier() public { + address invalidVerifier = address(0); + vm.expectRevert(EmailRecoveryUniversalFactory.InvalidVerifier.selector); + new EmailRecoveryUniversalFactory(invalidVerifier, address(emailAuthImpl)); + } + + function test_Constructor_RevertWhen_InvalidEmailAuthImpl() public { + address invalidEmailAuth = address(0); + vm.expectRevert(EmailRecoveryUniversalFactory.InvalidEmailAuthImpl.selector); + new EmailRecoveryUniversalFactory(address(verifier), invalidEmailAuth); + } + + function test_Constructor() public { + EmailRecoveryUniversalFactory emailRecoveryFactory = + new EmailRecoveryUniversalFactory(address(verifier), address(emailAuthImpl)); + + assertEq(address(verifier), emailRecoveryFactory.verifier()); + assertEq(address(emailAuthImpl), emailRecoveryFactory.emailAuthImpl()); + } +} diff --git a/test/unit/assertErrorSelectors.t.sol b/test/unit/assertErrorSelectors.t.sol index 02bc372..eb303a3 100644 --- a/test/unit/assertErrorSelectors.t.sol +++ b/test/unit/assertErrorSelectors.t.sol @@ -6,6 +6,8 @@ import { console2 } from "forge-std/console2.sol"; import { EmailRecoverySubjectHandler } from "src/handlers/EmailRecoverySubjectHandler.sol"; import { SafeRecoverySubjectHandler } from "src/handlers/SafeRecoverySubjectHandler.sol"; import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol"; +import { EmailRecoveryUniversalFactory } from "src/factories/EmailRecoveryUniversalFactory.sol"; +import { EmailRecoveryFactory } from "src/factories/EmailRecoveryFactory.sol"; import { EmailRecoveryModule } from "src/modules/EmailRecoveryModule.sol"; import { UniversalEmailRecoveryModule } from "src/modules/UniversalEmailRecoveryModule.sol"; import { EnumerableGuardianMap } from "src/libraries/EnumerableGuardianMap.sol"; @@ -31,6 +33,9 @@ contract LogErrorSelectors_Test is Test { } function test_IEmailRecoveryManager_AssertSelectors() public { + assertEq(IEmailRecoveryManager.InvalidVerifier.selector, bytes4(0xbaa3de5f)); + assertEq(IEmailRecoveryManager.InvalidDkimRegistry.selector, bytes4(0x260ce05b)); + assertEq(IEmailRecoveryManager.InvalidEmailAuthImpl.selector, bytes4(0xe98100fb)); assertEq(IEmailRecoveryManager.InvalidSubjectHandler.selector, bytes4(0x436dcac5)); assertEq(IEmailRecoveryManager.InitializerNotDeployer.selector, bytes4(0x3b141fc4)); assertEq(IEmailRecoveryManager.InvalidRecoveryModule.selector, bytes4(0x7f263111)); @@ -53,8 +58,19 @@ contract LogErrorSelectors_Test is Test { assertEq(IEmailRecoveryManager.SetupNotCalled.selector, bytes4(0xae69115b)); } + function test_EmailRecoveryUniversalFactory_AssertSelectors() public { + assertEq(EmailRecoveryUniversalFactory.InvalidVerifier.selector, bytes4(0xbaa3de5f)); + assertEq(EmailRecoveryUniversalFactory.InvalidEmailAuthImpl.selector, bytes4(0xe98100fb)); + } + + function test_EmailRecoveryFactory_AssertSelectors() public { + assertEq(EmailRecoveryFactory.InvalidVerifier.selector, bytes4(0xbaa3de5f)); + assertEq(EmailRecoveryFactory.InvalidEmailAuthImpl.selector, bytes4(0xe98100fb)); + } + function test_EmailRecoveryModule_AssertSelectors() public { assertEq(EmailRecoveryModule.InvalidSelector.selector, bytes4(0x12ba286f)); + assertEq(EmailRecoveryModule.InvalidManager.selector, bytes4(0x34e70f7a)); assertEq(EmailRecoveryModule.InvalidOnInstallData.selector, bytes4(0x5c223882)); assertEq(EmailRecoveryModule.InvalidValidator.selector, bytes4(0x11d5c560)); assertEq(EmailRecoveryModule.NotTrustedRecoveryManager.selector, bytes4(0x38f1b648)); @@ -62,6 +78,7 @@ contract LogErrorSelectors_Test is Test { } function test_UniversalEmailRecoveryModule_AssertSelectors() public { + assertEq(UniversalEmailRecoveryModule.InvalidManager.selector, bytes4(0x34e70f7a)); assertEq(UniversalEmailRecoveryModule.InvalidSelector.selector, bytes4(0x12ba286f)); assertEq( UniversalEmailRecoveryModule.RecoveryModuleNotInitialized.selector, bytes4(0x0b088c23) diff --git a/test/unit/modules/EmailRecoveryModule/constructor.t.sol b/test/unit/modules/EmailRecoveryModule/constructor.t.sol index fb9f7d5..b8f1f3f 100644 --- a/test/unit/modules/EmailRecoveryModule/constructor.t.sol +++ b/test/unit/modules/EmailRecoveryModule/constructor.t.sol @@ -11,6 +11,20 @@ contract EmailRecoveryManager_constructor_Test is EmailRecoveryModuleBase { super.setUp(); } + function test_Constructor_RevertWhen_InvalidManager() public { + address invalidManager = address(0); + vm.expectRevert(EmailRecoveryModule.InvalidManager.selector); + new EmailRecoveryModule(invalidManager, validatorAddress, functionSelector); + } + + function test_Constructor_RevertWhen_InvalidValidator() public { + address invalidValidator = address(0); + vm.expectRevert( + abi.encodeWithSelector(EmailRecoveryModule.InvalidValidator.selector, invalidValidator) + ); + new EmailRecoveryModule(emailRecoveryManagerAddress, invalidValidator, functionSelector); + } + function test_Constructor_RevertWhen_UnsafeOnInstallSelector() public { vm.expectRevert( abi.encodeWithSelector( @@ -33,6 +47,13 @@ contract EmailRecoveryManager_constructor_Test is EmailRecoveryModuleBase { ); } + function test_Constructor_RevertWhen_InvalidSelector() public { + vm.expectRevert( + abi.encodeWithSelector(EmailRecoveryModule.InvalidSelector.selector, bytes4(0)) + ); + new EmailRecoveryModule(emailRecoveryManagerAddress, validatorAddress, bytes4(0)); + } + function test_Constructor() public { EmailRecoveryModule emailRecoveryModule = new EmailRecoveryModule(emailRecoveryManagerAddress, validatorAddress, functionSelector); diff --git a/test/unit/modules/UniversalEmailRecoveryModule/constructor.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/constructor.t.sol index 3f15576..b6e581c 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/constructor.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/constructor.t.sol @@ -10,6 +10,12 @@ contract UniversalEmailRecoveryManager_constructor_Test is UnitBase { super.setUp(); } + function test_Constructor_RevertWhen_InvalidManager() public { + address invalidManager = address(0); + vm.expectRevert(UniversalEmailRecoveryModule.InvalidManager.selector); + new UniversalEmailRecoveryModule(invalidManager); + } + function test_Constructor() public { UniversalEmailRecoveryModule emailRecoveryModule = new UniversalEmailRecoveryModule(emailRecoveryManagerAddress); From 2ec79b9a91ae5cc06b92a6557b5ad832657bad44 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 24 Jul 2024 19:21:08 +0100 Subject: [PATCH 23/30] I8: Modifiers not above constructors --- src/EmailRecoveryManager.sol | 20 ++++++++++---------- src/modules/UniversalEmailRecoveryModule.sol | 14 +++++++------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 24ffaf4..7d3a386 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -82,6 +82,16 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco mapping(address account => EnumerableGuardianMap.AddressToGuardianMap guardian) internal guardiansStorage; + /** + * @notice Modifier to check recovery status. Reverts if recovery is in process for the account + */ + modifier onlyWhenNotRecovering() { + if (recoveryRequests[msg.sender].currentWeight > 0) { + revert RecoveryInProcess(); + } + _; + } + constructor( address _verifier, address _dkimRegistry, @@ -117,16 +127,6 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco emailRecoveryModule = _emailRecoveryModule; } - /** - * @notice Modifier to check recovery status. Reverts if recovery is in process for the account - */ - modifier onlyWhenNotRecovering() { - if (recoveryRequests[msg.sender].currentWeight > 0) { - revert RecoveryInProcess(); - } - _; - } - /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ /* RECOVERY CONFIG, REQUEST AND TEMPLATE GETTERS */ /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index c7647b0..77bc296 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -72,13 +72,6 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec mapping(bytes4 selector => mapping(address account => address validator)) internal selectorToValidator; - constructor(address _emailRecoveryManager) { - if (_emailRecoveryManager == address(0)) { - revert InvalidManager(); - } - emailRecoveryManager = _emailRecoveryManager; - } - /** * @notice Modifier to check whether the selector is safe. Reverts if the selector is for * "onInstall" or "onUninstall" @@ -104,6 +97,13 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec _; } + constructor(address _emailRecoveryManager) { + if (_emailRecoveryManager == address(0)) { + revert InvalidManager(); + } + emailRecoveryManager = _emailRecoveryManager; + } + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ /* CONFIG */ /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ From 5ec1cfa9a900008ff7fc4bade35c87d57af2ea72 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 24 Jul 2024 19:21:38 +0100 Subject: [PATCH 24/30] I9: Safe validateRecoverySubject optimization --- src/handlers/SafeRecoverySubjectHandler.sol | 7 ++++--- .../validateRecoverySubject.t.sol | 12 +++++++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/handlers/SafeRecoverySubjectHandler.sol b/src/handlers/SafeRecoverySubjectHandler.sol index 91e0f1d..d632628 100644 --- a/src/handlers/SafeRecoverySubjectHandler.sol +++ b/src/handlers/SafeRecoverySubjectHandler.sol @@ -151,12 +151,13 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { address newOwnerInEmail = abi.decode(subjectParams[2], (address)); address recoveryModuleInEmail = abi.decode(subjectParams[3], (address)); - bool isOwner = ISafe(accountInEmail).isOwner(oldOwnerInEmail); - if (!isOwner) { + bool isOldAddressOwner = ISafe(accountInEmail).isOwner(oldOwnerInEmail); + if (!isOldAddressOwner) { revert InvalidOldOwner(); } - if (newOwnerInEmail == address(0)) { + bool isNewAddressOwner = ISafe(accountInEmail).isOwner(newOwnerInEmail); + if (isNewAddressOwner) { revert InvalidNewOwner(); } diff --git a/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol b/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol index e6d14a8..e2e87e1 100644 --- a/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol +++ b/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol @@ -66,7 +66,7 @@ contract SafeRecoverySubjectHandler_validateRecoverySubject_Test is SafeUnitBase ); } - function test_ValidateRecoverySubject_RevertWhen_InvalidNewOwner() public { + function test_ValidateRecoverySubject_RevertWhen_ZeroNewOwner() public { skipIfNotSafeAccountType(); subjectParams[2] = abi.encode(address(0)); @@ -76,6 +76,16 @@ contract SafeRecoverySubjectHandler_validateRecoverySubject_Test is SafeUnitBase ); } + function test_ValidateRecoverySubject_RevertWhen_InvalidNewOwner() public { + skipIfNotSafeAccountType(); + subjectParams[2] = abi.encode(owner1); + + vm.expectRevert(SafeRecoverySubjectHandler.InvalidNewOwner.selector); + safeRecoverySubjectHandler.validateRecoverySubject( + templateIdx, subjectParams, emailRecoveryManagerAddress + ); + } + function test_ValidateRecoverySubject_RevertWhen_RecoveryModuleAddressIsZero() public { skipIfNotSafeAccountType(); subjectParams[3] = abi.encode(address(0)); From cef7751d4d51e508844bdaffbb12316d0fff466a Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 24 Jul 2024 19:23:56 +0100 Subject: [PATCH 25/30] I10: Unused using-for directive --- src/handlers/SafeRecoverySubjectHandler.sol | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/handlers/SafeRecoverySubjectHandler.sol b/src/handlers/SafeRecoverySubjectHandler.sol index d632628..ce4310a 100644 --- a/src/handlers/SafeRecoverySubjectHandler.sol +++ b/src/handlers/SafeRecoverySubjectHandler.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.25; -import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; import { IEmailRecoverySubjectHandler } from "../interfaces/IEmailRecoverySubjectHandler.sol"; import { ISafe } from "../interfaces/ISafe.sol"; import { EmailRecoveryManager } from "../EmailRecoveryManager.sol"; @@ -11,8 +10,6 @@ import { EmailRecoveryManager } from "../EmailRecoveryManager.sol"; * This is a custom subject handler that will work with Safes and defines custom validation. */ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { - using Strings for uint256; - error InvalidSubjectParams(); error InvalidTemplateIndex(); error InvalidOldOwner(); From 397e64ce2eb78260e01451f08228f9a35c6908f6 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Thu, 25 Jul 2024 11:21:35 +0100 Subject: [PATCH 26/30] Add missing zero address check --- src/handlers/SafeRecoverySubjectHandler.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/handlers/SafeRecoverySubjectHandler.sol b/src/handlers/SafeRecoverySubjectHandler.sol index ce4310a..7159952 100644 --- a/src/handlers/SafeRecoverySubjectHandler.sol +++ b/src/handlers/SafeRecoverySubjectHandler.sol @@ -154,7 +154,7 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { } bool isNewAddressOwner = ISafe(accountInEmail).isOwner(newOwnerInEmail); - if (isNewAddressOwner) { + if (newOwnerInEmail == address(0) || isNewAddressOwner) { revert InvalidNewOwner(); } From 9c7db072f42422e4984c58c0be6d2a6e9a7c7fa8 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Sun, 28 Jul 2024 16:58:37 +0100 Subject: [PATCH 27/30] M3: Selector collisions in UniversalEmailRecoveryModule --- src/handlers/SafeRecoverySubjectHandler.sol | 3 ++- .../IUniversalEmailRecoveryModule.sol | 2 +- src/modules/UniversalEmailRecoveryModule.sol | 23 ++++++++----------- .../UniversalEmailRecoveryModuleBase.t.sol | 9 +++++--- .../SafeRecovery/SafeRecovery.t.sol | 3 ++- test/unit/SafeUnitBase.t.sol | 3 ++- test/unit/UnitBase.t.sol | 3 ++- .../UniversalEmailRecoveryModuleHarness.sol | 11 --------- .../parseRecoveryCalldataHash.t.sol | 5 ++-- .../onInstall.t.sol | 3 --- .../onUninstall.t.sol | 3 --- .../recover.t.sol | 21 ++++++++++++++++- 12 files changed, 47 insertions(+), 42 deletions(-) diff --git a/src/handlers/SafeRecoverySubjectHandler.sol b/src/handlers/SafeRecoverySubjectHandler.sol index 7159952..d8da39f 100644 --- a/src/handlers/SafeRecoverySubjectHandler.sol +++ b/src/handlers/SafeRecoverySubjectHandler.sol @@ -199,7 +199,8 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { bytes memory recoveryCallData = abi.encodeWithSignature( functionSignature, previousOwnerInLinkedList, oldOwnerInEmail, newOwnerInEmail ); - return keccak256(recoveryCallData); + bytes memory recoveryData = abi.encode(accountInEmail, recoveryCallData); + return keccak256(recoveryData); } /** diff --git a/src/interfaces/IUniversalEmailRecoveryModule.sol b/src/interfaces/IUniversalEmailRecoveryModule.sol index fb6c141..3780635 100644 --- a/src/interfaces/IUniversalEmailRecoveryModule.sol +++ b/src/interfaces/IUniversalEmailRecoveryModule.sol @@ -10,7 +10,7 @@ interface IUniversalEmailRecoveryModule { external view returns (bool); - function recover(address account, bytes memory recoveryCalldata) external; + function recover(address account, bytes memory recoveryData) external; /** * Returns validators in reverse order that they were added */ diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index 77bc296..b41eec4 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -66,11 +66,6 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec */ mapping(address validatorModule => mapping(address account => bytes4 allowedSelector)) internal allowedSelectors; - /** - * function selector to account address to validator address - */ - mapping(bytes4 selector => mapping(address account => address validator)) internal - selectorToValidator; /** * @notice Modifier to check whether the selector is safe. Reverts if the selector is for @@ -177,7 +172,6 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec validatorCount[msg.sender]++; allowedSelectors[validator][msg.sender] = recoverySelector; - selectorToValidator[recoverySelector][msg.sender] = validator; emit NewValidatorRecovery({ account: msg.sender, validator: validator, @@ -210,7 +204,6 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec } delete allowedSelectors[validator][msg.sender]; - delete selectorToValidator[recoverySelector][msg.sender]; emit RemovedValidatorRecovery({ account: msg.sender, @@ -228,7 +221,6 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec for (uint256 i; i < allowedValidators.length; i++) { bytes4 allowedSelector = allowedSelectors[allowedValidators[i]][msg.sender]; - delete selectorToValidator[allowedSelector][msg.sender]; delete allowedSelectors[allowedValidators[i]][msg.sender]; } @@ -285,17 +277,22 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec /** * @notice Executes recovery on a validator. Must be called by the trusted recovery manager * @param account The account to execute recovery for - * @param recoveryCalldata The recovery calldata that should be executed on the validator - * being recovered + * @param recoveryData The recovery calldata that should be executed on the validator being + * recovered, along with the target validator */ - function recover(address account, bytes calldata recoveryCalldata) external { + function recover(address account, bytes calldata recoveryData) external { if (msg.sender != emailRecoveryManager) { revert NotTrustedRecoveryManager(); } - bytes4 selector = bytes4(recoveryCalldata[:4]); + (address validator, bytes memory recoveryCalldata) = + abi.decode(recoveryData, (address, bytes)); + + bytes4 selector; + assembly { + selector := mload(add(recoveryCalldata, 32)) + } - address validator = selectorToValidator[selector][account]; bytes4 allowedSelector = allowedSelectors[validator][account]; if (allowedSelector != selector) { revert InvalidSelector(selector); diff --git a/test/integration/OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModuleBase.t.sol b/test/integration/OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModuleBase.t.sol index bfb9ec6..e73c0bb 100644 --- a/test/integration/OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModuleBase.t.sol +++ b/test/integration/OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModuleBase.t.sol @@ -71,9 +71,12 @@ abstract contract OwnableValidatorRecovery_UniversalEmailRecoveryModule_Base is validatorAddress = address(validator); isInstalledContext = bytes("0"); functionSelector = bytes4(keccak256(bytes("changeOwner(address)"))); - recoveryCalldata1 = abi.encodeWithSelector(functionSelector, newOwner1); - recoveryCalldata2 = abi.encodeWithSelector(functionSelector, newOwner2); - recoveryCalldata3 = abi.encodeWithSelector(functionSelector, newOwner3); + bytes memory changeOwnerCalldata1 = abi.encodeWithSelector(functionSelector, newOwner1); + bytes memory changeOwnerCalldata2 = abi.encodeWithSelector(functionSelector, newOwner2); + bytes memory changeOwnerCalldata3 = abi.encodeWithSelector(functionSelector, newOwner3); + recoveryCalldata1 = abi.encode(validatorAddress, changeOwnerCalldata1); + recoveryCalldata2 = abi.encode(validatorAddress, changeOwnerCalldata2); + recoveryCalldata3 = abi.encode(validatorAddress, changeOwnerCalldata3); calldataHash1 = keccak256(recoveryCalldata1); calldataHash2 = keccak256(recoveryCalldata2); calldataHash3 = keccak256(recoveryCalldata3); diff --git a/test/integration/SafeRecovery/SafeRecovery.t.sol b/test/integration/SafeRecovery/SafeRecovery.t.sol index 044a07e..f3456d4 100644 --- a/test/integration/SafeRecovery/SafeRecovery.t.sol +++ b/test/integration/SafeRecovery/SafeRecovery.t.sol @@ -25,9 +25,10 @@ contract SafeRecovery_Integration_Test is SafeIntegrationBase { vm.skip(true); } - bytes memory recoveryCalldata = abi.encodeWithSignature( + bytes memory swapOwnerCalldata = abi.encodeWithSignature( "swapOwner(address,address,address)", address(1), owner1, newOwner1 ); + bytes memory recoveryCalldata = abi.encode(accountAddress1, swapOwnerCalldata); bytes32 calldataHash = keccak256(recoveryCalldata); bytes[] memory subjectParamsForRecovery = new bytes[](4); diff --git a/test/unit/SafeUnitBase.t.sol b/test/unit/SafeUnitBase.t.sol index bd38126..39ba89e 100644 --- a/test/unit/SafeUnitBase.t.sol +++ b/test/unit/SafeUnitBase.t.sol @@ -77,9 +77,10 @@ abstract contract SafeUnitBase is IntegrationBase { address previousOwnerInLinkedList = address(1); // address previousOwnerInLinkedList = // safeRecoverySubjectHandler.previousOwnerInLinkedList(accountAddress, owner); - recoveryCalldata = abi.encodeWithSignature( + bytes memory swapOwnerCalldata = abi.encodeWithSignature( "swapOwner(address,address,address)", previousOwnerInLinkedList, owner1, newOwner1 ); + bytes memory recoveryCalldata = abi.encode(accountAddress1, swapOwnerCalldata); calldataHash = keccak256(recoveryCalldata); isInstalledContext = bytes("0"); diff --git a/test/unit/UnitBase.t.sol b/test/unit/UnitBase.t.sol index 79c64f8..1b76829 100644 --- a/test/unit/UnitBase.t.sol +++ b/test/unit/UnitBase.t.sol @@ -162,7 +162,8 @@ abstract contract UnitBase is RhinestoneModuleKit, Test { validatorAddress = address(validator); isInstalledContext = bytes("0"); functionSelector = bytes4(keccak256(bytes("changeOwner(address)"))); - recoveryCalldata = abi.encodeWithSelector(functionSelector, newOwner); + bytes memory changeOwnerCalldata = abi.encodeWithSelector(functionSelector, newOwner); + recoveryCalldata = abi.encode(validatorAddress, changeOwnerCalldata); calldataHash = keccak256(recoveryCalldata); // Install modules diff --git a/test/unit/UniversalEmailRecoveryModuleHarness.sol b/test/unit/UniversalEmailRecoveryModuleHarness.sol index 67da770..cc47dff 100644 --- a/test/unit/UniversalEmailRecoveryModuleHarness.sol +++ b/test/unit/UniversalEmailRecoveryModuleHarness.sol @@ -34,15 +34,4 @@ contract UniversalEmailRecoveryModuleHarness is UniversalEmailRecoveryModule { { return allowedSelectors[validator][account]; } - - function exposed_selectorToValidator( - bytes4 selector, - address account - ) - external - view - returns (address) - { - return selectorToValidator[selector][account]; - } } diff --git a/test/unit/handlers/SafeRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol b/test/unit/handlers/SafeRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol index 9e910e7..37924ba 100644 --- a/test/unit/handlers/SafeRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol +++ b/test/unit/handlers/SafeRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol @@ -28,11 +28,10 @@ contract SafeRecoverySubjectHandler_parseRecoveryCalldataHash_Test is SafeUnitBa function test_ParseRecoveryCalldataHash_Succeeds() public { skipIfNotSafeAccountType(); - bytes32 expectedCalldataHash = keccak256(recoveryCalldata); - bytes32 calldataHash = + bytes32 actualCalldataHash = safeRecoverySubjectHandler.parseRecoveryCalldataHash(templateIdx, subjectParams); - assertEq(calldataHash, expectedCalldataHash); + assertEq(actualCalldataHash, calldataHash); } } diff --git a/test/unit/modules/UniversalEmailRecoveryModule/onInstall.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/onInstall.t.sol index 1630e04..1f93542 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/onInstall.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/onInstall.t.sol @@ -52,11 +52,8 @@ contract UniversalEmailRecoveryModule_onInstall_Test is UnitBase { bytes4 allowedSelector = emailRecoveryModule.exposed_allowedSelectors(validatorAddress, accountAddress); - address allowedValidator = - emailRecoveryModule.exposed_selectorToValidator(functionSelector, accountAddress); assertEq(allowedSelector, functionSelector); - assertEq(allowedValidator, validatorAddress); address[] memory allowedValidators = emailRecoveryModule.getAllowedValidators(accountAddress); diff --git a/test/unit/modules/UniversalEmailRecoveryModule/onUninstall.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/onUninstall.t.sol index 0d17ce7..e9e20e1 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/onUninstall.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/onUninstall.t.sol @@ -29,10 +29,7 @@ contract UniversalEmailRecoveryModule_onUninstall_Test is UnitBase { bytes4 allowedSelector = emailRecoveryModule.exposed_allowedSelectors(validatorAddress, accountAddress); - address allowedValidator = - emailRecoveryModule.exposed_selectorToValidator(functionSelector, accountAddress); assertEq(allowedSelector, bytes4(0)); - assertEq(allowedValidator, address(0)); address[] memory allowedValidators = emailRecoveryModule.getAllowedValidators(accountAddress); diff --git a/test/unit/modules/UniversalEmailRecoveryModule/recover.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/recover.t.sol index eac36ca..78f8873 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/recover.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/recover.t.sol @@ -29,8 +29,9 @@ contract UniversalEmailRecoveryModule_recover_Test is UnitBase { function test_Recover_RevertWhen_InvalidCalldataSelector() public { bytes4 invalidSelector = bytes4(keccak256(bytes("wrongSelector(address,address,address)"))); - bytes memory invalidCalldata = + bytes memory changeOwnerCalldata = abi.encodeWithSelector(invalidSelector, accountAddress, recoveryModuleAddress, newOwner); + bytes memory invalidCalldata = abi.encode(accountAddress, changeOwnerCalldata); vm.startPrank(emailRecoveryManagerAddress); vm.expectRevert( @@ -41,6 +42,24 @@ contract UniversalEmailRecoveryModule_recover_Test is UnitBase { emailRecoveryModule.recover(accountAddress, invalidCalldata); } + function test_Recover_RevertWhen_InvalidZeroCalldataSelector() public { + bytes memory invalidChangeOwnerCaldata = bytes("0x"); + bytes memory invalidCalldata = abi.encode(accountAddress, invalidChangeOwnerCaldata); + + bytes4 expectedSelector; + assembly { + expectedSelector := mload(add(invalidChangeOwnerCaldata, 32)) + } + + vm.startPrank(emailRecoveryManagerAddress); + vm.expectRevert( + abi.encodeWithSelector( + UniversalEmailRecoveryModule.InvalidSelector.selector, expectedSelector + ) + ); + emailRecoveryModule.recover(accountAddress, invalidCalldata); + } + function test_Recover_Succeeds() public { vm.startPrank(emailRecoveryManagerAddress); vm.expectEmit(); From d24daebba3f9fece2b91ccf5f0c27ede29302f68 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Sun, 28 Jul 2024 19:03:45 +0100 Subject: [PATCH 28/30] W4: Gas inefficiencies in UniversalRecoveryModule --- src/modules/EmailRecoveryModule.sol | 19 +++++++------ src/modules/UniversalEmailRecoveryModule.sol | 27 +++++++++---------- .../UniversalEmailRecoveryModuleHarness.sol | 1 + 3 files changed, 23 insertions(+), 24 deletions(-) diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index 8293557..61b0e5d 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -124,31 +124,30 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IEmailRecoveryModule { /** * Check if the module is initialized - * @param smartAccount The smart account to check + * @param account The smart account to check * @return true if the module is initialized, false otherwise */ - function isInitialized(address smartAccount) external view returns (bool) { - return IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(smartAccount).threshold - != 0; + function isInitialized(address account) external view returns (bool) { + return IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(account).threshold != 0; } /** * Check if the recovery module is authorized to recover the account - * @param smartAccount The smart account to check + * @param account The smart account to check * @return true if the module is authorized, false otherwise */ - function isAuthorizedToBeRecovered(address smartAccount) external view returns (bool) { - return authorized[smartAccount]; + function isAuthorizedToBeRecovered(address account) external view returns (bool) { + return authorized[account]; } /** * Check if a recovery request can be initiated based on guardian acceptance - * @param smartAccount The smart account to check + * @param account The smart account to check * @return true if the recovery request can be started, false otherwise */ - function canStartRecoveryRequest(address smartAccount) external view returns (bool) { + function canStartRecoveryRequest(address account) external view returns (bool) { IEmailRecoveryManager.GuardianConfig memory guardianConfig = - IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(smartAccount); + IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(account); return guardianConfig.acceptedWeight >= guardianConfig.threshold; } diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index b41eec4..fcb1e6e 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -232,31 +232,30 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec /** * Check if the module is initialized - * @param smartAccount The smart account to check + * @param account The smart account to check * @return true if the module is initialized, false otherwise */ - function isInitialized(address smartAccount) public view returns (bool) { - return IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(smartAccount).threshold - != 0; + function isInitialized(address account) public view returns (bool) { + return IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(account).threshold != 0; } /** * Check if the recovery module is authorized to recover the account - * @param smartAccount The smart account to check + * @param account The smart account to check * @return true if the module is authorized, false otherwise */ - function isAuthorizedToBeRecovered(address smartAccount) external view returns (bool) { - return getAllowedValidators(smartAccount).length > 0; + function isAuthorizedToBeRecovered(address account) external view returns (bool) { + return validatorCount[account] > 0; } /** * Check if a recovery request can be initiated based on guardian acceptance - * @param smartAccount The smart account to check + * @param account The smart account to check * @param validator The validator to check * @return true if the recovery request can be started, false otherwise */ function canStartRecoveryRequest( - address smartAccount, + address account, address validator ) external @@ -264,10 +263,10 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec returns (bool) { IEmailRecoveryManager.GuardianConfig memory guardianConfig = - IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(smartAccount); + IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(account); return guardianConfig.acceptedWeight >= guardianConfig.threshold - && validators[smartAccount].contains(validator); + && validators[account].contains(validator); } /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ @@ -322,10 +321,10 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec */ function getAllowedSelectors(address account) external view returns (bytes4[] memory) { address[] memory allowedValidators = getAllowedValidators(account); - uint256 allowedValidatorsLength = allowedValidators.length; + uint256 validatorCount = allowedValidators.length; - bytes4[] memory selectors = new bytes4[](allowedValidatorsLength); - for (uint256 i; i < allowedValidatorsLength; i++) { + bytes4[] memory selectors = new bytes4[](validatorCount); + for (uint256 i; i < validatorCount; i++) { selectors[i] = allowedSelectors[allowedValidators[i]][account]; } diff --git a/test/unit/UniversalEmailRecoveryModuleHarness.sol b/test/unit/UniversalEmailRecoveryModuleHarness.sol index cc47dff..1836537 100644 --- a/test/unit/UniversalEmailRecoveryModuleHarness.sol +++ b/test/unit/UniversalEmailRecoveryModuleHarness.sol @@ -12,6 +12,7 @@ contract UniversalEmailRecoveryModuleHarness is UniversalEmailRecoveryModule { function workaround_validatorsPush(address account, address validator) external { validators[account].push(validator); + validatorCount[account]++; } function workaround_validatorsContains( From 88371b81a3dd4347dac8f2a5690c1434e86ff55f Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Mon, 29 Jul 2024 10:41:22 +0100 Subject: [PATCH 29/30] Fix build warnings --- src/handlers/EmailRecoverySubjectHandler.sol | 4 +++- src/handlers/SafeRecoverySubjectHandler.sol | 1 + src/interfaces/IEmailRecoverySubjectHandler.sol | 1 + src/modules/UniversalEmailRecoveryModule.sol | 10 +++------- .../disallowValidatorRecovery.t.sol | 14 +++++++------- .../getAllowedValidators.t.sol | 2 +- .../UniversalEmailRecoveryModule/onUninstall.t.sol | 2 +- 7 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/handlers/EmailRecoverySubjectHandler.sol b/src/handlers/EmailRecoverySubjectHandler.sol index 8ce1eae..48e4d5c 100644 --- a/src/handlers/EmailRecoverySubjectHandler.sol +++ b/src/handlers/EmailRecoverySubjectHandler.sol @@ -141,7 +141,8 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { address accountInEmail = abi.decode(subjectParams[0], (address)); address recoveryModuleInEmail = abi.decode(subjectParams[1], (address)); string memory calldataHashInEmail = abi.decode(subjectParams[2], (string)); - bytes32 calldataHash = StringUtils.hexToBytes32(calldataHashInEmail); + // hexToBytes32 validates the calldataHash is not zero bytes and has the correct length + StringUtils.hexToBytes32(calldataHashInEmail); if (accountInEmail == address(0)) { revert InvalidAccount(); @@ -172,6 +173,7 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { bytes[] calldata subjectParams ) external + pure returns (bytes32) { if (templateIdx != 0) { diff --git a/src/handlers/SafeRecoverySubjectHandler.sol b/src/handlers/SafeRecoverySubjectHandler.sol index d8da39f..8c130de 100644 --- a/src/handlers/SafeRecoverySubjectHandler.sol +++ b/src/handlers/SafeRecoverySubjectHandler.sol @@ -183,6 +183,7 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { bytes[] calldata subjectParams ) external + view returns (bytes32) { if (templateIdx != 0) { diff --git a/src/interfaces/IEmailRecoverySubjectHandler.sol b/src/interfaces/IEmailRecoverySubjectHandler.sol index ee6c5ea..aeca8a4 100644 --- a/src/interfaces/IEmailRecoverySubjectHandler.sol +++ b/src/interfaces/IEmailRecoverySubjectHandler.sol @@ -43,5 +43,6 @@ interface IEmailRecoverySubjectHandler { bytes[] memory subjectParams ) external + view returns (bytes32); } diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index fcb1e6e..7d5f6eb 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -183,14 +183,11 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec * @notice Disallows a validator and function selector that has been configured for recovery * @param validator The validator to disallow * @param prevValidator The previous validator in the validators linked list - * @param isInstalledContext additional context data that the smart account may - * interpret to identifiy conditions under which the module is installed. * @param recoverySelector The function selector to disallow */ function disallowValidatorRecovery( address validator, address prevValidator, - bytes calldata isInstalledContext, bytes4 recoverySelector ) public @@ -220,7 +217,6 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec address[] memory allowedValidators = getAllowedValidators(msg.sender); for (uint256 i; i < allowedValidators.length; i++) { - bytes4 allowedSelector = allowedSelectors[allowedValidators[i]][msg.sender]; delete allowedSelectors[allowedValidators[i]][msg.sender]; } @@ -321,10 +317,10 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec */ function getAllowedSelectors(address account) external view returns (bytes4[] memory) { address[] memory allowedValidators = getAllowedValidators(account); - uint256 validatorCount = allowedValidators.length; + uint256 validatorsLength = allowedValidators.length; - bytes4[] memory selectors = new bytes4[](validatorCount); - for (uint256 i; i < validatorCount; i++) { + bytes4[] memory selectors = new bytes4[](validatorsLength); + for (uint256 i; i < validatorsLength; i++) { selectors[i] = allowedSelectors[allowedValidators[i]][account]; } diff --git a/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol index ad80fda..4214dd3 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/disallowValidatorRecovery.t.sol @@ -26,7 +26,7 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase vm.startPrank(accountAddress); vm.expectRevert(UniversalEmailRecoveryModule.RecoveryModuleNotInitialized.selector); emailRecoveryModule.disallowValidatorRecovery( - validatorAddress, address(1), bytes("0"), functionSelector + validatorAddress, address(1), functionSelector ); } @@ -41,7 +41,7 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase ) ); emailRecoveryModule.disallowValidatorRecovery( - validatorAddress, invalidPreviousValidator, "", functionSelector + validatorAddress, invalidPreviousValidator, functionSelector ); } @@ -66,7 +66,7 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase ) ); emailRecoveryModule.disallowValidatorRecovery( - newValidatorAddress, prevValidator, "", functionSelector + newValidatorAddress, prevValidator, functionSelector ); } @@ -84,7 +84,7 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase ) ); emailRecoveryModule.disallowValidatorRecovery( - validatorAddress, prevValidator, "", invalidSelector + validatorAddress, prevValidator, invalidSelector ); allowedValidators = emailRecoveryModule.getAllowedValidators(accountAddress); @@ -100,7 +100,7 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase vm.startPrank(accountAddress); emailRecoveryModule.disallowValidatorRecovery( - validatorAddress, prevValidator, "", functionSelector + validatorAddress, prevValidator, functionSelector ); allowedValidators = emailRecoveryModule.getAllowedValidators(accountAddress); @@ -118,7 +118,7 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase vm.startPrank(accountAddress); emailRecoveryModule.disallowValidatorRecovery( - validatorAddress, prevValidator, bytes("0"), functionSelector + validatorAddress, prevValidator, functionSelector ); allowedValidators = emailRecoveryModule.getAllowedValidators(accountAddress); @@ -152,7 +152,7 @@ contract UniversalEmailRecoveryModule_disallowValidatorRecovery_Test is UnitBase recoverySelector: functionSelector }); emailRecoveryModule.disallowValidatorRecovery( - validatorAddress, prevValidator, "", functionSelector + validatorAddress, prevValidator, functionSelector ); allowedValidators = emailRecoveryModule.getAllowedValidators(accountAddress); diff --git a/test/unit/modules/UniversalEmailRecoveryModule/getAllowedValidators.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/getAllowedValidators.t.sol index 323f22b..de96c28 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/getAllowedValidators.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/getAllowedValidators.t.sol @@ -23,7 +23,7 @@ contract UniversalEmailRecoveryModule_getAllowedValidators_Test is UnitBase { vm.startPrank(accountAddress); emailRecoveryModule.disallowValidatorRecovery( - validatorAddress, prevValidator, "", functionSelector + validatorAddress, prevValidator, functionSelector ); vm.stopPrank(); diff --git a/test/unit/modules/UniversalEmailRecoveryModule/onUninstall.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/onUninstall.t.sol index e9e20e1..ae1a84f 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/onUninstall.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/onUninstall.t.sol @@ -45,7 +45,7 @@ contract UniversalEmailRecoveryModule_onUninstall_Test is UnitBase { vm.startPrank(accountAddress); emailRecoveryModule.disallowValidatorRecovery( - validatorAddress, prevValidator, "", functionSelector + validatorAddress, prevValidator, functionSelector ); vm.stopPrank(); From 5b26a9ade08257ccfcba14fe675f5343e306aa57 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Thu, 1 Aug 2024 10:23:14 +0100 Subject: [PATCH 30/30] Add additional selector checks to modules --- src/interfaces/ISafe.sol | 2 + src/modules/EmailRecoveryModule.sol | 5 +- src/modules/UniversalEmailRecoveryModule.sol | 12 +++-- .../EmailRecoveryModule/constructor.t.sol | 35 ++++++++++++++ .../allowValidatorRecovery.t.sol | 48 +++++++++++++++++++ 5 files changed, 96 insertions(+), 6 deletions(-) diff --git a/src/interfaces/ISafe.sol b/src/interfaces/ISafe.sol index ed1c7c2..e8d0adb 100644 --- a/src/interfaces/ISafe.sol +++ b/src/interfaces/ISafe.sol @@ -5,4 +5,6 @@ interface ISafe { function swapOwner(address prevOwner, address oldOwner, address newOwner) external; function isOwner(address owner) external view returns (bool); function getOwners() external view returns (address[] memory); + function setFallbackHandler(address handler) external; + function setGuard(address guard) external; } diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index 61b0e5d..d057455 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.25; 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 { IEmailRecoveryModule } from "../interfaces/IEmailRecoveryModule.sol"; import { IEmailRecoveryManager } from "../interfaces/IEmailRecoveryManager.sol"; @@ -61,7 +62,9 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IEmailRecoveryModule { } if ( _selector == IModule.onUninstall.selector || _selector == IModule.onInstall.selector - || _selector == bytes4(0) + || _selector == IERC7579Account.execute.selector + || _selector == ISafe.setFallbackHandler.selector + || _selector == ISafe.setGuard.selector || _selector == bytes4(0) ) { revert InvalidSelector(_selector); } diff --git a/src/modules/UniversalEmailRecoveryModule.sol b/src/modules/UniversalEmailRecoveryModule.sol index 7d5f6eb..74e9b8f 100644 --- a/src/modules/UniversalEmailRecoveryModule.sol +++ b/src/modules/UniversalEmailRecoveryModule.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.25; 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 { IUniversalEmailRecoveryModule } from "../interfaces/IUniversalEmailRecoveryModule.sol"; import { IEmailRecoveryManager } from "../interfaces/IEmailRecoveryManager.sol"; @@ -71,14 +72,15 @@ contract UniversalEmailRecoveryModule is ERC7579ExecutorBase, IUniversalEmailRec * @notice Modifier to check whether the selector is safe. Reverts if the selector is for * "onInstall" or "onUninstall" */ - modifier withoutUnsafeSelector(bytes4 recoverySelector) { + modifier withoutUnsafeSelector(bytes4 selector) { if ( - recoverySelector == IModule.onUninstall.selector - || recoverySelector == IModule.onInstall.selector + 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(recoverySelector); + revert InvalidSelector(selector); } - _; } diff --git a/test/unit/modules/EmailRecoveryModule/constructor.t.sol b/test/unit/modules/EmailRecoveryModule/constructor.t.sol index b8f1f3f..b182f9b 100644 --- a/test/unit/modules/EmailRecoveryModule/constructor.t.sol +++ b/test/unit/modules/EmailRecoveryModule/constructor.t.sol @@ -3,6 +3,8 @@ pragma solidity ^0.8.25; import { console2 } from "forge-std/console2.sol"; import { IModule } from "erc7579/interfaces/IERC7579Module.sol"; +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"; @@ -47,6 +49,39 @@ contract EmailRecoveryManager_constructor_Test is EmailRecoveryModuleBase { ); } + function test_Constructor_RevertWhen_UnsafeExecuteSelector() public { + vm.expectRevert( + abi.encodeWithSelector( + EmailRecoveryModule.InvalidSelector.selector, IERC7579Account.execute.selector + ) + ); + new EmailRecoveryModule( + emailRecoveryManagerAddress, validatorAddress, IERC7579Account.execute.selector + ); + } + + function test_Constructor_RevertWhen_UnsafeSetFallbackHandlerSelector() public { + vm.expectRevert( + abi.encodeWithSelector( + EmailRecoveryModule.InvalidSelector.selector, ISafe.setFallbackHandler.selector + ) + ); + new EmailRecoveryModule( + emailRecoveryManagerAddress, validatorAddress, ISafe.setFallbackHandler.selector + ); + } + + function test_Constructor_RevertWhen_UnsafeSetGuardSelector() public { + vm.expectRevert( + abi.encodeWithSelector( + EmailRecoveryModule.InvalidSelector.selector, ISafe.setGuard.selector + ) + ); + new EmailRecoveryModule( + emailRecoveryManagerAddress, validatorAddress, ISafe.setGuard.selector + ); + } + function test_Constructor_RevertWhen_InvalidSelector() public { vm.expectRevert( abi.encodeWithSelector(EmailRecoveryModule.InvalidSelector.selector, bytes4(0)) diff --git a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol index a6d8335..cb2b737 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/allowValidatorRecovery.t.sol @@ -5,6 +5,8 @@ import { console2 } from "forge-std/console2.sol"; import { ModuleKitHelpers } from "modulekit/ModuleKit.sol"; import { MODULE_TYPE_EXECUTOR, MODULE_TYPE_VALIDATOR } from "modulekit/external/ERC7579.sol"; import { IModule } from "erc7579/interfaces/IERC7579Module.sol"; +import { IERC7579Account } from "erc7579/interfaces/IERC7579Account.sol"; +import { ISafe } from "src/interfaces/ISafe.sol"; import { SentinelListLib } from "sentinellist/SentinelList.sol"; import { OwnableValidator } from "src/test/OwnableValidator.sol"; import { UniversalEmailRecoveryModule } from "src/modules/UniversalEmailRecoveryModule.sol"; @@ -50,6 +52,52 @@ 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)) + ); + vm.startPrank(accountAddress); + emailRecoveryModule.allowValidatorRecovery(validatorAddress, bytes("0"), bytes4(0)); + } + function test_AllowValidatorRecovery_RevertWhen_InvalidValidator() public { OwnableValidator newValidator = new OwnableValidator(); address newValidatorAddress = address(newValidator);