feat(cashu): C4 — escrow primitives (2-of-3 NUT-11 lock, sign, redeem, reclaim) - #236
feat(cashu): C4 — escrow primitives (2-of-3 NUT-11 lock, sign, redeem, reclaim)#236grunch wants to merge 6 commits into
Conversation
…, reclaim) Phase C4 of docs/cashu/README.md, the cryptographic heart of Cashu-mode trading. No UI and no bridge surface, so review can be about correctness alone. Stacked on C2 (#235) — the escrow methods hang off the wallet from that phase. `rust/src/cashu/escrow.rs`: - `xonly_to_cashu_pubkey` — the `02`-prefix mapping the daemon applies in `cashu_pubkey_from_xonly_hex`. Rejects anything that is not exactly 64 hex chars instead of padding or truncating: a silently reinterpreted key locks funds to nobody, and nothing downstream would notice. - `escrow_conditions` — the 2-of-3 of §2: data = P_S, pubkeys = [P_B, P_M], n_sigs = 2, SIG_INPUTS, locktime, refund = [P_S], n_sigs_refund = 1. - `fee_conditions` — 1-of-1 to Mostro. The fee is a payment, not an escrow; conditions would make it unspendable for the node. - `verify_conditions` / `verify_escrow_token` — the client-side mirror of the daemon's composite check, run *per proof*: a token whose first proof is correct and whose second is locked to the builder alone would pass a spot check and walk away with the difference. - `sign_proofs`, `combine_and_redeem`, `reclaim_after_locktime` — one signature per proof keyed by that proof's secret, matching the wire form C0 pinned. A missing peer signature fails before the mint is contacted. The spike (docs/cashu/cdk-spike.md) established that cdk exposes NUT-11 with custom tags in full, so none of this hand-builds secrets or witness encodings. Tests - Unit: the `02` mapping against a fresh key; five malformed-key shapes rejected; the built condition checked field by field against §2 (a wrong default cannot hide behind a passing round trip); a past locktime refused at construction; and verification refused for every way an escrow can be wrong — wrong seller, n_sigs=1, an extra refund key, a bare P2PK, a missing counterparty, a locktime shorter than required. - Integration (`#[ignore]`, MOSTRO_TEST_MINT_URL): full lock → verify → sign → combine → redeem against a real mint; one signature alone cannot move an escrow; an impostor's signature does not settle one; a short-changed escrow fails verification.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review — C4 (strict pass)
Scope reviewed: 🛑 Critical1. if !pubkeys.contains(&parties.buyer) || !pubkeys.contains(&parties.mostro) {
bail!("InvalidEscrowToken: buyer or Mostro key missing");
}This checks the two keys are present. It never checks nothing else is. A seller can build an escrow with Every other field is exact-matched, including if pubkeys.len() != 2
|| !pubkeys.contains(&parties.buyer)
|| !pubkeys.contains(&parties.mostro)
{
bail!("InvalidEscrowToken: pubkeys must be exactly [buyer, mostro]");
}Note 🔴 Major2. No currency-unit check —
3. No distinctness check on the three parties — Nothing rejects 4. Reserved-proof leak on a failed Same defect as 🟡 Minor5. 6. ✅ What is right
Test gaps
|
…-of-3 Addresses the strict review on #236. Critical - `verify_conditions` checked that the buyer and Mostro keys were *present* in `pubkeys`, never that nothing else was. A seller could lock with `pubkeys = [P_B, P_M, P_attacker]` where they hold the extra key: with `data = P_S` and `n_sigs = 2`, seller + attacker is two of four, and the escrow is spendable unilaterally the moment it is funded — while this verifier called the token good. Now matched exactly, like every other field. The test table gained the row that was missing, which is what let it through. Major - No currency-unit check. `Token::value()` sums proof amounts irrespective of denomination, so a token in another unit with the right numeric total passed the amount check. Unit is now asserted before the amount. - Nothing rejected `buyer == seller`, `seller == mostro` or `buyer == mostro`. A duplicate collapses the threshold, and whether one signature then satisfies `n_sigs = 2` is mint-implementation-defined. `EscrowParties::ensure_distinct` runs at construction and again in verification. - `build_locked_token` leaked the whole escrow amount as reserved proofs when `confirm` failed (it consumes the handle, so nothing else could release them). It now reclaims and reports how much came back. Minor - `combine_and_redeem` scanned the peer signatures per proof; indexed by secret once instead. - `reclaim_after_locktime` relied on the mint to refuse a premature spend. The locktime is in the secret, so it now says "the refund path opens in N seconds" instead of surfacing an opaque mint error. Tests: the extra-pubkey case, the three degenerate-party cases, and a wrong-key-with-right-count case that keeps the membership check honest now that the count check fires first.
|
@coderabbitai review |
✅ Action performedReview finished.
|
…mint Same treatment as C2, applied to the escrow suite once a real nutshell showed the tests could not pass at all, and then could not pass twice. - The four funds-using tests asserted "fund the seller wallet first" and returned. They now fund themselves through `mint_for_test`, so a reviewer can actually run them. - Fixed seeds plus a fresh DB replay NUT-13 blinding secrets; the mint answers "Blinded Message is already signed" on the second run. Seeds are unique per process and per call now. - The settlement test pinned the redeemed amount and the seller's balance to the face value. nutshell's default keyset charges a swap fee, so redeeming 16 sat yields less and locking costs more. The face value is what a validator checks; the assertions bound the rest. - `one_signature_is_not_enough_to_move_an_escrow` expected the *mint* to refuse a premature reclaim. Since the local locktime check added in this PR, the client refuses first with the time remaining — a better message for the same property. Either refusal is accepted. Verified 8/8 against nutshell 0.20.3, twice in a row (MINT_RATE_LIMIT=FALSE — the mint rate limits by default).
|
@coderabbitai Review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Reviewed in depth, with the integration suite actually running: nutshell 0.20.3 in Docker, tests built against cdk 0.17.3 as pinned in Cargo.lock. Your baseline reproduces — 8 passed, including the four security properties. I read your strict pass first; all six of its findings are addressed in the tree.
The shape is right and the round-1 fix is real: pubkeys.len() != 2 closes the extra-key hole exactly as described, ensure_distinct() on both the way in and the way out is the correct place for it, per-proof verification is the right granularity, and checking the unit before the amount is a genuinely non-obvious ordering that pays for itself. I also chased the duplicate-proof short-change variant — a token repeating one proof to inflate value() — and it is already closed upstream: TokenV4::value() returns Error::DuplicateProofs when secrets collide, and verify_escrow_token calls value() before it iterates. Worth knowing so it doesn't get re-raised.
One finding below is exploitable against a real mint, and I have a PoC. Two are minor.
🛑 sign_proofs is a signing oracle — demonstrated, 15 sat taken
Proof::sign_p2pk signs self.secret.to_bytes() and nothing else. Under SIG_INPUTS the signature commits to the secret string alone — not the amount, the keyset, the C value, or the token it arrived in. So a BIP-340 signature over secret S by key K is valid for any proof carrying secret S, wherever it appears. sign_proofs signs every proof in whatever token it is handed, and in the release and cooperative-cancel flows the counterparty is exactly who supplies that token.
Against a live mint:
- Seller builds a genuine 16 sat escrow; buyer verifies it with
verify_escrow_token— passes, as it should. - Buyer fabricates a decoy: the escrow's proofs with the same secrets, same keyset, amounts rewritten to 1 sat, witnesses stripped, re-encoded with a
"trivial refund, please sign"memo. A different token by any comparison. - Seller calls
sign_proofson the decoy. They never touch the real escrow in this step. - Buyer replays those signatures against the real escrow with their own key: two of three.
[PoC] ORACLE OPEN — redeemed 15 sat with a harvested signature
15 sat of the 16 sat escrow (the missing sat is the mint's swap fee), no fiat paid. Reproduced twice.
The fix, verified
I applied the change and re-ran everything:
pub async fn sign_proofs(
&self,
encoded: &str,
key: SecretKey,
parties: &EscrowParties,
expected_amount: u64,
min_locktime: u64,
) -> Result<Vec<ProofSignature>> {
self.verify_escrow_token(encoded, parties, expected_amount, min_locktime)
.await?;
// … unchangedThe PoC then dies before any signature exists — [PoC] signing refused: InvalidEscrowToken: expected 16 sat, got 1 — and your original 8 integration tests still pass. Cost: three call sites in tests, all of which already have parties and locktime in scope. There is no legitimate non-escrow use of this function (the fee token is 1-of-1 to Mostro and is redeemed by the node with its own key, not signed through here), so requiring the escrow's parameters is the honest signature rather than an added restriction. And it is free today precisely because the module has no callers until C5.
I'd rather see this than a documented precondition. A primitive whose misuse costs the escrow should not have an unsafe form for C6/C7 to forget about — C5 only locks, so sign_proofs gets its first real callers when release and cooperative cancel land.
⚠️ A residue the fix does not remove — and I don't think it can, here
I tested the fix against a smarter decoy: identical amounts and conditions, differing only in the memo. It passes verify_escrow_token, so the seller signs, and the signatures redeem the real escrow.
That is not an escalation of the bug above — the decoy is the same escrow with the same parties, amount and locktime, and the caller supplied those parameters themselves, so signing it is consenting to release that escrow. The fix holds.
But it exposes something worth a decision at the spec level, not in this PR. Under SIG_INPUTS the signed message is the input secrets only; under SIG_ALL it is secret_0 || C_0 || … || amount_0 || B_0 || …, which commits to the outputs. So with SIG_INPUTS, a signature authorises spending the proof, never where the value goes. A seller's release signature and a seller's cooperative-cancel signature over the same escrow are byte-identical in what they authorise: whoever holds one plus a second key can direct the funds either way.
§2 of docs/cashu/README.md pins SIG_INPUTS, so this is upstream of the PR. But C6 and C7 are where it becomes reachable — release and cooperative cancel are the two flows that produce signatures over the same escrow for opposite destinations, and they will need to guarantee that no party ever signs for both. Worth an issue against the spec doc so the constraint is written down before the flow that has to honour it gets built.
Two smaller things
normalize_token is bypassed. wallet.rs strips cashu: / cashu:// prefixes before parsing, with a comment explaining that QR payloads routinely carry them. All four escrow entry points use bare encoded.trim().parse() instead. A token the wallet's own receive path accepts will fail escrow verification with "unparseable", which is the wrong explanation. One line each.
The finding-5 fix addressed the complexity but not the diagnosability. The HashMap is the right structure and the O(n) half of the comment is correct, but it also says the index "makes a duplicate secret in the peer's list a visible collision rather than a silent first-wins" — collect() into a HashMap does neither, it silently keeps the last value for a repeated key. Your original concern stands where it was: a duplicate secret still produces no error here and still surfaces as an opaque mint failure later. Only the complexity changed. Either reject duplicates explicitly when building the index, or drop the clause — a comment asserting a safety property that isn't there is worse than none, because the next reader builds on it.
verify_escrow_token is purely structural, and the doc comment doesn't say so. It proves the token is shaped like the right escrow; it does not prove the proofs are unspent (no NUT-07 check) or mint-issued (cdk verifies DLEQ on receive — after the fiat leg). A seller can hand over a structurally perfect token whose proofs were spent an hour ago. If that check belongs to the daemon or to C5, fine, but the boundary should be stated on the function, because "returned Ok" reads as a much stronger claim than it is.
Reproducing the PoC
Against the mint your own instructions describe:
docker run -d --name nutshell -p 3338:3338 \
-e MINT_LISTEN_HOST=0.0.0.0 -e MINT_LISTEN_PORT=3338 \
-e MINT_BACKEND_BOLT11_SAT=FakeWallet -e MINT_PRIVATE_KEY=TEST_PRIVATE_KEY \
-e MINT_RATE_LIMIT=FALSE cashubtc/nutshell:latest poetry run mint
The test builds a genuine escrow, rewrites its proofs' amounts while keeping their secrets, re-encodes with Token::new, has the seller sign that, and feeds the resulting signatures to combine_and_redeem on the original token. I'm happy to open it as a PR against your branch if you'd like it as a regression test — it belongs next to a_signature_from_the_wrong_key_does_not_settle_an_escrow.
Housekeeping
GitHub reports this as CONFLICTING against main — expected while stacked on #235, but it needs a rebase before it can land.
2 participants
Phase C4 of
docs/cashu/README.md— the cryptographic heart of Cashu-modetrading. No UI, no bridge surface, so a review can be about correctness alone.
What it does
The escrow is one NUT-11 P2PK secret per proof, locked 2-of-3 (§2 of the doc):
xonly_to_cashu_pubkey— the02-prefix mapping the daemon applies incashu_pubkey_from_xonly_hex. Anything that is not exactly 64 hex charactersis rejected rather than padded or truncated: a silently reinterpreted key
locks funds to nobody, and nothing downstream would notice.
escrow_conditions/fee_conditions— the 2-of-3 above, and a plain1-of-1 to Mostro for the fee. The fee is a payment, not an escrow; giving it
the escrow's conditions would make it unspendable for the node.
verify_escrow_token— the client-side mirror of the daemon's compositecheck, run per proof. A token whose first proof is correct and whose
second is locked to the builder alone would pass a spot check and walk away
with the difference. Also checks mint and total amount.
sign_proofs/combine_and_redeem/reclaim_after_locktime— onesignature per proof, keyed by that proof's secret, matching the wire form C0
pinned. A missing peer signature fails before the mint is contacted, so a
half-signed spend is never attempted.
The spike (
docs/cashu/cdk-spike.md, in #235) established that cdk exposesNUT-11 with custom tags in full — so none of this hand-builds secrets or witness
encodings, which is what kept the phase to this size.
Tests
Unit (run in CI): the
02mapping against a fresh key; five malformed-keyshapes rejected, including a 33-byte compressed key passed where x-only was
expected; the built condition asserted field by field against §2, so a wrong
default cannot hide behind a passing round trip; a past locktime refused at
construction; and verification refused for every way an escrow can be wrong —
wrong seller key,
n_sigs=1, an extra refund key, a bare P2PK, a missingcounterparty, a locktime shorter than required.
Integration (
#[ignore], needsMOSTRO_TEST_MINT_URL) — the "done when" ofthe phase:
refused by the mint);
Verification
No
frb-generateneeded — this phase adds nothing underrust/src/api/.Test plan
x-only input (compare against
cashu_pubkey_from_xonly_hexon the Track Abranch).
until C5.
Next: C5 wires this into the take flow.
Manual verification
This PR is pure cryptography with no UI, so the verification that matters is
the integration suite — and it now genuinely runs (it could not before; see the
last commit). The security properties are the point: verify those four tests
fail if you break the code, not just that they pass.
Already run on this branch
Plus the integration suite against a real mint — 8/8, twice in a row
(nutshell 0.20.3).
A · Integration suite against a live mint
Expect 8 passed, and 8 again on a second run.
MINT_RATE_LIMIT=FALSEisrequired for the second run (nutshell rate limits by default).
Four of those eight are the security properties, against a real mint with real
blind signatures:
an_escrow_locks_and_settles_with_two_of_three_signaturesone_signature_is_not_enough_to_move_an_escrowa_signature_from_the_wrong_key_does_not_settle_an_escrowan_escrow_for_the_wrong_amount_fails_verificationB · Confirm the fix that this PR's review found
The verifier used to accept extra pubkeys, which let a seller add a key they
also control and spend the escrow alone. To convince yourself it is closed:
Then break it deliberately — in
rust/src/cashu/escrow.rs, delete thepubkeys.len() != 2check — and re-run. The test must fail on the"extra pubkey" row. Put it back.
Do the same with
three_parties_must_be_three_different_keysby removing theensure_distinct()call inescrow_conditions.C · Key encoding against the daemon
The
02-prefix mapping must match the daemon'scashu_pubkey_from_xonly_hexexactly, or the escrow locks to a key nobody holds.
cargo test --lib an_xonly_key_maps_to_the_daemons_compressed_formcovers the shape; if you havethe daemon's Track A branch checked out, feed the same x-only hex to both and
compare the output byte for byte.
D · Nothing else moved
This PR adds a module and two crate-internal accessors on the wallet. It has
no callers until C5, so a Lightning build cannot reach any of it. The
regression check is simply that
cargo testand the app behave as on the basebranch.