Skip to content

Harden withdrawal off-ramp: nonce replay, review-hold race, fee semantics, worker ledger - #8384

Open
Scottcjn wants to merge 1 commit into
mainfrom
fix/withdrawal-offramp-hardening
Open

Scottcjn wants to merge 1 commit into
mainfrom
fix/withdrawal-offramp-hardening

Conversation

@Scottcjn

Copy link
Copy Markdown
Owner

GPT-6 Astra audit of the RTC withdrawal off-ramp (funds leaving the chain), every finding source-verified against origin/main. The visible request-time debit path was already sound (BEGIN IMMEDIATE, guarded atomic debit, finite/positive/precision checks, bool amount rejected) — these close the seams around it.

#1 — Nonce type-encoding replay (request_withdrawal)

The signature is verified over the stringified nonce (f"...:{nonce}") while the replay-dedup key is stored TEXT in withdrawal_nonces. JSON true (Python bool) and the string "True" render identically in the signed message, but SQLite stores the bool as INTEGER 1 → TEXT "1" and the string as "True"two distinct dedup keys, one signature replayable once. Bounded (destination is inside the signed message, so no redirection — griefing / double-process, not theft). Fix: reject bool nonces and canonicalize to the signed string form, so int 5 and "5" collapse to a single dedup key.

#2 — Payout worker ledger mismatch + latent double-debit (payout_worker.py)

The node debits the canonical balances ledger (amount + fee) at request time; the worker debited a non-existent accounts table again at send time — a latent double-debit against a phantom ledger, masked only because production broadcast is stubbed to refuse. Fix: removed the send-time debit (claim-only, exactly-once via the pending→processing rowcount gate); refund-on-pre-broadcast-failure now credits balances (schema-tolerant, mirroring the node's _apply_wallet_balance_delta) exactly once, gated on the processing→failed transition; a failure before the claim leaves the row pending for retry rather than fabricating a refund.

#3 — Review-hold TOCTOU (request_withdrawal)

The pre-BEGIN review gate left a race window before the debit. Fix: added an in-transaction re-check on the reserved write connection (plain indexed read on wallet_review_holds/blocked_wallets, no DDL — tables already ensured by the pre-check), closing the window with no separate-connection lock contention.

#5 — Fee semantics

Fee is charged on top (debit = amount + fee; destination receives amount), but the response reported net_amount = amount - fee, contradicting the debit. Fix: report net_amount = amount and add total_debited = amount + fee.

Tests

  • New test_withdrawal_nonce_replay_and_fee.py (bool rejected; int/str share dedup key; fee response matches debit).
  • New test_payout_worker_ledger.py (no accounts reference; success doesn't double-debit; pre-broadcast failure refunds to balances; refund exactly-once).
  • Updated test_payout_worker_recovery.py — its last case asserted the old phantom-accounts double-debit (100→89); now asserts the worker leaves balances untouched, keeping the exactly-once-claim coverage.

Targeted withdrawal + payout suite: 39 passed. The wider node suite has 102 pre-existing isolation/ordering failures identical on origin/main (each passes in isolation), unrelated to this change.

⚠️ Not deployed — live money-movement consensus code; holding for review + explicit deploy approval.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KbyXP4eiiRYEa8GsQtQPhR

…tics, worker ledger

GPT-6 Astra audit of the RTC withdrawal off-ramp (funds leaving the chain),
each finding source-verified. The visible request-time debit path was already
sound (BEGIN IMMEDIATE, guarded atomic debit, finite/positive/precision checks);
these close the seams around it.

#1 nonce type-encoding replay (request_withdrawal): the signature is verified
   over the STRINGIFIED nonce while the dedup key is stored TEXT, so JSON `true`
   and the string "True" render identically in the signed message yet store as
   distinct dedup keys -> one signature replayable once. Reject bool nonces and
   canonicalize to the signed string form (int 5 and "5" now collapse to one key).

#2 payout worker ledger mismatch + latent double-debit: the node debits the
   canonical `balances` ledger (amount + fee) at REQUEST time, but the worker
   debited a NON-EXISTENT `accounts` table again at send time. Removed the
   send-time debit (claim-only, exactly-once via pending->processing rowcount);
   refund-on-pre-broadcast-failure now credits `balances` (schema-tolerant,
   mirroring the node) exactly once, gated on the processing->failed transition.

#3 review-hold TOCTOU (request_withdrawal): the pre-BEGIN gate left a race
   window before the debit. Added an in-transaction re-check on the reserved
   write connection (plain indexed read, no DDL) so a hold applied mid-request
   is caught without introducing lock contention.

#5 fee semantics: fee is charged on top (debit = amount + fee; destination
   receives amount). Response reported net_amount = amount - fee, contradicting
   the debit. Now reports net_amount = amount and total_debited = amount + fee.

Tests: added test_withdrawal_nonce_replay_and_fee.py and
test_payout_worker_ledger.py; updated test_payout_worker_recovery.py (its last
case asserted the old phantom-accounts double-debit). Targeted withdrawal +
payout suite: 39 passed. The wider node suite has 102 pre-existing
isolation-ordering failures identical on origin/main (unrelated to this change).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KbyXP4eiiRYEa8GsQtQPhR
@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) BCOS-L2 Beacon Certified Open Source tier BCOS-L2 (required for non-doc PRs) node Node server related tests Test suite changes size/L PR: 201-500 lines labels Sep 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ BCOS v2 Scan Results

Metric Value
Trust Score 36/100
Certificate ID BCOS-79ef672c
Tier L1 (not met)

BCOS Badge

What does this mean?

The BCOS (Beacon Certified Open Source) engine scans for:

  • SPDX license header compliance
  • Known CVE vulnerabilities (OSV database)
  • Static analysis findings (Semgrep)
  • SBOM completeness
  • Dependency freshness
  • Test infrastructure evidence
  • Review attestation tier

Full report | What is BCOS?


BCOS v2 Engine - Free & Open Source (MIT) - Elyan Labs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) BCOS-L2 Beacon Certified Open Source tier BCOS-L2 (required for non-doc PRs) node Node server related size/L PR: 201-500 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants