Skip to content

Commit

Permalink
Merge pull request #22 from zkemail/ackee-audit-fixes
Browse files Browse the repository at this point in the history 
Ackee audit fixes
  • Loading branch information
@JohnGuilding
JohnGuilding authored Aug 1, 2024
2 parents 3d82e2d + 5b26a9a commit 482d295
Show file tree
Hide file tree
Showing 70 changed files with 1,338 additions and 429 deletions.
4 changes: 4 additions & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,8 @@ The `recover()` function on the module is the key entry point where recovery is

When writing a custom subject handler, an account developer would likely chose to deploy a `EmailRecoveryModule` instance rather than a `UniversalEmailRecoveryModule` instance. This is because a custom subject handler would likely be specific to an validator implementation, so using the recovery module for specific validators is more appropriate than the generic recovery module.

**Note:** This module is an executor and does not abide by the 4337 validation rules. The `onInstall` function breaks the validation rules and it is possible for it to be called during account deployment in the first userOp. So you cannot install this module during account deployment as onInstall will be called as part of the validation phase. Supporting executor initialization during account deployment is not mandated by ERC7579 - if required, install this module after the account has been setup.

### UniversalEmailRecoveryModule.sol
A recovery module that recovers any validator.

Expand All @@ -198,6 +200,8 @@ The `recover()` function on the module is the key entry point where recovery is

`completeRecovery()` calls into the account specific recovery module and can call executeFromExecutor to execute the account specific recovery logic. The call from the executor retains the context of the account so the `msg.sender` of the next call is the account itself. This simplifies access control in the validator being recovered as it can just do a `msg.sender` check.

**Note:** This module is an executor and does not abide by the 4337 validation rules. The `onInstall` function breaks the validation rules and it is possible for it to be called during account deployment in the first userOp. So you cannot install this module during account deployment as `onInstall` will be called as part of the validation phase. Supporting executor initialization during account deployment is not mandated by ERC7579 - if required, install this module after the account has been setup.

### EmailRecoveryFactory.sol
The factory for deploying new instances of `EmailRecoveryModule.sol` and associated managers and subject handlers. Because the relationship between the recovery module and manager is security critical, The factory ensures there is a tight coupling between a deployed module, and associated manager and subject handler.

Expand Down
111 changes: 66 additions & 45 deletions src/EmailRecoveryManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
/**
* Deployer address stored to prevent frontrunning at initialization
*/
address private deployer;
address private immutable deployer;

/**
* Account address to recovery config
Expand All @@ -82,19 +82,38 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
mapping(address account => EnumerableGuardianMap.AddressToGuardianMap guardian) internal
guardiansStorage;

/**
* @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();
}
_;
}

constructor(
address _verifier,
address _dkimRegistry,
address _emailAuthImpl,
address _subjectHandler
) {
if (_verifier == address(0)) {
revert InvalidVerifier();
}
if (_dkimRegistry == address(0)) {
revert InvalidDkimRegistry();
}
if (_emailAuthImpl == address(0)) {
revert InvalidEmailAuthImpl();
}
if (_subjectHandler == address(0)) {
revert InvalidSubjectHandler();
}
verifierAddr = _verifier;
dkimAddr = _dkimRegistry;
emailAuthImplementationAddr = _emailAuthImpl;
subjectHandler = _subjectHandler;
if (_subjectHandler == address(0)) {
revert InvalidSubjectHandler();
}
deployer = msg.sender;
}

Expand All @@ -108,16 +127,6 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
emailRecoveryModule = _emailRecoveryModule;
}

/**
* @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 */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
Expand Down Expand Up @@ -225,8 +234,8 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
* @param expiry The expiry time after which the recovery attempt is invalid
*/
function configureRecovery(
address[] memory guardians,
uint256[] memory weights,
address[] calldata guardians,
uint256[] calldata weights,
uint256 threshold,
uint256 delay,
uint256 expiry
Expand All @@ -241,21 +250,17 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
revert SetupAlreadyCalled();
}

if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToRecover(account)) {
if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToBeRecovered(account)) {
revert RecoveryModuleNotAuthorized();
}

// Allow recovery configuration without configuring guardians
if (guardians.length == 0 && weights.length == 0 && threshold == 0) {
guardianConfigs[account].initialized = true;
} else {
(uint256 guardianCount, uint256 totalWeight) =
setupGuardians(account, guardians, weights, threshold);
}

RecoveryConfig memory recoveryConfig = RecoveryConfig(delay, expiry);
updateRecoveryConfig(recoveryConfig);

emit RecoveryConfigured(account, guardians.length);
emit RecoveryConfigured(account, guardianCount, totalWeight, threshold);
}

/**
Expand All @@ -270,7 +275,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
{
address account = msg.sender;

if (!guardianConfigs[account].initialized) {
if (guardianConfigs[account].threshold == 0) {
revert AccountNotConfigured();
}
if (recoveryConfig.delay > recoveryConfig.expiry) {
Expand Down Expand Up @@ -299,20 +304,18 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
* @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
* @param {nullifier} Unused parameter. The nullifier acts as a unique identifier for an email,
* but it is not required in this implementation
*/
function acceptGuardian(
address guardian,
uint256 templateIdx,
bytes[] memory subjectParams,
bytes32
bytes32 /* nullifier */
)
internal
override
{
if (templateIdx != 0) {
revert InvalidTemplateIndex();
}

address account = IEmailRecoverySubjectHandler(subjectHandler).validateAcceptanceSubject(
templateIdx, subjectParams
);
Expand All @@ -321,7 +324,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
revert RecoveryInProcess();
}

if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToRecover(account)) {
if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToBeRecovered(account)) {
revert RecoveryModuleNotAuthorized();
}

Expand All @@ -333,6 +336,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
}

guardiansStorage.updateGuardianStatus(account, guardian, GuardianStatus.ACCEPTED);
guardianConfigs[account].acceptedWeight += guardianStorage.weight;

emit GuardianAccepted(account, guardian);
}
Expand All @@ -348,27 +352,31 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
* @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
* @param {nullifier} Unused parameter. The nullifier acts as a unique identifier for an email,
* but it is not required in this implementation
*/
function processRecovery(
address guardian,
uint256 templateIdx,
bytes[] memory subjectParams,
bytes32
bytes32 /* nullifier */
)
internal
override
{
if (templateIdx != 0) {
revert InvalidTemplateIndex();
}

(address account, bytes32 calldataHash) = IEmailRecoverySubjectHandler(subjectHandler)
.validateRecoverySubject(templateIdx, subjectParams, address(this));
address account = IEmailRecoverySubjectHandler(subjectHandler).validateRecoverySubject(
templateIdx, subjectParams, address(this)
);

if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToRecover(account)) {
if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToBeRecovered(account)) {
revert RecoveryModuleNotAuthorized();
}

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

// This check ensures GuardianStatus is correct and also implicitly that the
// account in email is a valid account
GuardianStorage memory guardianStorage = getGuardian(account, guardian);
Expand All @@ -380,16 +388,18 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco

recoveryRequest.currentWeight += guardianStorage.weight;

uint256 threshold = guardianConfigs[account].threshold;
if (recoveryRequest.currentWeight >= threshold) {
if (recoveryRequest.currentWeight >= guardianConfig.threshold) {
bytes32 calldataHash = IEmailRecoverySubjectHandler(subjectHandler)
.parseRecoveryCalldataHash(templateIdx, subjectParams);

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);
emit RecoveryProcessed(account, guardian, executeAfter, executeBefore, calldataHash);
}
}

Expand All @@ -409,7 +419,7 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
* @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 {
function completeRecovery(address account, bytes calldata recoveryCalldata) public override {
if (account == address(0)) {
revert InvalidAccountAddress();
}
Expand Down Expand Up @@ -453,6 +463,9 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
* @dev Deletes the current recovery request associated with the caller's account
*/
function cancelRecovery() external virtual {
if (recoveryRequests[msg.sender].currentWeight == 0) {
revert NoRecoveryInProcess();
}
delete recoveryRequests[msg.sender];
emit RecoveryCancelled(msg.sender);
}
Expand Down Expand Up @@ -524,13 +537,15 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
*/
function setupGuardians(
address account,
address[] memory guardians,
uint256[] memory weights,
address[] calldata guardians,
uint256[] calldata weights,
uint256 threshold
)
internal
returns (uint256, uint256)
{
guardianConfigs.setupGuardians(guardiansStorage, account, guardians, weights, threshold);
return
guardianConfigs.setupGuardians(guardiansStorage, account, guardians, weights, threshold);
}

/**
Expand All @@ -541,6 +556,12 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
* @param weight The weight assigned to the guardian
*/
function addGuardian(address guardian, uint256 weight) external onlyWhenNotRecovering {
// Threshold can only be 0 at initialization.
// Check ensures that setup function should be called first
if (guardianConfigs[msg.sender].threshold == 0) {
revert SetupNotCalled();
}

guardiansStorage.addGuardian(guardianConfigs, msg.sender, guardian, weight);
}

Expand Down
21 changes: 18 additions & 3 deletions src/factories/EmailRecoveryFactory.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,24 @@ contract EmailRecoveryFactory {
*/
address public immutable emailAuthImpl;

error InvalidVerifier();
error InvalidEmailAuthImpl();

event EmailRecoveryModuleDeployed(
address emailRecoveryModule, address emailRecoveryManager, address subjectHandler
address emailRecoveryModule,
address emailRecoveryManager,
address subjectHandler,
address validator,
bytes4 functionSelector
);

constructor(address _verifier, address _emailAuthImpl) {
if (_verifier == address(0)) {
revert InvalidVerifier();
}
if (_emailAuthImpl == address(0)) {
revert InvalidEmailAuthImpl();
}
verifier = _verifier;
emailAuthImpl = _emailAuthImpl;
}
Expand Down Expand Up @@ -57,7 +70,7 @@ contract EmailRecoveryFactory {
bytes32 subjectHandlerSalt,
bytes32 recoveryManagerSalt,
bytes32 recoveryModuleSalt,
bytes memory subjectHandlerBytecode,
bytes calldata subjectHandlerBytecode,
address dkimRegistry,
address validator,
bytes4 functionSelector
Expand All @@ -84,7 +97,9 @@ contract EmailRecoveryFactory {

// Initialize recovery manager with module address
EmailRecoveryManager(emailRecoveryManager).initialize(emailRecoveryModule);
emit EmailRecoveryModuleDeployed(emailRecoveryModule, emailRecoveryManager, subjectHandler);
emit EmailRecoveryModuleDeployed(
emailRecoveryModule, emailRecoveryManager, subjectHandler, validator, functionSelector
);

return (emailRecoveryModule, emailRecoveryManager, subjectHandler);
}
Expand Down
17 changes: 14 additions & 3 deletions src/factories/EmailRecoveryUniversalFactory.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,20 @@ contract EmailRecoveryUniversalFactory {
address public immutable verifier;
address public immutable emailAuthImpl;

event EmailRecoveryModuleDeployed(
event UniversalEmailRecoveryModuleDeployed(
address emailRecoveryModule, address emailRecoveryManager, address subjectHandler
);

error InvalidVerifier();
error InvalidEmailAuthImpl();

constructor(address _verifier, address _emailAuthImpl) {
if (_verifier == address(0)) {
revert InvalidVerifier();
}
if (_emailAuthImpl == address(0)) {
revert InvalidEmailAuthImpl();
}
verifier = _verifier;
emailAuthImpl = _emailAuthImpl;
}
Expand Down Expand Up @@ -52,7 +61,7 @@ contract EmailRecoveryUniversalFactory {
bytes32 subjectHandlerSalt,
bytes32 recoveryManagerSalt,
bytes32 recoveryModuleSalt,
bytes memory subjectHandlerBytecode,
bytes calldata subjectHandlerBytecode,
address dkimRegistry
)
external
Expand All @@ -75,7 +84,9 @@ contract EmailRecoveryUniversalFactory {

// Initialize recovery manager with module address
EmailRecoveryManager(emailRecoveryManager).initialize(emailRecoveryModule);
emit EmailRecoveryModuleDeployed(emailRecoveryModule, emailRecoveryManager, subjectHandler);
emit UniversalEmailRecoveryModuleDeployed(
emailRecoveryModule, emailRecoveryManager, subjectHandler
);

return (emailRecoveryModule, emailRecoveryManager, subjectHandler);
}
Expand Down
Loading

0 comments on commit 482d295

Please sign in to comment.