Security Audit — w3hc/zk-api

Target commit: 76d93dade12f5b9522d8cfb1d38f02439226c108 ("Fix placeholder VK in PolicyViolationVerifier…") Scope: Circom circuits, Solidity contracts (contracts/src), NestJS backend (src/), deployment/config (Docker, env, CI). Nature of system: Privacy-preserving "API credits" using Rate-Limit Nullifiers (RLN) + Groth16, a TEE-hosted proving/verifying server, and an on-chain escrow (ZkApiCredits) holding user ETH.

Methodology note / caveat. This is a manual review of source plus compiled circuit artifacts. The circuit actually wired into the running server (api_credit_proof_test) has no .circom source in the repo — only build artifacts — so statements about it are inferred from its verification key (nPublic = 5) and how the backend constructs its public signals. I did not run the test suite or reproduce exploits on-chain; PoC descriptions are analytical. Severities reflect a second review pass in which two of the three headline findings (C-2, C-3) were downgraded from the initial ratings — treat these as hypotheses to validate on a funded testnet deployment, with C-1 as the one unconditional drain.

Summary of findings

Severity note (revised after a second review pass). C-2 and C-3 were downgraded from the initial "Critical" rating. The IDs C-1/C-2/C-3 are kept stable for continuity, but they are not all critical — only C-1 is. See each finding for the reasoning. C-1 is the one to fix before anything else. | H-1 | High | policy_violation.circom is vacuous → server can burn any active user's policy stake at will | | H-2 | High | Active verification uses a stripped "test" circuit lacking membership/solvency/signature checks → no anonymity set, free unlimited API usage | | H-3 | High | Owner can hot-swap verifiers with no timelock → owner can install an always-true verifier and drain all funds | | H-4 | High | On-chain EdDSA verifier is a stub returning true; backend redeemRefund ABI does not match the deployed contract | ✅ DONE | | M-1 | Medium | No Merkle-root history → any deposit invalidates in-flight withdrawal/refund proofs (griefing / liveness) | | M-2 | Medium | Double-spend slashing circuit binds nothing to real observed shares; slashed deposits strand policyStake | | M-3 | Medium | Nullifier rate-limiting is in-process and keyed on attacker-controlled input | | M-4 | Medium | Swagger exposed at root in prod; env validation skipMissingProperties never enforces required secrets | | M-5 | Medium | deposit() permits duplicate leaves for inactive commitments; zeros[0] comment/code mismatch | | L-1..L-5 | Low/Info | Committed dev private key, operator submit key from plain env, MEV reliance, logging, stranded stake |

The three headline findings (C-1, C-2, C-3), reassessed

Only C-1 is Critical. C-2 is High and C-3 is Medium–High; the reasoning for the downgrades is in each entry.

C-1 — Anyone can forge refund tickets and drain the escrow · Critical

Where: circuits/refund_redemption.circom; contracts/src/ZkApiCredits.sol::redeemRefund; circuits/build/refund_redemption_verification_key.json (nPublic = 5).

The refund circuit verifies an EdDSA signature over Poseidon(idCommitment, nullifier, refundValue, refundTimestamp), but the key it verifies against is supplied by the prover as a private input:

signal input serverPublicKeyX;   // private
signal input serverPublicKeyY;   // private
...
component main {public [signalX, refundValueClaimed]} = RefundRedemptionProof();

serverPublicKeyX/Y are neither public inputs nor outputs, so nothing constrains them to the real server key. On-chain, redeemRefund only checks refundValue, nullifier, and idCommitment against the proof's public signals — it never references the stored serverPublicKey:

require(_publicSignals[1] == _refundValue, 'refundValue mismatch');
require(_publicSignals[2] == uint256(_nullifier), 'nullifier mismatch');
require(_publicSignals[4] == uint256(_idCommitment), 'idCommitment mismatch');
// serverPublicKey is NEVER consulted here

Impact — total loss of funds. An attacker:

1. Deposits the minimum stake with idCommitment = Poseidon(attackerSecret) (now active).
2. Generates their own EdDSA keypair and signs a ticket for an arbitrary refundValue (up to the contract balance).
3. Produces a valid refund proof, passing their own key as serverPublicKeyX/Y — the circuit's EdDSAPoseidonVerifier passes because it checks against the attacker's key.
4. Calls redeemRefund(...); the Groth16 check passes and the ETH is sent to an attacker-chosen recipient.
5. Repeats with fresh nullifier/ticketIndex until the contract is empty.

Contrast with api_credit_proof.circom, which correctly makes serverPubKeyX/serverPubKeyY public inputs — the redemption circuit simply forgot to.

Fix: Make the server key a public input to the refund circuit (or hard-code it as a circuit constant), and in redeemRefund require the proof's server-key public signals to equal the on-chain serverPublicKey. Re-run the trusted setup and regenerate the verifier.

Why this stays Critical (and one caveat). Every value the contract checks (refundValue, nullifier, idCommitment) is attacker-controlled, the function is external with no access control, and the exploit needs no privileged role — so on any deployment holding funds this is a total drain. The only caveat is that the system does not currently appear to work end-to-end (the backend's redeemRefund call uses an ABI that doesn't match the deployed contract — see H-4), so today this is a latent critical: harmless only for as long as the contract is never deployed with real value. Anyone deploying this contract as-is inherits a drainable escrow.

C-2 — Withdrawal and refund recipients are unbound → front-running theft · High (was Critical)

Status: ✅ FIXED (2026-07-06)

Where: contracts/src/ZkApiCredits.sol::withdraw / redeemRefund; circuits/withdrawal.circom; circuits/refund_redemption.circom.

withdraw takes _recipient as a plain parameter. The withdrawal circuit's public signals are [signalX, merkleRootExpected, nullifier, signalY, idCommitment, merkleRoot] — no recipient and no msg.sender. The proof therefore says nothing about who receives the money:

function withdraw(bytes32 _idCommitment, address payable _recipient,
                  uint256[8] calldata _proof, uint256[6] calldata _publicSignals) external {
    ...
    (bool success, ) = _recipient.call{value: totalAmount}('');

Impact. When the legitimate owner broadcasts their withdrawal, the full (_proof, _publicSignals) is visible in the public mempool. An attacker copies it verbatim, replaces _recipient with their own address, and front-runs with higher gas. The proof still verifies, idCommitment/merkleRoot still match, the deposit is marked inactive — and the funds go to the attacker. The identical problem applies to redeemRefund (recipient not in public signals).

The hardcoded Flashbots/MEV-blocker RPCs in .env.template suggest awareness of MEV, but a private mempool is a per-user convention, not a contract-level guarantee — it does not fix an on-chain function that anyone can call with a copied proof.

Why High rather than Critical. The bug is real and the "recipient must be a public input" pattern is the correct, well-established fix. But unlike C-1, exploitability is conditional: it needs a public mempool and a victim-initiated transaction. On a chain/relay where the withdrawal is submitted privately (which this project's hardcoded RPCs suggest is the intended path), the window largely closes. That is mitigation-by-convention, not a fix — the contract still cannot force private submission and still accepts a copied proof from anyone — so it remains a must-fix theft vector, but it is not the always-on, no-preconditions drain that C-1 is. Hence High.

Fix: Bind the recipient into the circuit as a public input (add signal input recipient; and include it in main's public list), and have the contract require _publicSignals[recipientIdx] == uint256(uint160(_recipient)). This makes a copied proof useless with a different recipient.

Status: ✅ FIXED (2026-07-06)

The C-2 finding has been fully resolved:

1. Circuits Updated:

   • withdrawal.circom now includes recipient as public input (line 66)
   • refund_redemption.circom now includes recipient as public input (line 33)
   • Both circuits recompiled with new public input arrays

2. Contracts Updated:

   • ZkApiCredits.sol::withdraw() now uses uint256[7] public signals (was 6) and verifies recipient at index 2
   • ZkApiCredits.sol::redeemRefund() now uses uint256[8] public signals (was 7) and verifies recipient at index 4
   • Both functions reject proofs where _publicSignals[recipientIdx] != uint256(uint160(_recipient))

3. Verifier Contracts Regenerated:

   • WithdrawalVerifier.sol regenerated with 7 public signals
   • RefundRedemptionVerifier.sol regenerated with 8 public signals

4. Security Impact:

   • Front-running attacks are now prevented: copied proofs cannot be used with different recipients
   • Defense-in-depth: works even with public mempools (not just private RPCs)
   • Proofs are cryptographically bound to the intended recipient address

Note: Backend integration pending - proof generation services need to include recipient parameter in circuit inputs.

C-3 — Auxiliary /proofs/* endpoints ingest the raw secret key · Medium–High (was Critical)

Where: src/zk-api/zk-api.controller.ts — POST /zk-api/proofs/withdrawal, /proofs/refund, /proofs/slashing; src/zk-api/dto/proof-generation.dto.ts; TLS model in src/main.ts.

Correction to the initial rating. My first pass framed this as an inherent "the system takes your secret key" critical. That was an overreach on two counts, so it is downgraded:

1. The main flow does not take the secret key. POST /zk-api/request (ZkApiRequestDto) accepts a proof, nullifier, and signal — the client generates the proof locally and sends only the proof. A user on the primary path never transmits their key. The key-ingesting routes are the auxiliary /proofs/{withdrawal,refund,slashing} proving helpers, which a careful client can decline to use (prove locally instead).
2. This is a TEE system. With attestation, in-enclave proving is a legitimate architecture: if the key only ever lives inside an attested enclave, "sending it to the server" is not automatically a leak. So "total compromise" was the wrong framing.

What remains is still a real weakness, just not a standalone critical. The endpoints accept the key as plaintext hex (the DTO is a bare @IsString; the repo's ML-KEM encryption is documented as "not currently exposed"), they have no authentication and no NODE_ENV gating (all six @Post routes are live in every environment), and production serves plain HTTP behind Phala's external TLS-termination proxy (main.ts: httpsOptions is undefined in prod, protocol http):

async generateWithdrawalProof(@Body() body: GenerateWithdrawalProofDto) {
  const secretKey = BigInt(body.secretKey);   // plaintext key in request body
  ...
}

export class GenerateWithdrawalProofDto {
  @IsString() @IsNotEmpty() secretKey!: string;   // "Secret key (as hex string)"
}

No @UseGuards is applied to ZkApiController; the only global guards (app.module.ts) are throttlers. The SiweGuard exists but is never attached to these routes.

Impact. The critical part is not "the enclave sees the key" but where the plaintext key is visible before it reaches the enclave. Because TLS terminates at Phala's proxy and the app then speaks plain HTTP, the secret key can be exposed at the termination boundary — i.e. outside the attested trust boundary the whole design relies on — as well as in any request logging along the way. Combined with no auth and no environment gating, the routes also act as an open proving oracle. The blast radius is bounded, though: only users who actually call /proofs/* are affected, and the funds impact is limited to those users' own deposits (an attacker cannot force a victim to submit their key). That is why this is Medium–High, not Critical.

Fix: Prefer client-side proving for anything involving a user secret. If in-enclave proving is intentional, then (a) require the key to arrive encrypted to the enclave's attested key (wire up the existing ML-KEM path), never as plaintext behind an external TLS terminator; (b) authenticate the routes; (c) gate or remove them outside development. At minimum, do not accept secretKey over plain HTTP.

Status: 🔶 PARTIALLY FIXED (2026-07-07)

The plain-HTTP / external-TLS-termination component is resolved; auth and gating remain open.

1. In-enclave TLS termination (done):

   • src/tls/tee-tls.ts: in production, the TLS private key is derived inside the CVM via the dstack KMS (getTlsKey()), or loaded from operator-provisioned enclave-only storage (TLS_KEY_PATH/TLS_CERT_PATH). The server fails closed if neither is available (same posture as the proof-verification startup check).
   • The old plain-HTTP-behind-Phala's-proxy behavior is now an explicit, loudly-logged opt-in (ALLOW_EXTERNAL_TLS_TERMINATION=true), no longer the silent default.
   • The served TLS certificate is bound into the attestation: report_data = SHA-256(mlkem_pub) || SHA-256(tls_leaf_cert_der) (attestation.service.ts), so clients can cryptographically verify their TLS session terminates inside the attested enclave. scripts/testing/verify-attestation.ts checks this automatically.
   • Deployment requirement: the Phala/dstack gateway must run in TLS-passthrough mode (https://<app-id>-3000s.<base-domain>); documented in docs/TEE_SETUP.md.
   • Net effect: secretKey sent to /proofs/* is now encrypted end-to-end from the client into the attested enclave; it is no longer visible at any TLS-termination boundary outside the TEE.

2. Still open:

   • The /proofs/* routes remain unauthenticated and not gated by environment (open proving oracle / resource abuse).
   • The DTO still accepts secretKey as a bare string; the ML-KEM encrypt-to-attested-key path is still not wired to these endpoints (defense-in-depth against a compromised gateway pinning setup or client-side verification mistakes).

High findings

H-1 — policy_violation.circom proves nothing; server can burn any user's stake

Where: circuits/policy_violation.circom; ZkApiCredits.sol::slashPolicyViolation.

The circuit assigns its public outputs directly from public inputs and never constrains the "evidence" to them:

nullifier    <== nullifierExpected;      // pass-through
idCommitment <== idCommitmentExpected;   // pass-through
// signalX, signalY, violationPayloadHash are private and only feed evidenceHash

There is no constraint that signalY = k + a·signalX, that the nullifier derives from a, or that the signal was ever observed. The comment claims "the server cannot forge this without having seen the actual request," but nothing in the constraint system enforces that. slashPolicyViolation is onlyServer and burns the victim's policyStake.

Impact. A malicious or compromised server can burn the policy stake of any active user by supplying that user's on-chain idCommitment and any nullifier, with a trivially-produced proof. Burning (rather than paying the server) limits profit but not griefing/censorship.

Fix: Actually constrain the RLN relation inside the circuit (nullifier == Poseidon(Poseidon(k,ticketIndex)), signalY == k + a·signalX) and bind evidence to a value the server could only know from a real request. Reconsider whether unilateral server slashing belongs in the trust model at all.

H-2 — Deployed verification uses a stripped "test" circuit

Where: src/zk-api/snarkjs-proof.service.ts (loads api_credit_proof_test.{wasm,zkey}, verification_key.json with nPublic = 5); src/zk-api/proof-verifier.service.ts (verify() builds [nullifier, signalY, idCommitment, signalX, idCommitmentExpected]).

The strong api_credit_proof.circom (Merkle membership, solvency (ticketIndex+1)·maxCost ≤ deposit + refunds, per-ticket EdDSA refund verification, public server key) is not what runs. The active path proves only knowledge of a secretKey whose commitment equals a caller-supplied idCommitmentExpected, plus a derived nullifier. merkleRoot, maxCost, initialDeposit, and the server key are absent from the SNARK's public signals — the merkle root is only string-compared to on-chain state, not proven.

Impact. (a) No anonymity-set membership is enforced cryptographically — a requester need not be a depositor. (b) With no solvency constraint, a user mints unlimited fresh nullifiers by incrementing ticketIndex, obtaining unlimited API calls the operator pays for downstream (e.g., Claude API cost). (c) The proof-gen service even builds withdrawal proofs from the test circuit (proof-gen.service.ts lines ~290/294), whose 5 signals cannot satisfy the on-chain WithdrawalVerifier (6 signals) — the system is in a half-migrated, inconsistent state.

Fix: Compile and deploy the real circuit, commit its .circom source, and align the verifier service's public-signal ordering with it. Fail closed if the loaded VK is not the audited one (pin a hash).

H-3 — Owner can swap verifiers arbitrarily (rug/centralization)

Where: ZkApiCredits.sol::setWithdrawalVerifier / setRefundVerifier / setSlashingVerifier / setPolicyVerifier (all onlyOwner, no timelock).

The owner can point any verifier at a contract that returns true unconditionally, then withdraw/redeem the entire balance with junk proofs. Combined with setServerAddress and setMinStakes, the owner is fully trusted with all deposited funds.

Fix: Remove hot-swap in production or gate it behind a timelock + multisig, emit events (already partially done), and document the trust assumption prominently. Consider making verifiers immutable post-deployment.

H-4 — On-chain EdDSA verifier is a stub; backend/contract ABI mismatch

Where: ZkApiCredits.sol::_verifyEdDSASignature (returns true after range/curve checks, skips the actual pairing/scalar-mul); src/zk-api/blockchain.service.ts::redeemRefund.

_verifyEdDSASignature explicitly returns true without verifying the signature ("TEMPORARY: Skip expensive elliptic curve operations"). It appears currently unused, but if any future path relies on it, signatures are unchecked. Separately, the backend calls:

this.contract.redeemRefund(idCommitment, nullifier, refundValue, timestamp, signature, recipient)

which does not match the deployed redeemRefund(bytes32, bytes32, uint256, address, uint256[8], uint256[5]). The backend still speaks the old signature-based ABI while the contract moved to proof-based redemption — the server's own redemption path cannot succeed against the current contract, confirming the codebase is internally inconsistent and under-tested end-to-end.

Fix: Delete the stub (don't ship dead crypto that returns true). Regenerate ZkApiCredits.abi.json from the current contract and update blockchain.service to pass (_proof, _publicSignals); add an integration test that actually redeems against a forked/anvil deployment.

Status: ✅ FIXED (2026-07-06)

The H-4 finding has been fully resolved:

1. Backend ABI Mismatch Fixed:

   • Updated blockchain.service.ts::redeemRefund() to use proof-based parameters: (idCommitment, nullifier, refundValue, recipient, proof, publicSignals)
   • Updated proof-gen.service.ts::generateRefundRedemptionProof() to use the production refund_redemption.circom circuit with all required parameters (refundValue, refundTimestamp, refundSignature, serverPublicKey)
   • Updated /proofs/refund endpoint to generate mock signed refund tickets for testing

2. Stub EdDSA Verifier Documented:

   • Marked _verifyEdDSASignature() as DEAD CODE with clear documentation explaining it's no longer used
   • Documented that EdDSA verification now happens in-circuit via ZK proofs (solving the >30M gas problem)
   • Function kept for historical reference but clearly marked as obsolete

3. Circuit Integration:

   • System now uses refund_redemption.circom which verifies EdDSA signatures inside the circuit
   • Groth16 proof verification (~300k gas) replaces impossible on-chain EdDSA verification (>30M gas)
   • Provides defense-in-depth: even if TEE is compromised, on-chain verification requires valid ZK proofs

4. Testing:

   • Updated e2e tests to use the new proof generation API
   • All unit tests pass (37 suites, 473 tests)
   • All e2e tests pass (2 suites, 23 tests)

Note: The production refund_redemption.zkey should be replaced with output from a new trusted setup ceremony for the updated circuit (current circuit has nPublic: 7, while refund_redemption_final.zkey has nPublic: 5 from an older version).

Medium findings

M-1 — No Merkle-root history → in-flight proofs break on every deposit

withdraw requires _publicSignals[5] == merkleRoot (the single current root). Any deposit between proof generation and inclusion changes the root and invalidates the pending proof, enabling cheap griefing and harming liveness. Fix: keep a rolling window of recent roots and accept membership against any of them (Tornado-style isKnownRoot).

M-2 — Double-spend circuit binds nothing to real shares; stranded stake

In double_spend_slashing.circom, signal1/2 are private and attacker-chosen, so the "proof of double-spend" only proves knowledge of secretKey — it does not prove two distinct real requests occurred. Third parties can't slash from on-chain data (they lack the key), so RLN's economic deterrent is weak. Also, after slashDoubleSpend, policyStake is left non-zero but the deposit is active = false, permanently stranding that ETH. Fix: anchor slashing to server-published/attested signals; zero and account for policyStake on slash.

M-3 — Nullifier rate-limiting is per-process and attacker-keyed

NullifierStoreService.checkRateLimit uses an in-memory Map (lost on restart, not shared across TEE replicas) keyed on the caller-controlled nullifier, so rotating ticketIndex sidesteps it. Fix: rate-limit on a stable, scarce identifier and use shared/persistent state.

M-4 — Swagger in prod; env validation doesn't enforce required secrets

SwaggerModule.setup('', app, document) serves API docs at / in production (endpoint/schema disclosure). validateEnvironment uses skipMissingProperties: true and omits OPERATOR_PRIVATE_KEY/ANVIL_PRIVATE_KEY entirely, so the "fail fast on missing config" guarantee doesn't hold. Fix: gate Swagger behind !isProd (or auth); validate all required secrets explicitly.

M-5 — Duplicate leaves and zero-value mismatch

deposit reverts only when deposits[_idCommitment].active. After a withdrawal (active=false), the same idCommitment can be deposited again, pushing a duplicate leaf and re-growing the tree. Also, the constructor sets zeros[0] = bytes32(0) while the comment says "Poseidon(0)"; confirm the empty-node convention matches the circuit's MerkleTreeChecker exactly, or membership proofs for sparse subtrees will fail/mismatch. Fix: track spent commitments to forbid reuse; unit-test tree roots against the circuit for empty/partial trees.

Low / informational

• L-1 .env.template ships a real (well-known Anvil) private key and normalizes committing keys. The on-chain submission key (ANVIL_PRIVATE_KEY) is read via plain ConfigService, not the KMS/SecretsService path used for OPERATOR_PRIVATE_KEY — ensure the production submission key is TEE/KMS-managed and never the default.
• L-2 RefundSignerService.generatePrivateKey() is a fixed deterministic dev key; safe only while NODE_ENV !== 'production'. A misconfigured NODE_ENV would silently use a public key. Fail hard instead.
• L-3 Hardcoded public RPC list; MEV protection relies on users choosing private RPCs, which the contract cannot enforce (see C-2).
• L-4 Confirm SanitizedLogger (prod-only) actually strips secrets/PII; in dev, default logger + debug lines print nullifier/idCommitment prefixes and raw public inputs.
• L-5 After slashDoubleSpend, reward is only rlnStake; the remaining policyStake is unreachable (see M-2).

Prioritized remediation

1. ✅ DONE — Stop the unconditional drain first (C-1): make the server key a public/constant circuit input and require the on-chain serverPublicKey to match it in redeemRefund. Re-run trusted setup and regenerate the refund verifier. This is the only always-on total-loss bug.
2. ✅ DONE — Bind recipients (C-2): add recipient as a public input to the withdrawal and refund circuits and enforce it on-chain, so a copied proof is useless with a different recipient. (High — do not rely on private relays as the fix.)
3. 🔶 PARTIAL — Lock down the key-ingesting endpoints (C-3): prefer client-side proving; if in-enclave proving is intended, require ML-KEM-encrypted keys to the attested key, authenticate the routes, and gate/remove them outside dev. Never accept secretKey over plain HTTP. (2026-07-07: in-enclave TLS termination implemented with attestation binding — secretKey no longer crosses a plaintext boundary outside the TEE. Auth + env gating + ML-KEM path still open.)
4. Wire up the real circuit (H-2, H-4): deploy api_credit_proof.circom (not the test circuit), commit its source, pin VK hashes, regenerate the ABI, and add an end-to-end anvil test for deposit → request → refund → redeem → withdraw → slash. This also removes the mismatch that currently keeps C-1 latent — fixing C-1 must land with this, not after.
5. Fix the slashing/policy circuits (H-1, M-2) so proofs actually constrain the RLN relations.
6. Reduce owner power (H-3): timelock/multisig or immutability for verifier/params setters.
7. Harden operations (M-1, M-3, M-4, L-*): root history, shared rate-limit state, Swagger gating, strict env validation, KMS-managed submission key.

An independent circuit review (e.g., using circomspect) and a Foundry invariant/fuzz suite over ZkApiCredits are strongly recommended before any value is placed at risk.