Fix balance width - #831
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
| await cft.register(); | ||
| }); | ||
|
|
||
| it('spends a balance accumulated past the per-transfer bound', async () => { |
There was a problem hiding this comment.
🔴 blocking: The audit names debit, approval, and escrow spend as the locked paths; this covers only the _debit site (via _burn), leaving the approve claim site (ConfidentialFungibleToken.compact:908) unpinned. Extend it: cache 2n * MAX128, approve(BOB, MAX128), then complete with transferFrom.
added by claude (dev3-midnight-basic-review)
| // invariant here instead of leaving it to inspection of those two circuits. | ||
| // The cast fails closed, so an out-of-range claim aborts rather than wraps. | ||
| const escrowPlaintext = | ||
| wit_PlaintextBalance(entry.spenderCt) as Uint<128>; |
There was a problem hiding this comment.
🔴 blocking: "The cast fails closed" has no pinning test. Cache (1n << 128n) + escrowValue for the spender's escrow ct and expect transferFrom to throw — a truncating cast would make it succeed silently (the truncated value equals the real escrow), so that test discriminates exactly.
added by claude (dev3-midnight-basic-review)
| await cft._burn(MAX128); | ||
| }); | ||
|
|
||
| it('keeps the transfer bound at the Uint<128> maximum', async () => { |
There was a problem hiding this comment.
🔴 blocking: This pins the bound's value but not the widths on the compiled circuit: narrowing the witness to e.g. Uint<136> passes both new tests while collapsing the headroom from 2^120 to 2^8. Assert from MockConfidentialFungibleToken's contract-info.json that wit_PlaintextBalance's result maxval is 2^248-1 and transfer's value arg is 2^128-1 (parse bigint-aware — both exceed double precision).
added by claude (dev3-midnight-basic-review)
| pk: JubjubPoint, | ||
| ek: Bytes<32>, | ||
| claimedValue: Uint<128> | ||
| claimedValue: Uint<248> |
There was a problem hiding this comment.
🔴 blocking: MockElGamal.compact:116 still declares claimedValue: Uint<128> — it compiles (implicit widening) but misstates this signature and makes a >2^128 claim untestable at the ElGamal level. Widen the wrapper and add one unit test claiming 2n * MAX128 against addCiphertexts(encrypt(pk, MAX128, r1), encrypt(pk, MAX128, r2)).
added by claude (dev3-midnight-basic-review)
| * wrong plaintext fails the decryption check. Lifted special case of | ||
| * `assertDecryptsToPoint` with the message point `g^claimedValue`. | ||
| * | ||
| * @notice `claimedValue` is the widest `Uint` Compact offers, so this parameter |
There was a problem hiding this comment.
🔵 followup: The safety bound is ℓ, not the language cap — they coincide by only ~4 bits (2^248 < ℓ ≈ 2^251.86). One sentence stating the injectivity precondition (claim domain below the subgroup order) prevents a naive widening if the compiler cap ever rises past 251.
added by claude (dev3-midnight-basic-review)
| * `2^48` cap that kept wallet-side BSGS tractable is gone. Amounts stay well under | ||
| * the Jubjub scalar-field order, so the homomorphic balance sums (and the | ||
| * `g^value` encoding) do not wrap. Balances remain Uint<128>. | ||
| * `g^value` encoding) do not wrap. |
There was a problem hiding this comment.
⚪ nitpick: With accumulation, sums can wrap mod ℓ after ~2^124 maximal credits — same resource-bound class as the 2^120 paragraph below, and post-wrap the balance decrypts to sum mod ℓ (silent value loss, not lock). Worth the same qualifier here.
added by claude (dev3-midnight-basic-review)
0xisk
left a comment
0xisk
left a comment
There was a problem hiding this comment.
Looking good! left some comments.
2 participants
Resolves #795