plm-lottery/BUGS.md

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-25 … B-49. B-25, B-26 and B-27 are fixed (see "Previously fixed" below); the other 22 are open. 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 three fixes so far brought the suite from 139 to 151).

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. Outgoing transactions reconcile; deposits do not (B-29). The payout phase is now fully recoverable; the "drawing" phase (waiting on a block) still has no equivalent resume-after-restart or stall visibility (B-36).

For limitations that are accepted by design rather than bugs (single-shared-token admin auth, single-process assumptions, no user-facing history, etc.), see "Known gaps / TODO" in CLAUDE.md.

---

Critical

B-28 — A hostile Electrum server (or a MITM) can choose the winner

electrum/listener.py:167-186 accepts any header whose height >= tip_height: no proof-of-work check, no linkage to the previous block hash. That header is the sole source of entropy for the draw (scheduler.py:129).

In parallel, electrum/client.py:104-106 sets check_hostname = False and verify_mode = CERT_NONE. The comment justifies this with "the protocol's trust model is server consensus" — but there is no consensus here: one server at a time, rotated over a list of arbitrary third parties. So a hostile server, or anyone able to MITM a connection that validates no certificate, can fabricate a header and thereby decide who wins every round.

Proposed fix, in order of value. (1) Validate headers before accepting them: check the PoW against the claimed target and that prev_block matches the current tip; reject anything that fails. (2) Do not trust one server for the draw — fetch the header for draw_block_height from several endpoints in the rotation and require agreement before using it as the seed. (3) Pin certificates (or verify hostnames) for the configured servers rather than disabling verification wholesale. Longer term this is the argument for replacing the v1 draw algorithm — CLAUDE.md already calls it a replaceable component — with a scheme that does not depend on a single unauthenticated data source.

---

High

B-29 — detect_external_spends is irreversible and trusts a single response

deposits/service.py:87 marks spent_txid = "external-spend" for any UTXO missing from the current listunspent. There is no path to undo it: credit_confirmed_utxos skips (txid, vout) keys that already exist, regardless of their spent status (:19-31).

One incomplete listunspent — a rotated-to server that is broken or behind, an empty reply on error, or a reorg — permanently and silently zeroes a user's balance, recoverable only by editing the database. Crediting is idempotent and conservative; debiting is neither, and it acts on a single reply from a single unauthenticated server.

Proposed fix. Treat a missing outpoint as evidence, not proof. Require the same UTXO to be absent across N consecutive refreshes (or confirm the spend by looking up the outpoint's spending tx) before marking it, and skip the whole pass when listunspent returns empty for an address the DB believes is funded. Make the mark reversible: re-crediting should clear spent_txid when the sentinel value is present and the outpoint reappears as unspent.

B-30 — No deposit-side reconciler: one missed subscription means deposits are never credited

electrum/listener.py:61-67 (address_for_new_user) fires asyncio.create_task(...) without retaining the reference and without handling exceptions. If self.client becomes None between the check and the task running, the assert at :163 raises inside an orphan task and the exception is swallowed.

What makes this serious is what happens next: deposits are credited exclusively by scripthash notifications. There is no periodic routine reconciling balances against the chain (the reconciler only covers outgoing transactions). On a healthy keepalive'd connection there are no reconnects, so a lost subscription is never recovered and that user never sees their deposits, indefinitely.

Proposed fix. Two parts. (a) Make the subscription reliable: retain the task, log its exceptions, and retry with backoff instead of relying on a reconnect. (b) Add the missing safety net — a periodic sweep (say every few minutes, similar in shape to PendingTransactionReconciler) that re-runs _refresh_user for users whose scripthash is not in _scripthash_to_user, or simply round-robins over all users so a missed notification is always eventually caught.

B-31 — Reconnect costs O(users) sequential round-trips and stalls the draw

In _run_once the order is: subscribe headers → _subscribe_all_users() → then start the consumer tasks (electrum/listener.py:118-134). _subscribe_all_users iterates users sequentially, and each iteration is a subscribe plus a listunspent plus a DB write (:154-165).

At 5.000 users that is 10.000 serialized round-trips (15s timeout each). Throughout, _consume_headers is not running, so tip_height is frozen and _wait_for_next_block makes no progress: a reconnect stalls an in-flight draw for the entire resubscribe. And since registration has no rate limiting, the user count is attacker-controlled.

Proposed fix. Start the consumer tasks (headers especially) before resubscribing, so tip updates keep flowing during the sweep. Batch the resubscribe with bounded concurrency (e.g. asyncio.Semaphore(20) over asyncio.gather) instead of a serial loop, and decouple the listunspent refresh from the subscribe so the initial refresh can proceed in the background.

B-32 — bump_fee can loop forever on rebroadcasts the node always rejects

tx/broadcast.py:87-88 forces fee_delta = 1 when fee_delta <= 0. A one-satoshi total fee increase violates BIP125 rule 4 (a replacement must pay at least the incremental relay fee times its own size), so the node rejects it. bump_fee raises before updating pending, so fee_rate_sat_vb never advances and the next tick retries with identical parameters, every 30 seconds, forever.

This triggers whenever the real fee exceeds the estimate — i.e. whenever dust change was absorbed into the fee, which is an explicitly supported path (wallet/psbt_builder.py:105-107).

Related, same function: new_fee_rate = pending.fee_rate_sat_vb + 1 on every bump, with no ceiling. A transaction stuck for a day reaches ~96 sat/vB, eating the user's change, and it ignores the le=10_000 bound the admin panel enforces on the config field.

Proposed fix. Compute the delta from the actual replacement vsize (fee_delta = max(new_fee - old_fee, ceil(vsize * incremental_relay_rate))) so the bump is always relay-valid. Cap new_fee_rate at the configured maximum and raise RbfError once reached, so the transaction falls through to the reconciler (which, since B-27, correctly abandons it) rather than being retried indefinitely.

B-33 — No brute-force protection on a custodial wallet

POST /auth/login (auth/routes.py:83) has no rate limiting, no lockout, no delay and no CAPTCHA, and the password minimum is 8 characters. Argon2 slows a single attempt but not a patient distributed attack against an enumerable username list — and 409 username_taken on registration is a perfect enumeration oracle.

"No rate limiting" is listed as a generic known gap; on a system where guessing a password means withdrawing someone's funds, it deserves to be tracked separately and treated as a blocker.

Proposed fix. Per-username and per-IP throttling with exponential backoff on failed logins (slowapi, or a small DB-backed counter — but note the in-process caveat if workers are ever scaled). Return an identical response for unknown-user and wrong-password. Rate-limit registration too, which also bounds B-31's attacker-controlled user count.

B-34 — Password change and admin reset do not invalidate existing sessions

Neither /users/me/change-password nor /admin/users/{id}/reset-password invalidates already-issued JWTs (24h default lifetime, no revocation, no token_version on the user). The admin reset exists precisely for the "account compromised" case and does not evict the attacker.

Proposed fix. Add a token_version (or password_changed_at) column on User, embed it in the JWT claims, and reject any token whose value is stale in auth/dependencies.py:get_current_user. Bump it on both endpoints.

---

Medium

B-35 — Every API timestamp is naive, so the frontend renders it in the wrong timezone

Verified empirically: the DateTime columns carry no timezone, so SQLite returns naive datetimes and .isoformat() produces 2026-07-27T06:56:47.489110 — no Z. JavaScript's new Date() parses that as local time, so every date in /admin (rounds, pending transactions, audit log, via fmtDate in app/static/admin.js:50) and created_at in /users/me display two hours off in Italy.

The codebase knows about this — api/routes/rounds.py calls .replace(tzinfo=timezone.utc) on opened_at explicitly — but the fix was never applied systematically.

Proposed fix. Make the columns DateTime(timezone=True) (Alembic migration) so the value round-trips as aware, rather than patching each call site. Until then, at minimum a shared serialization helper that stamps UTC, used by every .isoformat() in the API layer.

B-36 — _wait_for_next_block waits forever, with no timeout and no visibility

rounds/scheduler.py:158-161 loops until a higher block arrives. No timeout, no log, no audit entry. If the connection dies in a way that stops the tip advancing, the round sits in drawing indefinitely and the admin panel shows nothing at all — just a frozen state with no explanation.

Proposed fix. Log progress periodically while waiting, and past a threshold (a few multiples of the 120s block time) write a draw_stalled audit entry so it surfaces in /admin. Surface the wait in GET /rounds/current too (it already returns chain_tip_height; draw_waiting_since would make the stall self-evident to users).

B-37 — Displayed balance and spendable balance diverge, and the error does not explain it

After a bet the change is unconfirmed, so cached_balance_sats ≈ 0 while the UI shows pending_balance_sats (the real figure). A withdrawal attempted right after validates against confirmed UTXOs (withdrawals/service.py:54-60) and answers insufficient_balance.

The user sees "1.000 PLM" on screen and is told they have no funds. The mechanism is a documented design decision, but the error does not distinguish "you don't have the money" from "your money is waiting to confirm" — two very different situations for whoever reads it.

Proposed fix. A distinct error code (e.g. balance_pending_confirmation) raised when the requested amount is covered by pending_balance_sats but not by the confirmed balance, carrying the pending amount in params, plus its error.* entry in all 7 languages. The withdrawal form should also cap/hint the max against the confirmed balance rather than the displayed one.

B-38 — The 500-subscriber SSE cap is a zero-cost DoS of the realtime feature

GET /rounds/stream requires no authentication and each connection takes a slot on a global counter (rounds/events.py:33-38). Anyone opening 500 connections degrades every real user to polling. The comment describes it as a defensive cap; it is in fact the vector, not the defence.

Proposed fix. Cap per client IP (and, once available, per authenticated user) rather than globally, and evict the oldest idle subscriber instead of refusing new ones. The reverse proxy is the right place for the connection-count limit — Caddy can enforce it before the request reaches the app.

B-39 — SQLite with no WAL, no busy_timeout, and five concurrent writer tasks

db/base.py:6 calls create_async_engine(settings.database_url) with no connect_args, and there is no PRAGMA anywhere in the repo (verified by grep). Without journal_mode=WAL readers block writers, and the concurrent writers are five background tasks plus every HTTP handler. database is locked under load is realistic, and nothing handles it.

Proposed fix. Set journal_mode=WAL, synchronous=NORMAL and a busy_timeout of a few seconds on connect (a connect event listener on the engine, applied only for the SQLite dialect), and retry OperationalError: database is locked in the background loops. Longer term this is an argument for PostgreSQL, which the single-process constraints in CLAUDE.md also point at.

B-40 — bump_fee holds a DB session open across N network calls

tx/broadcast.py:80 issues one get_transaction per input (up to 15s each) and then a broadcast, all with the session open. This is precisely the pattern B-18 removed from _trigger_payout via its three-phase structure; it survives here.

Side note in the same function: _prevout_amount does round(value_coins * 100_000_000) on a float from the server — acceptable at these magnitudes, but it is floating-point money arithmetic in a codebase that is otherwise strictly integer-satoshi.

Proposed fix. Restructure into the same three phases: read what is needed and close the session, do the chain work, then reopen to persist. For the float: prefer the raw (non-verbose) transaction and parse the output value as an integer with embit, which is what reconcile.py:_release_inputs already does for inputs.

B-41 — All confirmation logic depends on verbose=True, which is not universally supported

poll_once, reconcile._tx_exists_on_chain and bump_fee all 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, no bumps — and the code would read that as a transport error and stay silent.

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 — a crash or rejected broadcast could leave money on-chain with no DB record, or leave the round wedged with no way to retry safely), B-26 (a transient failure or a process restart at payout time wedged the round in paying_out forever, with most failure paths logging nothing) and B-27 (every RBF bump reset the reconciler's own abandon clock, so a repeatedly-bumped-but-never-mined transaction was never abandoned) are fixed as of 2026-07-27. Together B-25 and B-26 make paying_out fully recoverable — a payout retry is now safe (persisted before broadcast, guarded against double-spend) and automatic (the scheduler retries a stuck round on its own, throttled, with every failure audit-logged with a reason, including across a process restart). See git history (commits f13f685, 50a43ae, 933760e) for the fix-by-fix breakdown; the regression suite grew from 139 to 151 tests over the three.

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).