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 and B-26 are fixed as of 2026-07-27; the
other 23 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 (B-25 and B-26
together brought the suite from 139 to 148).

The recurring pattern across B-27, B-29 and B-36 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.

**`paying_out` is now fully recoverable, not just idempotent.** B-25 made a payout retry
*safe* (persisted before broadcast, guarded against double-spend); B-26 made it *automatic*
(the scheduler retries a stuck `paying_out` round on its own, throttled, and every failure —
including ones that used to fail silently — is now audit-logged with a reason). Together
these close every way a payout specifically could wedge the lottery forever. What's still open
in the same family is narrower: the "drawing" phase (waiting on a block) has no equivalent
resume-after-restart or stall visibility — see B-36 and the "scheduler doesn't resume" entry
in CLAUDE.md's Known gaps, which this doesn't touch.

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](CLAUDE.md).

---

## Critical

### B-27 — Every RBF bump resets the reconciler's abandon clock, so it never fires

`tx/broadcast.py:122` sets `pending.broadcast_at = now` on each bump, but
`tx/reconcile.py:133` computes the 6-hour abandon deadline **from that same field**.

With the default `rbf_timeout_seconds = 900`, a transaction that is successfully bumped every
15 minutes but never mined resets the counter long before it can reach 6 hours: it is **never
abandoned**, its UTXOs never return to the user, and if it is a bet the round stays in
`closing` indefinitely (`scheduler.py:90-91`). `reconcile.py` exists precisely to prevent
this, and the bumper disarms it.

**Proposed fix.** Split the field: keep `broadcast_at` as the *first* broadcast (never
rewritten — it is what `_is_due` must use) and add `last_broadcast_at`, updated by
`bump_fee` and used by `should_bump`. Alembic migration backfilling `last_broadcast_at =
broadcast_at`.

### 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 needs B-27 fixed to
actually act on 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](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 has no two-phase write, unlike bets and withdrawals

`rounds/scheduler.py` used to broadcast the payout and only afterwards write `payout_txid`
and the `PendingTransaction`. A crash in that window — and `docker-compose.yml` sets
`restart: unless-stopped`, so a crash means an automatic restart — left a payout on-chain
with **no record at all**: the round stuck in `paying_out`, the reconciler with nothing to
resolve, and a manual retry that would pay the winner a second time (pool UTXOs are not
tracked in `utxo_events`, so nothing reserved them).

This was exactly what B-08 fixed for `place_bet`/`request_withdrawal`; the same fix had never
been applied to the path that moves the most money.

**Fixed:** `_trigger_payout` (`rounds/scheduler.py`) now has four phases instead of three —
read, *build* (network read only, no write), *persist the intent as `PendingTransaction(kind=
"payout", status="building")` and commit*, then broadcast and promote to `"pending"`. A
broadcast rejection now leaves that `"building"` row behind for the existing reconciler
(`tx/reconcile.py`) to resolve — its generic `building`/`pending` handling already covered a
`payout` kind correctly (including clearing `payout_txid` on abandonment), so no changes were
needed there.

Two guards were added alongside the two-phase write, since pool UTXOs are invisible to
`utxo_events` and so can never be released/reserved the way a user's own UTXOs are:
`_trigger_payout` now refuses to build a second payout for a round that already has a
non-terminal `PendingTransaction(kind="payout")`, and the payout builder excludes any UTXO
already referenced by *any* non-terminal payout transaction (`_reserved_payout_outpoints`) —
not just this round's — so a stale payout from an earlier round that the reconciler hasn't
abandoned yet can't be double-spent by a fresh attempt. `should_bump`/reconciler retry timing
around a fee-bumped payout is unaffected by this fix (see B-27, still open).

This made a payout retry *safe*; B-26 (below) is what makes one *automatic*. Regression
tests: `tests/unit/test_scheduler.py`
(`test_trigger_payout_persists_before_broadcasting`,
`test_trigger_payout_broadcast_failure_leaves_a_recoverable_row`,
`test_trigger_payout_skips_when_already_in_flight`,
`test_reserved_payout_outpoints_excludes_utxos_claimed_by_a_stale_payout`).

### B-26 — A transient failure at payout time wedges the lottery permanently

`rounds/scheduler.py`: if `listener.client is None` when `_trigger_payout` starts, it returned
without recording anything. `_trigger_payout` was called exactly once, from `_close_and_draw`,
and `_tick` ignored any round not in `open`/`closing`. The round stayed in `paying_out`, no new
round could open, and — unlike the generic `except Exception` branch — nothing was written to
`audit_log`, so `/admin` showed a stalled state with no explanation.

The payout runs immediately after a ~2-minute wait on a block, so an Electrum drop in that
window is entirely plausible. Same shape applied to `InsufficientFundsError`, a missing
`fee_address` and a missing winner user — none of them logged anything either.

CLAUDE.md listed "payout retry" as an accepted gap, but treated it as an operational
inconvenience; in practice it was a single point of failure that stopped the whole platform,
including across a process restart while a round was `paying_out`.

**Fixed:** two changes, matching the proposed fix exactly. (a) Every early return in
`_trigger_payout` — not connected, no `fee_address`, winner not found, insufficient pool
UTXOs, a build error, a rejected broadcast — now calls `_log_payout_failure` with a `reason`
string in the payload, so `/admin`'s audit log always shows *why* a round is stuck, not just
that it is. (b) `_tick` now handles `status == "paying_out"` by calling the new
`_retry_payout_if_due`, which re-invokes `_trigger_payout` unless the most recent
`payout_failed` audit entry for this round is younger than `_PAYOUT_RETRY_INTERVAL_SECONDS`
(60s) — throttled so a persistently-broken payout (e.g. an operator hasn't set `fee_address`
yet) doesn't retry, and re-log a failure, on every 5-second tick.

Because B-25 already made `_trigger_payout` idempotent (it no-ops if a non-terminal payout
`PendingTransaction` already exists for the round) and persists before broadcasting, this
retry is safe to fire on a process restart too: a round found `paying_out` at startup — whose
payout may have already broadcast, may never have been attempted, or may have been abandoned
by the reconciler — is retried the same way, closing the `paying_out` half of the "scheduler
doesn't resume mid-flight rounds after a restart" gap in CLAUDE.md (the "drawing"/block-wait
half is unrelated and still open, see B-36).

Regression tests: `tests/unit/test_scheduler.py`
(`test_trigger_payout_logs_a_failure_when_not_connected`,
`test_trigger_payout_logs_a_failure_when_fee_address_missing`,
`test_tick_retries_a_stuck_paying_out_round_with_no_recent_failure`,
`test_tick_throttles_retry_after_a_recent_payout_failure`,
`test_tick_retries_once_the_throttle_window_has_elapsed`).

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