Fix Citadel security audit findings by HDauven · Pull Request #137 · dusk-network/citadel

HDauven · 2026-07-10T18:35:45Z

Summary

Harden the license ZK circuit point-validity checks against torsion-shifted witness points.
Validate public keys and stealth addresses before DH, signature, and proof use.
Pin wallet verification to trusted issuer/policy inputs and contract verifier/circuit metadata; remove argv password input and document the env/prompt path.
Regenerate circuit setup material in contract/build.rs, embed the verifier from OUT_DIR, and test metadata/verifier consistency.
Keep session-capacity policy out of the base contract and specify that uncommitted cookie binding data does not secure a bearer cookie.
Document the Dusk VM panic-as-revert model for contract entrypoints and the fail-fast error model for required build artifacts.

Verification

cargo fmt --all -- --check
cargo test -p zk-citadel --release --features zk
cargo clippy -p zk-citadel --all-targets --features zk --release -- -D warnings
cargo test -p zk-citadel-wallet --release
cargo clippy -p zk-citadel-wallet --all-targets --release -- -D warnings
make contract
make test-contract
cargo clippy -p license-contract --all-targets --release -- -D warnings
GPT 5.5 xhigh final-head red-team at 0958b47: no PR-blocking security or correctness findings.
controlecentrum review-pr dusk-network/citadel#137 --mode deep --blind --head-sha 0958b47 --budget-minutes 20 with GPT 5.5 xhigh: final verdict COMMENT, 0 validated findings.

Notes

Session lifecycle, state capacity, and replacing the linear session map remain separate deployment/design work before production.
Public GitHub issue creation for the remaining security findings is pending explicit disclosure approval.

xevisalle

I left several comments. In general I see several protocol changes that should be explained. Also, I think the specification needs to be updated accordingly before touching the code.

xevisalle · 2026-07-13T22:31:54Z

    const MERKLE_ARITY: usize = 4;
    const DEPTH: usize = 16; // the depth of LicenseCircuit's Merkle tree
    const ROOT_HISTORY_SIZE: usize = 8;
+    const MAX_SESSIONS: usize = 1 << DEPTH;


I don't get this. Why is 2^16 the max number of sessions?

I don't get this. Why is 2^16 the max number of sessions?

This was done as a defensive state bound, but tying it to the Merkle depth was arbitrary and way too constraining. I agree it should be removed

xevisalle · 2026-07-13T22:36:02Z

 const CIRCUIT_MARKER_PATH: &str = "../target/license-circuit-build-id";
-const CIRCUIT_BUILD_ID: &str = "zk-citadel-license-circuit";
+const OUT_DIR_VERIFIER_FILENAME: &str = "license_verifier";
+const CIRCUIT_BUILD_DOMAIN: &[u8] = b"zk-citadel-license-circuit-v2";


why v2?

We don't really need to version the build ID as is. The fingerprint is opaque and checked by exact equality, and we do not support both the old static marker and this source derived format. I'll replace v2 with a descriptive source fingerprint domain.

xevisalle · 2026-07-13T22:37:38Z

+];

 #[tokio::main]
 async fn main() {


what are the advantages of all these changes to the contract built?

what are the advantages of all these changes to the contract built?

Prior setup files could be reused after circuit source changes because the marker was static. These changes make Cargo track the circuit sources, regenerating the prover/verifier together, embedding the verifier produced by the build and expose the fingerprints so wallets can reject mismatched artifacts.

It gives stronger guarantees

xevisalle · 2026-07-13T22:42:22Z

    /// Optional exact root freshness check for this policy.
    pub expected_root: Option<BlsScalar>,
+    /// Optional exact binding data expected by this policy.
+    pub expected_binding_data: Option<[BlsScalar; 4]>,


what's this?

what's this?

It was intended to enforce profile defined accounts or channel binding. binding_data is currently not committed though by the proof or session transcript, so checking it alone does not provide any cryptographic binding. Question is if we want to do it or not, as it was flagged as a risk area

xevisalle · 2026-07-13T22:46:04Z

-    assert_not_identity(composer, eight_p);
+    // The cofactor map [8] has the prime-order subgroup as its image. Proving
+    // point = [8]Q for an on-curve Q excludes hidden torsion components.
+    let inv_eight = JubJubScalar::from(8u64).invert().unwrap();


isn't this underconstrained? and, what's the reason of the added complexity?

isn't this underconstrained? and, what's the reason of the added complexity?

This was added because the old check didn't catch all invalid points. It rejected points that were fully low order, but not a valid point with a low-order part added. The test covers that case.

As I understand it, Q isn't underconstrained because the circuit checks that it is on the curve and that [8]Q = P. But I'm not attached to this implementation. If there is a simpler or more standard way to do the subgroup check here, I'd rather use that. This came about from the audit by the way

Ok I'll think about it

HDauven requested a review from xevisalle July 10, 2026 18:35

Fix Citadel security audit findings

51c201e

HDauven force-pushed the security/citadel-audit-fixes branch from be3ddc1 to 51c201e Compare July 10, 2026 18:44

xevisalle reviewed Jul 13, 2026

View reviewed changes

HDauven added 2 commits July 14, 2026 02:13

Address security review feedback

7e264fe

Make subgroup synthesis fail closed

0958b47

This was referenced Jul 14, 2026

Define session registry lifecycle and state budget #141

Open

Decide whether session cookies need authenticated service binding data #143

Open

Uh oh!

Conversation

HDauven commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Notes

Uh oh!

xevisalle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

HDauven commented Jul 10, 2026 •

edited

Loading