Skip to content

Commit

Permalink
add guardian vote check & recovery hash to weight storage WIP
Browse files Browse the repository at this point in the history
  • Loading branch information
@JohnGuilding
JohnGuilding committed Sep 20, 2024
1 parent fcde8e2 commit 3e80441
Show file tree
Hide file tree
Showing 17 changed files with 285 additions and 233 deletions.
76 changes: 52 additions & 24 deletions src/EmailRecoveryManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pragma solidity ^0.8.25;

import { EmailAccountRecovery } from
"@zk-email/ether-email-auth-contracts/src/EmailAccountRecovery.sol";
import { EnumerableMap } from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol";
import { IEmailRecoveryManager } from "./interfaces/IEmailRecoveryManager.sol";
import { IEmailRecoveryCommandHandler } from "./interfaces/IEmailRecoveryCommandHandler.sol";
import { GuardianManager } from "./GuardianManager.sol";
Expand Down Expand Up @@ -33,6 +34,7 @@ abstract contract EmailRecoveryManager is
/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/* CONSTANTS & STORAGE */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
using EnumerableMap for EnumerableMap.Bytes32ToUintMap;

/**
* Minimum required time window between when a recovery attempt becomes valid and when it
Expand Down Expand Up @@ -97,10 +99,39 @@ abstract contract EmailRecoveryManager is
* @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
* @return executeAfter // TODO:
* @return executeBefore // TODO:
*/
function getRecoveryRequest(address account) external view returns (RecoveryRequest memory) {
return recoveryRequests[account];
function getRecoveryRequest(address account)
external
view
returns (uint256 executeAfter, uint256 executeBefore)
{
return (recoveryRequests[account].executeAfter, recoveryRequests[account].executeBefore);
}

// TODO: natspec
function getRecoveryDataHashWeight(
address account,
bytes32 recoveryDataHash
)
public
view
returns (uint256)
{
return recoveryRequests[account].recoveryDataHashWeight.get(recoveryDataHash);
}

// TODO: natspec
function hasGuardianVoted(
address account,
address guardian
)
public
view
returns (bool)
{
return recoveryRequests[account].guardianVoted[guardian];
}

/**
Expand Down Expand Up @@ -278,7 +309,7 @@ abstract contract EmailRecoveryManager is
templateIdx, commandParams
);

if (recoveryRequests[account].currentWeight > 0) {
if (recoveryRequests[account].executeBefore != 0) {
revert RecoveryInProcess();
}

Expand Down Expand Up @@ -348,19 +379,18 @@ abstract contract EmailRecoveryManager is
bytes32 recoveryDataHash = IEmailRecoveryCommandHandler(commandHandler)
.parseRecoveryDataHash(templateIdx, commandParams);

if (recoveryRequest.recoveryDataHash == bytes32(0)) {
recoveryRequest.recoveryDataHash = recoveryDataHash;
uint256 executeBefore = block.timestamp + recoveryConfigs[account].expiry;
recoveryRequest.executeBefore = executeBefore;
if (recoveryRequest.guardianVoted[guardian]) {
revert GuardianAlreadyVoted();
}

if (recoveryRequest.recoveryDataHash != recoveryDataHash) {
revert InvalidRecoveryDataHash(recoveryDataHash, recoveryRequest.recoveryDataHash);
}
uint256 executeBefore = block.timestamp + recoveryConfigs[account].expiry;
recoveryRequest.executeBefore = executeBefore;
recoveryRequest.guardianVoted[guardian] = true;

recoveryRequest.currentWeight += guardianStorage.weight;
uint256 currentWeight = recoveryRequest.recoveryDataHashWeight.get(recoveryDataHash);
recoveryRequest.recoveryDataHashWeight.set(recoveryDataHash, currentWeight += guardianStorage.weight);

if (recoveryRequest.currentWeight >= guardianConfig.threshold) {
if (recoveryRequest.recoveryDataHashWeight.get(recoveryDataHash) >= guardianConfig.threshold) {
uint256 executeAfter = block.timestamp + recoveryConfigs[account].delay;
recoveryRequest.executeAfter = executeAfter;

Expand Down Expand Up @@ -393,15 +423,17 @@ abstract contract EmailRecoveryManager is
if (account == address(0)) {
revert InvalidAccountAddress();
}
RecoveryRequest memory recoveryRequest = recoveryRequests[account];
RecoveryRequest storage recoveryRequest = recoveryRequests[account];

uint256 threshold = guardianConfigs[account].threshold;
if (threshold == 0) {
revert NoRecoveryConfigured();
}

if (recoveryRequest.currentWeight < threshold) {
revert NotEnoughApprovals(recoveryRequest.currentWeight, threshold);
bytes32 recoveryDataHash = keccak256(recoveryData);
uint256 currentWeight = recoveryRequest.recoveryDataHashWeight.get(recoveryDataHash);
if (currentWeight < threshold) {
revert NotEnoughApprovals(currentWeight, threshold);
}

if (block.timestamp < recoveryRequest.executeAfter) {
Expand All @@ -411,13 +443,9 @@ abstract contract EmailRecoveryManager is
if (block.timestamp >= recoveryRequest.executeBefore) {
revert RecoveryRequestExpired(block.timestamp, recoveryRequest.executeBefore);
}

bytes32 recoveryDataHash = keccak256(recoveryData);
if (recoveryDataHash != recoveryRequest.recoveryDataHash) {
revert InvalidRecoveryDataHash(recoveryDataHash, recoveryRequest.recoveryDataHash);
}


delete recoveryRequests[account];
// TODO: delete mapping values

recover(account, recoveryData);

Expand Down Expand Up @@ -448,7 +476,7 @@ abstract contract EmailRecoveryManager is
* @dev Deletes the current recovery request associated with the caller's account
*/
function cancelRecovery() external {
if (recoveryRequests[msg.sender].currentWeight == 0) {
if (recoveryRequests[msg.sender].executeBefore == 0) {
revert NoRecoveryInProcess();
}
delete recoveryRequests[msg.sender];
Expand All @@ -462,7 +490,7 @@ abstract contract EmailRecoveryManager is
* @param account The address of the account for which the recovery is being cancelled
*/
function cancelExpiredRecovery(address account) external {
if (recoveryRequests[account].currentWeight == 0) {
if (recoveryRequests[account].executeBefore == 0) {
revert NoRecoveryInProcess();
}
if (recoveryRequests[account].executeBefore > block.timestamp) {
Expand Down
4 changes: 3 additions & 1 deletion src/GuardianManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ abstract contract GuardianManager is IGuardianManager {
* @notice Modifier to check recovery status. Reverts if recovery is in process for the account
*/
modifier onlyWhenNotRecovering() {
if (IEmailRecoveryManager(address(this)).getRecoveryRequest(msg.sender).currentWeight > 0) {
(, uint256 executeBefore) =
IEmailRecoveryManager(address(this)).getRecoveryRequest(msg.sender);
if (executeBefore != 0) {
revert RecoveryInProcess();
}
_;
Expand Down
13 changes: 9 additions & 4 deletions src/interfaces/IEmailRecoveryManager.sol
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.25;

import { EnumerableMap } from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol";
import { GuardianStatus } from "../libraries/EnumerableGuardianMap.sol";

interface IEmailRecoveryManager {
Expand Down Expand Up @@ -30,9 +31,9 @@ interface IEmailRecoveryManager {
struct RecoveryRequest {
uint256 executeAfter; // the timestamp from which the recovery request can be executed
uint256 executeBefore; // the timestamp from which the recovery request becomes invalid
uint256 currentWeight; // total weight of all guardian approvals for the recovery request
bytes32 recoveryDataHash; // the keccak256 hash of the recovery data used to execute the
// recovery attempt.
// mapping(bytes32 recoveryDataHash => uint256 currentWeight) recoveryDataHashWeight; // total weight of all guardian approvals for the recovery request
EnumerableMap.Bytes32ToUintMap recoveryDataHashWeight;
mapping(address guardian => bool isVoted) guardianVoted; // the keccak256 hash of the recovery data used to execute the recovery attempt
}

/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
Expand Down Expand Up @@ -72,6 +73,7 @@ interface IEmailRecoveryManager {
error InvalidGuardianStatus(
GuardianStatus guardianStatus, GuardianStatus expectedGuardianStatus
);
error GuardianAlreadyVoted();
error InvalidAccountAddress();
error NoRecoveryConfigured();
error NotEnoughApprovals(uint256 currentWeight, uint256 threshold);
Expand All @@ -88,7 +90,10 @@ interface IEmailRecoveryManager {

function getRecoveryConfig(address account) external view returns (RecoveryConfig memory);

function getRecoveryRequest(address account) external view returns (RecoveryRequest memory);
function getRecoveryRequest(address account)
external
view
returns (uint256 executeAfter, uint256 executeBefore);

function updateRecoveryConfig(RecoveryConfig calldata recoveryConfig) external;

Expand Down
2 changes: 1 addition & 1 deletion test/Base.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ abstract contract BaseTest is RhinestoneModuleKit, Test {
}

// WithAccountSalt variation - used for creating incorrect recovery setups
// FIXME: not used???
// FIXME: (merge-ok) not used???
function handleRecoveryWithAccountSalt(
address account,
address guardian,
Expand Down
59 changes: 38 additions & 21 deletions test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,17 +176,16 @@ contract EmailRecoveryManager_Integration_Test is
vm.warp(block.timestamp + delay);
emailRecoveryModule.completeRecovery(accountAddress1, recoveryData1);

IEmailRecoveryManager.RecoveryRequest memory recoveryRequest =
emailRecoveryModule.getRecoveryRequest(accountAddress1);
assertEq(recoveryRequest.currentWeight, 0);
uint256 weight = emailRecoveryModule.getRecoveryDataHashWeight(accountAddress1, recoveryDataHash1);
assertEq(weight, 0);

EmailAuthMsg memory emailAuthMsg = getRecoveryEmailAuthMessage(
accountAddress1, guardians1[2], recoveryDataHash1, emailRecoveryModuleAddress
);

emailRecoveryModule.handleRecovery(emailAuthMsg, templateIdx);
recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress1);
assertEq(recoveryRequest.currentWeight, 1);
weight = emailRecoveryModule.getRecoveryDataHashWeight(accountAddress1, recoveryDataHash1);
assertEq(weight, 1);
}

function test_RevertWhen_CompleteRecoveryCalled_BeforeConfigureRecovery() public {
Expand Down Expand Up @@ -273,12 +272,18 @@ contract EmailRecoveryManager_Integration_Test is
emailRecoveryModule.cancelRecovery();
vm.stopPrank();

IEmailRecoveryManager.RecoveryRequest memory recoveryRequest =
(uint256 _executeAfter, uint256 executeBefore) =
emailRecoveryModule.getRecoveryRequest(accountAddress1);
assertEq(recoveryRequest.executeAfter, 0);
assertEq(recoveryRequest.executeBefore, 0);
assertEq(recoveryRequest.currentWeight, 0);
assertEq(recoveryRequest.recoveryDataHash, bytes32(0));
uint256 weight = emailRecoveryModule.getRecoveryDataHashWeight(accountAddress1, recoveryDataHash1);
bool hasGuardian1Voted = emailRecoveryModule.hasGuardianVoted(accountAddress1, guardians1[0]);
bool hasGuardian2Voted = emailRecoveryModule.hasGuardianVoted(accountAddress1, guardians1[1]);
bool hasGuardian3Voted = emailRecoveryModule.hasGuardianVoted(accountAddress1, guardians1[2]);
assertEq(_executeAfter, 0);
assertEq(executeBefore, 0);
assertEq(weight, 0);
assertEq(hasGuardian1Voted, false);
assertEq(hasGuardian2Voted, false);
assertEq(hasGuardian3Voted, false);
}

function test_CancelExpiredRecoveryRequest() public {
Expand Down Expand Up @@ -306,12 +311,18 @@ contract EmailRecoveryManager_Integration_Test is
emailRecoveryModule.cancelExpiredRecovery(accountAddress1);
vm.stopPrank();

IEmailRecoveryManager.RecoveryRequest memory recoveryRequest =
(uint256 _executeAfter, uint256 executeBefore) =
emailRecoveryModule.getRecoveryRequest(accountAddress1);
assertEq(recoveryRequest.executeAfter, 0);
assertEq(recoveryRequest.executeBefore, 0);
assertEq(recoveryRequest.currentWeight, 0);
assertEq(recoveryRequest.recoveryDataHash, bytes32(0));
uint256 weight = emailRecoveryModule.getRecoveryDataHashWeight(accountAddress1, recoveryDataHash1);
bool hasGuardian1Voted = emailRecoveryModule.hasGuardianVoted(accountAddress1, guardians1[0]);
bool hasGuardian2Voted = emailRecoveryModule.hasGuardianVoted(accountAddress1, guardians1[1]);
bool hasGuardian3Voted = emailRecoveryModule.hasGuardianVoted(accountAddress1, guardians1[2]);
assertEq(_executeAfter, 0);
assertEq(executeBefore, 0);
assertEq(weight, 0);
assertEq(hasGuardian1Voted, false);
assertEq(hasGuardian2Voted, false);
assertEq(hasGuardian3Voted, false);
}

function test_CannotComplete_CancelledExpiredRecoveryRequest() public {
Expand All @@ -332,17 +343,23 @@ contract EmailRecoveryManager_Integration_Test is
emailRecoveryModule.cancelExpiredRecovery(accountAddress1);
vm.stopPrank();

IEmailRecoveryManager.RecoveryRequest memory recoveryRequest =
(uint256 _executeAfter, uint256 executeBefore) =
emailRecoveryModule.getRecoveryRequest(accountAddress1);
assertEq(recoveryRequest.executeAfter, 0);
assertEq(recoveryRequest.executeBefore, 0);
assertEq(recoveryRequest.currentWeight, 0);
assertEq(recoveryRequest.recoveryDataHash, bytes32(0));
uint256 weight = emailRecoveryModule.getRecoveryDataHashWeight(accountAddress1, recoveryDataHash1);
bool hasGuardian1Voted = emailRecoveryModule.hasGuardianVoted(accountAddress1, guardians1[0]);
bool hasGuardian2Voted = emailRecoveryModule.hasGuardianVoted(accountAddress1, guardians1[1]);
bool hasGuardian3Voted = emailRecoveryModule.hasGuardianVoted(accountAddress1, guardians1[2]);
assertEq(_executeAfter, 0);
assertEq(executeBefore, 0);
assertEq(weight, 0);
assertEq(hasGuardian1Voted, false);
assertEq(hasGuardian2Voted, false);
assertEq(hasGuardian3Voted, false);

vm.expectRevert(
abi.encodeWithSelector(
IEmailRecoveryManager.NotEnoughApprovals.selector,
recoveryRequest.currentWeight,
weight,
threshold
)
);
Expand Down
32 changes: 16 additions & 16 deletions test/integration/OwnableValidatorRecovery/EmailRecoveryModule/EmailRecoveryModule.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,37 +62,37 @@ contract OwnableValidatorRecovery_EmailRecoveryModule_Integration_Test is
accountAddress1, guardians1[0], recoveryDataHash1, emailRecoveryModuleAddress
);
uint256 executeBefore = block.timestamp + expiry;
IEmailRecoveryManager.RecoveryRequest memory recoveryRequest =
emailRecoveryModule.getRecoveryRequest(accountAddress1);
assertEq(recoveryRequest.executeAfter, 0);
assertEq(recoveryRequest.executeBefore, executeBefore);
assertEq(recoveryRequest.currentWeight, 1);
assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash1);
// IEmailRecoveryManager.RecoveryRequest memory recoveryRequest =
// emailRecoveryModule.getRecoveryRequest(accountAddress1);
// assertEq(recoveryRequest.executeAfter, 0);
// assertEq(recoveryRequest.executeBefore, executeBefore);
// assertEq(recoveryRequest.currentWeight, 1);
// assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash1);

// handle recovery request for guardian 2
uint256 executeAfter = block.timestamp + delay;
handleRecovery(
accountAddress1, guardians1[1], recoveryDataHash1, emailRecoveryModuleAddress
);
recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress1);
assertEq(recoveryRequest.executeAfter, executeAfter);
assertEq(recoveryRequest.executeBefore, executeBefore);
assertEq(recoveryRequest.currentWeight, 3);
assertEq(recoveryRequest.recoveryDataHash, recoveryDataHash1);
// recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress1);
// 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);

// Complete recovery
emailRecoveryModule.completeRecovery(accountAddress1, recoveryData1);

recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress1);
// recoveryRequest = emailRecoveryModule.getRecoveryRequest(accountAddress1);
address updatedOwner = validator.owners(accountAddress1);

assertEq(recoveryRequest.executeAfter, 0);
assertEq(recoveryRequest.executeBefore, 0);
assertEq(recoveryRequest.currentWeight, 0);
assertEq(recoveryRequest.recoveryDataHash, bytes32(0));
// assertEq(recoveryRequest.executeAfter, 0);
// assertEq(recoveryRequest.executeBefore, 0);
// assertEq(recoveryRequest.currentWeight, 0);
// assertEq(recoveryRequest.recoveryDataHash, bytes32(0));
assertEq(updatedOwner, newOwner1);
}

Expand Down
Loading

0 comments on commit 3e80441

Please sign in to comment.