Skip to content

private-bond/custom-utxo: committed Verifier.sol cannot verify the committed circuit #79

Description

@Himess

circuits/src/main.nr emits 8 public inputs. contracts/src/Verifier.sol accepts 5 (:7 21 minus :267 16). Nothing in the repo can verify a proof from this circuit, and the contract's three proof-bearing functions are written for three different array shapes, none of which is 8.

This edit replaces my original body: the first version reported the arity mismatch alone, and its fix list was incomplete. Sections 5 and 6 are new, and fix item 3 changed.


1. The measurement

I generated a verifier from the current circuit and ran both against the same real proof, in the repo's own Foundry setup, reading circuits/target/ through the fs_permissions entry at foundry.toml:5:

real proof: 8256 bytes, 8 public inputs
  [5] 0x...70dbd880   1893456000   input_maturity_date
  [6] 0x...70dbd880   1893456000   output_maturity_date
  [7] 0x...00                      redeem flag

generated verifier   ->  verify() returned TRUE
committed verifier   ->  reverted

The circuit, the witness and the prover are all fine. Only Verifier.sol is wrong.

The two verifiers differ in one constant:

generated committed
N / LOG_N 16384 / 14 16384 / 14
NUMBER_OF_PUBLIC_INPUTS 24 21

Same gate count, three extra public values. That is a visibility difference, not a different circuit.

One caveat so it is not mistaken for arity evidence: the committed verifier rejects the real proof with ProofLengthWrong() (8256 bytes vs the 507 × 32 at Verifier.sol:1680). That is a bb encoding difference between generations, not the arity. Every arity measurement below uses a correctly-sized dummy proof so the length gate at :1691 is cleared before the arity check at :1698 is reached.

2. It was never in sync

Verifier.sol has one revision, at 7ab2006. main.nr has two, with identical pub markers at both. They were committed together. 0fc2aa5 later moved input_values and output_values from Field to u64, which adds range constraints, so the key is stale on that count too.

3. Not confined to tests

MockVerifier (PrivateBond.t.sol:8-12) returns true for everything and is why the suite is green; the real HonkVerifier is imported at :6 and never deployed in a test. But script/PrivateBond.s.sol:24 does deploy it, and the wallet calls the contract over RPC. Of the four contract calls the wallet makes, mintBatch (main.rs:212), transfer (:548), atomicSwap (:944) and burn (:1241), only mintBatch can succeed, because it carries no proof.

4. Three functions, three array shapes

transfer:36-41 and burn:119-124 are byte-for-byte the same five assignments:

36:        bytes32[] memory publicInputs = new bytes32[](5);
37:        publicInputs[0] = root;
38:        publicInputs[1] = nullifiersIn[0];
39:        publicInputs[2] = nullifiersIn[1];
40:        publicInputs[3] = commitmentsOut[0];
41:        publicInputs[4] = commitmentsOut[1];

atomicSwap does not build an array — it receives one (:142, :145) and reads slots [0] through [3]. What those slots mean is fixed by its callers, and they agree: all nine new bytes32[](4) sites in PrivateBond.t.sol, and the wallet at main.rs:946-948, build [root, one nullifier, one commitment, maturity]. Against that layout atomicSwap is internally consistent:147 reading [3] as a maturity date and :166 pushing [2] as a commitment are both correct. I want to be precise about this because it is not an index-shift bug.

The problem is that nothing produces a four-element vector:

artifact app-level public inputs
circuits/src/main.nr 8:48, :49, :50, :56, :65, :68
contracts/src/Verifier.sol 5:7 minus :267
circuits/Prover.toml 5
transfer / burn 5
atomicSwap and all its callers 4

Measured, with a correctly-sized dummy proof so the length gate is cleared first:

atomicSwap, 4 elements  ->  PublicInputsLengthWrong()
atomicSwap, 5 elements, identical bytes  ->  ShpleminiFailed()

The circuit emits two nullifiers and two commitments per proof. The four-element layout has room for one of each, plus a maturity date the five-input verifier does not expose at all.

5. Reindexing alone will not fix atomicSwap

Because the function is written for one nullifier and one commitment per side, migrating the array to the circuit's eight-element order changes what the body has to do, not just which indices it reads. :157-158 read a single nullifier per side, :163-164 mark it, and :166-167 push a single value:

157:        bytes32 nullifierA = publicInputsA[1];
158:        bytes32 nullifierB = publicInputsB[1];
163:        nullifiers[nullifierA] = true;
164:        nullifiers[nullifierB] = true;
166:        commitments.push(publicInputsA[2]);
167:        commitments.push(publicInputsB[2]);

Under the eight-element order [1] and [2] are both nullifiers and [3] and [4] are both output commitments. So changing only the indices would leave the second input note unmarked in nullifiers, never insert the second output commitment, and push what is now nullifiersIn[1] into the leaf set — where commitments (:14) is rebuilt into a root at :170 and accepted into knownRoots at :171.

transfer already does this correctly and is the model: :32-33 check both nullifiers, :45-46 mark both, :48-49 push both commitments.

This part is testable today with the existing MockVerifier, with no proving and no toolchain, by asserting on nullifiers and commitments after a call. I have not written that test.

6. The same move decides whether two gates in burn end up proof-bound

burn declares inputMaturityDate and isRedeem as standalone parameters and checks them, but never puts them in the array it verifies:

106:        bytes32 inputMaturityDate,
107:        bytes32 isRedeem
108:    ) external {
113:        require(block.timestamp >= uint256(inputMaturityDate), "Bond not at maturity yet");
116:        uint256 isRedeemUint = uint256(isRedeem);
117:        require(isRedeemUint == 1, "Output notes must have 0 value for redemption");
119:        bytes32[] memory publicInputs = new bytes32[](5);

:119-124 builds from root, the two nullifiers and the two commitments only. Neither checked value is covered by the proof: inputMaturityDate = 0 satisfies :113 at any block, and isRedeem = 1 satisfies :117 alongside any valid transfer proof, whose outputs sum to the full input rather than zero. burn has no access modifier.

Today that is latent rather than reachable, for the reason this issue is about — no proof completes the function either way. It becomes live the moment the verifier is regenerated, and the array change is what closes it. Both values are already public inputs of the circuit — main.nr:56 for the maturity date, and the returned flag at :68, computed at :107 — so under the eight-element order they land at [5] and [7]. If the migration brings the array to eight but keeps checking the standalone bytes32 parameters, the gates stay open with a working verifier in place.

SPEC.md:148 already lists maturity among the public inputs, which is what reading [5] would make true.

What an issuer settles off-chain against a burn is outside the contract and I did not trace it.


Fix

1. Add scripts/generate-verifiers.sh. Six PoCs have one, and every circuit/verifier pair in those six agrees with its circuit's public-input count: sixteen pairs, from 4 inputs to 55. This is the only pair that does not. a8f3aad already documents the failure mode in tee_swap/README.md.

2. Regenerate Verifier.sol and run it against a real proof once. That would be new for this repo rather than a restoration: none of the six PoCs currently tests a real verifier with a real proof, which is also why no suite here could have caught this.

3. Bring all three call sites to eight elements in circuit order, and change the bodies to match. The ordering convention is already written down in ClaimContract.sol:9-10:

Indexes into the claim circuit's 10 public inputs. Order MUST match circuits/claim/src/main.nr main(...) parameter order.

and all six PoCs follow it element for element. Concretely:

  • transfer and burn — extend the array from five to eight; the existing two-nullifier, two-commitment handling is already correct.
  • atomicSwap — extend to eight and mark two nullifiers and push two commitments per side, as transfer:32-33, :45-46 and :48-49 do. The maturity check at :147-148 then reads [5].
  • burn — check maturity against publicInputs[5] and the redeem flag against publicInputs[7] rather than the standalone parameters at :106-107.

The only open question I see is whether the redeem flag should be asserted as well as passed through, and that one is yours.

4. Pin the Noir version. main.nr does not compile with nargo 1.0.0-beta.25 (u1 was removed); beta.19 with bb 4.0.0-nightly.20260120 does.


Happy to send this as a PR following the same pattern as the other six: the generation script, the regenerated verifier, the three call sites in circuit order with the body changes above, and tests that build the arrays. Two things worth knowing before you say yes. transfer has no maturity source today, so it would need a parameter and the wallet would change with it. And transfer is the one function the twelve tests never call, though the wallet uses it most (main.rs:548), so I would add coverage for it rather than rely on the suite staying green.

Run locally against 3a31a9e. Scratch test files created, run and deleted; nothing pushed.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions