Add generic validator recovery module and update zk email contracts to use it

[JohnGuilding]

No description provided.

[wshino]

In my thought, callData in mail subject is not user-friendly.

How about the following implementation? I'd like to know what do you think

Add public mapping for function that user wants to request

mapping(address -> string) public 
recoveryFunction

Add public mapping for param indexes that user wants to use with the above function

mapping(address -> uint[]) public recoveryParamIndexes

Add callFuntionName into arguments of configureRecovery function

// for example, “recovery(string, string)”
recoveryFunction[account] = callFuntionName;

Add callParamIndexes into arguments of configureRecovery function

// for example, [1, 2]
recoveryParamIndexes[account] = callParamIndexes;

Regarding accountIndex. At this time, I think accountIndex is always 0 it’s fine.

Build callParams in processRecovery function

uint256 paramLength = recoveryParamIndexes[account].length;
bytes[] memory callParams = new bytes[](paramLength);

for (uint256 i = 0; i < subjectParams.length; i++) {
    for (uint256 j = 0; j < recoveryParamIndexes[account].length; j++) {
        if (i == recoveryParamIndexes[account][j]) {
            callParams[j] = subjectParams[i];
        }
    }
}

Build callData in processRecovery function

bytes memory data = abi.encodeSignature(signature, callParams)

[JohnGuilding]

Summary of Technical Discussion Points

Some key discussion points, summarised in this PR for visibility. Originally scaffolded by chapgpt

Indexing Templates by Account Address

Context

Allow templates to be indexed by an account address (could also be indexed by module address), negating need for hardcoded subjects and making ZkEmailRecovery.sol generic.

Relevant Links

• New storage
• Interface change for acceptance templates
• Interface change for recovery templates

Resolution 11/06

Decided against to keep EmailAccountRecovery changes minimal. See section titled "note on EmailAccountRecovery changes"

Storing Templates in a Registry

Context

Suggested by Rhinestone. An attestation flow could be used to maintain the subject templates. Storing them per module and not per smart account could make it easier for the user to opt-in

Relevant Links

• Rhinestone PR

Resolution 11/06

Decided against to keep EmailAccountRecovery changes minimal. Should be considered if we change how templates are stored. See section titled "note on EmailAccountRecovery changes"

Adding Account Address to completeRecovery

Context

Include account address as an argument in completeRecovery.

Simplifies ZkEMailRecovery logic and reduces LOC by about 50, removes an entire contract from audit, and reduces burden for developers to understand the router contract. This does require changes to relayer logic and integration with Clave. Router contract currently solves this problem so is technically unnecessary for these changes, and adds more mandatory arguments. However, provides quite a good tradeoff for the number of solidity changes required. The account address could also be assumed to be part of the potentially new recoveryCalldata param, which if we add that param, means these change could introduce no new lines of code to EmailAccountRecovery beyond what is already being added

Relevant Links

• completeRecovery changes

Resolution 11/06

Included account address as an argument

Subject Calldata Builder

Context

Introduce a SubjectCalldataBuilder which would allow us to keep email subjects more human readable, while keeping the flexibility that using calldata gives us.

Downsides to this idea are the subject templates must be abi.encoded off chain properly. Also you would need to include all arguments that would get passed in calldata into the subject - this could lead to long subjects and difficulty in understanding subject templates, as you are basically surfacing all of the required arguments e.g. Safe has a parameter of previousOwner for it's recovery function which isn't actually the current/previous owner, it is a owner address that points to the existing owner in a linked list. If you weren't making a generic contract, you'd have some flexibility to fetch data like this dynamically (like the Safe recovery module does). So cases like this would not be user friendly, but on the whole, this approach would provide much richer email subjects then just passing in a calldata hash.

Could be modified to use different ordering than in subject template. E.g. if your subject read better if in a certain order, which is different to what the calldata construction needed. Also processRecovery assumes accountAddress is the first subjectParam still, this could be kept the same, could be changed by using different ordering as just described. Or if we didn't want to assume that the account address was always passed in here, we could add the account address to EmailAccountRecovery.handleRecovery()

Relevant Links

• Code Example
• Implementation

Resolution 11/06

Decided against because too technical/abstract

Guardians for Multiple Validators

Context

Allow the same guardian to be used for multiple validators on the same account

Increases flexibility for modular accounts but this might be too much of a scope change given timeline and could be framed as an edge case.

Relevant Links

• Code changes that could resolve this in EmailAccountRecovery

Resolution 11/06

Not addressed yet

Deploy one module for every account vs one module per wallet provider/validator implementation

Context

Deploy a generic module per wallet provider vs. a single module for multiple accounts.

Module per provider could help with adding security features such as hardcoding recovery function selectors, but requires more deployments and adds friction for account developers - so is away from direction of fully generic recovery module.

Resolution 11/06

Deployed once per implementation. Adding the security feature of hardcoding function selectors didn't work cleanly, as would require a different module to account for difference in constructor args in comparison to generic validator module. Currently wondering if this is the right approach and whether all contracts should be deployed once for all implementations (except handlers), this would require changes to EmailAccountRecovery that could be redundant in non-modular account scenarios.

note on EmailAccountRecovery changes

EmailAccountRecovery is also meant to work for non-modular accounts, so there is motivation to avoid parameters that could be redundant in some cases e.g. account address. One approach we used before was having a router contract deployed for recovery configuration. This solved the issue but did add complexity to the recovery contracts. Another solution is to have a modular and non-modular EmailAccountRecovery, this could lead to code duplication and would also increase audit cost.

Lack of validation at process recovery stage

Context

One downside of the fully generic module approach is that we cannot validate the functional data in the subjects like we did before. The assumption (which Rhinestone agreed) was that validator modules should do this validation in the recovery function that is getting called. However, because we don't do validation in processRecovery like zero address checks or checking the old owner exists etc, there could be an error in the calldata that gets propagated at the processRecovery stage, and we don't find out until recovery is attempted. In this case, you need a way for a new recovery request to be started. The issue here is that the owner cannot cancel the recovery attempt because they have lost their private key.

So some possible solutions here would be 1) trust and give the guardians permission to restart recovery attempts to correct the invalid calldata, or 2) you delete the recovery request when the recover function on the validator returns an error. Both of which have their own tradeoffs

Resolution 11/06

Addressed via adding handler contracts that define subject templates and how to validate them

[zeroknots]

consider using uint48 here. would allow packing both executeAfter and executeBefore into one slot. since these are sloaded together

[zeroknots]

I quite like the new design of initializing EmailRecoveryManager with different SubjectHandlers

[zeroknots]

it would be good to add a set/remove function for the recovery allowedSelectors after installation as well

[JohnGuilding]

Agree will add that

[zeroknots]

I assume this is just for PoC and in the final version we will support multiple validators

[JohnGuilding]

Yes - just pushed an initial commit for this. It's a little messy with multiple mappings but wanted to push something as a POC.

Let me know if you can think of any cleaner ways of doing this

[jacque006]

Few minor post-merge suggestions, didn't see anything major. Like the continued breakup of recovery flow into more standardized components 👍

[jacque006]

For production/mainnet deploys, you may want to switch this to the user overrideable registry by default https://github.com/zkemail/zk-email-verify/blob/main/packages/contracts/UserOverrideableDKIMRegistry.sol

[jacque006]

Is there a reason requires are used in this file instead of errors? Would be good to be consistent, and can use error param data to include expected vs actual to aid in debugging.

[jacque006]

Docstrings/natspec on this would be helpful for implementers & for understanding implementations above ^