Merge pull request #41 from zkemail/feat/audit-fix-08

Add audit fixes
zkemail · Sep 10, 2024 · f7db679 · f7db679
2 parents e2a1a43 + ee2c007
commit f7db679
Show file tree

Hide file tree

Showing 31 changed files with 668 additions and 253 deletions.
diff --git a/.solhint.json b/.solhint.json
@@ -13,6 +13,7 @@
     "no-empty-blocks": "off",
     "not-rely-on-time": "off",
     "one-contract-per-file": "off",
-    "var-name-mixedcase": "off"
+    "var-name-mixedcase": "off",
+    "immutable-vars-naming": "off"
   }
 }
diff --git a/script/DeployEmailRecoveryModule.s.sol b/script/DeployEmailRecoveryModule.s.sol
@@ -10,7 +10,7 @@ import { ECDSAOwnedDKIMRegistry } from
 import { EmailAuth } from "ether-email-auth/packages/contracts/src/EmailAuth.sol";
 import { EmailRecoveryFactory } from "src/factories/EmailRecoveryFactory.sol";
 import { OwnableValidator } from "src/test/OwnableValidator.sol";
-import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
 
 contract DeployEmailRecoveryModuleScript is Script {
     function run() public {
@@ -24,14 +24,10 @@ contract DeployEmailRecoveryModuleScript is Script {
         address initialOwner = vm.addr(vm.envUint("PRIVATE_KEY"));
 
         if (verifier == address(0)) {
-           Verifier verifierImpl = new Verifier();
-            console.log(
-                "Verifier implementation deployed at: %s",
-                address(verifierImpl)
-            );
+            Verifier verifierImpl = new Verifier();
+            console.log("Verifier implementation deployed at: %s", address(verifierImpl));
             ERC1967Proxy verifierProxy = new ERC1967Proxy(
-                address(verifierImpl),
-                abi.encodeCall(verifierImpl.initialize, (initialOwner))
+                address(verifierImpl), abi.encodeCall(verifierImpl.initialize, (initialOwner))
             );
             verifier = address(Verifier(address(verifierProxy)));
             vm.setEnv("VERIFIER", vm.toString(address(verifier)));
@@ -42,10 +38,7 @@ contract DeployEmailRecoveryModuleScript is Script {
             require(dkimRegistrySigner != address(0), "DKIM_REGISTRY_SIGNER is required");
 
             ECDSAOwnedDKIMRegistry dkimImpl = new ECDSAOwnedDKIMRegistry();
-            console.log(
-                "ECDSAOwnedDKIMRegistry implementation deployed at: %s",
-                address(dkimImpl)
-            );
+            console.log("ECDSAOwnedDKIMRegistry implementation deployed at: %s", address(dkimImpl));
             ERC1967Proxy dkimProxy = new ERC1967Proxy(
                 address(dkimImpl),
                 abi.encodeCall(dkimImpl.initialize, (initialOwner, dkimRegistrySigner))

diff --git a/script/DeploySafeNativeRecovery.s.sol b/script/DeploySafeNativeRecovery.s.sol
@@ -9,7 +9,7 @@ import { ECDSAOwnedDKIMRegistry } from
 import { EmailAuth } from "ether-email-auth/packages/contracts/src/EmailAuth.sol";
 import { SafeRecoverySubjectHandler } from "src/handlers/SafeRecoverySubjectHandler.sol";
 import { SafeEmailRecoveryModule } from "src/modules/SafeEmailRecoveryModule.sol";
-import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
 
 contract DeploySafeNativeRecovery_Script is Script {
     function run() public {
@@ -23,14 +23,10 @@ contract DeploySafeNativeRecovery_Script is Script {
         address initialOwner = vm.addr(vm.envUint("PRIVATE_KEY"));
 
         if (verifier == address(0)) {
-           Verifier verifierImpl = new Verifier();
-            console.log(
-                "Verifier implementation deployed at: %s",
-                address(verifierImpl)
-            );
+            Verifier verifierImpl = new Verifier();
+            console.log("Verifier implementation deployed at: %s", address(verifierImpl));
             ERC1967Proxy verifierProxy = new ERC1967Proxy(
-                address(verifierImpl),
-                abi.encodeCall(verifierImpl.initialize, (initialOwner))
+                address(verifierImpl), abi.encodeCall(verifierImpl.initialize, (initialOwner))
             );
             verifier = address(Verifier(address(verifierProxy)));
             vm.setEnv("VERIFIER", vm.toString(address(verifier)));
@@ -41,10 +37,7 @@ contract DeploySafeNativeRecovery_Script is Script {
             require(dkimRegistrySigner != address(0), "DKIM_REGISTRY_SIGNER is required");
 
             ECDSAOwnedDKIMRegistry dkimImpl = new ECDSAOwnedDKIMRegistry();
-            console.log(
-                "ECDSAOwnedDKIMRegistry implementation deployed at: %s",
-                address(dkimImpl)
-            );
+            console.log("ECDSAOwnedDKIMRegistry implementation deployed at: %s", address(dkimImpl));
             ERC1967Proxy dkimProxy = new ERC1967Proxy(
                 address(dkimImpl),
                 abi.encodeCall(dkimImpl.initialize, (initialOwner, dkimRegistrySigner))

diff --git a/script/DeploySafeRecovery.s.sol b/script/DeploySafeRecovery.s.sol
@@ -14,7 +14,7 @@ import { EmailAuth } from "ether-email-auth/packages/contracts/src/EmailAuth.sol
 import { Safe7579 } from "safe7579/Safe7579.sol";
 import { Safe7579Launchpad } from "safe7579/Safe7579Launchpad.sol";
 import { IERC7484 } from "safe7579/interfaces/IERC7484.sol";
-import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
 
 // 1. `source .env`
 // 2. `forge script --chain sepolia script/DeploySafeRecovery.s.sol:DeploySafeRecovery_Script
@@ -33,14 +33,10 @@ contract DeploySafeRecovery_Script is Script {
         address initialOwner = vm.addr(vm.envUint("PRIVATE_KEY"));
 
         if (verifier == address(0)) {
-           Verifier verifierImpl = new Verifier();
-            console.log(
-                "Verifier implementation deployed at: %s",
-                address(verifierImpl)
-            );
+            Verifier verifierImpl = new Verifier();
+            console.log("Verifier implementation deployed at: %s", address(verifierImpl));
             ERC1967Proxy verifierProxy = new ERC1967Proxy(
-                address(verifierImpl),
-                abi.encodeCall(verifierImpl.initialize, (initialOwner))
+                address(verifierImpl), abi.encodeCall(verifierImpl.initialize, (initialOwner))
             );
             verifier = address(Verifier(address(verifierProxy)));
             vm.setEnv("VERIFIER", vm.toString(address(verifier)));
@@ -51,10 +47,7 @@ contract DeploySafeRecovery_Script is Script {
             require(dkimRegistrySigner != address(0), "DKIM_REGISTRY_SIGNER is required");
 
             ECDSAOwnedDKIMRegistry dkimImpl = new ECDSAOwnedDKIMRegistry();
-            console.log(
-                "ECDSAOwnedDKIMRegistry implementation deployed at: %s",
-                address(dkimImpl)
-            );
+            console.log("ECDSAOwnedDKIMRegistry implementation deployed at: %s", address(dkimImpl));
             ERC1967Proxy dkimProxy = new ERC1967Proxy(
                 address(dkimImpl),
                 abi.encodeCall(dkimImpl.initialize, (initialOwner, dkimRegistrySigner))

diff --git a/script/DeployUniversalEmailRecoveryModule.s.sol b/script/DeployUniversalEmailRecoveryModule.s.sol
@@ -9,7 +9,7 @@ import { ECDSAOwnedDKIMRegistry } from
     "ether-email-auth/packages/contracts/src/utils/ECDSAOwnedDKIMRegistry.sol";
 import { EmailAuth } from "ether-email-auth/packages/contracts/src/EmailAuth.sol";
 import { EmailRecoveryUniversalFactory } from "src/factories/EmailRecoveryUniversalFactory.sol";
-import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
 
 contract DeployUniversalEmailRecoveryModuleScript is Script {
     function run() public {
@@ -22,14 +22,10 @@ contract DeployUniversalEmailRecoveryModuleScript is Script {
         address initialOwner = vm.addr(vm.envUint("PRIVATE_KEY"));
 
         if (verifier == address(0)) {
-           Verifier verifierImpl = new Verifier();
-            console.log(
-                "Verifier implementation deployed at: %s",
-                address(verifierImpl)
-            );
+            Verifier verifierImpl = new Verifier();
+            console.log("Verifier implementation deployed at: %s", address(verifierImpl));
             ERC1967Proxy verifierProxy = new ERC1967Proxy(
-                address(verifierImpl),
-                abi.encodeCall(verifierImpl.initialize, (initialOwner))
+                address(verifierImpl), abi.encodeCall(verifierImpl.initialize, (initialOwner))
             );
             verifier = address(Verifier(address(verifierProxy)));
             vm.setEnv("VERIFIER", vm.toString(address(verifier)));
@@ -40,10 +36,7 @@ contract DeployUniversalEmailRecoveryModuleScript is Script {
             require(dkimRegistrySigner != address(0), "DKIM_REGISTRY_SIGNER is required");
 
             ECDSAOwnedDKIMRegistry dkimImpl = new ECDSAOwnedDKIMRegistry();
-            console.log(
-                "ECDSAOwnedDKIMRegistry implementation deployed at: %s",
-                address(dkimImpl)
-            );
+            console.log("ECDSAOwnedDKIMRegistry implementation deployed at: %s", address(dkimImpl));
             ERC1967Proxy dkimProxy = new ERC1967Proxy(
                 address(dkimImpl),
                 abi.encodeCall(dkimImpl.initialize, (initialOwner, dkimRegistrySigner))

diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol
@@ -186,8 +186,8 @@ abstract contract EmailRecoveryManager is
      * @notice Configures recovery for the caller's account. This is the first core function
      * that must be called during the end-to-end recovery flow
      * @dev Can only be called once for configuration. Sets up the guardians, and validates config
-     * parameters, ensuring that no recovery is in process. It is possible to configure guardians at
-     * a later stage if neccessary
+     * parameters, ensuring that no recovery is in process. It is possible to update configuration
+     * at a later stage if neccessary
      * @param guardians An array of guardian addresses
      * @param weights An array of weights corresponding to each guardian
      * @param threshold The threshold weight required for recovery
@@ -201,7 +201,7 @@ abstract contract EmailRecoveryManager is
         uint256 delay,
         uint256 expiry
     )
-        public
+        internal
     {
         address account = msg.sender;
 
@@ -226,7 +226,9 @@ abstract contract EmailRecoveryManager is
      * that no recovery is in process.
      * @param recoveryConfig The new recovery configuration to be set for the caller's account
      */
-    function updateRecoveryConfig(RecoveryConfig memory recoveryConfig)
+    function updateRecoveryConfig(
+        RecoveryConfig memory recoveryConfig
+    )
         public
         onlyWhenNotRecovering
     {
@@ -350,6 +352,8 @@ abstract contract EmailRecoveryManager is
 
         if (recoveryRequest.recoveryDataHash == bytes32(0)) {
             recoveryRequest.recoveryDataHash = recoveryDataHash;
+            uint256 executeBefore = block.timestamp + recoveryConfigs[account].expiry;
+            recoveryRequest.executeBefore = executeBefore;
         }
 
         if (recoveryRequest.recoveryDataHash != recoveryDataHash) {
@@ -360,11 +364,11 @@ abstract contract EmailRecoveryManager is
 
         if (recoveryRequest.currentWeight >= guardianConfig.threshold) {
             uint256 executeAfter = block.timestamp + recoveryConfigs[account].delay;
-            uint256 executeBefore = block.timestamp + recoveryConfigs[account].expiry;
             recoveryRequest.executeAfter = executeAfter;
-            recoveryRequest.executeBefore = executeBefore;
 
-            emit RecoveryProcessed(account, guardian, executeAfter, executeBefore, recoveryDataHash);
+            emit RecoveryProcessed(
+                account, guardian, executeAfter, recoveryRequest.executeBefore, recoveryDataHash
+            );
         }
     }
 
@@ -422,6 +426,19 @@ abstract contract EmailRecoveryManager is
         emit RecoveryCompleted(account);
     }
 
+    /**
+     * @notice Called during completeRecovery to finalize recovery. Contains recovery module
+     * implementation-specific logic to recover an account/module
+     * @dev this is the only function that must be implemented by consuming contracts to use the
+     * email recovery manager. This does not encompass other important logic such as module
+     * installation, that logic is specific to each module and must be implemeted separately
+     * @param account The address of the account for which the recovery is being completed
+     * @param recoveryData The data that is passed to recover the validator or account.
+     * recoveryData = abi.encode(validatorOrAccount, recoveryFunctionCalldata). Although, it is
+     * possible to design a recovery module using this manager without encoding the validator or
+     * account, depending on how the handler.parseRecoveryDataHash() and module.recover() functions
+     * are implemented
+     */
     function recover(address account, bytes calldata recoveryData) internal virtual;
 
     /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
@@ -441,17 +458,49 @@ abstract contract EmailRecoveryManager is
     }
 
     /**
-     * @notice Removes all state related to an account.
+     * @notice Cancels the recovery request for a given account if it is expired.
+     * @dev Deletes the current recovery request associated with the given account if the recovery
+     * request has expired.
+     * @param account The address of the account for which the recovery is being cancelled
+     */
+    function cancelExpiredRecovery(address account) external {
+        if (recoveryRequests[account].currentWeight == 0) {
+            revert NoRecoveryInProcess();
+        }
+        if (recoveryRequests[account].executeBefore > block.timestamp) {
+            revert RecoveryHasNotExpired(
+                account, block.timestamp, recoveryRequests[account].executeBefore
+            );
+        }
+        delete recoveryRequests[account];
+        emit RecoveryCancelled(account);
+    }
+
+    /**
+     * @notice Removes all state related to msg.sender.
      * @dev In order to prevent unexpected behaviour when reinstalling account modules, the module
-     * should be deinitialized. This should include remove state accociated with an account.
+     * should be deinitialized. This should include removing state accociated with an account.
      */
     function deInitRecoveryModule() internal onlyWhenNotRecovering {
-        delete recoveryConfigs[msg.sender];
-        delete recoveryRequests[msg.sender];
+        address account = msg.sender;
+        deInitRecoveryModule(account);
+    }
+
+    /**
+     * @notice Removes all state related to an account.
+     * @dev Although this function is internal, it should be used carefully as it can be called by
+     * anyone. In order to prevent unexpected behaviour when reinstalling account modules, the
+     * module should be deinitialized. This should include removing state accociated with an
+     * account
+     * @param account The address of the account for which recovery is being deinitialized
+     */
+    function deInitRecoveryModule(address account) internal onlyWhenNotRecovering {
+        delete recoveryConfigs[account];
+        delete recoveryRequests[account];
 
-        removeAllGuardians(msg.sender);
-        delete guardianConfigs[msg.sender];
+        removeAllGuardians(account);
+        delete guardianConfigs[account];
 
-        emit RecoveryDeInitialized(msg.sender);
+        emit RecoveryDeInitialized(account);
     }
 }
diff --git a/src/handlers/EmailRecoverySubjectHandler.sol b/src/handlers/EmailRecoverySubjectHandler.sol
@@ -124,7 +124,7 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler {
         bytes[] calldata subjectParams
     )
         public
-        view
+        pure
         returns (address)
     {
         if (templateIdx != 0) {

diff --git a/src/interfaces/IEmailRecoveryManager.sol b/src/interfaces/IEmailRecoveryManager.sol
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.25;
 
-import { GuardianStorage, GuardianStatus } from "../libraries/EnumerableGuardianMap.sol";
+import { GuardianStatus } from "../libraries/EnumerableGuardianMap.sol";
 
 interface IEmailRecoveryManager {
     /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
@@ -13,12 +13,13 @@ interface IEmailRecoveryManager {
      * Config should be maintained over subsequent recovery attempts unless explicitly modified
      */
     struct RecoveryConfig {
-        uint256 delay; // the time from when recovery is started until the recovery request can be
-        // executed
+        uint256 delay; // the time from when the threshold for a recovery request has passed (when
+            // the attempt is successful), until the recovery request can be executed
         uint256 expiry; // the time from when recovery is started until the recovery request becomes
             // invalid. The recovery expiry encourages the timely execution of successful recovery
             // attempts, and reduces the risk of unauthorized access through stale or outdated
-            // requests.
+            // requests. After the recovery expiry has passed, anyone can cancel the recovery
+            // request
     }
 
     /**
@@ -77,6 +78,7 @@ interface IEmailRecoveryManager {
     error RecoveryRequestExpired(uint256 blockTimestamp, uint256 executeBefore);
     error InvalidRecoveryDataHash(bytes32 recoveryDataHash, bytes32 expectedRecoveryDataHash);
     error NoRecoveryInProcess();
+    error RecoveryHasNotExpired(address account, uint256 blockTimestamp, uint256 executeBefore);
     error RecoveryIsNotActivated();
 
     /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/

diff --git a/src/interfaces/ISafe.sol b/src/interfaces/ISafe.sol
@@ -6,13 +6,17 @@ interface ISafe {
     function getOwners() external view returns (address[] memory);
     function setFallbackHandler(address handler) external;
     function setGuard(address guard) external;
-    function execTransactionFromModule(
+    function execTransactionFromModuleReturnData(
         address to,
         uint256 value,
         bytes memory data,
         uint8 operation
     )
         external
-        returns (bool success);
-     function isModuleEnabled(address module) external view returns (bool);
+        returns (bool success, bytes memory returnData);
+    function isModuleEnabled(address module) external view returns (bool);
+    function addOwnerWithThreshold(address owner, uint256 _threshold) external;
+    function removeOwner(address prevOwner, address owner, uint256 _threshold) external;
+    function swapOwner(address prevOwner, address oldOwner, address newOwner) external;
+    function changeThreshold(uint256 _threshold) external;
 }