From 730abb17ceb433818064c49b5aab32a9bc00a46e Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Thu, 1 Aug 2024 23:34:31 +0100 Subject: [PATCH] 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, "");