Skip to content

Commit

Permalink
Merge pull request #25 from zkemail/feat/add-custom-error-arguments
Browse files Browse the repository at this point in the history 
Feat/add custom error arguments
  • Loading branch information
@JohnGuilding
JohnGuilding authored Aug 1, 2024
2 parents 482d295 + a388fff commit f2ab698
Show file tree
Hide file tree
Showing 22 changed files with 313 additions and 116 deletions.
19 changes: 11 additions & 8 deletions src/EmailRecoveryManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,10 +279,11 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
revert AccountNotConfigured();
}
if (recoveryConfig.delay > recoveryConfig.expiry) {
revert DelayMoreThanExpiry();
revert DelayMoreThanExpiry(recoveryConfig.delay, recoveryConfig.expiry);
}
if (recoveryConfig.expiry - recoveryConfig.delay < MINIMUM_RECOVERY_WINDOW) {
revert RecoveryWindowTooShort();
uint256 recoveryWindow = recoveryConfig.expiry - recoveryConfig.delay;
if (recoveryWindow < MINIMUM_RECOVERY_WINDOW) {
revert RecoveryWindowTooShort(recoveryWindow);
}

recoveryConfigs[account] = recoveryConfig;
Expand Down Expand Up @@ -374,7 +375,9 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco

GuardianConfig memory guardianConfig = guardianConfigs[account];
if (guardianConfig.threshold > guardianConfig.acceptedWeight) {
revert ThresholdExceedsAcceptedWeight();
revert ThresholdExceedsAcceptedWeight(
guardianConfig.threshold, guardianConfig.acceptedWeight
);
}

// This check ensures GuardianStatus is correct and also implicitly that the
Expand Down Expand Up @@ -431,20 +434,20 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
}

if (recoveryRequest.currentWeight < threshold) {
revert NotEnoughApprovals();
revert NotEnoughApprovals(recoveryRequest.currentWeight, threshold);
}

if (block.timestamp < recoveryRequest.executeAfter) {
revert DelayNotPassed();
revert DelayNotPassed(block.timestamp, recoveryRequest.executeAfter);
}

if (block.timestamp >= recoveryRequest.executeBefore) {
revert RecoveryRequestExpired();
revert RecoveryRequestExpired(block.timestamp, recoveryRequest.executeBefore);
}

bytes32 calldataHash = keccak256(recoveryCalldata);
if (calldataHash != recoveryRequest.calldataHash) {
revert InvalidCalldataHash();
revert InvalidCalldataHash(calldataHash, recoveryRequest.calldataHash);
}

delete recoveryRequests[account];
Expand Down
18 changes: 9 additions & 9 deletions src/handlers/EmailRecoverySubjectHandler.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ import { StringUtils } from "../libraries/StringUtils.sol";
* This is the default subject handler that will work with any validator.
*/
contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler {
error InvalidTemplateIndex();
error InvalidSubjectParams();
error InvalidTemplateIndex(uint256 templateIdx, uint256 expectedTemplateIdx);
error InvalidSubjectParams(uint256 paramsLength, uint256 expectedParamsLength);
error InvalidAccount();
error InvalidRecoveryModule();
error InvalidRecoveryModule(address recoveryModule);

/**
* @notice Returns a hard-coded two-dimensional array of strings representing the subject
Expand Down Expand Up @@ -102,10 +102,10 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler {
returns (address)
{
if (templateIdx != 0) {
revert InvalidTemplateIndex();
revert InvalidTemplateIndex(templateIdx, 0);
}
if (subjectParams.length != 1) {
revert InvalidSubjectParams();
revert InvalidSubjectParams(subjectParams.length, 1);
}

// The GuardianStatus check in acceptGuardian implicitly
Expand All @@ -132,10 +132,10 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler {
returns (address)
{
if (templateIdx != 0) {
revert InvalidTemplateIndex();
revert InvalidTemplateIndex(templateIdx, 0);
}
if (subjectParams.length != 3) {
revert InvalidSubjectParams();
revert InvalidSubjectParams(subjectParams.length, 3);
}

address accountInEmail = abi.decode(subjectParams[0], (address));
Expand All @@ -155,7 +155,7 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler {
address expectedRecoveryModule = EmailRecoveryManager(recoveryManager).emailRecoveryModule();
if (recoveryModuleInEmail == address(0) || recoveryModuleInEmail != expectedRecoveryModule)
{
revert InvalidRecoveryModule();
revert InvalidRecoveryModule(recoveryModuleInEmail);
}

return accountInEmail;
Expand All @@ -177,7 +177,7 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler {
returns (bytes32)
{
if (templateIdx != 0) {
revert InvalidTemplateIndex();
revert InvalidTemplateIndex(templateIdx, 0);
}

string memory calldataHashInEmail = abi.decode(subjectParams[2], (string));
Expand Down
26 changes: 13 additions & 13 deletions src/handlers/SafeRecoverySubjectHandler.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ import { EmailRecoveryManager } from "../EmailRecoveryManager.sol";
* This is a custom subject handler that will work with Safes and defines custom validation.
*/
contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler {
error InvalidSubjectParams();
error InvalidTemplateIndex();
error InvalidOldOwner();
error InvalidNewOwner();
error InvalidRecoveryModule();
error InvalidTemplateIndex(uint256 templateIdx, uint256 expectedTemplateIdx);
error InvalidSubjectParams(uint256 paramsLength, uint256 expectedParamsLength);
error InvalidOldOwner(address oldOwner);
error InvalidNewOwner(address newOwner);
error InvalidRecoveryModule(address recoveryModule);

/**
* @notice Returns a hard-coded two-dimensional array of strings representing the subject
Expand Down Expand Up @@ -107,10 +107,10 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler {
returns (address)
{
if (templateIdx != 0) {
revert InvalidTemplateIndex();
revert InvalidTemplateIndex(templateIdx, 0);
}
if (subjectParams.length != 1) {
revert InvalidSubjectParams();
revert InvalidSubjectParams(subjectParams.length, 1);
}

// The GuardianStatus check in acceptGuardian implicitly
Expand All @@ -137,10 +137,10 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler {
returns (address)
{
if (templateIdx != 0) {
revert InvalidTemplateIndex();
revert InvalidTemplateIndex(templateIdx, 0);
}
if (subjectParams.length != 4) {
revert InvalidSubjectParams();
revert InvalidSubjectParams(subjectParams.length, 4);
}

address accountInEmail = abi.decode(subjectParams[0], (address));
Expand All @@ -150,12 +150,12 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler {

bool isOldAddressOwner = ISafe(accountInEmail).isOwner(oldOwnerInEmail);
if (!isOldAddressOwner) {
revert InvalidOldOwner();
revert InvalidOldOwner(oldOwnerInEmail);
}

bool isNewAddressOwner = ISafe(accountInEmail).isOwner(newOwnerInEmail);
if (newOwnerInEmail == address(0) || isNewAddressOwner) {
revert InvalidNewOwner();
revert InvalidNewOwner(newOwnerInEmail);
}

// Even though someone could use a malicious contract as the recoveryManager argument, it
Expand All @@ -165,7 +165,7 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler {
address expectedRecoveryModule = EmailRecoveryManager(recoveryManager).emailRecoveryModule();
if (recoveryModuleInEmail == address(0) || recoveryModuleInEmail != expectedRecoveryModule)
{
revert InvalidRecoveryModule();
revert InvalidRecoveryModule(recoveryModuleInEmail);
}

return accountInEmail;
Expand All @@ -187,7 +187,7 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler {
returns (bytes32)
{
if (templateIdx != 0) {
revert InvalidTemplateIndex();
revert InvalidTemplateIndex(templateIdx, 0);
}

address accountInEmail = abi.decode(subjectParams[0], (address));
Expand Down
16 changes: 8 additions & 8 deletions src/interfaces/IEmailRecoveryManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,28 +74,28 @@ interface IEmailRecoveryManager {
ERRORS
//////////////////////////////////////////////////////////////////////////*/

error RecoveryInProcess();
error InvalidVerifier();
error InvalidDkimRegistry();
error InvalidEmailAuthImpl();
error InvalidSubjectHandler();
error InitializerNotDeployer();
error InvalidRecoveryModule();
error RecoveryInProcess();
error SetupAlreadyCalled();
error AccountNotConfigured();
error RecoveryModuleNotAuthorized();
error DelayMoreThanExpiry();
error RecoveryWindowTooShort();
error ThresholdExceedsAcceptedWeight();
error DelayMoreThanExpiry(uint256 delay, uint256 expiry);
error RecoveryWindowTooShort(uint256 recoveryWindow);
error ThresholdExceedsAcceptedWeight(uint256 threshold, uint256 acceptedWeight);
error InvalidGuardianStatus(
GuardianStatus guardianStatus, GuardianStatus expectedGuardianStatus
);
error InvalidAccountAddress();
error NoRecoveryConfigured();
error NotEnoughApprovals();
error DelayNotPassed();
error RecoveryRequestExpired();
error InvalidCalldataHash();
error NotEnoughApprovals(uint256 currentWeight, uint256 threshold);
error DelayNotPassed(uint256 blockTimestamp, uint256 executeAfter);
error RecoveryRequestExpired(uint256 blockTimestamp, uint256 executeBefore);
error InvalidCalldataHash(bytes32 calldataHash, bytes32 expectedCalldataHash);
error NoRecoveryInProcess();
error NotRecoveryModule();
error SetupNotCalled();
Expand Down
23 changes: 12 additions & 11 deletions src/libraries/GuardianUtils.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ library GuardianUtils {
event RemovedGuardian(address indexed account, address indexed guardian, uint256 weight);
event ChangedThreshold(address indexed account, uint256 threshold);

error IncorrectNumberOfWeights();
error IncorrectNumberOfWeights(uint256 guardianCount, uint256 weightCount);
error ThresholdCannotBeZero();
error InvalidGuardianAddress();
error InvalidGuardianAddress(address guardian);
error InvalidGuardianWeight();
error AddressAlreadyGuardian();
error ThresholdExceedsTotalWeight();
error StatusCannotBeTheSame();
error ThresholdExceedsTotalWeight(uint256 threshold, uint256 totalWeight);
error StatusCannotBeTheSame(GuardianStatus newStatus);
error SetupNotCalled();
error AddressNotGuardianForAccount();

Expand Down Expand Up @@ -69,7 +69,7 @@ library GuardianUtils {
uint256 guardianCount = guardians.length;

if (guardianCount != weights.length) {
revert IncorrectNumberOfWeights();
revert IncorrectNumberOfWeights(guardianCount, weights.length);
}

if (threshold == 0) {
Expand All @@ -82,7 +82,7 @@ library GuardianUtils {

uint256 totalWeight = guardianConfigs[account].totalWeight;
if (threshold > totalWeight) {
revert ThresholdExceedsTotalWeight();
revert ThresholdExceedsTotalWeight(threshold, totalWeight);
}

guardianConfigs[account].threshold = threshold;
Expand All @@ -106,7 +106,7 @@ library GuardianUtils {
{
GuardianStorage memory guardianStorage = guardiansStorage[account].get(guardian);
if (newStatus == guardianStorage.status) {
revert StatusCannotBeTheSame();
revert StatusCannotBeTheSame(newStatus);
}

guardiansStorage[account].set({
Expand Down Expand Up @@ -134,7 +134,7 @@ library GuardianUtils {
internal
{
if (guardian == address(0) || guardian == account) {
revert InvalidGuardianAddress();
revert InvalidGuardianAddress(guardian);
}

if (weight == 0) {
Expand Down Expand Up @@ -180,8 +180,9 @@ library GuardianUtils {
}

// Only allow guardian removal if threshold can still be reached.
if (guardianConfig.totalWeight - guardianStorage.weight < guardianConfig.threshold) {
revert ThresholdExceedsTotalWeight();
uint256 newTotalWeight = guardianConfig.totalWeight - guardianStorage.weight;
if (newTotalWeight < guardianConfig.threshold) {
revert ThresholdExceedsTotalWeight(newTotalWeight, guardianConfig.threshold);
}

guardianConfigs[account].guardianCount--;
Expand Down Expand Up @@ -228,7 +229,7 @@ library GuardianUtils {

// Validate that threshold is smaller than the total weight.
if (threshold > guardianConfigs[account].totalWeight) {
revert ThresholdExceedsTotalWeight();
revert ThresholdExceedsTotalWeight(threshold, guardianConfigs[account].totalWeight);
}

// Guardian weight should be at least 1
Expand Down
15 changes: 12 additions & 3 deletions test/integration/EmailRecoveryManager/EmailRecoveryManager.integration.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,14 +170,18 @@ contract EmailRecoveryManager_Integration_Test is
}

function test_RevertWhen_CompleteRecoveryCalled_BeforeHandleAcceptance() public {
vm.expectRevert(IEmailRecoveryManager.NotEnoughApprovals.selector);
vm.expectRevert(
abi.encodeWithSelector(IEmailRecoveryManager.NotEnoughApprovals.selector, 0, threshold)
);
emailRecoveryManager.completeRecovery(accountAddress1, recoveryCalldata1);
}

function test_RevertWhen_CompleteRecoveryCalled_BeforeProcessRecovery() public {
acceptGuardian(accountAddress1, guardians1[0]);

vm.expectRevert(IEmailRecoveryManager.NotEnoughApprovals.selector);
vm.expectRevert(
abi.encodeWithSelector(IEmailRecoveryManager.NotEnoughApprovals.selector, 0, threshold)
);
emailRecoveryManager.completeRecovery(accountAddress1, recoveryCalldata1);
}

Expand Down Expand Up @@ -241,10 +245,15 @@ contract EmailRecoveryManager_Integration_Test is
vm.warp(12 seconds);
handleRecovery(accountAddress1, guardians1[0], calldataHash1);
handleRecovery(accountAddress1, guardians1[1], calldataHash1);
uint256 executeAfter = block.timestamp + expiry;

vm.warp(10 weeks);

vm.expectRevert(IEmailRecoveryManager.RecoveryRequestExpired.selector);
vm.expectRevert(
abi.encodeWithSelector(
IEmailRecoveryManager.RecoveryRequestExpired.selector, block.timestamp, executeAfter
)
);
emailRecoveryManager.completeRecovery(accountAddress1, recoveryCalldata1);

// Can cancel recovery even when stale
Expand Down
Loading

0 comments on commit f2ab698

Please sign in to comment.