Skip to content

Feat/gnark zkemail#114

Open
Peartes wants to merge 31 commits intomainfrom
feat/gnark-zkemail
Open

Feat/gnark zkemail#114
Peartes wants to merge 31 commits intomainfrom
feat/gnark-zkemail

Conversation

@Peartes
Copy link
Contributor

@Peartes Peartes commented Dec 18, 2025

This commit adds in the zk email authenticator into the AA contract

ash-burnt and others added 28 commits November 17, 2024 00:01
ZKEmail Authenticator supporting latest ZKEmail circuits
Updated the zk-email interface to match the dkim module
Added allowed-email-hosts field in zk-email auth to whitelist hosts to generate the signatures
@crucible-burnt
Copy link

🔍 Crucible Security Review

Summary

Adds ZK Email authenticator support to the Account contract, enabling email-based authentication via DKIM proofs. Integrates with the xion DKIM module for proof verification.

Security Assessment

  • Risk Level: Medium (feature addition, new attack surface)
  • Proof validation: Properly delegates to xion DKIM module via gRPC query
  • Signature length check: 700-byte minimum enforces reasonable proof size
  • Email host validation: Non-empty check prevents misconfiguration
  • Verification before save: Signature verified before storing authenticator

Potential Concerns:

  • Email salt handling: Verify salt is truly random and properly isolated per user; check if public_inputs use leaks information
  • Allowed hosts update: Requires signature verification before allowing new hosts (current: unverified update possible)

Immunefi Pattern Check

  • ✅ No proto.Message singletons
  • ✅ No JWT validation gaps
  • ✅ No fee bypass paths
  • ✅ Proper state validation
  • ✅ Signature verification before storage

False Report Risk

  • Email salt format needs documentation to prevent claims of "plaintext email storage"
  • Add comments on DKIM verification to prevent "unverified signature" reports

Code Quality

  • Strong error handling with descriptive types
  • Good cosmwasm_std abstractions
  • Test coverage with sample proofs ✓
  • Note: remove_auth_method validation logic change needs separate verification

Recommendation

Approve with minor improvements:

  • Document email_salt format and privacy implications
  • Add signature check to update_allowed_email_hosts
  • Verify remove_auth_method change separately

Copy link

@crucible-burnt crucible-burnt left a comment

Choose a reason for hiding this comment

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

🔍 Crucible Security Review

Summary

Adds gnark-based ZKEmail verification support. Updates CosmWasm dependencies and adds new protobuf/cosmos SDK dependencies.

Security Assessment

  • Risk Level: Medium ⚠️ (new ZK functionality)
  • Dependency updates:
    • cosmwasm-* 2.2.2 → 2.3.2
    • anyhow 1.0.100 → 1.0.101
    • bytes 1.10.1 → 1.11.1
  • New dependencies:
    • cosmos-sdk-proto from burnt-labs fork (feat/xion-zk branch)
    • pbjson, pbjson-types, prost, tendermint-proto
    • chrono for timestamp handling
    • aho-corasick for pattern matching (likely for email parsing)

Immunefi Pattern Check

  • New attack surface: Email verification via ZK proofs
  • Custom cosmos-sdk-proto fork requires supply chain review
  • No known matches to existing patterns (new functionality)

False Report Risk

  • ZKEmail is complex — document security assumptions clearly
  • Email parsing has historically been a source of vulnerabilities

Code Quality Notes

  • Using forked cosmos-sdk-proto from burnt-labs — ensure fork maintenance plan
  • chrono added without tz feature — verify timezone handling is intentional
  • bitflags 2.10.0 added as new dependency
  • Recommendation: Pin cosmos-sdk-proto to specific commit rather than branch

Status

Requires deeper cryptographic review. ZKEmail implementations need specialist attention. Recommend:

  1. Document fork maintenance plan for cosmos-sdk-proto
  2. Ensure email parsing is robust against malformed inputs
  3. Dedicated ZK circuit audit before mainnet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants