ERC-7913 signature verifier for ZKEmail

[Amxx]

Based on #96 and #98

The relevant file here is contracts/utils/cryptography/ERC7913SignatureVerifierZKEmail.sol

[arr00]

Is this a canonical address or one particular to the DKIM public key?

[ernestognw]

Not really, the ZK Email team even suggests to deploy it yourself: https://docs.zk.email/email-tx-builder/quickstart#build-and-deploy

[JohnGuilding]

Is this a canonical address or one particular to the DKIM public key?

actually, we do envision this as a canonical address per chain, as this is the dkim registry that get's updated by our infrastructure. Users can also override the dkim keys on the canonical registry if they want

Ofc users can still deploy their own but they would have to update it themselves.

This is not clear from our docs so we will update them!

[ernestognw]

actually, we do envision this as a canonical address per chain, as this is the dkim registry that get's updated by our infrastructure.

This would be amazing! If that's the case we could update the SignerZKEmail and this ERC7913SignatureVerifierZKEmail to hardcode the address of the canonical registry in the DKIMRegistry function, similar to how we hardcode the Entrypoint in Accounts. Ideally I would like developers to not have to worry about locating/deploying the registry

[JohnGuilding]

yeah ok we could look at doing that. We can run a new deployment and provide an address that would be consistent across chains

[ernestognw]

Opened an issue to track this: #140

[JohnGuilding]

Sweet. We'll aim to do the multi-chain deploy by end of next week

[JohnGuilding]

I missed this change from the last PR regarding the use of ECDSAOwnedDKIMRegistry.

We should actually use UserOverrideableDKIMRegistry which comes from @zk-email/contracts.

We soft deprecated ECDSAOwnedDKIMRegistry in favour of UserOverrideableDKIMRegistry as the latter is overridable and thus provides an escape hatch to users. We should make a stronger note of this in our repo/docs

Since UserOverrideableDKIMRegistry is the contract we would use, would it be possible to update all references of ECDSAOwnedDKIMRegistry to the that instead?

Example import

[ernestognw]

I tried using the UserOverrideableDKIMRegistry but it has an initializer and the constructor is already set. I think using ECDSAOwnedDKIMRegistry in the tests is fine. Note that the actual ZKEmailUtils and SignerZKEmail only require the IDKIMRegistry interface, so both should be compatible.

It's something we can also emphasize more in our contracts. For example, giving concrete instructions to use the UserOverrideableDKIMRegistry wherever the IDKIMRegistry is declared, wdyt?

[JohnGuilding]

but it has an initializer and the constructor is already set

I don't understand this point, can you clarify?

It's something we can also emphasize more in our contracts. For example, giving concrete instructions to use the UserOverrideableDKIMRegistry wherever the IDKIMRegistry is declared, wdyt?

The main point I would make is that we should not use ECDSAOwnedDKIMRegistry in the production contracts and examples. For tests I am not as opinionated about so happy to go with your call, as long as there are explicit docs that state UserOverrideableDKIMRegistry is the registry used in production

[ernestognw]

I don't understand this point, can you clarify?

Yes, so the UserOverrideableDKIMRegistry requires initialization and uses onlyInitializing through __Ownable_init, which complicates the tests setup. I think I should either be able to write true to the _initializing bool in Initializable (the other alternative is a top-level call). Creating a proxy has the same issue, we could work around it if the UserOverrideableDKIMRegistry was abstract so we can initialize during construction.

The main point I would make is that we should not use ECDSAOwnedDKIMRegistry in the production contracts and examples.

Yeah, that makes sense. I'm happy to note it in the documentation in a follow up PR 😄

[JohnGuilding]

What is the thinking here behind adding verifier and templateId, but not dkimRegistry and accountSalt? I noticed other 7913 verifiers do not have any state variables

[ernestognw]

Ahh good point. The standard defines stateless verifiers, its only purpose is to implement a universal verification mechanism for the algorithm they implement. In the case of the ERC7913SignatureVerifierZKEmail, it should be able to verify for any dkimRegistry and accountSalt, but sanitizing those is responsibility of the developer using the verifier (e.g. whitelisting key || verifier pairs)

[ernestognw]

I've been thinking about this. These variables are immutable in compliance with ERC-7913 statelessness since they're fixed for multiple emails. However, one could argue the dkimRegistry could fall into the same category, and similarly, an argument can be made that both must be part of the encoding.

I've come to the conclusion that the most elegant solution is to have all elements in the key, so that developers can override each value if they want to:

function _decodeKey(
    bytes calldata key
)
    internal
    view
    virtual
    returns (IDKIMRegistry registry, bytes32 accountSalt, IVerifier verifier, uint256 templateId)
{
    return abi.decode(key, (IDKIMRegistry, bytes32, IVerifier, uint256));
}

For example:

function _decodeKey(
    bytes calldata key
)
    internal
    view
    override
    returns (IDKIMRegistry registry, bytes32 accountSalt, IVerifier verifier, uint256 templateId)
{
   (registry, accountSalt, verifier, templateId) = abi.decode(key, (IDKIMRegistry, bytes32, IVerifier, uint256));
    // Validate any of these against immutable variables (e.g. require(verifier == cachedImmutableVerifier))
    return super._decodeKey(key);
}

This also has the benefit that different templates are allowed and just interpreted as different keys. I'll push an update

[JohnGuilding]

I've come to the conclusion that the most elegant solution is to have all elements in the key, so that developers can override each value if they want to

Sweet sounds good to me

[ernestognw]

Looks good, thanks all for your feedback. I'm merging so we can move forward, I want to rename the ERC7913SignatureVerifiers to just ERC7913Verifier, which I think is clearer.

I'll also work on a proposal to add multiple templateIds in the SignerZKEmail