Add onlyWhenActive modifier to GuardianManager

zkemail · Nov 2, 2024 · a40aecf · a40aecf
1 parent 228e0fb
commit a40aecf
Show file tree

Hide file tree

Showing 17 changed files with 73 additions and 23 deletions.
diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol
@@ -81,13 +81,6 @@ abstract contract EmailRecoveryManager is
     mapping(address account => PreviousRecoveryRequest previousRecoveryRequest) internal
         previousRecoveryRequests;
 
-    modifier onlyWhenActive() {
-        if (killSwitchEnabled) {
-            revert KillSwitchEnabled();
-        }
-        _;
-    }
-
     constructor(
         address _verifier,
         address _dkimRegistry,

diff --git a/src/GuardianManager.sol b/src/GuardianManager.sol
@@ -40,6 +40,18 @@ abstract contract GuardianManager is IGuardianManager {
         _;
     }
 
+    /**
+     * @notice Modifier to check if the kill switch has been enabled
+     * @dev This impacts EmailRecoveryManager & GuardianManager
+     */
+    modifier onlyWhenActive() {
+        bool killSwitchEnabled = IEmailRecoveryManager(address(this)).killSwitchEnabled();
+        if (killSwitchEnabled) {
+            revert KillSwitchEnabled();
+        }
+        _;
+    }
+
     /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
     /*                       GUARDIAN LOGIC                       */
     /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
@@ -120,7 +132,14 @@ abstract contract GuardianManager is IGuardianManager {
      * @param guardian The address of the guardian to be added
      * @param weight The weight assigned to the guardian
      */
-    function addGuardian(address guardian, uint256 weight) public onlyWhenNotRecovering {
+    function addGuardian(
+        address guardian,
+        uint256 weight
+    )
+        public
+        onlyWhenNotRecovering
+        onlyWhenActive
+    {
         // Threshold can only be 0 at initialization.
         // Check ensures that setup function should be called first
         if (guardianConfigs[msg.sender].threshold == 0) {
@@ -165,7 +184,7 @@ abstract contract GuardianManager is IGuardianManager {
      * no recovery is in process
      * @param guardian The address of the guardian to be removed
      */
-    function removeGuardian(address guardian) external onlyWhenNotRecovering {
+    function removeGuardian(address guardian) external onlyWhenNotRecovering onlyWhenActive {
         GuardianConfig memory guardianConfig = guardianConfigs[msg.sender];
         GuardianStorage memory guardianStorage = guardiansStorage[msg.sender].get(guardian);
 
@@ -197,7 +216,7 @@ abstract contract GuardianManager is IGuardianManager {
      * only if no recovery is in process
      * @param threshold The new threshold for guardian approvals
      */
-    function changeThreshold(uint256 threshold) external onlyWhenNotRecovering {
+    function changeThreshold(uint256 threshold) external onlyWhenNotRecovering onlyWhenActive {
         // Threshold can only be 0 at initialization.
         // Check ensures that setup function should be called first
         if (guardianConfigs[msg.sender].threshold == 0) {

diff --git a/src/interfaces/IEmailRecoveryManager.sol b/src/interfaces/IEmailRecoveryManager.sol
@@ -91,7 +91,6 @@ interface IEmailRecoveryManager {
     /*                           ERRORS                           */
     /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
 
-    error KillSwitchEnabled();
     error InvalidVerifier();
     error InvalidDkimRegistry();
     error InvalidEmailAuthImpl();
@@ -124,6 +123,8 @@ interface IEmailRecoveryManager {
     /*                          FUNCTIONS                         */
     /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
 
+    function killSwitchEnabled() external returns (bool);
+
     function getRecoveryConfig(address account) external view returns (RecoveryConfig memory);
 
     function getRecoveryRequest(address account)

diff --git a/src/interfaces/IGuardianManager.sol b/src/interfaces/IGuardianManager.sol
@@ -28,6 +28,7 @@ interface IGuardianManager {
     event ChangedThreshold(address indexed account, uint256 threshold);
 
     error RecoveryInProcess();
+    error KillSwitchEnabled();
     error IncorrectNumberOfWeights(uint256 guardianCount, uint256 weightCount);
     error ThresholdCannotBeZero();
     error InvalidGuardianAddress(address guardian);

diff --git a/test/integration/OwnableValidatorRecovery/EmailRecoveryModule/EmailRecoveryModule.t.sol b/test/integration/OwnableValidatorRecovery/EmailRecoveryModule/EmailRecoveryModule.t.sol
@@ -352,7 +352,7 @@ contract OwnableValidatorRecovery_EmailRecoveryModule_Integration_Test is
         executeRecoveryFlowForAccount(accountAddress1, guardians1, recoveryDataHash1, recoveryData1);
 
         // new module should not be useable
-        vm.expectRevert(IEmailRecoveryManager.KillSwitchEnabled.selector);
+        vm.expectRevert(IGuardianManager.KillSwitchEnabled.selector);
         EmailAccountRecovery(newRecoveryModuleAddress).completeRecovery(
             accountAddress1, abi.encodePacked("1")
         );
@@ -386,7 +386,7 @@ contract OwnableValidatorRecovery_EmailRecoveryModule_Integration_Test is
 
         instance1.uninstallModule(MODULE_TYPE_EXECUTOR, emailRecoveryModuleAddress, "");
         // the second module installation should fail after the kill switch is enabled.
-        instance1.expect4337Revert(IEmailRecoveryManager.KillSwitchEnabled.selector);
+        instance1.expect4337Revert(IGuardianManager.KillSwitchEnabled.selector);
         instance1.installModule({
             moduleTypeId: MODULE_TYPE_EXECUTOR,
             module: emailRecoveryModuleAddress,

diff --git a/.../OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModule.t.sol b/.../OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModule.t.sol
@@ -486,7 +486,7 @@ contract OwnableValidatorRecovery_UniversalEmailRecoveryModule_Integration_Test
         string memory currentAccountType = vm.envOr("ACCOUNT_TYPE", string(""));
         if (bytes(currentAccountType).length != 0) {
             vm.skip(true);
-        } 
+        }
         executeRecoveryFlowForAccount(accountAddress1, guardians1, recoveryDataHash1, recoveryData1);
         address updatedOwner1 = validator.owners(accountAddress1);
         assertEq(updatedOwner1, newOwner1);
@@ -497,7 +497,7 @@ contract OwnableValidatorRecovery_UniversalEmailRecoveryModule_Integration_Test
         IEmailRecoveryManager(emailRecoveryModuleAddress).toggleKillSwitch();
         vm.stopPrank();
 
-        instance1.expect4337Revert(IEmailRecoveryManager.KillSwitchEnabled.selector);
+        instance1.expect4337Revert(IGuardianManager.KillSwitchEnabled.selector);
         instance1.installModule({
             moduleTypeId: MODULE_TYPE_EXECUTOR,
             module: emailRecoveryModuleAddress,

diff --git a/test/unit/EmailRecoveryManager/acceptGuardian.t.sol b/test/unit/EmailRecoveryManager/acceptGuardian.t.sol
@@ -38,7 +38,7 @@ contract EmailRecoveryManager_acceptGuardian_Test is UnitBase {
         emailRecoveryModule.toggleKillSwitch();
         vm.stopPrank();
 
-        vm.expectRevert(IEmailRecoveryManager.KillSwitchEnabled.selector);
+        vm.expectRevert(IGuardianManager.KillSwitchEnabled.selector);
         emailRecoveryModule.exposed_acceptGuardian(
             guardians1[0], templateIdx, commandParams, nullifier
         );

diff --git a/test/unit/EmailRecoveryManager/cancelExpiredRecovery.t.sol b/test/unit/EmailRecoveryManager/cancelExpiredRecovery.t.sol
@@ -3,6 +3,7 @@ pragma solidity ^0.8.25;
 
 import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";
 import { UnitBase } from "../UnitBase.t.sol";
+import { IGuardianManager } from "src/interfaces/IGuardianManager.sol";
 import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol";
 
 contract EmailRecoveryManager_cancelExpiredRecovery_Test is UnitBase {
@@ -17,7 +18,7 @@ contract EmailRecoveryManager_cancelExpiredRecovery_Test is UnitBase {
         emailRecoveryModule.toggleKillSwitch();
         vm.stopPrank();
 
-        vm.expectRevert(IEmailRecoveryManager.KillSwitchEnabled.selector);
+        vm.expectRevert(IGuardianManager.KillSwitchEnabled.selector);
         emailRecoveryModule.cancelExpiredRecovery(accountAddress1);
     }
 

diff --git a/test/unit/EmailRecoveryManager/cancelRecovery.t.sol b/test/unit/EmailRecoveryManager/cancelRecovery.t.sol
@@ -3,6 +3,7 @@ pragma solidity ^0.8.25;
 
 import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";
 import { UnitBase } from "../UnitBase.t.sol";
+import { IGuardianManager } from "src/interfaces/IGuardianManager.sol";
 import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol";
 
 contract EmailRecoveryManager_cancelRecovery_Test is UnitBase {
@@ -17,7 +18,7 @@ contract EmailRecoveryManager_cancelRecovery_Test is UnitBase {
         emailRecoveryModule.toggleKillSwitch();
         vm.stopPrank();
 
-        vm.expectRevert(IEmailRecoveryManager.KillSwitchEnabled.selector);
+        vm.expectRevert(IGuardianManager.KillSwitchEnabled.selector);
         emailRecoveryModule.cancelRecovery();
     }
 

diff --git a/test/unit/EmailRecoveryManager/completeRecovery.t.sol b/test/unit/EmailRecoveryManager/completeRecovery.t.sol
@@ -2,6 +2,7 @@
 pragma solidity ^0.8.25;
 
 import { UnitBase } from "../UnitBase.t.sol";
+import { IGuardianManager } from "src/interfaces/IGuardianManager.sol";
 import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol";
 
 contract EmailRecoveryManager_completeRecovery_Test is UnitBase {
@@ -14,7 +15,7 @@ contract EmailRecoveryManager_completeRecovery_Test is UnitBase {
         emailRecoveryModule.toggleKillSwitch();
         vm.stopPrank();
 
-        vm.expectRevert(IEmailRecoveryManager.KillSwitchEnabled.selector);
+        vm.expectRevert(IGuardianManager.KillSwitchEnabled.selector);
         emailRecoveryModule.completeRecovery(accountAddress1, recoveryData);
     }
 

diff --git a/test/unit/EmailRecoveryManager/configureRecovery.t.sol b/test/unit/EmailRecoveryManager/configureRecovery.t.sol
@@ -21,7 +21,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase {
         vm.stopPrank();
 
         vm.startPrank(accountAddress1);
-        vm.expectRevert(IEmailRecoveryManager.KillSwitchEnabled.selector);
+        vm.expectRevert(IGuardianManager.KillSwitchEnabled.selector);
         emailRecoveryModule.exposed_configureRecovery(
             guardians1, guardianWeights, threshold, delay, expiry
         );

diff --git a/test/unit/EmailRecoveryManager/processRecovery.t.sol b/test/unit/EmailRecoveryManager/processRecovery.t.sol
@@ -9,6 +9,7 @@ import { CommandHandlerType } from "../../Base.t.sol";
 import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol";
 import { IEmailRecoveryCommandHandler } from "src/interfaces/IEmailRecoveryCommandHandler.sol";
 import { GuardianStatus } from "src/libraries/EnumerableGuardianMap.sol";
+import { IGuardianManager } from "src/interfaces/IGuardianManager.sol";
 
 contract EmailRecoveryManager_processRecovery_Test is UnitBase {
     using ModuleKitHelpers for *;
@@ -50,7 +51,7 @@ contract EmailRecoveryManager_processRecovery_Test is UnitBase {
         emailRecoveryModule.toggleKillSwitch();
         vm.stopPrank();
 
-        vm.expectRevert(IEmailRecoveryManager.KillSwitchEnabled.selector);
+        vm.expectRevert(IGuardianManager.KillSwitchEnabled.selector);
         emailRecoveryModule.exposed_processRecovery(
             guardians1[0], templateIdx, commandParams, nullifier
         );

diff --git a/test/unit/EmailRecoveryManager/updateRecoveryConfig.t.sol b/test/unit/EmailRecoveryManager/updateRecoveryConfig.t.sol
@@ -24,7 +24,7 @@ contract EmailRecoveryManager_updateRecoveryConfig_Test is UnitBase {
         vm.stopPrank();
 
         vm.startPrank(accountAddress1);
-        vm.expectRevert(IEmailRecoveryManager.KillSwitchEnabled.selector);
+        vm.expectRevert(IGuardianManager.KillSwitchEnabled.selector);
         emailRecoveryModule.updateRecoveryConfig(recoveryConfig);
     }
 

diff --git a/test/unit/GuardianManager/addGuardian.t.sol b/test/unit/GuardianManager/addGuardian.t.sol
@@ -14,6 +14,16 @@ contract GuardianManager_addGuardian_Test is UnitBase {
         super.setUp();
     }
 
+    function test_AddGuardian_RevertWhen_KillSwitchEnabled() public {
+        vm.prank(killSwitchAuthorizer);
+        emailRecoveryModule.toggleKillSwitch();
+        vm.stopPrank();
+
+        vm.startPrank(accountAddress1);
+        vm.expectRevert(IGuardianManager.KillSwitchEnabled.selector);
+        emailRecoveryModule.addGuardian(guardians1[0], guardianWeights[0]);
+    }
+
     function test_AddGuardian_RevertWhen_AlreadyRecovering() public {
         acceptGuardian(accountAddress1, guardians1[0], emailRecoveryModuleAddress);
         acceptGuardian(accountAddress1, guardians1[1], emailRecoveryModuleAddress);

diff --git a/test/unit/GuardianManager/changeThreshold.t.sol b/test/unit/GuardianManager/changeThreshold.t.sol
@@ -9,6 +9,16 @@ contract GuardianManager_changeThreshold_Test is UnitBase {
         super.setUp();
     }
 
+    function test_RevertWhen_KillSwitchEnabled() public {
+        vm.prank(killSwitchAuthorizer);
+        emailRecoveryModule.toggleKillSwitch();
+        vm.stopPrank();
+
+        vm.startPrank(accountAddress1);
+        vm.expectRevert(IGuardianManager.KillSwitchEnabled.selector);
+        emailRecoveryModule.changeThreshold(threshold);
+    }
+
     function test_RevertWhen_AlreadyRecovering() public {
         acceptGuardian(accountAddress1, guardians1[0], emailRecoveryModuleAddress);
         acceptGuardian(accountAddress1, guardians1[1], emailRecoveryModuleAddress);

diff --git a/test/unit/GuardianManager/removeGuardian.t.sol b/test/unit/GuardianManager/removeGuardian.t.sol
@@ -10,6 +10,18 @@ contract GuardianManager_removeGuardian_Test is UnitBase {
         super.setUp();
     }
 
+    function test_RemoveGuardian_RevertWhen_KillSwitchEnabled() public {
+        address guardian = guardians1[0];
+
+        vm.prank(killSwitchAuthorizer);
+        emailRecoveryModule.toggleKillSwitch();
+        vm.stopPrank();
+
+        vm.startPrank(accountAddress1);
+        vm.expectRevert(IGuardianManager.KillSwitchEnabled.selector);
+        emailRecoveryModule.removeGuardian(guardian);
+    }
+
     function test_RemoveGuardian_RevertWhen_AlreadyRecovering() public {
         address guardian = guardians1[0];
 

diff --git a/test/unit/assertErrorSelectors.t.sol b/test/unit/assertErrorSelectors.t.sol
@@ -48,7 +48,6 @@ contract LogErrorSelectors_Test is Test {
     }
 
     function test_IEmailRecoveryManager_AssertSelectors() public pure {
-        assertEq(IEmailRecoveryManager.KillSwitchEnabled.selector, bytes4(0xec7fb2a0));
         assertEq(IEmailRecoveryManager.InvalidVerifier.selector, bytes4(0xbaa3de5f));
         assertEq(IEmailRecoveryManager.InvalidDkimRegistry.selector, bytes4(0x260ce05b));
         assertEq(IEmailRecoveryManager.InvalidEmailAuthImpl.selector, bytes4(0xe98100fb));
@@ -117,6 +116,7 @@ contract LogErrorSelectors_Test is Test {
 
     function test_IGuardianManager_AssertSelectors() public pure {
         assertEq(IGuardianManager.RecoveryInProcess.selector, bytes4(0xf90ea6fc));
+        assertEq(IGuardianManager.KillSwitchEnabled.selector, bytes4(0xec7fb2a0));
         assertEq(IGuardianManager.IncorrectNumberOfWeights.selector, bytes4(0x166e79bd));
         assertEq(IGuardianManager.ThresholdCannotBeZero.selector, bytes4(0xf4124166));
         assertEq(IGuardianManager.InvalidGuardianAddress.selector, bytes4(0x1af74975));