ZK Shuffle circuits#116
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a ZK shuffle contract for the XION blockchain that verifies shuffle-encrypt and decrypt zero-knowledge proofs. The contract provides a minimal implementation to test proof verification at the XION module level using Groth16 proofs.
Changes:
- CosmWasm contract that verifies shuffle encrypt and decrypt proofs via XION's ZK module
- Shell scripts for testing proof verification with sample proof data
- Verification keys, deployment configurations, and comprehensive documentation
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/zk-shuffle/src/contract.rs | Main contract entry points for instantiate, execute, and query functions |
| contracts/zk-shuffle/src/zkshuffle.rs | ZK proof verification logic using XION's gRPC ZK module interface |
| contracts/zk-shuffle/src/types.rs | Groth16Proof type definition |
| contracts/zk-shuffle/src/state.rs | State management for tracking verification counts |
| contracts/zk-shuffle/src/msg.rs | Message types for contract interactions |
| contracts/zk-shuffle/src/error.rs | Contract error definitions |
| contracts/zk-shuffle/vkeys/shuffle_encrupt.json | Verification key for shuffle encrypt circuit (filename has typo) |
| contracts/zk-shuffle/vkeys/decrypt.json | Verification key for decrypt circuit |
| contracts/zk-shuffle/scripts/*.sh | Shell scripts for testing proof verification |
| contracts/zk-shuffle/Cargo.toml | Dependencies including cosmos-sdk-proto from burnt-labs fork |
| contracts/zk-shuffle/README.md | Comprehensive deployment and usage documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn verify_shuffle_proof( | ||
| deps: Deps, | ||
| proof: &([Uint256; 2], [[Uint256; 2]; 2], [Uint256; 2]), | ||
| public_inputs: &[Uint256], | ||
| verifier_name: &str, | ||
| ) -> Result<bool, ContractError> { | ||
| let snarkjs_proof = groth16_proof_to_snarkjs(&proof.0, &proof.1, &proof.2); | ||
| let public_inputs_str = public_inputs_to_string(public_inputs); | ||
|
|
||
| let verify_request = QueryVerifyRequest { | ||
| proof: serde_json::to_vec(&snarkjs_proof).map_err(|e| { | ||
| ContractError::Std(cosmwasm_std::StdError::generic_err(format!( | ||
| "Failed to serialize proof: {}", | ||
| e | ||
| ))) | ||
| })?, | ||
| public_inputs: public_inputs_str, | ||
| vkey_name: verifier_name.to_string(), | ||
| vkey_id: 3, | ||
| }; | ||
|
|
||
| let request_bytes = verify_request.to_bytes().map_err(|e| { | ||
| ContractError::Std(cosmwasm_std::StdError::generic_err(format!( | ||
| "Failed to encode verify request: {}", | ||
| e | ||
| ))) | ||
| })?; | ||
|
|
||
| let response: cosmwasm_std::Binary = deps | ||
| .querier | ||
| .query_grpc( | ||
| "/xion.zk.v1.Query/ProofVerify".to_string(), | ||
| cosmwasm_std::Binary::from(request_bytes), | ||
| ) | ||
| .map_err(|e| { | ||
| ContractError::Std(cosmwasm_std::StdError::generic_err(format!( | ||
| "Failed to query zk module: {}", | ||
| e | ||
| ))) | ||
| })?; | ||
|
|
||
| let verify_response: ProofVerifyResponse = ProofVerifyResponse::decode(response.as_slice()) | ||
| .map_err(|e| { | ||
| ContractError::Std(cosmwasm_std::StdError::generic_err(format!( | ||
| "Failed to decode verify response: {}", | ||
| e | ||
| ))) | ||
| })?; | ||
|
|
||
| Ok(verify_response.verified) | ||
| } | ||
|
|
||
| /// Verify a decryption proof using the XION zk module | ||
| /// | ||
| /// # Arguments | ||
| /// * `deps` - Deps for querier access | ||
| /// * `proof` - Groth16 proof (a, b, c components) | ||
| /// * `public_inputs` - Public inputs for the proof circuit | ||
| /// * `verifier_name` - Name of the verifier key stored in the zk module | ||
| /// | ||
| /// # Returns | ||
| /// * `Ok(true)` if proof is valid | ||
| /// * `Ok(false)` if proof is invalid | ||
| /// * `Err(ContractError)` if verification fails due to other errors | ||
| pub fn verify_decrypt_proof( | ||
| deps: Deps, | ||
| proof: &([Uint256; 2], [[Uint256; 2]; 2], [Uint256; 2]), | ||
| public_inputs: &[Uint256], | ||
| verifier_name: &str, | ||
| ) -> Result<bool, ContractError> { | ||
| let snarkjs_proof = groth16_proof_to_snarkjs(&proof.0, &proof.1, &proof.2); | ||
| let public_inputs_str = public_inputs_to_string(public_inputs); | ||
|
|
||
| let verify_request = QueryVerifyRequest { | ||
| proof: serde_json::to_vec(&snarkjs_proof).map_err(|e| { | ||
| ContractError::Std(cosmwasm_std::StdError::generic_err(format!( | ||
| "Failed to serialize proof: {}", | ||
| e | ||
| ))) | ||
| })?, | ||
| public_inputs: public_inputs_str, | ||
| vkey_name: verifier_name.to_string(), | ||
| vkey_id: 2, | ||
| }; | ||
|
|
||
| let request_bytes = verify_request.to_bytes().map_err(|e| { | ||
| ContractError::Std(cosmwasm_std::StdError::generic_err(format!( | ||
| "Failed to encode verify request: {}", | ||
| e | ||
| ))) | ||
| })?; | ||
|
|
||
| let response: cosmwasm_std::Binary = deps | ||
| .querier | ||
| .query_grpc( | ||
| "/xion.zk.v1.Query/ProofVerify".to_string(), | ||
| cosmwasm_std::Binary::from(request_bytes), | ||
| ) | ||
| .map_err(|e| { | ||
| ContractError::Std(cosmwasm_std::StdError::generic_err(format!( | ||
| "Failed to query zk module: {}", | ||
| e | ||
| ))) | ||
| })?; | ||
|
|
||
| let verify_response: ProofVerifyResponse = ProofVerifyResponse::decode(response.as_slice()) | ||
| .map_err(|e| { | ||
| ContractError::Std(cosmwasm_std::StdError::generic_err(format!( | ||
| "Failed to decode verify response: {}", | ||
| e | ||
| ))) | ||
| })?; | ||
|
|
||
| Ok(verify_response.verified) | ||
| } |
There was a problem hiding this comment.
The code has significant duplication between verify_shuffle_proof and verify_decrypt_proof functions. The only differences are the vkey_id values (3 vs 2). Consider refactoring to a single generic verify_proof function that takes vkey_id as a parameter to reduce code duplication and improve maintainability.
| //! Minimal zkShuffle contract for testing proof verification at XION module level | ||
|
|
||
| #[cfg(not(feature = "library"))] | ||
| use cosmwasm_std::entry_point; | ||
| use cosmwasm_std::{ | ||
| to_json_binary, Addr, Binary, Deps, DepsMut, Env, MessageInfo, Response, StdResult, | ||
| }; | ||
|
|
||
| use crate::error::ContractError; | ||
| use crate::msg::{ExecuteMsg, InstantiateMsg, QueryMsg, VerificationCountResponse}; | ||
| use crate::state::{VerificationState, VERIFICATION_STATE}; | ||
| use crate::types::Groth16Proof; | ||
| use crate::zkshuffle::{verify_decrypt_proof, verify_shuffle_proof}; | ||
|
|
||
| const CONTRACT_NAME: &str = "crates.io:zk-shuffle"; | ||
| const CONTRACT_VERSION: &str = env!("CARGO_PKG_VERSION"); | ||
|
|
||
| #[cfg_attr(not(feature = "library"), entry_point)] | ||
| pub fn instantiate( | ||
| deps: DepsMut, | ||
| _env: Env, | ||
| _info: MessageInfo, | ||
| _msg: InstantiateMsg, | ||
| ) -> Result<Response, ContractError> { | ||
| let state = VerificationState::new(); | ||
| VERIFICATION_STATE.save(deps.storage, &state)?; | ||
|
|
||
| Ok(Response::new().add_attribute("action", "instantiate")) | ||
| } | ||
|
|
||
| #[cfg_attr(not(feature = "library"), entry_point)] | ||
| pub fn execute( | ||
| deps: DepsMut, | ||
| _env: Env, | ||
| _info: MessageInfo, | ||
| msg: ExecuteMsg, | ||
| ) -> Result<Response, ContractError> { | ||
| match msg { | ||
| ExecuteMsg::VerifyShuffleProof { | ||
| proof, | ||
| public_inputs, | ||
| } => execute_verify_shuffle_proof(deps, proof, public_inputs), | ||
| ExecuteMsg::VerifyDecryptProof { | ||
| proof, | ||
| public_inputs, | ||
| } => execute_verify_decrypt_proof(deps, proof, public_inputs), | ||
| } | ||
| } | ||
|
|
||
| fn execute_verify_shuffle_proof( | ||
| deps: DepsMut, | ||
| proof: Groth16Proof, | ||
| public_inputs: Vec<cosmwasm_std::Uint256>, | ||
| ) -> Result<Response, ContractError> { | ||
| let proof_tuple = (proof.a, proof.b, proof.c); | ||
| let verifier_name = "shuffle_encrypt"; | ||
| let verified = | ||
| verify_shuffle_proof(deps.as_ref(), &proof_tuple, &public_inputs, verifier_name)?; | ||
|
|
||
| if !verified { | ||
| return Err(ContractError::InvalidProof); | ||
| } | ||
| let mut state = VERIFICATION_STATE.load(deps.storage)?; | ||
| state.shuffle_verifications += 1; | ||
| VERIFICATION_STATE.save(deps.storage, &state)?; | ||
|
|
||
| Ok(Response::new() | ||
| .add_attribute("action", "verify_shuffle_proof") | ||
| .add_attribute("result", "success") | ||
| .add_attribute( | ||
| "total_shuffle_verifications", | ||
| state.shuffle_verifications.to_string(), | ||
| )) | ||
| } | ||
|
|
||
| fn execute_verify_decrypt_proof( | ||
| deps: DepsMut, | ||
| proof: Groth16Proof, | ||
| public_inputs: Vec<cosmwasm_std::Uint256>, | ||
| ) -> Result<Response, ContractError> { | ||
| let proof_tuple = (proof.a, proof.b, proof.c); | ||
| let verifier_name = "decrypt"; | ||
| let verified = | ||
| verify_decrypt_proof(deps.as_ref(), &proof_tuple, &public_inputs, verifier_name)?; | ||
|
|
||
| if !verified { | ||
| return Err(ContractError::InvalidProof); | ||
| } | ||
| let mut state = VERIFICATION_STATE.load(deps.storage)?; | ||
| state.decrypt_verifications += 1; | ||
| VERIFICATION_STATE.save(deps.storage, &state)?; | ||
|
|
||
| Ok(Response::new() | ||
| .add_attribute("action", "verify_decrypt_proof") | ||
| .add_attribute("result", "success") | ||
| .add_attribute( | ||
| "total_decrypt_verifications", | ||
| state.decrypt_verifications.to_string(), | ||
| )) | ||
| } | ||
|
|
||
| #[cfg_attr(not(feature = "library"), entry_point)] | ||
| pub fn query(deps: Deps, _env: Env, msg: QueryMsg) -> StdResult<Binary> { | ||
| match msg { | ||
| QueryMsg::VerificationCount {} => to_json_binary(&query_verification_count(deps)?), | ||
| } | ||
| } | ||
|
|
||
| fn query_verification_count(deps: Deps) -> StdResult<VerificationCountResponse> { | ||
| let state = VERIFICATION_STATE.load(deps.storage)?; | ||
| Ok(VerificationCountResponse { | ||
| shuffle_verifications: state.shuffle_verifications, | ||
| decrypt_verifications: state.decrypt_verifications, | ||
| }) | ||
| } |
There was a problem hiding this comment.
The contract module (contract.rs) lacks test coverage for the execute and query functions. The only tests are in zkshuffle.rs for utility functions. Consider adding integration tests or unit tests with mocked dependencies for execute_verify_shuffle_proof, execute_verify_decrypt_proof, and query_verification_count to ensure proper error handling and state management.
| @@ -0,0 +1,1155 @@ | |||
| { | |||
There was a problem hiding this comment.
The filename contains a spelling error: "shuffle_encrupt.json" should be "shuffle_encrypt.json" to match the naming used throughout the codebase (e.g., "shuffle_encrypt" in contract.rs line 56).
| docker run --rm -v "$(pwd)":/code -v ~/.ssh:/root/.ssh:ro -e SSH_AUTH_SOCK=/ssh-agent \ | ||
| --mount type=volume,source="$(basename "$(pwd)")_cache",target=/target \ | ||
| --mount type=volume,source=registry_cache,target=/usr/local/cargo/registry \ | ||
| wasm_optimizer ./ |
There was a problem hiding this comment.
Mounting the host ~/.ssh directory into the build container (-v ~/.ssh:/root/.ssh:ro) exposes your SSH private keys to any code running inside the optimizer image. A compromised base image or malicious build dependency (e.g., a build.rs script from a third-party crate) could read /root/.ssh and exfiltrate your keys over the network, leading to repository or infrastructure compromise. To mitigate this, avoid mounting raw SSH key material into the container (prefer agent forwarding or narrowly scoped deploy keys) and restrict network access for the build container where possible.
crucible-burnt
left a comment
There was a problem hiding this comment.
Review — ZK Shuffle Circuits
6k line addition — new contract for ZK shuffle encrypt/decrypt proof verification. This is a significant crypto primitive.
High-level observations:
- New contract at
contracts/zk-shuffle/with Groth16 verification for shuffle encrypt + decrypt proofs - Includes verification keys, test data, and shell scripts for testing
⚠️ vkeys/shuffle_encrupt.json— typo: should beshuffle_encrypt.json
Recommendation: This contract deals with zero-knowledge proof verification — a domain where subtle bugs can break soundness guarantees. Strongly recommend a dedicated ZK/crypto specialist review before merging. Specifically:
- Verification key generation process and trusted setup assumptions
- Public input binding — ensure all relevant inputs are committed
- Proof malleability — check if the verifier rejects modified proofs
- Gas costs — Groth16 verification on-chain can be expensive; verify gas benchmarks
Happy to do a line-by-line code review, but the cryptographic correctness needs a specialist.
crucible-burnt
left a comment
There was a problem hiding this comment.
🔍 Crucible Security Review
Summary
Adds ZK Shuffle contract infrastructure for zero-knowledge card shuffling. Large PR adding new contract with cryptographic dependencies.
Security Assessment
- Risk Level: Medium
⚠️ (new ZK functionality requires careful review) - New contract at
contracts/zk-shuffle/with:- Cargo.lock with cryptographic dependencies (ark-bls12-381, ark-ec, ark-ff, ark-poly)
- Environment configuration examples
- Dependencies include:
ark-*suite for elliptic curve and field arithmeticrayonfor parallel computation- Standard CosmWasm dependencies
Immunefi Pattern Check
- New attack surface: ZK proof verification contracts are high-value targets
- Requires dedicated cryptographic review beyond automated scanning
- No known matches to existing vulnerability patterns (new code)
False Report Risk
- ZK implementations commonly attract security researcher attention
- Recommend thorough documentation of cryptographic assumptions
Code Quality Notes
- Large dependency tree — review for pinned versions and supply chain security
.env.exampleincludes testnet configuration — appropriate for development- Contract code not visible in diff excerpt — full review needed
- Recommendation: Ensure ZK circuits have been formally verified or audited
Status
Requires deeper cryptographic review. Standard security patterns appear followed, but ZK implementations need specialist attention. Flag for dedicated audit before mainnet deployment.
crucible-burnt
left a comment
There was a problem hiding this comment.
🔍 Crucible Security Review
Summary
Adds ZK Shuffle circuits contract. Large PR (6k+ lines) — majority is Cargo.lock and circuit boilerplate.
Security Assessment
- Risk Level: Low — new contract addition, not modifying existing security-critical code
Notes:
.env.exampleincludes testnet addresses and a TX hash — verify no sensitive keys are included (looks clean on inspection).- Cargo.lock committed (good for reproducibility).
- This is a new feature contract; security review of the ZK verification logic itself would require a dedicated audit of the circuit constraints and verification paths.
Immunefi Pattern Check
- No matches against known vulnerability classes.
False Report Risk
- Low. New standalone contract.
Code Quality Notes
- Large PR — consider splitting Cargo.lock updates from contract logic in future PRs for easier review.
Status
No security concerns from a surface review. ZK circuit correctness would need a dedicated cryptographic audit beyond this automated scan.
crucible-burnt
left a comment
There was a problem hiding this comment.
🔍 Crucible Security Review
Summary
New zk-shuffle contract implementing Groth16 proof verification for shuffle-encrypt and decrypt operations via XION's on-chain ZK module. Delegates all cryptographic verification to the chain's gRPC endpoint.
Security Assessment
- Risk Level: Medium
Findings:
-
Hardcoded
vkey_id: 3(zkshuffle.rs): Bothverify_shuffle_proofandverify_decrypt_proofusevkey_id: 3hardcoded in theQueryVerifyRequest. This means:- If the vkey at ID 3 changes, all verifications break or become insecure
- There's no governance path to update this without redeploying
- The
verifier_nameparameter is passed butvkey_idoverrides it — unclear which takes precedence in the xion zk module
Action: Makevkey_idconfigurable via contract state (set at instantiation or via admin update).
-
No access control on execute: Anyone can call
VerifyShuffleProofandVerifyDecryptProof. While verification itself is read-only in effect (just increments a counter), the gas cost of proof verification is non-trivial. An attacker could submit many invalid proofs to waste gas. Consider rate limiting or requiring a deposit. -
Verification counter has no practical utility: The
VerificationStatecounter increments on success but isn't used for any logic. If this is just for metrics, consider using events/attributes only (already emitted) to save storage writes. -
Separate Cargo.lock: The contract has its own
Cargo.lock(1848 lines) separate from the workspace. This can lead to dependency version drift and makes auditing harder. Consider integrating into the workspace Cargo.lock. -
Typo in vkey filename:
vkeys/shuffle_encrupt.json— should beshuffle_encrypt.json. If the vkey is loaded at deploy time, this is cosmetic; if loaded at runtime by name, it could cause lookup failures.
Immunefi Pattern Check
- The Groth16 verification delegates to xion's ZK module — the contract itself doesn't perform any cryptographic operations. Attack surface is in the chain module, not the contract.
- No JWT, signer validation, or fee handling patterns present.
False Report Risk
- The hardcoded
vkey_id: 3could be flagged as a "configuration vulnerability" — it's more of a maintainability concern than a security one, since the chain module controls what vkey ID 3 maps to. - The permissionless verify calls could be flagged as "gas griefing" — standard for public verification contracts but worth documenting.
Code Quality Notes
- Clean separation between contract logic and ZK verification module
- Good use of xion's gRPC interface for proof verification
- The
SnarkJsProofformat conversion is correct for bn128/Groth16 - Missing unit tests for the contract itself (only shell scripts for integration testing)
.env.examplecontains testnet-specific values (code ID, tx hash, contract address) — should be placeholders
Status
Functional ZK verification contract with delegation to chain module. Key recommendations: (1) make vkey_id configurable, (2) fix the typo in vkey filename, (3) add unit tests, (4) consider workspace Cargo.lock integration.
crucible-burnt
left a comment
There was a problem hiding this comment.
🔍 Crucible Security Review
Summary
New CosmWasm contract (zk-shuffle) for verifying Groth16 ZK proofs for card shuffle and decryption operations, delegating verification to XION's x/zk module via gRPC queries.
Security Assessment
- Risk Level: Medium
1. Hardcoded vkey_id values (zkshuffle.rs:98, :152)
verify_shuffle_proofhardcodesvkey_id: 3andverify_decrypt_proofhardcodesvkey_id: 2. These IDs are environment-specific (testnet) and will break if vkeys are re-registered or on a different deployment. Thevkey_nameparameter is already passed — consider removingvkey_idor setting it to0to let the module resolve by name only.
2. No access control on execute messages (contract.rs)
VerifyShuffleProofandVerifyDecryptProofare callable by anyone. While verification itself is read-only at the module level, the contract writes state (increments counters). In production, this means anyone can inflate verification counters. If counters are used for any downstream logic, this could be exploitable.
3. Dependency on custom fork (Cargo.toml:49-52)
cosmos-sdk-protois sourced fromhttps://github.com/burnt-labs/cosmos-rustbranchfeat/xion-zk. This is expected for the customxion.v1.zkproto types, but the branch should be pinned to a specific commit hash (rev = "abc123") to prevent supply-chain issues from force-pushes.
4. .env.example contains testnet-specific data (.env.example)
- Includes real
TX_HASH,CODE_ID,CONTRACT_ADDRESSfrom testnet. Not a security risk, but could mislead users into interacting with stale deployments. Consider documenting this is testnet-only.
5. Typo in vkey filename (vkeys/shuffle_encrupt.json)
- Should be
shuffle_encrypt.json. If the vkey_name in the zk module isshuffle_encryptbut the file is namedshuffle_encrupt, this creates confusion.
Immunefi Pattern Check
- ✅ Proof verification is delegated to the chain's zk module — no custom cryptographic implementation
- ✅ No fee manipulation or gas abuse vectors (standard CosmWasm gas metering applies)
- ℹ️ The Groth16 proof format matches SnarkJS conventions (pi_a, pi_b, pi_c with affine point encoding)
⚠️ Thevkey_id+vkey_namedual-specification could theoretically cause the module to verify against a different key than intended if they point to different vkeys. Recommend using name-only resolution.
False Report Risk
- The lack of access control could invite reports about "unauthorized proof submission" — worth adding a comment that this is intentional for a testing/demo contract
- The hardcoded vkey_ids could invite reports about "wrong verification key" attacks
Code Quality Notes
verify_shuffle_proofandverify_decrypt_proofare nearly identical (~40 lines each) — should be refactored into a singleverify_proofhelper- No unit tests for contract logic (only serialization tests in
zkshuffle.rs) - Good: Contract properly uses
query_grpcfor module-level verification rather than implementing its own verifier Cargo.lockis committed (1848 lines) — standard for contract repos but increases review surface
Status
Functional for testnet testing. Before any mainnet deployment: fix the hardcoded vkey_ids (#1), pin the cosmos-sdk-proto dependency (#3), add access control or document its absence (#2), and add integration tests.
crucible-burnt
left a comment
There was a problem hiding this comment.
🔍 Crucible Security Review
Summary
New zk-shuffle CosmWasm contract for verifying Groth16 shuffle-encrypt and decrypt proofs via XION's module-level zk API. This is a testnet verification contract with no funds handling.
Security Assessment
- Risk Level: Medium
- No access control on execute:
execute_verify_shuffle_proofandexecute_verify_decrypt_proofhave no sender validation — anyone can call them. Since these only verify proofs and increment counters, this is likely intentional for a testing contract, but worth confirming. If this contract evolves to manage game state, access control will be critical. - Hardcoded vkey_id values:
vkey_id: 3for shuffle andvkey_id: 2for decrypt are hardcoded inzkshuffle.rs. If the zk module's key registry changes, these will silently break. Consider making them configurable via instantiate params or a governance-updatable config. - No input size validation:
public_inputs: Vec<Uint256>is unbounded. A malicious caller could submit an extremely large vector, potentially causing high gas consumption. The zk module query should reject mismatched input counts, but the serialization cost is borne by the caller before the query. Low risk given gas metering, but worth noting. - Code duplication:
verify_shuffle_proofandverify_decrypt_proofare nearly identical (~50 lines each), differing only invkey_id. Consider refactoring to a singleverify_prooffunction parameterized by vkey_id/name. - Committed
.env.example: Contains testnet contract address and code ID. Fine for testnet, just ensure no mainnet secrets leak into this pattern.
Immunefi Pattern Check
- No matches against known vulnerability classes. This contract doesn't handle funds, auth, or IBC — it's purely a ZK verification wrapper.
- No JWT, fee bypass, expiry, or signer validation patterns involved.
False Report Risk
- Low: The lack of access control could be flagged by auditors as a vulnerability, but for a proof-verification-only contract this is acceptable. Adding a brief comment in
contract.rsexplaining this is intentional (e.g., "Permissionless verification — no state mutation beyond counters") would preempt false reports. - The
Unauthorizederror variant inerror.rsis defined but never used — could confuse auditors into thinking auth was intended but not implemented. Either remove it or add a TODO comment.
Code Quality Notes
- Clean contract structure with proper separation (msg, state, types, contract, error, zkshuffle modules).
- Good unit tests for proof conversion and serialization.
Cargo.lockis committed (6086 lines) — standard for CosmWasm contracts but adds review noise.use serde_json as _;inzkshuffle.rsis unusual — appears to be ensuring the crate is linked. If it's just forserde_json::to_vec, the normal import suffices.Dockerfileconfigures SSH and git for private dependency fetching — thecosmos-sdk-protogit dependency points toburnt-labs/cosmos-ruston a feature branch. Ensure this branch is stable before mainnet deployment.- No integration tests with mock querier for the gRPC path.
Status
Solid initial implementation for a testnet ZK verification contract. Medium risk due to missing access control documentation and hardcoded vkey IDs. Recommended: add intent comments for permissionless access, refactor duplicated verification logic, and add configurable vkey management before any mainnet consideration. No blockers for testnet deployment.
crucible-burnt
left a comment
There was a problem hiding this comment.
🔍 Crucible Security Review
Summary
New zk-shuffle CosmWasm contract for on-chain ZK proof verification of card shuffle and decrypt operations via XION's zk module gRPC queries. Significant new contract with cryptographic operations.
Security Assessment
- Risk Level: Medium-High
Critical Findings:
-
Hardcoded
vkey_idvalues (zkshuffle.rs:101andzkshuffle.rs:165):vkey_id: 3for shuffle andvkey_id: 2for decrypt are hardcoded alongsidevkey_name. The XION zk module'sProofVerifyquery usesvkey_namewhen set (takes precedence overvkey_id), but if the module behavior changes orvkey_nameis empty, the wrong vkey could be used. These IDs should either be configurable viaInstantiateMsgor thevkey_idfield should be set to 0 when using name-based lookup. -
No access control on
ExecuteMsg(contract.rs:35-50): Anyone can callVerifyShuffleProofandVerifyDecryptProof. While proof verification itself is safe (read-only query), the state mutation (incrementing counters) is unprotected. If this is a test contract, acceptable — but for production, consider restricting callers or making it clear this is intentionally permissionless. -
.env.examplecontains real testnet data (.env.example:4-8): Contains actualTX_HASH,CODE_ID,CONTRACT_ADDRESSfor testnet. While testnet data isn't secret, it could cause confusion if someone copies these values expecting them to work. -
Typo in vkey filename:
vkeys/shuffle_encrupt.jsonshould beshuffle_encrypt.json— this typo will propagate into any tooling that references this file. -
serde_jsonunused import (zkshuffle.rs:12):use serde_json as _;is imported but only used within function bodies. This is fine for preventing unused crate warnings, but verify it's needed.
Immunefi Pattern Check
- State manipulation: Counter increments (
shuffle_verifications,decrypt_verifications) are protected by successful proof verification — no bypass path. - No fee bypass: Contract doesn't handle funds.
- Proto.Message singletons: Uses
cosmos_sdk_prototypes for gRPC queries — these are properly constructed per-call, no singleton reuse. - No matches against known Immunefi vulnerability classes.
False Report Risk
- The hardcoded
vkey_idvalues could be flagged as "wrong key used for verification" — document why these specific IDs are used. - The
UnauthorizedandNotSupportederror variants inerror.rsare defined but unused — auditors may flag dead code. AlreadyDecryptedandMissingCallbackerrors are also unused — suggests this is a stripped-down version of a larger contract. Document this to avoid confusion.
Code Quality Notes
- Good use of XION's native gRPC query for proof verification instead of re-implementing crypto in WASM.
- Well-structured proof conversion (
groth16_proof_to_snarkjs) with clear documentation of the affine point format. - Unit tests cover proof conversion and serialization but not the actual contract execute/query paths (would need mock querier).
Cargo.lockcommitted — appropriate for binary/contract crates.- Duplicate code:
verify_shuffle_proofandverify_decrypt_proofare nearly identical. Extract a commonverify_groth16_proofhelper.
Status
New contract with medium-high risk due to ZK verification surface and hardcoded vkey IDs. The hardcoded IDs are the primary concern — recommend making them configurable. No critical security vulnerabilities, but several code quality improvements recommended before production use.
Changes Summary