Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.24;

import {IDKIMRegistry} from "@zk-email/contracts/DKIMRegistry.sol";
import {IVerifier} from "@zk-email/email-tx-builder/interfaces/IVerifier.sol";
import {EmailAuthMsg} from "@zk-email/email-tx-builder/interfaces/IEmailTypes.sol";
import {IERC7913SignatureVerifier} from "../../interfaces/IERC7913.sol";
import {ZKEmailUtils} from "./ZKEmailUtils.sol";

/**
* @dev ERC-7913 signature verifier that supports ZKEmail accounts.
*
* This verifier validates signatures produced through ZKEmail's zero-knowledge
* proofs which allows users to authenticate using their email addresses.
*/
abstract contract ERC7913SignatureVerifierZKEmail is IERC7913SignatureVerifier {
using ZKEmailUtils for EmailAuthMsg;

IVerifier private immutable _verifier;
uint256 private immutable _templateId;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@ernestognw ernestognw May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


constructor(IVerifier verifier_, uint256 templateId_) {
_verifier = verifier_;
_templateId = templateId_;
}

/// @dev An instance of the Verifier contract.
/// See https://docs.zk.email/architecture/zk-proofs#how-zk-email-uses-zero-knowledge-proofs[ZK Proofs].
function verifier() public view virtual returns (IVerifier) {
return _verifier;
}

/// @dev The command template of the sign hash command.
function templateId() public view virtual returns (uint256) {
return _templateId;
}

/**
* @dev Verifies a zero-knowledge proof of an email signature validated by a {DKIMRegistry} contract.
*
* The key format is ABI-encoded (IDKIMRegistry, bytes32) where:
* - IDKIMRegistry: The registry contract that validates DKIM public key hashes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ernestognw ernestognw Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@JohnGuilding JohnGuilding Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an issue to track this: #140

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* - bytes32: The account salt that uniquely identifies the user's email address
*
* The signature is an ABI-encoded {ZKEmailUtils-EmailAuthMsg} struct containing
* the command parameters, template ID, and proof details.
*
* Key encoding:
*
* ```solidity
* bytes memory key = abi.encode(registry, accountSalt);
* ```
*
* Signature encoding:
*
* ```solidity
* bytes memory signature = abi.encode(EmailAuthMsg({
* templateId: 1,
* commandParams: [hash],
* proof: {
* domainName: "example.com", // The domain name of the email sender
* publicKeyHash: bytes32(0x...), // Hash of the DKIM public key used to sign the email
* timestamp: block.timestamp, // When the email was sent
* maskedCommand: "Sign hash", // The command being executed, with sensitive data masked
* emailNullifier: bytes32(0x...), // Unique identifier for the email to prevent replay attacks
* accountSalt: bytes32(0x...), // Unique identifier derived from email and account code
* isCodeExist: true, // Whether the account code exists in the proof
* proof: bytes(0x...) // The zero-knowledge proof verifying the email's authenticity
* }
* }));
* ```
*/
function verify(bytes calldata key, bytes32 hash, bytes calldata signature) public view virtual returns (bytes4) {
(IDKIMRegistry registry, bytes32 accountSalt) = abi.decode(key, (IDKIMRegistry, bytes32));
EmailAuthMsg memory emailAuthMsg = abi.decode(signature, (EmailAuthMsg));

return
(abi.decode(emailAuthMsg.commandParams[0], (bytes32)) == hash &&
emailAuthMsg.templateId == templateId() &&
emailAuthMsg.proof.accountSalt == accountSalt &&
emailAuthMsg.isValidZKEmail(registry, verifier()) == ZKEmailUtils.EmailProofError.NoError)
? IERC7913SignatureVerifier.verify.selector
: bytes4(0xffffffff);
}
}
72 changes: 69 additions & 3 deletions test/account/AccountERC7913.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');

const { getDomain } = require('@openzeppelin/contracts/test/helpers/eip712');
const { ERC4337Helper } = require('../helpers/erc4337');
const { NonNativeSigner, P256SigningKey, RSASHA256SigningKey } = require('../helpers/signers');
const { NonNativeSigner, P256SigningKey, RSASHA256SigningKey, ZKEmailSigningKey } = require('../helpers/signers');
const { PackedUserOperation } = require('../helpers/eip712-types');

const { shouldBehaveLikeAccountCore, shouldBehaveLikeAccountHolder } = require('./Account.behavior');
Expand All @@ -15,15 +15,39 @@ const signerECDSA = ethers.Wallet.createRandom();
const signerP256 = new NonNativeSigner(P256SigningKey.random());
const signerRSA = new NonNativeSigner(RSASHA256SigningKey.random());

// Constants for ZKEmail
const accountSalt = '0x046582bce36cdd0a8953b9d40b8f20d58302bacf3bcecffeb6741c98a52725e2'; // keccak256("[email protected]")
const selector = '12345';
const domainName = 'gmail.com';
const publicKeyHash = '0x0ea9c777dc7110e5a9e89b13f0cfc540e3845ba120b2b6dc24024d61488d4788';
const emailNullifier = '0x00a83fce3d4b1c9ef0f600644c1ecc6c8115b57b1596e0e3295e2c5105fbfd8a';
const templateId = ethers.solidityPackedKeccak256(['string', 'uint256'], ['TEST', 0n]);

// Minimal fixture common to the different signer verifiers
async function fixture() {
// EOAs and environment
const [beneficiary, other] = await ethers.getSigners();
const [admin, beneficiary, other] = await ethers.getSigners();
const target = await ethers.deployContract('CallReceiverMockExtended');

// DKIM Registry for ZKEmail
const dkim = await ethers.deployContract('ECDSAOwnedDKIMRegistry');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ernestognw ernestognw May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ernestognw ernestognw May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

await dkim.initialize(admin, admin);
await dkim
.SET_PREFIX()
.then(prefix => dkim.computeSignedMsg(prefix, domainName, publicKeyHash))
.then(message => admin.signMessage(message))
.then(signature => dkim.setDKIMPublicKeyHash(selector, domainName, publicKeyHash, signature));

// ZKEmail Verifier
const zkEmailVerifier = await ethers.deployContract('ZKEmailVerifierMock');

// ERC-7913 verifiers
const verifierP256 = await ethers.deployContract('ERC7913SignatureVerifierP256');
const verifierRSA = await ethers.deployContract('ERC7913SignatureVerifierRSA');
const verifierZKEmail = await ethers.deployContract('$ERC7913SignatureVerifierZKEmail', [
zkEmailVerifier.target,
templateId,
]);

// ERC-4337 env
const helper = new ERC4337Helper();
Expand All @@ -43,7 +67,20 @@ async function fixture() {
.then(signature => Object.assign(userOp, { signature }));
};

return { helper, verifierP256, verifierRSA, domain, target, beneficiary, other, makeMock, signUserOp };
return {
helper,
verifierP256,
verifierRSA,
verifierZKEmail,
dkim,
zkEmailVerifier,
domain,
target,
beneficiary,
other,
makeMock,
signUserOp,
};
}

describe('AccountERC7913', function () {
Expand Down Expand Up @@ -103,4 +140,33 @@ describe('AccountERC7913', function () {
shouldBehaveLikeERC1271({ erc7739: true });
shouldBehaveLikeERC7821();
});

// Using ZKEmail with an ERC-7913 verifier
describe('ZKEmail', function () {
beforeEach(async function () {
// Create ZKEmail signer
this.signer = new NonNativeSigner(
new ZKEmailSigningKey(domainName, publicKeyHash, emailNullifier, accountSalt, templateId),
);

// Create account with ZKEmail verifier
this.mock = await this.makeMock(
ethers.concat([
this.verifierZKEmail.target,
ethers.AbiCoder.defaultAbiCoder().encode(['address', 'bytes32'], [this.dkim.target, accountSalt]),
]),
);

// Override the signUserOp function to use the ZKEmail signer
this.signUserOp = async userOp => {
const hash = await userOp.hash();
return Object.assign(userOp, { signature: this.signer.signingKey.sign(hash).serialized });
};
});

shouldBehaveLikeAccountCore();
shouldBehaveLikeAccountHolder();
shouldBehaveLikeERC1271({ erc7739: true });
shouldBehaveLikeERC7821();
});
});
Loading