-
Notifications
You must be signed in to change notification settings - Fork 24
ERC-7913 signature verifier for ZKEmail #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
27d2e52
a0d9c1d
b24564c
e2f86b9
4a10716
330bedf
359af07
238b86d
214188d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
// 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 contract verifies 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; | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This would be amazing! If that's the case we could update the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opened an issue to track this: #140 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I missed this change from the last PR regarding the use of We should actually use We soft deprecated Since Example import There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried using the It's something we can also emphasize more in our contracts. For example, giving concrete instructions to use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't understand this point, can you clarify?
The main point I would make is that we should not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, so the
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(); | ||
|
@@ -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 () { | ||
|
@@ -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(); | ||
}); | ||
}); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 anydkimRegistry
andaccountSalt
, but sanitizing those is responsibility of the developer using the verifier (e.g. whitelistingkey || verifier
pairs)Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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:
For example:
This also has the benefit that different templates are allowed and just interpreted as different keys. I'll push an update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet sounds good to me