From c9d4f142fa79fb1673f7bf784d9388820a25807f Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Sat, 6 Jul 2024 13:01:34 +0200 Subject: [PATCH 1/5] Add native safe recovery module --- foundry.toml | 4 +- script/DeploySafeNativeRecovery.s.sol | 42 ++ src/interfaces/ISafe.sol | 9 + src/modules/SafeEmailRecoveryModule.sol | 569 ++++++++++++++++++ .../SafeNativeIntegrationBase.t.sol | 224 +++++++ .../SafeRecoveryNativeModule.t.sol | 98 +++ 6 files changed, 944 insertions(+), 2 deletions(-) create mode 100644 script/DeploySafeNativeRecovery.s.sol create mode 100644 src/modules/SafeEmailRecoveryModule.sol create mode 100644 test/integration/SafeRecovery/SafeNativeIntegrationBase.t.sol create mode 100644 test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol diff --git a/foundry.toml b/foundry.toml index beec336f..6e27de20 100644 --- a/foundry.toml +++ b/foundry.toml @@ -24,8 +24,8 @@ ignored_warnings_from = [ [rpc_endpoints] sepolia = "${BASE_SEPOLIA_RPC_URL}" -# [etherscan] -# sepolia = { key = "${BASE_SCAN_API_KEY}" } +[etherscan] +sepolia = { key = "${BASE_SCAN_API_KEY}" } [fmt] bracket_spacing = true diff --git a/script/DeploySafeNativeRecovery.s.sol b/script/DeploySafeNativeRecovery.s.sol new file mode 100644 index 00000000..087eccf2 --- /dev/null +++ b/script/DeploySafeNativeRecovery.s.sol @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.25; + +import { Script } from "forge-std/Script.sol"; +import { console } from "forge-std/console.sol"; +import { SafeEmailRecoveryModule } from "src/modules/SafeEmailRecoveryModule.sol"; +import { Verifier } from "ether-email-auth/packages/contracts/src/utils/Verifier.sol"; +import { ECDSAOwnedDKIMRegistry } from + "ether-email-auth/packages/contracts/src/utils/ECDSAOwnedDKIMRegistry.sol"; +import { EmailAuth } from "ether-email-auth/packages/contracts/src/EmailAuth.sol"; + +contract DeploySafeNativeRecovery_Script is Script { + function run() public { + vm.startBroadcast(vm.envUint("PRIVATE_KEY")); + address verifier = vm.envOr("VERIFIER", address(0)); + address dkimRegistry = vm.envOr("DKIM_REGISTRY", address(0)); + address dkimRegistrySigner = vm.envOr("SIGNER", address(0)); + address emailAuthImpl = vm.envOr("EMAIL_AUTH_IMPL", address(0)); + + if (verifier == address(0)) { + verifier = address(new Verifier()); + console.log("Deployed Verifier at", verifier); + } + + if (dkimRegistry == address(0)) { + require(dkimRegistrySigner != address(0), "DKIM_REGISTRY_SIGNER is required"); + dkimRegistry = address(new ECDSAOwnedDKIMRegistry(dkimRegistrySigner)); + console.log("Deployed DKIM Registry at", dkimRegistry); + } + + if (emailAuthImpl == address(0)) { + emailAuthImpl = address(new EmailAuth()); + console.log("Deployed Email Auth at", emailAuthImpl); + } + + address module = address(new SafeEmailRecoveryModule(verifier, emailAuthImpl, dkimRegistry)); + + console.log("Deployed Email Recovery Module at ", vm.toString(module)); + + vm.stopBroadcast(); + } +} diff --git a/src/interfaces/ISafe.sol b/src/interfaces/ISafe.sol index e8d0adbf..c0304f1e 100644 --- a/src/interfaces/ISafe.sol +++ b/src/interfaces/ISafe.sol @@ -7,4 +7,13 @@ interface ISafe { function getOwners() external view returns (address[] memory); function setFallbackHandler(address handler) external; function setGuard(address guard) external; + function execTransactionFromModule( + address to, + uint256 value, + bytes memory data, + uint8 operation + ) + external + returns (bool success); + function isModuleEnabled(address module) external view returns (bool); } diff --git a/src/modules/SafeEmailRecoveryModule.sol b/src/modules/SafeEmailRecoveryModule.sol new file mode 100644 index 00000000..1d182e1c --- /dev/null +++ b/src/modules/SafeEmailRecoveryModule.sol @@ -0,0 +1,569 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.25; + +import { EmailAccountRecovery } from + "ether-email-auth/packages/contracts/src/EmailAccountRecovery.sol"; +import { IEmailRecoveryManager } from "../interfaces/IEmailRecoveryManager.sol"; +import { IEmailRecoverySubjectHandler } from "../interfaces/IEmailRecoverySubjectHandler.sol"; +import { IEmailRecoveryModule } from "../interfaces/IEmailRecoveryModule.sol"; +import { + EnumerableGuardianMap, + GuardianStorage, + GuardianStatus +} from "../libraries/EnumerableGuardianMap.sol"; +import { GuardianUtils } from "../libraries/GuardianUtils.sol"; +import { ISafe } from "../interfaces/ISafe.sol"; +import { console2 } from "forge-std/console2.sol"; + +/** + * A safe plugin that recovers a safe owner via a zkp of an email. + */ +contract SafeEmailRecoveryModule is EmailAccountRecovery, IEmailRecoveryManager { + using GuardianUtils for mapping(address => GuardianConfig); + using GuardianUtils for mapping(address => EnumerableGuardianMap.AddressToGuardianMap); + + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* CONSTANTS & STORAGE */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /** + * Minimum required time window between when a recovery attempt becomes valid and when it + * becomes invalid + */ + uint256 public constant MINIMUM_RECOVERY_WINDOW = 2 days; + + /** + * Account address to recovery config + */ + mapping(address account => RecoveryConfig recoveryConfig) internal recoveryConfigs; + + /** + * Account address to recovery request + */ + mapping(address account => RecoveryRequest recoveryRequest) internal recoveryRequests; + + /** + * Account to guardian config + */ + mapping(address account => GuardianConfig guardianConfig) internal guardianConfigs; + + /** + * Account address to guardian address to guardian storage + */ + mapping(address account => EnumerableGuardianMap.AddressToGuardianMap guardian) internal + guardiansStorage; + + error InvalidSubjectParams(); + error InvalidOldOwner(); + error InvalidNewOwner(); + + constructor(address _verifier, address _dkimRegistry, address _emailAuthImpl) { + verifierAddr = _verifier; + dkimAddr = _dkimRegistry; + emailAuthImplementationAddr = _emailAuthImpl; + } + + /** + * @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 */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /** + * @notice Retrieves the recovery configuration for a given account + * @param account The address of the account for which the recovery configuration is being + * retrieved + * @return RecoveryConfig The recovery configuration for the specified account + */ + function getRecoveryConfig(address account) external view returns (RecoveryConfig memory) { + return recoveryConfigs[account]; + } + + /** + * @notice Retrieves the recovery request details for a given account + * @param account The address of the account for which the recovery request details are being + * retrieved + * @return RecoveryRequest The recovery request details for the specified account + */ + function getRecoveryRequest(address account) external view returns (RecoveryRequest memory) { + return recoveryRequests[account]; + } + + /** + * @notice Returns a two-dimensional array of strings representing the subject templates for an + * acceptance by a new guardian. + * @dev This is retrieved from the associated subject handler. Developers can write their own + * subject handlers, this is useful for account implementations which require different data in + * the subject or if the email should be in a language that is not English. + * @return string[][] A two-dimensional array of strings, where each inner array represents a + * set of fixed strings and matchers for a subject template. + */ + function acceptanceSubjectTemplates() public view override returns (string[][] memory) { + string[][] memory templates = new string[][](1); + templates[0] = new string[](5); + templates[0][0] = "Accept"; + templates[0][1] = "guardian"; + templates[0][2] = "request"; + templates[0][3] = "for"; + templates[0][4] = "{ethAddr}"; + return templates; + } + + /** + * @notice Returns a two-dimensional array of strings representing the subject templates for + * email recovery. + * @dev This is retrieved from the associated subject handler. Developers can write their own + * subject handlers, this is useful for account implementations which require different data in + * the subject or if the email should be in a language that is not English. + * @return string[][] A two-dimensional array of strings, where each inner array represents a + * set of fixed strings and matchers for a subject template. + */ + function recoverySubjectTemplates() public view override returns (string[][] memory) { + string[][] memory templates = new string[][](1); + templates[0] = new string[](15); + templates[0][0] = "Recover"; + templates[0][1] = "account"; + templates[0][2] = "{ethAddr}"; + templates[0][3] = "from"; + templates[0][4] = "old"; + templates[0][5] = "owner"; + templates[0][6] = "{ethAddr}"; + templates[0][7] = "to"; + templates[0][8] = "new"; + templates[0][9] = "owner"; + templates[0][10] = "{ethAddr}"; + templates[0][11] = "using"; + templates[0][12] = "recovery"; + templates[0][13] = "module"; + templates[0][14] = "{ethAddr}"; + return templates; + } + + /** + * @notice Extracts the account address to be recovered from the subject parameters of an + * acceptance email. + * @dev This is retrieved from the associated subject handler. + * @param subjectParams The subject parameters of the acceptance email. + * @param templateIdx The index of the acceptance subject template. + */ + function extractRecoveredAccountFromAcceptanceSubject( + bytes[] memory subjectParams, + uint256 templateIdx + ) + public + view + override + returns (address) + { + return abi.decode(subjectParams[0], (address)); + } + + /** + * @notice Extracts the account address to be recovered from the subject parameters of a + * recovery email. + * @dev This is retrieved from the associated subject handler. + * @param subjectParams The subject parameters of the recovery email. + * @param templateIdx The index of the recovery subject template. + */ + function extractRecoveredAccountFromRecoverySubject( + bytes[] memory subjectParams, + uint256 templateIdx + ) + public + view + override + returns (address) + { + return abi.decode(subjectParams[0], (address)); + } + + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* CONFIGURE RECOVERY */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /** + * @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 + * @param guardians An array of guardian addresses + * @param weights An array of weights corresponding to each guardian + * @param threshold The threshold weight required for recovery + * @param delay The delay period before recovery can be executed + * @param expiry The expiry time after which the recovery attempt is invalid + */ + function configureRecovery( + address[] memory guardians, + uint256[] memory weights, + uint256 threshold, + uint256 delay, + uint256 expiry + ) + external + { + address account = msg.sender; + + // Threshold can only be 0 at initialization. + // Check ensures that setup function can only be called once. + if (guardianConfigs[account].threshold > 0) { + revert SetupAlreadyCalled(); + } + + bool moduleEnabled = ISafe(account).isModuleEnabled(address(this)); + if (!moduleEnabled) { + revert RecoveryModuleNotAuthorized(); + } + + // Allow recovery configuration without configuring guardians + if (guardians.length == 0 && weights.length == 0 && threshold == 0) { + guardianConfigs[account].initialized = true; + } else { + setupGuardians(account, guardians, weights, threshold); + } + + RecoveryConfig memory recoveryConfig = RecoveryConfig(delay, expiry); + updateRecoveryConfig(recoveryConfig); + + emit RecoveryConfigured(account, guardians.length); + } + + /** + * @notice Updates and validates the recovery configuration for the caller's account + * @dev Validates and sets the new recovery configuration for the caller's account, ensuring + * 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) + public + onlyWhenNotRecovering + { + address account = msg.sender; + + if (!guardianConfigs[account].initialized) { + revert AccountNotConfigured(); + } + if (recoveryConfig.delay > recoveryConfig.expiry) { + revert DelayMoreThanExpiry(); + } + if (recoveryConfig.expiry - recoveryConfig.delay < MINIMUM_RECOVERY_WINDOW) { + revert RecoveryWindowTooShort(); + } + + recoveryConfigs[account] = recoveryConfig; + + emit RecoveryConfigUpdated(account, recoveryConfig.delay, recoveryConfig.expiry); + } + + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* HANDLE ACCEPTANCE */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /** + * @notice Accepts a guardian for the specified account. This is the second core function + * that must be called during the end-to-end recovery flow + * @dev Called once per guardian added. Although this adds an extra step to recovery, this + * acceptance flow is an important security feature to ensure that no typos are made when adding + * a guardian, and that the guardian is in control of the specified email address. Called as + * part of handleAcceptance in EmailAccountRecovery + * @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 + */ + function acceptGuardian( + address guardian, + uint256 templateIdx, + bytes[] memory subjectParams, + bytes32 + ) + internal + override + { + if (templateIdx != 0) { + revert InvalidTemplateIndex(); + } + if (subjectParams.length != 1) revert InvalidSubjectParams(); + + address account = abi.decode(subjectParams[0], (address)); + + if (recoveryRequests[account].currentWeight > 0) { + revert RecoveryInProcess(); + } + + bool moduleEnabled = ISafe(account).isModuleEnabled(address(this)); + if (!moduleEnabled) { + revert RecoveryModuleNotAuthorized(); + } + + // This check ensures GuardianStatus is correct and also implicitly that the + // account in email is a valid account + GuardianStorage memory guardianStorage = getGuardian(account, guardian); + if (guardianStorage.status != GuardianStatus.REQUESTED) { + revert InvalidGuardianStatus(guardianStorage.status, GuardianStatus.REQUESTED); + } + + guardiansStorage.updateGuardianStatus(account, guardian, GuardianStatus.ACCEPTED); + + emit GuardianAccepted(account, guardian); + } + + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* HANDLE RECOVERY */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /** + * @notice Processes a recovery request for a given account. This is the third core function + * that must be called during the end-to-end recovery flow + * @dev Called once per guardian until the threshold is reached + * @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 + */ + function processRecovery( + address guardian, + uint256 templateIdx, + bytes[] memory subjectParams, + bytes32 + ) + internal + override + { + if (templateIdx != 0) { + revert InvalidTemplateIndex(); + } + if (subjectParams.length != 4) { + revert InvalidSubjectParams(); + } + + address account = abi.decode(subjectParams[0], (address)); + address oldOwner = abi.decode(subjectParams[1], (address)); + address newOwner = abi.decode(subjectParams[2], (address)); + // FIXME: recovery module address? + + bool moduleEnabled = ISafe(account).isModuleEnabled(address(this)); + + if (!moduleEnabled) { + revert RecoveryModuleNotAuthorized(); + } + + // This check ensures GuardianStatus is correct and also implicitly that the + // account in email is a valid account + GuardianStorage memory guardianStorage = getGuardian(account, guardian); + if (guardianStorage.status != GuardianStatus.ACCEPTED) { + revert InvalidGuardianStatus(guardianStorage.status, GuardianStatus.ACCEPTED); + } + bool isOwner = ISafe(account).isOwner(oldOwner); + if (!isOwner) { + revert InvalidOldOwner(); + } + if (newOwner == address(0)) { + revert InvalidNewOwner(); + } + + address previousOwnerInLinkedList = getPreviousOwnerInLinkedList(account, oldOwner); + bytes memory recoveryCallData = abi.encodeWithSignature( + "swapOwner(address,address,address)", previousOwnerInLinkedList, oldOwner, newOwner + ); + bytes32 calldataHash = keccak256(recoveryCallData); + + RecoveryRequest storage recoveryRequest = recoveryRequests[account]; + recoveryRequest.currentWeight += guardianStorage.weight; + + uint256 threshold = guardianConfigs[account].threshold; + if (recoveryRequest.currentWeight >= threshold) { + uint256 executeAfter = block.timestamp + recoveryConfigs[account].delay; + uint256 executeBefore = block.timestamp + recoveryConfigs[account].expiry; + + recoveryRequest.executeAfter = executeAfter; + recoveryRequest.executeBefore = executeBefore; + recoveryRequest.calldataHash = calldataHash; + + emit RecoveryProcessed(account, executeAfter, executeBefore); + } + } + + /** + * @notice Gets the previous owner in the Safe owners linked list that points to the + * owner passed into the function + * @param safe The Safe account to query + * @param oldOwner The owner address to get the previous owner for + * @return previousOwner The previous owner in the Safe owners linked list pointing to the owner + * passed in + */ + function getPreviousOwnerInLinkedList( + address safe, + address oldOwner + ) + internal + view + returns (address) + { + address[] memory owners = ISafe(safe).getOwners(); + uint256 length = owners.length; + + uint256 oldOwnerIndex; + for (uint256 i; i < length; i++) { + if (owners[i] == oldOwner) { + oldOwnerIndex = i; + break; + } + } + address sentinelOwner = address(0x1); + return oldOwnerIndex == 0 ? sentinelOwner : owners[oldOwnerIndex - 1]; + } + + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* COMPLETE RECOVERY */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /** + * @notice Completes the recovery process for a given account. This is the forth and final + * core function that must be called during the end-to-end recovery flow. Can be called by + * anyone. + * @dev Validates the recovery request by checking the total weight, that the delay has passed, + * and the request has not expired. Triggers the recovery module to perform the recovery. The + * recovery module trusts that this contract has validated the recovery attempt. This function + * deletes the recovery request but recovery config state is maintained so future recovery + * requests can be made without having to reconfigure everything + * @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 { + if (account == address(0)) { + revert InvalidAccountAddress(); + } + RecoveryRequest memory recoveryRequest = recoveryRequests[account]; + + uint256 threshold = guardianConfigs[account].threshold; + if (threshold == 0) { + revert NoRecoveryConfigured(); + } + + if (recoveryRequest.currentWeight < threshold) { + revert NotEnoughApprovals(); + } + + if (block.timestamp < recoveryRequest.executeAfter) { + revert DelayNotPassed(); + } + + if (block.timestamp >= recoveryRequest.executeBefore) { + revert RecoveryRequestExpired(); + } + + bytes32 calldataHash = keccak256(recoveryCalldata); + if (calldataHash != recoveryRequest.calldataHash) { + revert InvalidCalldataHash(); + } + + delete recoveryRequests[account]; + + ISafe(account).execTransactionFromModule(account, 0, recoveryCalldata, 0); + + emit RecoveryCompleted(account); + } + + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* CANCEL LOGIC */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /** + * @notice Cancels the recovery request for the caller's account + * @dev Deletes the current recovery request associated with the caller's account + */ + function cancelRecovery() external virtual { + delete recoveryRequests[msg.sender]; + emit RecoveryCancelled(msg.sender); + } + + function deInitRecoveryFromModule(address account) external { } + + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* GUARDIAN LOGIC */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /** + * @notice Retrieves the guardian configuration for a given account + * @param account The address of the account for which the guardian configuration is being + * retrieved + * @return GuardianConfig The guardian configuration for the specified account + */ + function getGuardianConfig(address account) external view returns (GuardianConfig memory) { + return guardianConfigs[account]; + } + + /** + * @notice Retrieves the guardian storage details for a given guardian and account + * @param account The address of the account associated with the guardian + * @param guardian The address of the guardian + * @return GuardianStorage The guardian storage details for the specified guardian and account + */ + function getGuardian( + address account, + address guardian + ) + public + view + returns (GuardianStorage memory) + { + return guardiansStorage.getGuardianStorage(account, guardian); + } + + /** + * @notice Sets up guardians for a given account with specified weights and threshold + * @dev This function can only be called once and ensures the guardians, weights, and threshold + * are correctly configured + * @param account The address of the account for which guardians are being set up + * @param guardians An array of guardian addresses + * @param weights An array of weights corresponding to each guardian + * @param threshold The threshold weight required for guardians to approve recovery attempts + */ + function setupGuardians( + address account, + address[] memory guardians, + uint256[] memory weights, + uint256 threshold + ) + internal + { + guardianConfigs.setupGuardians(guardiansStorage, account, guardians, weights, threshold); + } + + /** + * @notice Adds a guardian for the caller's account with a specified weight + * @dev This function can only be called by the account associated with the guardian and only if + * no recovery is in process + * @param guardian The address of the guardian to be added + * @param weight The weight assigned to the guardian + */ + function addGuardian(address guardian, uint256 weight) external onlyWhenNotRecovering { + guardiansStorage.addGuardian(guardianConfigs, msg.sender, guardian, weight); + } + + /** + * @notice Removes a guardian for the caller's account + * @dev This function can only be called by the account associated with the guardian and only if + * no recovery is in process + * @param guardian The address of the guardian to be removed + */ + function removeGuardian(address guardian) external onlyWhenNotRecovering { + guardiansStorage.removeGuardian(guardianConfigs, msg.sender, guardian); + } + + /** + * @notice Changes the threshold for guardian approvals for the caller's account + * @dev This function can only be called by the account associated with the guardian config and + * only if no recovery is in process + * @param threshold The new threshold for guardian approvals + */ + function changeThreshold(uint256 threshold) external onlyWhenNotRecovering { + guardianConfigs.changeThreshold(msg.sender, threshold); + } +} diff --git a/test/integration/SafeRecovery/SafeNativeIntegrationBase.t.sol b/test/integration/SafeRecovery/SafeNativeIntegrationBase.t.sol new file mode 100644 index 00000000..3b181eb2 --- /dev/null +++ b/test/integration/SafeRecovery/SafeNativeIntegrationBase.t.sol @@ -0,0 +1,224 @@ +// SPDX-License-Identifier: MIT +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 { EmailAuthMsg, EmailProof } from "ether-email-auth/packages/contracts/src/EmailAuth.sol"; +import { SubjectUtils } from "ether-email-auth/packages/contracts/src/libraries/SubjectUtils.sol"; +import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; + +import { Safe } from "@safe-global/safe-contracts/contracts/Safe.sol"; +import { SafeProxy } from "@safe-global/safe-contracts/contracts/proxies/SafeProxy.sol"; +import { SafeEmailRecoveryModule } from "src/modules/SafeEmailRecoveryModule.sol"; +import { EmailRecoveryManager } from "src/EmailRecoveryManager.sol"; +import { UniversalEmailRecoveryModule } from "src/modules/UniversalEmailRecoveryModule.sol"; +import { SafeRecoverySubjectHandler } from "src/handlers/SafeRecoverySubjectHandler.sol"; +import { IntegrationBase } from "../IntegrationBase.t.sol"; + +abstract contract SafeNativeIntegrationBase is IntegrationBase { + using ModuleKitHelpers for *; + using Strings for uint256; + using Strings for address; + + SafeEmailRecoveryModule safeEmailRecoveryModule; + Safe public safeSingleton; + Safe public safe; + address public safeAddress; + address public owner; + bytes isInstalledContext; + bytes4 functionSelector; + uint256 nullifierCount; + + /** + * Helper function to return if current account type is safe or not + */ + function isAccountTypeSafe() public returns (bool) { + string memory currentAccountType = vm.envOr("ACCOUNT_TYPE", string("")); + if (Strings.equal(currentAccountType, "SAFE")) { + return true; + } else { + return false; + } + } + + function setUp() public virtual override { + if (!isAccountTypeSafe()) { + return; + } + super.setUp(); + + safeEmailRecoveryModule = new SafeEmailRecoveryModule( + address(verifier), address(ecdsaOwnedDkimRegistry), address(emailAuthImpl) + ); + + safeSingleton = new Safe(); + SafeProxy safeProxy = new SafeProxy(address(safeSingleton)); + safe = Safe(payable(address(safeProxy))); + safeAddress = address(safe); + // safe4337Module = new Safe4337Module(entryPointAddress); + // safeModuleSetup = new SafeModuleSetup(); + + isInstalledContext = bytes("0"); + functionSelector = bytes4(keccak256(bytes("swapOwner(address,address,address)"))); + + // Compute guardian addresses + guardians1 = new address[](3); + guardians1[0] = safeEmailRecoveryModule.computeEmailAuthAddress(safeAddress, accountSalt1); + guardians1[1] = safeEmailRecoveryModule.computeEmailAuthAddress(safeAddress, accountSalt2); + guardians1[2] = safeEmailRecoveryModule.computeEmailAuthAddress(safeAddress, accountSalt3); + + address[] memory owners = new address[](1); + owner = owner1; + owners[0] = owner; + + safe.setup( + owners, + 1, + address(0), + bytes("0"), + address(0), + // address(safeModuleSetup), + // abi.encodeCall(SafeModuleSetup.enableModules, (modules)), + // address(safe4337Module), + address(0), + 0, + payable(address(0)) + ); + + vm.startPrank(safeAddress); + safe.enableModule(address(safeEmailRecoveryModule)); + vm.stopPrank(); + } + + function generateMockEmailProof( + string memory subject, + bytes32 nullifier, + bytes32 accountSalt + ) + public + view + returns (EmailProof memory) + { + EmailProof memory emailProof; + emailProof.domainName = "gmail.com"; + emailProof.publicKeyHash = bytes32( + vm.parseUint( + "6632353713085157925504008443078919716322386156160602218536961028046468237192" + ) + ); + emailProof.timestamp = block.timestamp; + emailProof.maskedSubject = subject; + emailProof.emailNullifier = nullifier; + emailProof.accountSalt = accountSalt; + emailProof.isCodeExist = true; + emailProof.proof = bytes("0"); + + return emailProof; + } + + function getAccountSaltForGuardian(address guardian) public returns (bytes32) { + if (guardian == guardians1[0]) { + return accountSalt1; + } + if (guardian == guardians1[1]) { + return accountSalt2; + } + if (guardian == guardians1[2]) { + return accountSalt3; + } + + revert("Invalid guardian address"); + } + + function generateNewNullifier() public returns (bytes32) { + return keccak256(abi.encode(nullifierCount++)); + } + + function acceptGuardian(address account, address guardian) public { + EmailAuthMsg memory emailAuthMsg = getAcceptanceEmailAuthMessage(account, guardian); + safeEmailRecoveryModule.handleAcceptance(emailAuthMsg, templateIdx); + } + + function getAcceptanceEmailAuthMessage( + address account, + address guardian + ) + public + returns (EmailAuthMsg memory) + { + string memory accountString = SubjectUtils.addressToChecksumHexString(account); + string memory subject = string.concat("Accept guardian request for ", accountString); + bytes32 nullifier = generateNewNullifier(); + bytes32 accountSalt = getAccountSaltForGuardian(guardian); + + EmailProof memory emailProof = generateMockEmailProof(subject, nullifier, accountSalt); + + bytes[] memory subjectParamsForAcceptance = new bytes[](1); + subjectParamsForAcceptance[0] = abi.encode(account); + return EmailAuthMsg({ + templateId: safeEmailRecoveryModule.computeAcceptanceTemplateId(templateIdx), + subjectParams: subjectParamsForAcceptance, + skipedSubjectPrefix: 0, + proof: emailProof + }); + } + + function handleRecovery( + address account, + address oldOwner, + address newOwner, + address guardian + ) + public + { + EmailAuthMsg memory emailAuthMsg = + getRecoveryEmailAuthMessage(account, oldOwner, newOwner, guardian); + safeEmailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx); + } + + function getRecoveryEmailAuthMessage( + address account, + address oldOwner, + address newOwner, + address guardian + ) + public + returns (EmailAuthMsg memory) + { + string memory accountString = SubjectUtils.addressToChecksumHexString(account); + string memory oldOwnerString = SubjectUtils.addressToChecksumHexString(oldOwner); + string memory newOwnerString = SubjectUtils.addressToChecksumHexString(newOwner); + string memory recoveryModuleString = + SubjectUtils.addressToChecksumHexString(address(safeEmailRecoveryModule)); + + string memory subject = string.concat( + "Recover account ", + accountString, + " from old owner ", + oldOwnerString, + " to new owner ", + newOwnerString, + " using recovery module ", + recoveryModuleString + ); + bytes32 nullifier = generateNewNullifier(); + bytes32 accountSalt = getAccountSaltForGuardian(guardian); + + EmailProof memory emailProof = generateMockEmailProof(subject, nullifier, accountSalt); + + bytes[] memory subjectParamsForRecovery = new bytes[](4); + subjectParamsForRecovery[0] = abi.encode(account); + subjectParamsForRecovery[1] = abi.encode(oldOwner); + subjectParamsForRecovery[2] = abi.encode(newOwner); + subjectParamsForRecovery[3] = abi.encode(address(safeEmailRecoveryModule)); + + return EmailAuthMsg({ + templateId: safeEmailRecoveryModule.computeRecoveryTemplateId(templateIdx), + subjectParams: subjectParamsForRecovery, + skipedSubjectPrefix: 0, + proof: emailProof + }); + } +} diff --git a/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol b/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol new file mode 100644 index 00000000..8ed70460 --- /dev/null +++ b/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.25; + +import { console2 } from "forge-std/console2.sol"; +import { ModuleKitHelpers, ModuleKitUserOp } from "modulekit/ModuleKit.sol"; +import { MODULE_TYPE_EXECUTOR } from "erc7579/interfaces/IERC7579Module.sol"; +import { IERC7579Account } from "erc7579/interfaces/IERC7579Account.sol"; +import { Safe } from "@safe-global/safe-contracts/contracts/Safe.sol"; +import { SafeProxy } from "@safe-global/safe-contracts/contracts/proxies/SafeProxy.sol"; +import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; +import { EmailAuthMsg } from "ether-email-auth/packages/contracts/src/EmailAuth.sol"; +import { SafeEmailRecoveryModule } from "src/modules/SafeEmailRecoveryModule.sol"; +import { IEmailRecoveryManager } from "src/interfaces/IEmailRecoveryManager.sol"; +import { GuardianStorage, GuardianStatus } from "src/libraries/EnumerableGuardianMap.sol"; +import { SafeNativeIntegrationBase } from "./SafeNativeIntegrationBase.t.sol"; + +contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase { + function setUp() public override { + super.setUp(); + } + + function testIntegration_AccountRecovery() public { + bool moduleEnabled = safe.isModuleEnabled(address(safeEmailRecoveryModule)); + + address newOwner = owner2; + + // Configure recovery + vm.startPrank(safeAddress); + safeEmailRecoveryModule.configureRecovery( + guardians1, guardianWeights, threshold, delay, expiry + ); + vm.stopPrank(); + + bytes memory recoveryCalldata = abi.encodeWithSignature( + "swapOwner(address,address,address)", address(1), owner, newOwner + ); + bytes32 calldataHash = keccak256(recoveryCalldata); + + bytes[] memory subjectParamsForRecovery = new bytes[](4); + subjectParamsForRecovery[0] = abi.encode(safeAddress); + subjectParamsForRecovery[1] = abi.encode(owner); + subjectParamsForRecovery[2] = abi.encode(newOwner); + subjectParamsForRecovery[3] = abi.encode(address(safeEmailRecoveryModule)); + + // Accept guardian + EmailAuthMsg memory emailAuthMsg = getAcceptanceEmailAuthMessage(safeAddress, guardians1[0]); + safeEmailRecoveryModule.handleAcceptance(emailAuthMsg, templateIdx); + GuardianStorage memory guardianStorage1 = + safeEmailRecoveryModule.getGuardian(safeAddress, guardians1[0]); + assertEq(uint256(guardianStorage1.status), uint256(GuardianStatus.ACCEPTED)); + assertEq(guardianStorage1.weight, uint256(1)); + + // Accept guardian + emailAuthMsg = getAcceptanceEmailAuthMessage(safeAddress, guardians1[1]); + safeEmailRecoveryModule.handleAcceptance(emailAuthMsg, templateIdx); + GuardianStorage memory guardianStorage2 = + safeEmailRecoveryModule.getGuardian(safeAddress, guardians1[1]); + assertEq(uint256(guardianStorage2.status), uint256(GuardianStatus.ACCEPTED)); + assertEq(guardianStorage2.weight, uint256(2)); + + // Time travel so that EmailAuth timestamp is valid + vm.warp(12 seconds); + + // handle recovery request for guardian 1 + emailAuthMsg = getRecoveryEmailAuthMessage(safeAddress, owner, newOwner, guardians1[0]); + safeEmailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx); + IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = + safeEmailRecoveryModule.getRecoveryRequest(safeAddress); + assertEq(recoveryRequest.currentWeight, 1); + + // handle recovery request for guardian 2 + uint256 executeAfter = block.timestamp + delay; + uint256 executeBefore = block.timestamp + expiry; + emailAuthMsg = getRecoveryEmailAuthMessage(safeAddress, owner, newOwner, guardians1[1]); + safeEmailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx); + recoveryRequest = safeEmailRecoveryModule.getRecoveryRequest(safeAddress); + assertEq(recoveryRequest.executeAfter, executeAfter); + assertEq(recoveryRequest.executeBefore, executeBefore); + assertEq(recoveryRequest.currentWeight, 3); + + vm.warp(block.timestamp + delay); + + // Complete recovery + safeEmailRecoveryModule.completeRecovery(safeAddress, recoveryCalldata); + + recoveryRequest = safeEmailRecoveryModule.getRecoveryRequest(safeAddress); + assertEq(recoveryRequest.executeAfter, 0); + assertEq(recoveryRequest.executeBefore, 0); + assertEq(recoveryRequest.currentWeight, 0); + + vm.prank(safeAddress); + bool isOwner = Safe(payable(safeAddress)).isOwner(newOwner); + assertTrue(isOwner); + + bool oldOwnerIsOwner = Safe(payable(safeAddress)).isOwner(owner); + assertFalse(oldOwnerIsOwner); + } +} From b1a202bc7a470ae7e651854d90cde7ad5033341b Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Sun, 7 Jul 2024 14:07:06 +0200 Subject: [PATCH 2/5] Fix deployment bug --- script/ComputeSafeRecoveryCalldata.s.sol | 19 +++++++++++++++++++ script/DeploySafeNativeRecovery.s.sol | 4 ++-- 2 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 script/ComputeSafeRecoveryCalldata.s.sol diff --git a/script/ComputeSafeRecoveryCalldata.s.sol b/script/ComputeSafeRecoveryCalldata.s.sol new file mode 100644 index 00000000..44e5a603 --- /dev/null +++ b/script/ComputeSafeRecoveryCalldata.s.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.25; + +import { Script } from "forge-std/Script.sol"; +import { console } from "forge-std/console.sol"; + +contract ComputeSafeRecoveryCalldataScript is Script { + function run() public { + address oldOwner = vm.envAddress("OLD_OWNER"); + address newOwner = vm.envAddress("NEW_OWNER"); + address previousOwnerInLinkedList = address(1); + + bytes memory recoveryCalldata = abi.encodeWithSignature( + "swapOwner(address,address,address)", previousOwnerInLinkedList, oldOwner, newOwner + ); + + console.log("recoveryCalldata", vm.toString(recoveryCalldata)); + } +} diff --git a/script/DeploySafeNativeRecovery.s.sol b/script/DeploySafeNativeRecovery.s.sol index 087eccf2..0bb5f14d 100644 --- a/script/DeploySafeNativeRecovery.s.sol +++ b/script/DeploySafeNativeRecovery.s.sol @@ -14,7 +14,7 @@ contract DeploySafeNativeRecovery_Script is Script { vm.startBroadcast(vm.envUint("PRIVATE_KEY")); address verifier = vm.envOr("VERIFIER", address(0)); address dkimRegistry = vm.envOr("DKIM_REGISTRY", address(0)); - address dkimRegistrySigner = vm.envOr("SIGNER", address(0)); + address dkimRegistrySigner = vm.envOr("DKIM_SIGNER", address(0)); address emailAuthImpl = vm.envOr("EMAIL_AUTH_IMPL", address(0)); if (verifier == address(0)) { @@ -33,7 +33,7 @@ contract DeploySafeNativeRecovery_Script is Script { console.log("Deployed Email Auth at", emailAuthImpl); } - address module = address(new SafeEmailRecoveryModule(verifier, emailAuthImpl, dkimRegistry)); + address module = address(new SafeEmailRecoveryModule(verifier, dkimRegistry, emailAuthImpl)); console.log("Deployed Email Recovery Module at ", vm.toString(module)); From f4c289f7c87306e1962bd36c3aa138e217cadcb5 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 30 Jul 2024 17:45:12 +0100 Subject: [PATCH 3/5] Use EmailRecoveryManager with native safe module --- script/DeploySafeNativeRecovery.s.sol | 28 +- src/interfaces/ISafe.sol | 8 + src/modules/SafeEmailRecoveryModule.sol | 584 ++---------------- .../SafeNativeIntegrationBase.t.sol | 37 +- .../SafeRecoveryNativeModule.t.sol | 25 +- 5 files changed, 98 insertions(+), 584 deletions(-) diff --git a/script/DeploySafeNativeRecovery.s.sol b/script/DeploySafeNativeRecovery.s.sol index 0bb5f14d..081d6d1b 100644 --- a/script/DeploySafeNativeRecovery.s.sol +++ b/script/DeploySafeNativeRecovery.s.sol @@ -4,36 +4,18 @@ pragma solidity ^0.8.25; import { Script } from "forge-std/Script.sol"; import { console } from "forge-std/console.sol"; import { SafeEmailRecoveryModule } from "src/modules/SafeEmailRecoveryModule.sol"; -import { Verifier } from "ether-email-auth/packages/contracts/src/utils/Verifier.sol"; -import { ECDSAOwnedDKIMRegistry } from - "ether-email-auth/packages/contracts/src/utils/ECDSAOwnedDKIMRegistry.sol"; -import { EmailAuth } from "ether-email-auth/packages/contracts/src/EmailAuth.sol"; contract DeploySafeNativeRecovery_Script is Script { function run() public { vm.startBroadcast(vm.envUint("PRIVATE_KEY")); - address verifier = vm.envOr("VERIFIER", address(0)); - address dkimRegistry = vm.envOr("DKIM_REGISTRY", address(0)); - address dkimRegistrySigner = vm.envOr("DKIM_SIGNER", address(0)); - address emailAuthImpl = vm.envOr("EMAIL_AUTH_IMPL", address(0)); + address manager = vm.envOr("VERIFIER", address(0)); - if (verifier == address(0)) { - verifier = address(new Verifier()); - console.log("Deployed Verifier at", verifier); + if (manager == address(0)) { + manager = address(1); + console.log("Deployed Manager at", manager); } - if (dkimRegistry == address(0)) { - require(dkimRegistrySigner != address(0), "DKIM_REGISTRY_SIGNER is required"); - dkimRegistry = address(new ECDSAOwnedDKIMRegistry(dkimRegistrySigner)); - console.log("Deployed DKIM Registry at", dkimRegistry); - } - - if (emailAuthImpl == address(0)) { - emailAuthImpl = address(new EmailAuth()); - console.log("Deployed Email Auth at", emailAuthImpl); - } - - address module = address(new SafeEmailRecoveryModule(verifier, dkimRegistry, emailAuthImpl)); + address module = address(new SafeEmailRecoveryModule(manager)); console.log("Deployed Email Recovery Module at ", vm.toString(module)); diff --git a/src/interfaces/ISafe.sol b/src/interfaces/ISafe.sol index c0304f1e..e47642cd 100644 --- a/src/interfaces/ISafe.sol +++ b/src/interfaces/ISafe.sol @@ -15,5 +15,13 @@ interface ISafe { ) external returns (bool success); + function execTransactionFromModuleReturnData( + address to, + uint256 value, + bytes memory data, + uint8 operation + ) + external + returns (bool success, bytes memory returnData); function isModuleEnabled(address module) external view returns (bool); } diff --git a/src/modules/SafeEmailRecoveryModule.sol b/src/modules/SafeEmailRecoveryModule.sol index 1d182e1c..de740a81 100644 --- a/src/modules/SafeEmailRecoveryModule.sol +++ b/src/modules/SafeEmailRecoveryModule.sol @@ -1,569 +1,75 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.25; -import { EmailAccountRecovery } from - "ether-email-auth/packages/contracts/src/EmailAccountRecovery.sol"; -import { IEmailRecoveryManager } from "../interfaces/IEmailRecoveryManager.sol"; -import { IEmailRecoverySubjectHandler } from "../interfaces/IEmailRecoverySubjectHandler.sol"; -import { IEmailRecoveryModule } from "../interfaces/IEmailRecoveryModule.sol"; -import { - EnumerableGuardianMap, - GuardianStorage, - GuardianStatus -} from "../libraries/EnumerableGuardianMap.sol"; -import { GuardianUtils } from "../libraries/GuardianUtils.sol"; import { ISafe } from "../interfaces/ISafe.sol"; -import { console2 } from "forge-std/console2.sol"; +import { Enum } from "@safe-global/safe-contracts/contracts/common/Enum.sol"; /** - * A safe plugin that recovers a safe owner via a zkp of an email. + * A safe module that recovers a safe owner via ZK Email */ -contract SafeEmailRecoveryModule is EmailAccountRecovery, IEmailRecoveryManager { - using GuardianUtils for mapping(address => GuardianConfig); - using GuardianUtils for mapping(address => EnumerableGuardianMap.AddressToGuardianMap); - - /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ - /* CONSTANTS & STORAGE */ - /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ - - /** - * Minimum required time window between when a recovery attempt becomes valid and when it - * becomes invalid - */ - uint256 public constant MINIMUM_RECOVERY_WINDOW = 2 days; - - /** - * Account address to recovery config - */ - mapping(address account => RecoveryConfig recoveryConfig) internal recoveryConfigs; - - /** - * Account address to recovery request - */ - mapping(address account => RecoveryRequest recoveryRequest) internal recoveryRequests; +contract SafeEmailRecoveryModule { + bytes4 public constant selector = bytes4(keccak256(bytes("swapOwner(address,address,address)"))); /** - * Account to guardian config + * Trusted email recovery manager contract that handles recovery requests */ - mapping(address account => GuardianConfig guardianConfig) internal guardianConfigs; + address public immutable emailRecoveryManager; - /** - * Account address to guardian address to guardian storage - */ - mapping(address account => EnumerableGuardianMap.AddressToGuardianMap guardian) internal - guardiansStorage; + event RecoveryExecuted(address indexed account); - error InvalidSubjectParams(); - error InvalidOldOwner(); - error InvalidNewOwner(); + error ModuleEnabledCheckFailed(address account, address module); + error NotTrustedRecoveryManager(); + error InvalidSelector(bytes4 selector); + error RecoveryFailed(address account); - constructor(address _verifier, address _dkimRegistry, address _emailAuthImpl) { - verifierAddr = _verifier; - dkimAddr = _dkimRegistry; - emailAuthImplementationAddr = _emailAuthImpl; + constructor(address _emailRecoveryManager) { + emailRecoveryManager = _emailRecoveryManager; } /** - * @notice Modifier to check recovery status. Reverts if recovery is in process for the account + * Check if the recovery module is authorized to recover the account + * @param account The smart account to check + * @return true if the module is authorized, false otherwise */ - modifier onlyWhenNotRecovering() { - if (recoveryRequests[msg.sender].currentWeight > 0) { - revert RecoveryInProcess(); + function isAuthorizedToRecover(address account) external returns (bool) { + (bool success, bytes memory returnData) = ISafe(account).execTransactionFromModuleReturnData({ + to: account, + value: 0, + data: abi.encodeWithSignature("isModuleEnabled(address)", address(this)), + operation: uint8(Enum.Operation.Call) + }); + if (!success) { + revert ModuleEnabledCheckFailed(account, address(this)); } - _; - } - /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ - /* RECOVERY CONFIG, REQUEST AND TEMPLATE GETTERS */ - /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ - - /** - * @notice Retrieves the recovery configuration for a given account - * @param account The address of the account for which the recovery configuration is being - * retrieved - * @return RecoveryConfig The recovery configuration for the specified account - */ - function getRecoveryConfig(address account) external view returns (RecoveryConfig memory) { - return recoveryConfigs[account]; - } - - /** - * @notice Retrieves the recovery request details for a given account - * @param account The address of the account for which the recovery request details are being - * retrieved - * @return RecoveryRequest The recovery request details for the specified account - */ - function getRecoveryRequest(address account) external view returns (RecoveryRequest memory) { - return recoveryRequests[account]; - } - - /** - * @notice Returns a two-dimensional array of strings representing the subject templates for an - * acceptance by a new guardian. - * @dev This is retrieved from the associated subject handler. Developers can write their own - * subject handlers, this is useful for account implementations which require different data in - * the subject or if the email should be in a language that is not English. - * @return string[][] A two-dimensional array of strings, where each inner array represents a - * set of fixed strings and matchers for a subject template. - */ - function acceptanceSubjectTemplates() public view override returns (string[][] memory) { - string[][] memory templates = new string[][](1); - templates[0] = new string[](5); - templates[0][0] = "Accept"; - templates[0][1] = "guardian"; - templates[0][2] = "request"; - templates[0][3] = "for"; - templates[0][4] = "{ethAddr}"; - return templates; - } - - /** - * @notice Returns a two-dimensional array of strings representing the subject templates for - * email recovery. - * @dev This is retrieved from the associated subject handler. Developers can write their own - * subject handlers, this is useful for account implementations which require different data in - * the subject or if the email should be in a language that is not English. - * @return string[][] A two-dimensional array of strings, where each inner array represents a - * set of fixed strings and matchers for a subject template. - */ - function recoverySubjectTemplates() public view override returns (string[][] memory) { - string[][] memory templates = new string[][](1); - templates[0] = new string[](15); - templates[0][0] = "Recover"; - templates[0][1] = "account"; - templates[0][2] = "{ethAddr}"; - templates[0][3] = "from"; - templates[0][4] = "old"; - templates[0][5] = "owner"; - templates[0][6] = "{ethAddr}"; - templates[0][7] = "to"; - templates[0][8] = "new"; - templates[0][9] = "owner"; - templates[0][10] = "{ethAddr}"; - templates[0][11] = "using"; - templates[0][12] = "recovery"; - templates[0][13] = "module"; - templates[0][14] = "{ethAddr}"; - return templates; - } - - /** - * @notice Extracts the account address to be recovered from the subject parameters of an - * acceptance email. - * @dev This is retrieved from the associated subject handler. - * @param subjectParams The subject parameters of the acceptance email. - * @param templateIdx The index of the acceptance subject template. - */ - function extractRecoveredAccountFromAcceptanceSubject( - bytes[] memory subjectParams, - uint256 templateIdx - ) - public - view - override - returns (address) - { - return abi.decode(subjectParams[0], (address)); + return abi.decode(returnData, (bool)); } /** - * @notice Extracts the account address to be recovered from the subject parameters of a - * recovery email. - * @dev This is retrieved from the associated subject handler. - * @param subjectParams The subject parameters of the recovery email. - * @param templateIdx The index of the recovery subject template. + * @notice Executes recovery on a Safe account. 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 Safe + * being recovered */ - function extractRecoveredAccountFromRecoverySubject( - bytes[] memory subjectParams, - uint256 templateIdx - ) - public - view - override - returns (address) - { - return abi.decode(subjectParams[0], (address)); - } - - /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ - /* CONFIGURE RECOVERY */ - /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ - - /** - * @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 - * @param guardians An array of guardian addresses - * @param weights An array of weights corresponding to each guardian - * @param threshold The threshold weight required for recovery - * @param delay The delay period before recovery can be executed - * @param expiry The expiry time after which the recovery attempt is invalid - */ - function configureRecovery( - address[] memory guardians, - uint256[] memory weights, - uint256 threshold, - uint256 delay, - uint256 expiry - ) - external - { - address account = msg.sender; - - // Threshold can only be 0 at initialization. - // Check ensures that setup function can only be called once. - if (guardianConfigs[account].threshold > 0) { - revert SetupAlreadyCalled(); + function recover(address account, bytes calldata recoveryCalldata) public { + if (msg.sender != emailRecoveryManager) { + revert NotTrustedRecoveryManager(); } - bool moduleEnabled = ISafe(account).isModuleEnabled(address(this)); - if (!moduleEnabled) { - revert RecoveryModuleNotAuthorized(); + bytes4 calldataSelector = bytes4(recoveryCalldata[:4]); + if (calldataSelector != selector) { + revert InvalidSelector(calldataSelector); } - // Allow recovery configuration without configuring guardians - if (guardians.length == 0 && weights.length == 0 && threshold == 0) { - guardianConfigs[account].initialized = true; - } else { - setupGuardians(account, guardians, weights, threshold); - } - - RecoveryConfig memory recoveryConfig = RecoveryConfig(delay, expiry); - updateRecoveryConfig(recoveryConfig); - - emit RecoveryConfigured(account, guardians.length); - } - - /** - * @notice Updates and validates the recovery configuration for the caller's account - * @dev Validates and sets the new recovery configuration for the caller's account, ensuring - * 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) - public - onlyWhenNotRecovering - { - address account = msg.sender; - - if (!guardianConfigs[account].initialized) { - revert AccountNotConfigured(); - } - if (recoveryConfig.delay > recoveryConfig.expiry) { - revert DelayMoreThanExpiry(); - } - if (recoveryConfig.expiry - recoveryConfig.delay < MINIMUM_RECOVERY_WINDOW) { - revert RecoveryWindowTooShort(); + bool success = ISafe(account).execTransactionFromModule({ + to: account, + value: 0, + data: recoveryCalldata, + operation: uint8(Enum.Operation.Call) + }); + if (!success) { + revert RecoveryFailed(account); } - recoveryConfigs[account] = recoveryConfig; - - emit RecoveryConfigUpdated(account, recoveryConfig.delay, recoveryConfig.expiry); - } - - /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ - /* HANDLE ACCEPTANCE */ - /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ - - /** - * @notice Accepts a guardian for the specified account. This is the second core function - * that must be called during the end-to-end recovery flow - * @dev Called once per guardian added. Although this adds an extra step to recovery, this - * acceptance flow is an important security feature to ensure that no typos are made when adding - * a guardian, and that the guardian is in control of the specified email address. Called as - * part of handleAcceptance in EmailAccountRecovery - * @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 - */ - function acceptGuardian( - address guardian, - uint256 templateIdx, - bytes[] memory subjectParams, - bytes32 - ) - internal - override - { - if (templateIdx != 0) { - revert InvalidTemplateIndex(); - } - if (subjectParams.length != 1) revert InvalidSubjectParams(); - - address account = abi.decode(subjectParams[0], (address)); - - if (recoveryRequests[account].currentWeight > 0) { - revert RecoveryInProcess(); - } - - bool moduleEnabled = ISafe(account).isModuleEnabled(address(this)); - if (!moduleEnabled) { - revert RecoveryModuleNotAuthorized(); - } - - // This check ensures GuardianStatus is correct and also implicitly that the - // account in email is a valid account - GuardianStorage memory guardianStorage = getGuardian(account, guardian); - if (guardianStorage.status != GuardianStatus.REQUESTED) { - revert InvalidGuardianStatus(guardianStorage.status, GuardianStatus.REQUESTED); - } - - guardiansStorage.updateGuardianStatus(account, guardian, GuardianStatus.ACCEPTED); - - emit GuardianAccepted(account, guardian); - } - - /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ - /* HANDLE RECOVERY */ - /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ - - /** - * @notice Processes a recovery request for a given account. This is the third core function - * that must be called during the end-to-end recovery flow - * @dev Called once per guardian until the threshold is reached - * @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 - */ - function processRecovery( - address guardian, - uint256 templateIdx, - bytes[] memory subjectParams, - bytes32 - ) - internal - override - { - if (templateIdx != 0) { - revert InvalidTemplateIndex(); - } - if (subjectParams.length != 4) { - revert InvalidSubjectParams(); - } - - address account = abi.decode(subjectParams[0], (address)); - address oldOwner = abi.decode(subjectParams[1], (address)); - address newOwner = abi.decode(subjectParams[2], (address)); - // FIXME: recovery module address? - - bool moduleEnabled = ISafe(account).isModuleEnabled(address(this)); - - if (!moduleEnabled) { - revert RecoveryModuleNotAuthorized(); - } - - // This check ensures GuardianStatus is correct and also implicitly that the - // account in email is a valid account - GuardianStorage memory guardianStorage = getGuardian(account, guardian); - if (guardianStorage.status != GuardianStatus.ACCEPTED) { - revert InvalidGuardianStatus(guardianStorage.status, GuardianStatus.ACCEPTED); - } - bool isOwner = ISafe(account).isOwner(oldOwner); - if (!isOwner) { - revert InvalidOldOwner(); - } - if (newOwner == address(0)) { - revert InvalidNewOwner(); - } - - address previousOwnerInLinkedList = getPreviousOwnerInLinkedList(account, oldOwner); - bytes memory recoveryCallData = abi.encodeWithSignature( - "swapOwner(address,address,address)", previousOwnerInLinkedList, oldOwner, newOwner - ); - bytes32 calldataHash = keccak256(recoveryCallData); - - RecoveryRequest storage recoveryRequest = recoveryRequests[account]; - recoveryRequest.currentWeight += guardianStorage.weight; - - uint256 threshold = guardianConfigs[account].threshold; - if (recoveryRequest.currentWeight >= threshold) { - uint256 executeAfter = block.timestamp + recoveryConfigs[account].delay; - uint256 executeBefore = block.timestamp + recoveryConfigs[account].expiry; - - recoveryRequest.executeAfter = executeAfter; - recoveryRequest.executeBefore = executeBefore; - recoveryRequest.calldataHash = calldataHash; - - emit RecoveryProcessed(account, executeAfter, executeBefore); - } - } - - /** - * @notice Gets the previous owner in the Safe owners linked list that points to the - * owner passed into the function - * @param safe The Safe account to query - * @param oldOwner The owner address to get the previous owner for - * @return previousOwner The previous owner in the Safe owners linked list pointing to the owner - * passed in - */ - function getPreviousOwnerInLinkedList( - address safe, - address oldOwner - ) - internal - view - returns (address) - { - address[] memory owners = ISafe(safe).getOwners(); - uint256 length = owners.length; - - uint256 oldOwnerIndex; - for (uint256 i; i < length; i++) { - if (owners[i] == oldOwner) { - oldOwnerIndex = i; - break; - } - } - address sentinelOwner = address(0x1); - return oldOwnerIndex == 0 ? sentinelOwner : owners[oldOwnerIndex - 1]; - } - - /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ - /* COMPLETE RECOVERY */ - /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ - - /** - * @notice Completes the recovery process for a given account. This is the forth and final - * core function that must be called during the end-to-end recovery flow. Can be called by - * anyone. - * @dev Validates the recovery request by checking the total weight, that the delay has passed, - * and the request has not expired. Triggers the recovery module to perform the recovery. The - * recovery module trusts that this contract has validated the recovery attempt. This function - * deletes the recovery request but recovery config state is maintained so future recovery - * requests can be made without having to reconfigure everything - * @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 { - if (account == address(0)) { - revert InvalidAccountAddress(); - } - RecoveryRequest memory recoveryRequest = recoveryRequests[account]; - - uint256 threshold = guardianConfigs[account].threshold; - if (threshold == 0) { - revert NoRecoveryConfigured(); - } - - if (recoveryRequest.currentWeight < threshold) { - revert NotEnoughApprovals(); - } - - if (block.timestamp < recoveryRequest.executeAfter) { - revert DelayNotPassed(); - } - - if (block.timestamp >= recoveryRequest.executeBefore) { - revert RecoveryRequestExpired(); - } - - bytes32 calldataHash = keccak256(recoveryCalldata); - if (calldataHash != recoveryRequest.calldataHash) { - revert InvalidCalldataHash(); - } - - delete recoveryRequests[account]; - - ISafe(account).execTransactionFromModule(account, 0, recoveryCalldata, 0); - - emit RecoveryCompleted(account); - } - - /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ - /* CANCEL LOGIC */ - /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ - - /** - * @notice Cancels the recovery request for the caller's account - * @dev Deletes the current recovery request associated with the caller's account - */ - function cancelRecovery() external virtual { - delete recoveryRequests[msg.sender]; - emit RecoveryCancelled(msg.sender); - } - - function deInitRecoveryFromModule(address account) external { } - - /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ - /* GUARDIAN LOGIC */ - /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ - - /** - * @notice Retrieves the guardian configuration for a given account - * @param account The address of the account for which the guardian configuration is being - * retrieved - * @return GuardianConfig The guardian configuration for the specified account - */ - function getGuardianConfig(address account) external view returns (GuardianConfig memory) { - return guardianConfigs[account]; - } - - /** - * @notice Retrieves the guardian storage details for a given guardian and account - * @param account The address of the account associated with the guardian - * @param guardian The address of the guardian - * @return GuardianStorage The guardian storage details for the specified guardian and account - */ - function getGuardian( - address account, - address guardian - ) - public - view - returns (GuardianStorage memory) - { - return guardiansStorage.getGuardianStorage(account, guardian); - } - - /** - * @notice Sets up guardians for a given account with specified weights and threshold - * @dev This function can only be called once and ensures the guardians, weights, and threshold - * are correctly configured - * @param account The address of the account for which guardians are being set up - * @param guardians An array of guardian addresses - * @param weights An array of weights corresponding to each guardian - * @param threshold The threshold weight required for guardians to approve recovery attempts - */ - function setupGuardians( - address account, - address[] memory guardians, - uint256[] memory weights, - uint256 threshold - ) - internal - { - guardianConfigs.setupGuardians(guardiansStorage, account, guardians, weights, threshold); - } - - /** - * @notice Adds a guardian for the caller's account with a specified weight - * @dev This function can only be called by the account associated with the guardian and only if - * no recovery is in process - * @param guardian The address of the guardian to be added - * @param weight The weight assigned to the guardian - */ - function addGuardian(address guardian, uint256 weight) external onlyWhenNotRecovering { - guardiansStorage.addGuardian(guardianConfigs, msg.sender, guardian, weight); - } - - /** - * @notice Removes a guardian for the caller's account - * @dev This function can only be called by the account associated with the guardian and only if - * no recovery is in process - * @param guardian The address of the guardian to be removed - */ - function removeGuardian(address guardian) external onlyWhenNotRecovering { - guardiansStorage.removeGuardian(guardianConfigs, msg.sender, guardian); - } - - /** - * @notice Changes the threshold for guardian approvals for the caller's account - * @dev This function can only be called by the account associated with the guardian config and - * only if no recovery is in process - * @param threshold The new threshold for guardian approvals - */ - function changeThreshold(uint256 threshold) external onlyWhenNotRecovering { - guardianConfigs.changeThreshold(msg.sender, threshold); + emit RecoveryExecuted(account); } } diff --git a/test/integration/SafeRecovery/SafeNativeIntegrationBase.t.sol b/test/integration/SafeRecovery/SafeNativeIntegrationBase.t.sol index 3b181eb2..4dc54896 100644 --- a/test/integration/SafeRecovery/SafeNativeIntegrationBase.t.sol +++ b/test/integration/SafeRecovery/SafeNativeIntegrationBase.t.sol @@ -22,6 +22,8 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { using Strings for uint256; using Strings for address; + EmailRecoveryManager emailRecoveryManager; + address emailRecoveryManagerAddress; SafeEmailRecoveryModule safeEmailRecoveryModule; Safe public safeSingleton; Safe public safe; @@ -30,6 +32,7 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { bytes isInstalledContext; bytes4 functionSelector; uint256 nullifierCount; + address subjectHandler; /** * Helper function to return if current account type is safe or not @@ -43,15 +46,31 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { } } + function skipIfNotSafeAccountType() public { + if (isAccountTypeSafe()) { + vm.skip(false); + } else { + vm.skip(true); + } + } + function setUp() public virtual override { if (!isAccountTypeSafe()) { return; } super.setUp(); - safeEmailRecoveryModule = new SafeEmailRecoveryModule( - address(verifier), address(ecdsaOwnedDkimRegistry), address(emailAuthImpl) + subjectHandler = address(new SafeRecoverySubjectHandler()); + emailRecoveryManager = new EmailRecoveryManager( + address(verifier), + address(ecdsaOwnedDkimRegistry), + address(emailAuthImpl), + address(subjectHandler) ); + emailRecoveryManagerAddress = address(emailRecoveryManager); + + safeEmailRecoveryModule = new SafeEmailRecoveryModule(emailRecoveryManagerAddress); + emailRecoveryManager.initialize(address(safeEmailRecoveryModule)); safeSingleton = new Safe(); SafeProxy safeProxy = new SafeProxy(address(safeSingleton)); @@ -65,9 +84,9 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { // Compute guardian addresses guardians1 = new address[](3); - guardians1[0] = safeEmailRecoveryModule.computeEmailAuthAddress(safeAddress, accountSalt1); - guardians1[1] = safeEmailRecoveryModule.computeEmailAuthAddress(safeAddress, accountSalt2); - guardians1[2] = safeEmailRecoveryModule.computeEmailAuthAddress(safeAddress, accountSalt3); + guardians1[0] = emailRecoveryManager.computeEmailAuthAddress(safeAddress, accountSalt1); + guardians1[1] = emailRecoveryManager.computeEmailAuthAddress(safeAddress, accountSalt2); + guardians1[2] = emailRecoveryManager.computeEmailAuthAddress(safeAddress, accountSalt3); address[] memory owners = new address[](1); owner = owner1; @@ -138,7 +157,7 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { function acceptGuardian(address account, address guardian) public { EmailAuthMsg memory emailAuthMsg = getAcceptanceEmailAuthMessage(account, guardian); - safeEmailRecoveryModule.handleAcceptance(emailAuthMsg, templateIdx); + emailRecoveryManager.handleAcceptance(emailAuthMsg, templateIdx); } function getAcceptanceEmailAuthMessage( @@ -158,7 +177,7 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { bytes[] memory subjectParamsForAcceptance = new bytes[](1); subjectParamsForAcceptance[0] = abi.encode(account); return EmailAuthMsg({ - templateId: safeEmailRecoveryModule.computeAcceptanceTemplateId(templateIdx), + templateId: emailRecoveryManager.computeAcceptanceTemplateId(templateIdx), subjectParams: subjectParamsForAcceptance, skipedSubjectPrefix: 0, proof: emailProof @@ -175,7 +194,7 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { { EmailAuthMsg memory emailAuthMsg = getRecoveryEmailAuthMessage(account, oldOwner, newOwner, guardian); - safeEmailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx); + emailRecoveryManager.handleRecovery(emailAuthMsg, templateIdx); } function getRecoveryEmailAuthMessage( @@ -215,7 +234,7 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { subjectParamsForRecovery[3] = abi.encode(address(safeEmailRecoveryModule)); return EmailAuthMsg({ - templateId: safeEmailRecoveryModule.computeRecoveryTemplateId(templateIdx), + templateId: emailRecoveryManager.computeRecoveryTemplateId(templateIdx), subjectParams: subjectParamsForRecovery, skipedSubjectPrefix: 0, proof: emailProof diff --git a/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol b/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol index 8ed70460..fd10d18c 100644 --- a/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol +++ b/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol @@ -20,13 +20,12 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase } function testIntegration_AccountRecovery() public { - bool moduleEnabled = safe.isModuleEnabled(address(safeEmailRecoveryModule)); + skipIfNotSafeAccountType(); address newOwner = owner2; - // Configure recovery vm.startPrank(safeAddress); - safeEmailRecoveryModule.configureRecovery( + emailRecoveryManager.configureRecovery( guardians1, guardianWeights, threshold, delay, expiry ); vm.stopPrank(); @@ -44,17 +43,17 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase // Accept guardian EmailAuthMsg memory emailAuthMsg = getAcceptanceEmailAuthMessage(safeAddress, guardians1[0]); - safeEmailRecoveryModule.handleAcceptance(emailAuthMsg, templateIdx); + emailRecoveryManager.handleAcceptance(emailAuthMsg, templateIdx); GuardianStorage memory guardianStorage1 = - safeEmailRecoveryModule.getGuardian(safeAddress, guardians1[0]); + emailRecoveryManager.getGuardian(safeAddress, guardians1[0]); assertEq(uint256(guardianStorage1.status), uint256(GuardianStatus.ACCEPTED)); assertEq(guardianStorage1.weight, uint256(1)); // Accept guardian emailAuthMsg = getAcceptanceEmailAuthMessage(safeAddress, guardians1[1]); - safeEmailRecoveryModule.handleAcceptance(emailAuthMsg, templateIdx); + emailRecoveryManager.handleAcceptance(emailAuthMsg, templateIdx); GuardianStorage memory guardianStorage2 = - safeEmailRecoveryModule.getGuardian(safeAddress, guardians1[1]); + emailRecoveryManager.getGuardian(safeAddress, guardians1[1]); assertEq(uint256(guardianStorage2.status), uint256(GuardianStatus.ACCEPTED)); assertEq(guardianStorage2.weight, uint256(2)); @@ -63,17 +62,17 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase // handle recovery request for guardian 1 emailAuthMsg = getRecoveryEmailAuthMessage(safeAddress, owner, newOwner, guardians1[0]); - safeEmailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx); + emailRecoveryManager.handleRecovery(emailAuthMsg, templateIdx); IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = - safeEmailRecoveryModule.getRecoveryRequest(safeAddress); + emailRecoveryManager.getRecoveryRequest(safeAddress); assertEq(recoveryRequest.currentWeight, 1); // handle recovery request for guardian 2 uint256 executeAfter = block.timestamp + delay; uint256 executeBefore = block.timestamp + expiry; emailAuthMsg = getRecoveryEmailAuthMessage(safeAddress, owner, newOwner, guardians1[1]); - safeEmailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx); - recoveryRequest = safeEmailRecoveryModule.getRecoveryRequest(safeAddress); + emailRecoveryManager.handleRecovery(emailAuthMsg, templateIdx); + recoveryRequest = emailRecoveryManager.getRecoveryRequest(safeAddress); assertEq(recoveryRequest.executeAfter, executeAfter); assertEq(recoveryRequest.executeBefore, executeBefore); assertEq(recoveryRequest.currentWeight, 3); @@ -81,9 +80,9 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase vm.warp(block.timestamp + delay); // Complete recovery - safeEmailRecoveryModule.completeRecovery(safeAddress, recoveryCalldata); + emailRecoveryManager.completeRecovery(safeAddress, recoveryCalldata); - recoveryRequest = safeEmailRecoveryModule.getRecoveryRequest(safeAddress); + recoveryRequest = emailRecoveryManager.getRecoveryRequest(safeAddress); assertEq(recoveryRequest.executeAfter, 0); assertEq(recoveryRequest.executeBefore, 0); assertEq(recoveryRequest.currentWeight, 0); From 730abb17ceb433818064c49b5aab32a9bc00a46e Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Thu, 1 Aug 2024 23:34:31 +0100 Subject: [PATCH 4/5] Use latest manager with native safe module --- script/DeploySafeNativeRecovery.s.sol | 37 +++++++++++-- src/EmailRecoveryManager.sol | 2 +- src/modules/SafeEmailRecoveryModule.sol | 53 +++++++++---------- .../SafeNativeIntegrationBase.t.sol | 50 ++++++----------- .../SafeRecoveryNativeModule.t.sol | 29 +++++----- .../configureRecovery.t.sol | 20 +++---- .../UniversalEmailRecoveryModuleHarness.sol | 12 ----- .../onUninstall.t.sol | 7 --- 8 files changed, 94 insertions(+), 116 deletions(-) diff --git a/script/DeploySafeNativeRecovery.s.sol b/script/DeploySafeNativeRecovery.s.sol index 081d6d1b..b037b742 100644 --- a/script/DeploySafeNativeRecovery.s.sol +++ b/script/DeploySafeNativeRecovery.s.sol @@ -3,19 +3,46 @@ pragma solidity ^0.8.25; import { Script } from "forge-std/Script.sol"; import { console } from "forge-std/console.sol"; +import { Verifier } from "ether-email-auth/packages/contracts/src/utils/Verifier.sol"; +import { ECDSAOwnedDKIMRegistry } from + "ether-email-auth/packages/contracts/src/utils/ECDSAOwnedDKIMRegistry.sol"; +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"; contract DeploySafeNativeRecovery_Script is Script { function run() public { vm.startBroadcast(vm.envUint("PRIVATE_KEY")); - address manager = vm.envOr("VERIFIER", address(0)); + address verifier = vm.envOr("VERIFIER", address(0)); + address dkimRegistry = vm.envOr("DKIM_REGISTRY", address(0)); + address dkimRegistrySigner = vm.envOr("SIGNER", address(0)); + address emailAuthImpl = vm.envOr("EMAIL_AUTH_IMPL", address(0)); + address subjectHandler = vm.envOr("SUBJECT_HANDLER", address(0)); - if (manager == address(0)) { - manager = address(1); - console.log("Deployed Manager at", manager); + if (verifier == address(0)) { + verifier = address(new Verifier()); + console.log("Deployed Verifier at", verifier); } - address module = address(new SafeEmailRecoveryModule(manager)); + if (dkimRegistry == address(0)) { + require(dkimRegistrySigner != address(0), "DKIM_REGISTRY_SIGNER is required"); + dkimRegistry = address(new ECDSAOwnedDKIMRegistry(dkimRegistrySigner)); + console.log("Deployed DKIM Registry at", dkimRegistry); + } + + if (emailAuthImpl == address(0)) { + emailAuthImpl = address(new EmailAuth()); + console.log("Deployed Email Auth at", emailAuthImpl); + } + + if (subjectHandler == address(0)) { + subjectHandler = address(new SafeRecoverySubjectHandler()); + console.log("Deployed Subject Handler at", subjectHandler); + } + + address module = address( + new SafeEmailRecoveryModule(verifier, dkimRegistry, emailAuthImpl, subjectHandler) + ); console.log("Deployed Email Recovery Module at ", vm.toString(module)); diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 439fa8e1..372978c3 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -201,7 +201,7 @@ abstract contract EmailRecoveryManager is uint256 delay, uint256 expiry ) - internal + public { address account = msg.sender; diff --git a/src/modules/SafeEmailRecoveryModule.sol b/src/modules/SafeEmailRecoveryModule.sol index de740a81..e428873a 100644 --- a/src/modules/SafeEmailRecoveryModule.sol +++ b/src/modules/SafeEmailRecoveryModule.sol @@ -3,59 +3,54 @@ pragma solidity ^0.8.25; import { ISafe } from "../interfaces/ISafe.sol"; import { Enum } from "@safe-global/safe-contracts/contracts/common/Enum.sol"; +import { EmailRecoveryManager } from "../EmailRecoveryManager.sol"; /** * A safe module that recovers a safe owner via ZK Email */ -contract SafeEmailRecoveryModule { +contract SafeEmailRecoveryModule is EmailRecoveryManager { bytes4 public constant selector = bytes4(keccak256(bytes("swapOwner(address,address,address)"))); - /** - * Trusted email recovery manager contract that handles recovery requests - */ - address public immutable emailRecoveryManager; - event RecoveryExecuted(address indexed account); - error ModuleEnabledCheckFailed(address account, address module); - error NotTrustedRecoveryManager(); error InvalidSelector(bytes4 selector); error RecoveryFailed(address account); - constructor(address _emailRecoveryManager) { - emailRecoveryManager = _emailRecoveryManager; - } + constructor( + address verifier, + address dkimRegistry, + address emailAuthImpl, + address subjectHandler + ) + EmailRecoveryManager(verifier, dkimRegistry, emailAuthImpl, subjectHandler) + { } /** - * Check if the recovery module is authorized to recover the account + * Check if a recovery request can be initiated based on guardian acceptance * @param account The smart account to check - * @return true if the module is authorized, false otherwise + * @return true if the recovery request can be started, false otherwise */ - function isAuthorizedToRecover(address account) external returns (bool) { - (bool success, bytes memory returnData) = ISafe(account).execTransactionFromModuleReturnData({ - to: account, - value: 0, - data: abi.encodeWithSignature("isModuleEnabled(address)", address(this)), - operation: uint8(Enum.Operation.Call) - }); - if (!success) { - revert ModuleEnabledCheckFailed(account, address(this)); - } - return abi.decode(returnData, (bool)); + function canStartRecoveryRequest(address account) external view returns (bool) { + GuardianConfig memory guardianConfig = getGuardianConfig(account); + + return guardianConfig.acceptedWeight >= guardianConfig.threshold; } /** * @notice Executes recovery on a Safe account. 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 Safe + * @param recoveryData The recovery calldata that should be executed on the Safe * being recovered */ - function recover(address account, bytes calldata recoveryCalldata) public { - if (msg.sender != emailRecoveryManager) { - revert NotTrustedRecoveryManager(); + function recover(address account, bytes calldata recoveryData) internal override { + (, bytes memory recoveryCalldata) = abi.decode(recoveryData, (address, bytes)); + // FIXME: What if you use this module with a different subject handler? It could chose + // not to encode the account/validator along with the calldata + bytes4 calldataSelector; + assembly { + calldataSelector := mload(add(recoveryCalldata, 32)) } - bytes4 calldataSelector = bytes4(recoveryCalldata[:4]); if (calldataSelector != selector) { revert InvalidSelector(calldataSelector); } diff --git a/test/integration/SafeRecovery/SafeNativeIntegrationBase.t.sol b/test/integration/SafeRecovery/SafeNativeIntegrationBase.t.sol index 4dc54896..cefabaa3 100644 --- a/test/integration/SafeRecovery/SafeNativeIntegrationBase.t.sol +++ b/test/integration/SafeRecovery/SafeNativeIntegrationBase.t.sol @@ -12,8 +12,6 @@ import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; import { Safe } from "@safe-global/safe-contracts/contracts/Safe.sol"; import { SafeProxy } from "@safe-global/safe-contracts/contracts/proxies/SafeProxy.sol"; import { SafeEmailRecoveryModule } from "src/modules/SafeEmailRecoveryModule.sol"; -import { EmailRecoveryManager } from "src/EmailRecoveryManager.sol"; -import { UniversalEmailRecoveryModule } from "src/modules/UniversalEmailRecoveryModule.sol"; import { SafeRecoverySubjectHandler } from "src/handlers/SafeRecoverySubjectHandler.sol"; import { IntegrationBase } from "../IntegrationBase.t.sol"; @@ -22,9 +20,8 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { using Strings for uint256; using Strings for address; - EmailRecoveryManager emailRecoveryManager; - address emailRecoveryManagerAddress; - SafeEmailRecoveryModule safeEmailRecoveryModule; + SafeEmailRecoveryModule emailRecoveryModule; + address emailRecoveryModuleAddress; Safe public safeSingleton; Safe public safe; address public safeAddress; @@ -61,53 +58,38 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { super.setUp(); subjectHandler = address(new SafeRecoverySubjectHandler()); - emailRecoveryManager = new EmailRecoveryManager( + emailRecoveryModule = new SafeEmailRecoveryModule( address(verifier), - address(ecdsaOwnedDkimRegistry), + address(dkimRegistry), address(emailAuthImpl), address(subjectHandler) ); - emailRecoveryManagerAddress = address(emailRecoveryManager); - - safeEmailRecoveryModule = new SafeEmailRecoveryModule(emailRecoveryManagerAddress); - emailRecoveryManager.initialize(address(safeEmailRecoveryModule)); + emailRecoveryModuleAddress = address(emailRecoveryModule); safeSingleton = new Safe(); SafeProxy safeProxy = new SafeProxy(address(safeSingleton)); safe = Safe(payable(address(safeProxy))); safeAddress = address(safe); - // safe4337Module = new Safe4337Module(entryPointAddress); - // safeModuleSetup = new SafeModuleSetup(); isInstalledContext = bytes("0"); functionSelector = bytes4(keccak256(bytes("swapOwner(address,address,address)"))); // Compute guardian addresses guardians1 = new address[](3); - guardians1[0] = emailRecoveryManager.computeEmailAuthAddress(safeAddress, accountSalt1); - guardians1[1] = emailRecoveryManager.computeEmailAuthAddress(safeAddress, accountSalt2); - guardians1[2] = emailRecoveryManager.computeEmailAuthAddress(safeAddress, accountSalt3); + guardians1[0] = emailRecoveryModule.computeEmailAuthAddress(safeAddress, accountSalt1); + guardians1[1] = emailRecoveryModule.computeEmailAuthAddress(safeAddress, accountSalt2); + guardians1[2] = emailRecoveryModule.computeEmailAuthAddress(safeAddress, accountSalt3); address[] memory owners = new address[](1); owner = owner1; owners[0] = owner; safe.setup( - owners, - 1, - address(0), - bytes("0"), - address(0), - // address(safeModuleSetup), - // abi.encodeCall(SafeModuleSetup.enableModules, (modules)), - // address(safe4337Module), - address(0), - 0, - payable(address(0)) + owners, 1, address(0), bytes("0"), address(0), address(0), 0, payable(address(0)) ); vm.startPrank(safeAddress); - safe.enableModule(address(safeEmailRecoveryModule)); + safe.enableModule(address(emailRecoveryModule)); vm.stopPrank(); } @@ -157,7 +139,7 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { function acceptGuardian(address account, address guardian) public { EmailAuthMsg memory emailAuthMsg = getAcceptanceEmailAuthMessage(account, guardian); - emailRecoveryManager.handleAcceptance(emailAuthMsg, templateIdx); + emailRecoveryModule.handleAcceptance(emailAuthMsg, templateIdx); } function getAcceptanceEmailAuthMessage( @@ -177,7 +159,7 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { bytes[] memory subjectParamsForAcceptance = new bytes[](1); subjectParamsForAcceptance[0] = abi.encode(account); return EmailAuthMsg({ - templateId: emailRecoveryManager.computeAcceptanceTemplateId(templateIdx), + templateId: emailRecoveryModule.computeAcceptanceTemplateId(templateIdx), subjectParams: subjectParamsForAcceptance, skipedSubjectPrefix: 0, proof: emailProof @@ -194,7 +176,7 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { { EmailAuthMsg memory emailAuthMsg = getRecoveryEmailAuthMessage(account, oldOwner, newOwner, guardian); - emailRecoveryManager.handleRecovery(emailAuthMsg, templateIdx); + emailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx); } function getRecoveryEmailAuthMessage( @@ -210,7 +192,7 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { string memory oldOwnerString = SubjectUtils.addressToChecksumHexString(oldOwner); string memory newOwnerString = SubjectUtils.addressToChecksumHexString(newOwner); string memory recoveryModuleString = - SubjectUtils.addressToChecksumHexString(address(safeEmailRecoveryModule)); + SubjectUtils.addressToChecksumHexString(address(emailRecoveryModule)); string memory subject = string.concat( "Recover account ", @@ -231,10 +213,10 @@ abstract contract SafeNativeIntegrationBase is IntegrationBase { subjectParamsForRecovery[0] = abi.encode(account); subjectParamsForRecovery[1] = abi.encode(oldOwner); subjectParamsForRecovery[2] = abi.encode(newOwner); - subjectParamsForRecovery[3] = abi.encode(address(safeEmailRecoveryModule)); + subjectParamsForRecovery[3] = abi.encode(emailRecoveryModuleAddress); return EmailAuthMsg({ - templateId: emailRecoveryManager.computeRecoveryTemplateId(templateIdx), + templateId: emailRecoveryModule.computeRecoveryTemplateId(templateIdx), subjectParams: subjectParamsForRecovery, skipedSubjectPrefix: 0, proof: emailProof diff --git a/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol b/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol index fd10d18c..df30286f 100644 --- a/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol +++ b/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol @@ -25,35 +25,34 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase address newOwner = owner2; // Configure recovery vm.startPrank(safeAddress); - emailRecoveryManager.configureRecovery( - guardians1, guardianWeights, threshold, delay, expiry - ); + emailRecoveryModule.configureRecovery(guardians1, guardianWeights, threshold, delay, expiry); vm.stopPrank(); bytes memory recoveryCalldata = abi.encodeWithSignature( "swapOwner(address,address,address)", address(1), owner, newOwner ); - bytes32 calldataHash = keccak256(recoveryCalldata); + bytes memory recoveryData = abi.encode(safeAddress, recoveryCalldata); + bytes32 calldataHash = keccak256(recoveryData); bytes[] memory subjectParamsForRecovery = new bytes[](4); subjectParamsForRecovery[0] = abi.encode(safeAddress); subjectParamsForRecovery[1] = abi.encode(owner); subjectParamsForRecovery[2] = abi.encode(newOwner); - subjectParamsForRecovery[3] = abi.encode(address(safeEmailRecoveryModule)); + subjectParamsForRecovery[3] = abi.encode(emailRecoveryModuleAddress); // Accept guardian EmailAuthMsg memory emailAuthMsg = getAcceptanceEmailAuthMessage(safeAddress, guardians1[0]); - emailRecoveryManager.handleAcceptance(emailAuthMsg, templateIdx); + emailRecoveryModule.handleAcceptance(emailAuthMsg, templateIdx); GuardianStorage memory guardianStorage1 = - emailRecoveryManager.getGuardian(safeAddress, guardians1[0]); + emailRecoveryModule.getGuardian(safeAddress, guardians1[0]); assertEq(uint256(guardianStorage1.status), uint256(GuardianStatus.ACCEPTED)); assertEq(guardianStorage1.weight, uint256(1)); // Accept guardian emailAuthMsg = getAcceptanceEmailAuthMessage(safeAddress, guardians1[1]); - emailRecoveryManager.handleAcceptance(emailAuthMsg, templateIdx); + emailRecoveryModule.handleAcceptance(emailAuthMsg, templateIdx); GuardianStorage memory guardianStorage2 = - emailRecoveryManager.getGuardian(safeAddress, guardians1[1]); + emailRecoveryModule.getGuardian(safeAddress, guardians1[1]); assertEq(uint256(guardianStorage2.status), uint256(GuardianStatus.ACCEPTED)); assertEq(guardianStorage2.weight, uint256(2)); @@ -62,17 +61,17 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase // handle recovery request for guardian 1 emailAuthMsg = getRecoveryEmailAuthMessage(safeAddress, owner, newOwner, guardians1[0]); - emailRecoveryManager.handleRecovery(emailAuthMsg, templateIdx); + emailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx); IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = - emailRecoveryManager.getRecoveryRequest(safeAddress); + emailRecoveryModule.getRecoveryRequest(safeAddress); assertEq(recoveryRequest.currentWeight, 1); // handle recovery request for guardian 2 uint256 executeAfter = block.timestamp + delay; uint256 executeBefore = block.timestamp + expiry; emailAuthMsg = getRecoveryEmailAuthMessage(safeAddress, owner, newOwner, guardians1[1]); - emailRecoveryManager.handleRecovery(emailAuthMsg, templateIdx); - recoveryRequest = emailRecoveryManager.getRecoveryRequest(safeAddress); + emailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx); + recoveryRequest = emailRecoveryModule.getRecoveryRequest(safeAddress); assertEq(recoveryRequest.executeAfter, executeAfter); assertEq(recoveryRequest.executeBefore, executeBefore); assertEq(recoveryRequest.currentWeight, 3); @@ -80,9 +79,9 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase vm.warp(block.timestamp + delay); // Complete recovery - emailRecoveryManager.completeRecovery(safeAddress, recoveryCalldata); + emailRecoveryModule.completeRecovery(safeAddress, recoveryData); - recoveryRequest = emailRecoveryManager.getRecoveryRequest(safeAddress); + recoveryRequest = emailRecoveryModule.getRecoveryRequest(safeAddress); assertEq(recoveryRequest.executeAfter, 0); assertEq(recoveryRequest.executeBefore, 0); assertEq(recoveryRequest.currentWeight, 0); diff --git a/test/unit/EmailRecoveryManager/configureRecovery.t.sol b/test/unit/EmailRecoveryManager/configureRecovery.t.sol index 0b16dc1d..da8eb1ef 100644 --- a/test/unit/EmailRecoveryManager/configureRecovery.t.sol +++ b/test/unit/EmailRecoveryManager/configureRecovery.t.sol @@ -24,17 +24,13 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { vm.expectRevert(IEmailRecoveryManager.SetupAlreadyCalled.selector); vm.startPrank(accountAddress); - emailRecoveryModule.exposed_configureRecovery( - guardians, guardianWeights, threshold, delay, expiry - ); + emailRecoveryModule.configureRecovery(guardians, guardianWeights, threshold, delay, expiry); } function test_ConfigureRecovery_RevertWhen_ConfigureRecoveryCalledTwice() public { vm.startPrank(accountAddress); vm.expectRevert(IEmailRecoveryManager.SetupAlreadyCalled.selector); - emailRecoveryModule.exposed_configureRecovery( - guardians, guardianWeights, threshold, delay, expiry - ); + emailRecoveryModule.configureRecovery(guardians, guardianWeights, threshold, delay, expiry); } function test_ConfigureRecovery_Succeeds() public { @@ -49,9 +45,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { emit IEmailRecoveryManager.RecoveryConfigured( instance.account, guardians.length, totalWeight, threshold ); - emailRecoveryModule.exposed_configureRecovery( - guardians, guardianWeights, threshold, delay, expiry - ); + emailRecoveryModule.configureRecovery(guardians, guardianWeights, threshold, delay, expiry); IEmailRecoveryManager.RecoveryConfig memory recoveryConfig = emailRecoveryModule.getRecoveryConfig(accountAddress); @@ -87,7 +81,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { guardianWeights.length ) ); - emailRecoveryModule.exposed_configureRecovery( + emailRecoveryModule.configureRecovery( zeroGuardians, guardianWeights, threshold, delay, expiry ); } @@ -105,7 +99,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { zeroGuardianWeights.length ) ); - emailRecoveryModule.exposed_configureRecovery( + emailRecoveryModule.configureRecovery( guardians, zeroGuardianWeights, threshold, delay, expiry ); } @@ -117,7 +111,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { uint256 zeroThreshold = 0; vm.expectRevert(IGuardianManager.ThresholdCannotBeZero.selector); - emailRecoveryModule.exposed_configureRecovery( + emailRecoveryModule.configureRecovery( guardians, guardianWeights, zeroThreshold, delay, expiry ); } @@ -135,7 +129,7 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase { IGuardianManager.ThresholdExceedsTotalWeight.selector, threshold, 0 ) ); - emailRecoveryModule.exposed_configureRecovery( + emailRecoveryModule.configureRecovery( zeroGuardians, zeroGuardianWeights, threshold, delay, expiry ); } diff --git a/test/unit/UniversalEmailRecoveryModuleHarness.sol b/test/unit/UniversalEmailRecoveryModuleHarness.sol index c2694f82..0bd4ea40 100644 --- a/test/unit/UniversalEmailRecoveryModuleHarness.sol +++ b/test/unit/UniversalEmailRecoveryModuleHarness.sol @@ -18,18 +18,6 @@ contract UniversalEmailRecoveryModuleHarness is UniversalEmailRecoveryModule { UniversalEmailRecoveryModule(verifier, dkimRegistry, emailAuthImpl, subjectHandler) { } - function exposed_configureRecovery( - address[] memory guardians, - uint256[] memory weights, - uint256 threshold, - uint256 delay, - uint256 expiry - ) - external - { - configureRecovery(guardians, weights, threshold, delay, expiry); - } - function exposed_acceptGuardian( address guardian, uint256 templateIdx, diff --git a/test/unit/modules/UniversalEmailRecoveryModule/onUninstall.t.sol b/test/unit/modules/UniversalEmailRecoveryModule/onUninstall.t.sol index b9611f30..ad8b7621 100644 --- a/test/unit/modules/UniversalEmailRecoveryModule/onUninstall.t.sol +++ b/test/unit/modules/UniversalEmailRecoveryModule/onUninstall.t.sol @@ -15,13 +15,6 @@ contract UniversalEmailRecoveryModule_onUninstall_Test is UnitBase { super.setUp(); } - // TODO: Assess - function test_OnUninstall_RevertWhen_InvalidValidatorsLength() public { - vm.prank(accountAddress); - instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); - vm.stopPrank(); - } - function test_OnUninstall_Succeeds() public { vm.prank(accountAddress); instance.uninstallModule(MODULE_TYPE_EXECUTOR, recoveryModuleAddress, ""); From 1e7da8c0829a4bd95a57923a80b455ecb58fe9ef Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 7 Aug 2024 17:20:26 +0100 Subject: [PATCH 5/5] Use encoded account in Safe recovery module --- src/modules/SafeEmailRecoveryModule.sol | 18 +++++++++++------- .../EmailRecoveryModule.t.sol | 3 +++ .../UniversalEmailRecoveryModule.t.sol | 3 +++ .../SafeRecoveryNativeModule.t.sol | 7 ++++++- 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/modules/SafeEmailRecoveryModule.sol b/src/modules/SafeEmailRecoveryModule.sol index e428873a..c4dfe9de 100644 --- a/src/modules/SafeEmailRecoveryModule.sol +++ b/src/modules/SafeEmailRecoveryModule.sol @@ -13,6 +13,7 @@ contract SafeEmailRecoveryModule is EmailRecoveryManager { event RecoveryExecuted(address indexed account); + error InvalidAccount(address account); error InvalidSelector(bytes4 selector); error RecoveryFailed(address account); @@ -37,20 +38,23 @@ contract SafeEmailRecoveryModule is EmailRecoveryManager { } /** - * @notice Executes recovery on a Safe account. Must be called by the trusted recovery manager + * @notice Executes recovery on a Safe account. Called from the recovery manager * @param account The account to execute recovery for - * @param recoveryData The recovery calldata that should be executed on the Safe - * being recovered + * @param recoveryData The recovery data that should be executed on the Safe + * being recovered. recoveryData = abi.encode(safeAccount, recoveryFunctionCalldata) */ function recover(address account, bytes calldata recoveryData) internal override { - (, bytes memory recoveryCalldata) = abi.decode(recoveryData, (address, bytes)); - // FIXME: What if you use this module with a different subject handler? It could chose - // not to encode the account/validator along with the calldata + (address encodedAccount, bytes memory recoveryCalldata) = + abi.decode(recoveryData, (address, bytes)); + + if (encodedAccount == address(0) || encodedAccount != account) { + revert InvalidAccount(encodedAccount); + } + bytes4 calldataSelector; assembly { calldataSelector := mload(add(recoveryCalldata, 32)) } - if (calldataSelector != selector) { revert InvalidSelector(calldataSelector); } diff --git a/test/integration/OwnableValidatorRecovery/EmailRecoveryModule/EmailRecoveryModule.t.sol b/test/integration/OwnableValidatorRecovery/EmailRecoveryModule/EmailRecoveryModule.t.sol index 16705d1e..c63804bb 100644 --- a/test/integration/OwnableValidatorRecovery/EmailRecoveryModule/EmailRecoveryModule.t.sol +++ b/test/integration/OwnableValidatorRecovery/EmailRecoveryModule/EmailRecoveryModule.t.sol @@ -67,6 +67,7 @@ contract OwnableValidatorRecovery_EmailRecoveryModule_Integration_Test is assertEq(recoveryRequest.executeAfter, 0); assertEq(recoveryRequest.executeBefore, 0); assertEq(recoveryRequest.currentWeight, 1); + assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash1); // handle recovery request for guardian 2 uint256 executeAfter = block.timestamp + delay; @@ -76,6 +77,7 @@ contract OwnableValidatorRecovery_EmailRecoveryModule_Integration_Test is assertEq(recoveryRequest.executeAfter, executeAfter); assertEq(recoveryRequest.executeBefore, executeBefore); assertEq(recoveryRequest.currentWeight, 3); + assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash1); // Time travel so that the recovery delay has passed vm.warp(block.timestamp + delay); @@ -89,6 +91,7 @@ contract OwnableValidatorRecovery_EmailRecoveryModule_Integration_Test is assertEq(recoveryRequest.executeAfter, 0); assertEq(recoveryRequest.executeBefore, 0); assertEq(recoveryRequest.currentWeight, 0); + assertEq(recoveryRequest.recoveryDataHash, bytes32(0)); assertEq(updatedOwner, newOwner1); } diff --git a/test/integration/OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModule.t.sol b/test/integration/OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModule.t.sol index f5eb74cd..6fa94626 100644 --- a/test/integration/OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModule.t.sol +++ b/test/integration/OwnableValidatorRecovery/UniversalEmailRecoveryModule/UniversalEmailRecoveryModule.t.sol @@ -66,6 +66,7 @@ contract OwnableValidatorRecovery_UniversalEmailRecoveryModule_Integration_Test assertEq(recoveryRequest.executeAfter, 0); assertEq(recoveryRequest.executeBefore, 0); assertEq(recoveryRequest.currentWeight, 1); + assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash1); // handle recovery request for guardian 2 uint256 executeAfter = block.timestamp + delay; @@ -75,6 +76,7 @@ contract OwnableValidatorRecovery_UniversalEmailRecoveryModule_Integration_Test assertEq(recoveryRequest.executeAfter, executeAfter); assertEq(recoveryRequest.executeBefore, executeBefore); assertEq(recoveryRequest.currentWeight, 3); + assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash1); // Time travel so that the recovery delay has passed vm.warp(block.timestamp + delay); @@ -88,6 +90,7 @@ contract OwnableValidatorRecovery_UniversalEmailRecoveryModule_Integration_Test assertEq(recoveryRequest.executeAfter, 0); assertEq(recoveryRequest.executeBefore, 0); assertEq(recoveryRequest.currentWeight, 0); + assertEq(recoveryRequest.recoveryDataHash, bytes32(0)); assertEq(updatedOwner, newOwner1); } diff --git a/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol b/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol index df30286f..6eae5a2d 100644 --- a/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol +++ b/test/integration/SafeRecovery/SafeRecoveryNativeModule.t.sol @@ -32,7 +32,7 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase "swapOwner(address,address,address)", address(1), owner, newOwner ); bytes memory recoveryData = abi.encode(safeAddress, recoveryCalldata); - bytes32 calldataHash = keccak256(recoveryData); + bytes32 recoveryDataHash = keccak256(recoveryData); bytes[] memory subjectParamsForRecovery = new bytes[](4); subjectParamsForRecovery[0] = abi.encode(safeAddress); @@ -64,7 +64,10 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase emailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx); IEmailRecoveryManager.RecoveryRequest memory recoveryRequest = emailRecoveryModule.getRecoveryRequest(safeAddress); + assertEq(recoveryRequest.executeAfter, 0); + assertEq(recoveryRequest.executeBefore, 0); assertEq(recoveryRequest.currentWeight, 1); + assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); // handle recovery request for guardian 2 uint256 executeAfter = block.timestamp + delay; @@ -75,6 +78,7 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase assertEq(recoveryRequest.executeAfter, executeAfter); assertEq(recoveryRequest.executeBefore, executeBefore); assertEq(recoveryRequest.currentWeight, 3); + assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash); vm.warp(block.timestamp + delay); @@ -85,6 +89,7 @@ contract SafeRecoveryNativeModule_Integration_Test is SafeNativeIntegrationBase assertEq(recoveryRequest.executeAfter, 0); assertEq(recoveryRequest.executeBefore, 0); assertEq(recoveryRequest.currentWeight, 0); + assertEq(recoveryRequest.recoveryDataHash, bytes32(0)); vm.prank(safeAddress); bool isOwner = Safe(payable(safeAddress)).isOwner(newOwner);