Add SignerZKEmail and ZKEmailUtils

[ernestognw]

Context

OpenZeppelin will collaborate with ZKEmail https://github.com/zkemail to provide developers with an easy way to opt-in for this functionality for an account.

This PR includes an adaptation of their EmailSigner to be compatible with our AbstractSigner

[Amxx]

Is there tooling that we could use to have unit tests ?

[Amxx]

I noticed that this abstract signer is quite different from the other ones we already have:

• this abstractSigner requires further inheritance to set things such as the accountSalt(), DKIMRegistry() and verifier(). The other implementation we have storage everything they need (and have setters).
• this other abstractSigners are invisible from the outside, they only use private storage and internal functions. They don't expose anything publicly. This one exposes public functions.

This is not necessarily an issue, but its a pretty significant difference/inconsistency.

I'd personally remove the publics part of this abstract signer, and chose to selectively expose them in the contracts that inherit from this.

[ernestognw]

• this abstractSigner requires further inheritance to set things such as the accountSalt(), DKIMRegistry() and verifier(). The other implementation we have storage everything they need (and have setters).
• this other abstractSigners are invisible from the outside, they only use private storage and internal functions. They don't expose anything publicly. This one exposes public functions.

Right, these are good observations. For the first point, I didn't implement those functions because I wasn't sure whether it's better to put them in storage or not (now I think it's fine in storage as long as developers can override, I'll update).

For the second, I think the public functions are equivalent to the public key in each signer. For example, all signers have a public signer() getter, so I would say these public functions are a replace for signer(). Perhaps if this ZKEmailSigner exposes a signer() getter returning the accountSalt, DKIMRegistry and verifier would feel more consistent?

[Amxx]

For the second, I think the public functions are equivalent to the public key in each signer. For example, all signers have a public signer() getter, so I would say these public functions are a replace for signer().

Right.

I'm wondering if we should create a library that implements

library ZKEmailUtils {
    function verifyEmail(
        bytes32 accountSalt,
        IDKIMRegistry registry,
        IVerifier verifier,
        uint256 commandTemplate,
        EmailAuthMsg memory emailAuthMsg
    ) internal view virtual returns (bool, EmailProofError) { ... }
}

(and maybe an override that takes a bytes calldata and decode it to a emailAuthMsg transparently

That can be used in the abstract signer, but also in a 7913 signature verifier. Basically the idea would be to separate the crypto logic from the "integration" logic.

[Amxx]

We can move that down so that we don't pay the associated cost if the different checks (that don't use it) fails

[JohnGuilding]

Not for gas optimisation but another idea is that these could be stored as constants. Makes this function a bit cleaner.

Originally, we were replicating some earlier logic here where the command templates were dynamic

[ernestognw]

It makes sense to use constants as part of the library. For the "{uint}" one, I'm using the constant from your CommandUtils library.

Perhaps we add an template input to the isValidZKEmail?

function isValidZKEmail(
    EmailAuthMsg memory emailAuthMsg,
    IDKIMRegistry dkimregistry,
    IVerifier verifier,
    string memory template
) internal view returns (bool, EmailProofError) {
    if (!dkimregistry.isDKIMPublicKeyHashValid(emailAuthMsg.proof.domainName, emailAuthMsg.proof.publicKeyHash)) {
        return (false, EmailProofError.DKIMPublicKeyHash);
    } else if (bytes(emailAuthMsg.proof.maskedCommand).length > verifier.commandBytes()) {
        return (false, EmailProofError.MaskedCommandLength);
    } else if (emailAuthMsg.skippedCommandPrefix >= verifier.commandBytes()) {
        return (false, EmailProofError.SkippedCommandPrefixSize);
    } else {
        // Construct an expectedCommand from template and the values of emailAuthMsg.commandParams.
        string memory trimmedMaskedCommand = CommandUtils.removePrefix(
            emailAuthMsg.proof.maskedCommand,
            emailAuthMsg.skippedCommandPrefix
        );
        for (uint256 stringCase = 0; stringCase < 2; stringCase++) {
            if (
                CommandUtils.computeExpectedCommand(emailAuthMsg.commandParams, template, stringCase).equal(
                    trimmedMaskedCommand
                )
            ) {
                if (verifier.verifyEmailProof(emailAuthMsg.proof)) return (true, EmailProofError.NoError);
                else return (false, EmailProofError.EmailProof);
            }
        }
        return (false, EmailProofError.Command);
    }
}

In this way the implementation can construct the template out of constants. One of them being signHash.

[JohnGuilding]

Implementation looks good in regards to abidance with our logic. Some comments on your discussion:

I noticed that this abstract signer is quite different from the other ones we already have

Agree it makes sense to keep your signers consistent

Perhaps if this ZKEmailSigner exposes a signer() getter returning the accountSalt, DKIMRegistry and verifier would feel more consistent?

Nit: signer() feels like the wrong naming here for returning these values

the public functions are equivalent to the public key in each signer.

Yeah there are equivalencies here to your signers, and then there also ZK Email-specific abstractions.

The “public keys” here would be the public keys stored in the DKIM registry, and also the accountSalt. The account salt is an implementation detail of ZK Email in order to provide onchain privacy. More info here. We use the concept of an accountSalt in the EmailAuth circuits and ZK JWT circuits. Context: The EmailAuth circuits are the circuits we use to authenticate onchain actions with email

The verifier is kinda more analogous to the P256 and RSA libs you have. The commandTemplate is an implementation detail of the specific circuits that this signer would use - the EmailAuth circuits. More detail here.

I'm wondering if we should create a library that implements...

Open to the idea of a library. The thinking would then be that we would still store reference to the registry, verifier, salt etc in the signer @Amxx? Or something different

[JohnGuilding]

Not for gas optimisation but another idea is that these could be stored as constants. Makes this function a bit cleaner.

Originally, we were replicating some earlier logic here where the command templates were dynamic

[JohnGuilding]

@ernestognw merged a fix for the unused import. It's in main now

[Amxx]

Open to the idea of a library. The thinking would then be that we would still store reference to the registry, verifier, salt etc in the signer @Amxx? Or something different

It would be something like:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
import {IDKIMRegistry} from "@zk-email/contracts/DKIMRegistry.sol";
import {IVerifier, EmailProof} from "@zk-email/email-tx-builder/interfaces/IVerifier.sol";
import {EmailAuthMsg} from "@zk-email/email-tx-builder/interfaces/IEmailTypes.sol";
import {CommandUtils} from "@zk-email/email-tx-builder/libraries/CommandUtils.sol";
import {AbstractSigner} from "./AbstractSigner.sol";

library ZKEmailUtils {
    using Strings for string;

    enum EmailProofError {
        NoError,
        DKIMPublicKeyHash, // The DKIM public key hash verification fails
        MaskedCommandLength, // The masked command length exceeds the maximum
        SkippedCommandPrefixSize, // The skipped command prefix size is invalid
        Command, // The command format is invalid
        EmailProof // The email proof verification fails
    }

    function isValidZKEmail(
        EmailAuthMsg memory emailAuthMsg,
        IDKIMRegistry dkimregistry,
        IVerifier verifier
    ) internal view returns (bool, EmailProofError) {
        if (!dkimregistry.isDKIMPublicKeyHashValid(emailAuthMsg.proof.domainName, emailAuthMsg.proof.publicKeyHash)) {
            return (false, EmailProofError.DKIMPublicKeyHash);
        } else if (bytes(emailAuthMsg.proof.maskedCommand).length > verifier.commandBytes()) {
            return (false, EmailProofError.MaskedCommandLength);
        } else if (emailAuthMsg.skippedCommandPrefix >= verifier.commandBytes()) {
            return (false, EmailProofError.SkippedCommandPrefixSize);
        } else {
            string[] memory signHashTemplate = new string[](2);
            signHashTemplate[0] = "signHash";
            signHashTemplate[1] = "{uint}";
            
            // Construct an expectedCommand from template and the values of emailAuthMsg.commandParams.
            string memory expectedCommand = "";
            string memory trimmedMaskedCommand = CommandUtils.removePrefix(
                emailAuthMsg.proof.maskedCommand,
                emailAuthMsg.skippedCommandPrefix
            );
            for (uint256 stringCase = 0; stringCase < 2; ++stringCase) {
                if (
                    CommandUtils.computeExpectedCommand(
                        emailAuthMsg.commandParams,
                        signHashTemplate,
                        stringCase
                    ).equal(expectedCommand, trimmedMaskedCommand)
                ) {
                    if (verifier.verifyEmailProof(emailAuthMsg.proof)) 
                        return (true, EmailProofError.NoError);
                    else 
                        return (false, EmailProofError.EmailProof);
                }
            }
            return (false, EmailProofError.Command);
        }
    }
}

contract ZKEmailSigner is AbstractSigner {
    using ZKEmailUtils for *;

    // Need setters for these
    bytes32 private _accountSalt;
    IDKIMRegistry private _registry;
    IVerifier private _verifier;
    uint256 private _commandTemplate;


    function accountSalt() public view virtual returns (bytes32) { return _accountSalt; }
    function DKIMRegistry() public view virtual returns (IDKIMRegistry) { return _registry; }
    function verifier() public view virtual returns (IVerifier) { return _verifier; }
    function commandTemplate() public view virtual returns (uint256) { return _commandTemplate; }

    function _setAccountSalt(bytes32 accountSalt_) internal virtual { _accountSalt = accountSalt_; }
    function _setDKIMRegistry(IDKIMRegistry registry_) internal virtual { _registry = registry_; }
    function _setVerifier(IVerifier verifier_) internal virtual { _verifier = verifier_; }
    function _setCommandTemplate(uint256 commandTemplate_) internal virtual { _commandTemplate = commandTemplate_; }

    /// @inheritdoc AbstractSigner
    function _rawSignatureValidation(
        bytes32 hash,
        bytes calldata signature
    ) internal view virtual override returns (bool) {
        EmailAuthMsg memory emailAuthMsg = abi.decode(signature, (EmailAuthMsg));
        if (
            emailAuthMsg.commandParams[0] == hash &&
            emailAuthMsg.templateId == commandTemplate() &&
            emailAuthMsg.proof.accountSalt == accountSalt()
        ) {
            (bool verified, ) = emailAuthMsg.isValidZKEmail(DKIMRegistry(), verifier());
            return verified;
        } else {
            return false;
        }
    }
}

[JohnGuilding]

It would be something like:...

Yeah I'm a fan of the approach outlined.

This signer would still differ from your existing signers, as it would not contain the exact functions _setSigner or signer(). Is this ok for you or would you want to change something else here?

The variables are quite different to what is returned from signer() in the other signers, so it would feel like inconsistent naming to return the 4 zkemail-specific variables from a function called signer().

[ernestognw]

According to prettier --help, the default value already is [.gitignore, .prettierignore]

[Amxx]

we should also change that on the vanila repo

[ernestognw]

(and maybe an override that takes a bytes calldata and decode it to a emailAuthMsg transparently

The issue with this is that a library can't have virtual functions. So there's no way to override it.

That can be used in the abstract signer, but also in a 7913 signature verifier. Basically the idea would be to separate the crypto logic from the "integration" logic.

Yes, I agree that a library would be the best way to abstract the logic. I updated the PR accordingly.

The “public keys” here would be the public keys stored in the DKIM registry, and also the accountSalt. The account salt is an implementation detail of ZK Email in order to provide onchain privacy. More info here.

Added this as context to the current accountSalt() function NatSpec. I think it's important so that developers know how to override it if they want.

The verifier is kinda more analogous to the P256 and RSA libs you have. The commandTemplate is an implementation detail of the specific circuits that this signer would use - the EmailAuth circuits. More detail here.

Right. This is way @Amxx brought ERC-7913 (WIP) to the conversation. We think there's a broader use case for deployed key verifiers, so for example, they can be used in ERC-7579 modules or perhaps make a generic 7913 base contract that inherits AbstractSigner too (see #98)

I guess @Amxx point is that the ZKEmailUtils.isValidZKEmail may receive an IERC7913 instance instead of an IVerifier one, as in

function isValidZKEmail(
    EmailAuthMsg memory emailAuthMsg,
    IDKIMRegistry dkimregistry,
    IERC7913 verifier
) internal view virtual returns (bool, EmailProofError)

This signer would still differ from your existing signers, as it would not contain the exact functions _setSigner or signer(). Is this ok for you or would you want to change something else here?

This is a good point. I wouldn't mind that this signer doesn't expose the signer function since the function is not specified by any standard, so the AbstractSigner is mostly an internal interface that's enforced by the compiler (i.e. it requires an implementation to compile).

You seem convinced that we should keep the signers consistency in this regard, curious to hear your thoughts.

[Amxx]

I guess @Amxx point is that the ZKEmailUtils.isValidZKEmail may receive an IERC7913 instance instead of an IVerifier one

That was not what I had in mind

[Amxx]

Is there tooling that we could use to have unit tests ?

Any news on that part. Code looks quite good.

[JohnGuilding]

Any news on that part. Code looks quite good.

You can reference these for unit testing. Although they are forge tests not hardhat tests

[benceharomi]

@ernestognw the code looks great! added some comments to the unused imports I found

[benceharomi]

@JohnGuilding do we care about the order of checks here? If we don't wouldn't it be cheaper to go in this order?

// 1. SkippedCommandPrefixSize
// 2. MaskedCommandLength
// 3. MismatchedCommand
// 4. DKIMPublicKeyHash

[JohnGuilding]

I don't think the order matters here and changing it doesn't effect the readability imo. And yeah it would be cheaper to fail fast with the less expensive checks. If this was our code, would make this change. Not sure what the OZ view is on optimisations like this? @ernestognw

[ernestognw]

I think we can make this change. Generally, we don't worry about the cost of the failure case, especially because we don't know what branch is most likely to fail. For example, if the error distribution is 80% DKIMPublicKeyHash, we should prioritize that one first.

I'm updating the code since I don't think the order is a big issue.

[ernestognw]

@ernestognw the code looks great! added some comments to the unused imports I found

Thanks for your feedback @benceharomi, I just updated removing the unused imports 😄

[ernestognw]

All good on our side, gentlemen. The only thing we may be missing is just a real email proof that we can use for testing, currently everything is mocked.

Also, I noticed it may not be possible not ensure the signer doesn't revert as we want for the ERC-4337 validation phase. The issue is that we're always doing an abi.decode() over the signature in _rawValidateSignature, so reverting is as easy as just passing a wrong value. I'm adding a minimum length check that helps some tests pass but the only alternative might be directly calldata decoding, which is not an easy task and may require heavy use of assembly.

[ernestognw]

Merging, thanks everyone for their feedback ❤️