Files

T

davide 08c566d547 Restructure bump_fee into three phases, drop float fee math (B-40)

bump_fee issued one get_transaction per input (up to 15s each) and
then a broadcast, all with the caller's DB session held open - exactly
the pattern already fixed elsewhere for the same reason (B-18's
_trigger_payout, B-31's refresh_user). Also, _prevout_amount computed
a prevout's satoshi value via round(value_coins * 100_000_000) on a
float the server reported, in a codebase that is otherwise strictly
integer-satoshi.

bump_fee now takes a session_factory and a pending_id instead of a
live session and row, with three phases: read what's needed (the
signing key, current fee rate, raw tx) and close the session before
any network call; do the chain reads, signing and broadcast with no
session open; reopen a session only to persist the outcome.
_prevout_amount now asks for the raw (non-verbose) transaction and
reads embit's parsed TransactionOutput.value directly - already an
exact integer, no float conversion involved at all.

A pending_transaction that's no longer "pending" by the time bump_fee
actually runs (it confirmed in the meantime, a normal race) is now a
quiet no-op returning None, rather than being folded into RbfBumper's
error-logging path.

Suite grows from 214 to 217 tests. BUGS.md moves B-40 to Previously
fixed, and trims its own now-stale claim that bump_fee still depended
on verbose=True (B-41) - it no longer does.

2026-07-27 15:14:58 +02:00

8.0 KiB

Raw Blame History

Known bugs

A second full-codebase audit on 2026-07-27 found 25 further issues (4 critical, 6 high, 7 medium, 8 low), listed below as B-41 … B-49. B-25 through B-40 are fixed (see "Previously fixed" below) — no Critical-severity finding remains open; the other 9 are Medium/Low. The 139-test suite was green at the time of the audit, so none of these were caught by existing coverage — every fix lands with a regression test (the sixteen fixes so far brought the suite from 139 to 217).

The recurring pattern across the open findings is worth stating once: the code is rigorous about the failure modes that have actually been hit, and silent about the ones that have not. The payout phase is now fully recoverable; the "drawing" phase (waiting on a block) is now observable (B-36) but still has no equivalent resume-after-restart — see "Known gaps / TODO" in CLAUDE.md, which is also where other by-design limitations (single-shared-token admin auth, single-process assumptions, no user-facing history, etc.) are documented.

Medium

B-41 — Confirmation/reconciliation logic depends on `verbose=True`, which is not universally supported

poll_once and reconcile._tx_exists_on_chain call blockchain.transaction.get(txid, True). Several Electrum server implementations and versions reject the verbose flag ("verbose transactions are currently unsupported"). Falling back onto such a server means no confirmations, no reconciliation — and the code would read that as a transport error and stay silent. (bump_fee's own verbose=True call was removed as part of the B-40 fix — it now reads the raw transaction and parses the output value with embit instead, so bumps are unaffected by this finding.)

Related: reconcile.py:83 decides whether to abandon a transaction by substring-matching the error text ("missing", "not found", "no such", "unknown"). It works against ElectrumX; it is fragile as the basis for a decision that releases funds.

Proposed fix. Use blockchain.transaction.get_merkle (or the scripthash history) for confirmation and existence checks — both are portable and give the confirming height directly. Probe verbose support once at connect time and record it on the client, so an unsupported server is detected loudly at session start rather than silently mid-operation.

Low / hygiene

B-42 — `/docs` exposed in production

FastAPI mounts Swagger by default, so the entire API surface — /admin included — is publicly enumerable. The README advertises it. Fix: docs_url=None, redoc_url=None, openapi_url=None in production (env-gated), or place them behind require_admin.

B-43 — No HTTP security headers

The Caddyfile sets no CSP, no X-Frame-Options/frame-ancestors, and no HSTS (Caddy does not add it on its own). The JWT lives in localStorage, so any XSS exfiltrates it, and the page is iframeable. Fix: a header block in the Caddyfile with Strict-Transport-Security, X-Content-Type-Options: nosniff, Referrer-Policy and a CSP tight enough for two static pages with no external assets (default-src 'self').

B-44 — README and CLAUDE.md contradict each other

The README says to run uvicorn --reload directly and docker compose run --rm app python scripts/generate_master_key.py; CLAUDE.md says explicitly that neither is supported. Whoever opens the repo reads the README first. Fix: align the README's Quick start with the Docker-only workflow documented in CLAUDE.md and docs/setup.md.

B-45 — Unvalidated and unpaginated admin list endpoints

limit: int = 50 on /admin/rounds and /admin/audit-log has no bounds (-1 means "everything" on SQLite), and /admin/pending-transactions has no limit at all — it grows without end. Fix: Query(default=50, ge=1, le=500) on both, and the same treatment plus a status filter on the pending-transaction list.

B-46 — `secrets.compare_digest` on a `str` raises on non-ASCII input

api/routes/admin.py:27 raises TypeError — a 500 instead of a 403 — when the header contains non-ASCII characters. Fix: compare the UTF-8 encoded bytes of both sides.

B-47 — Unbounded `String` columns for large text

raw_tx_hex (db/models.py:146) and payload_json (:178) should be Text. It works on SQLite and PostgreSQL and breaks elsewhere. Fix: switch both to Text in a migration.

B-48 — No cap on input count in `select_utxos`

A user with hundreds of small UTXOs builds a huge transaction whose fee — deducted from the bet amount — materially erodes their contribution to the pool, and it can exceed standardness limits. Fix: cap the selected inputs (e.g. 50) and fail with a translatable error suggesting a consolidation, or consolidate the address automatically when the count crosses a threshold.

B-49 — Rollback paths do not publish an SSE update

bets/service.py:_release_failed_bet and withdrawals/service.py:_release_failed_withdrawal restore the balance without calling broadcaster.publish(), so dashboards only find out on their next poll. Fix: one broadcaster.publish() at the end of each, as every other state-changing path already does.

Previously fixed

B-25 — the payout had no two-phase write, unlike bets and withdrawals
B-26 — a payout failure or a process restart could wedge a round in paying_out forever
B-27 — an RBF bump reset the reconciler's own abandon clock, so a repeatedly-bumped tx was never abandoned
B-28 — a hostile Electrum server (or a MITM) could single-handedly pick the round's winner
B-29 — a UTXO absent from one server's listunspent was marked spent immediately, irreversibly, on a single unauthenticated reply
B-30 — a lost scripthash subscription meant a user's deposits were never credited, with no periodic safety net
B-31 — resubscribing on reconnect ran serially before anything else started, freezing the chain tip (and so an in-flight draw) for the whole sweep
B-32 — an RBF bump could retry forever below BIP125's relay-mandated minimum fee delta, with no ceiling on the fee rate either
B-33 — POST /auth/login had no rate limiting, so a password could be brute-forced against an enumerable username list
B-34 — password change/reset didn't invalidate already-issued JWTs, so a stolen token survived a change meant to lock it out
B-35 — API timestamps round-tripped as naive datetimes, so the frontend parsed them as local time instead of UTC
B-36 — a stalled draw wait had no timeout, no log, and no audit trail, so a frozen round showed nothing in /admin
B-37 — a withdrawal covered by unconfirmed change answered "insufficient balance" instead of distinguishing it from actually having no funds
B-38 — the SSE subscriber cap was global, so one client opening enough connections degraded every other user to polling
B-39 — SQLite ran without WAL or a busy_timeout, so a writer could block every reader and a second writer failed immediately instead of waiting
B-40 — bump_fee held a DB session open across N slow network calls, and computed a prevout's value from a server-reported float instead of an exact integer

See git history for the fix-by-fix breakdown (commits f13f685, 50a43ae, 933760e, and the B-28/B-29/B-30/B-31/B-32/B-33/B-34/B-35/B-36/B-37/B-38/B-39/B-40 fixes). Suite grew from 139 to 217 tests over the sixteen.

A full-codebase audit on 2026-07-26 (commit d4e0974) found 24 bugs across every Python module under app/, both static frontends, and the Docker/Caddy deployment — 5 critical, 7 high, 7 medium, 5 low. All 24 were fixed and verified against the current code on 2026-07-27; the fixes are covered by the regression suite (grew from 79 to 139 tests) and five of them were additionally confirmed against a real mainnet deployment (see git history between fb734bb (documenting the findings) and 845ba98 (recording the audit outcome) for the fix-by-fix breakdown — each commit message names the bugs it closes and where their tests live).

8.0 KiB Raw Blame History