plm-lottery/BUGS.md

# BUGS.md — audit of 2026-08-03

Third full-codebase audit, opened after the 2026-07-26 (B-01 … B-24) and
2026-07-27 (B-25 … B-49) lists were emptied. Numbering continues from the last
fixed finding, B-51.

Nothing in this list is fixed yet — it is the analysis pass only. Per CLAUDE.md's
convention each entry gets its own commit with its own regression test, and the
`B-nn` marker goes in a comment next to the fix so `git log --all --grep 'B-nn'`
finds it later.

State of the tree at audit time: 264 unit tests, all passing; `tests/integration/`
still empty; withdrawal and the RBF bump path still never live-broadcast.

Verified as *not* broken while looking for these: i18n key parity (146 identical
keys across all 7 languages), HTML escaping of every user-controlled value in
`admin.js`, the Alembic chain (linear, single head, matching `models.py`),
strictly-integer satoshi arithmetic everywhere, and `.gitignore` coverage of
secrets/DB/logs (nothing sensitive is tracked in git).

Severity is about consequence, not likelihood:
**critical** = money stuck or lost, or the lottery stops;
**high** = a security control that does not hold;
**medium** = wrong behaviour with a bounded blast radius;
**low** = drift between documentation and code.

---

## Critical

### B-52 — a round with more than ~50 participants deadlocks the platform permanently — **FIXED**

`app/wallet/psbt_builder.py:42` (`MAX_TX_INPUTS = 50`),
`app/rounds/scheduler.py:349-373`.

Every confirmed bet leaves exactly one UTXO on the pool address, and the payout's
selection target is `winner_share + commission`, i.e. the whole pool — so it needs
*all* n bet UTXOs as inputs. At n ≥ 51 `select_utxos` raises `too_many_inputs`,
`_trigger_payout` records `payout_failed`, and `_retry_payout_if_due` re-attempts
every 60 s forever. The round stays `paying_out`, so `open_new_round_if_needed`
never opens another round: the lottery halts, the pool is unspendable through the
normal path, and the only way out is a manual consolidation with the pool key.

CLAUDE.md presents B-48's input cap purely as a fragmented *user* address problem.
The pool case is structural rather than an edge case: participant count alone
causes it, with the default bet amount and no unusual deposit pattern.

**Fixed** by moving the limit from where it was *discovered* to where it can still be
*enforced*:

- `select_utxos` takes the cap as a parameter. Bets and withdrawals keep
  `MAX_TX_INPUTS = 50` (a user-protection limit: the fee comes out of the amount
  they are moving); the payout uses the new `MAX_PAYOUT_TX_INPUTS = 500`, since the
  pool's UTXO count is just the number of bets and the fee comes out of a 70% share
  of the whole pool. 500 inputs is ~34 kvB, about a third of the 100 kvB relay
  standardness budget; signing that many costs ~0.4 s of event loop, once per round,
  in a background task.
- `place_bet` refuses the bet past `MAX_PARTICIPANTS_PER_ROUND = 400` with a new
  `round_full` error (400, translated into all 7 languages), counting every
  participant row rather than only the confirmed ones. The cap sits below the input
  cap so the payout keeps headroom for pool change accumulated by earlier rounds.

The invariant is now "a round can always be paid out", enforced before any of the
401st player's money moves. A round already wedged with 51–499 participants pays out
by itself on the next `_retry_payout_if_due` tick.

Not addressed, and deliberately so: periodic pool consolidation, which is what would
be needed to go beyond this order of magnitude (see the audit discussion — it needs a
new PendingTransaction kind, must not run mid-round, and would force the payout math
to tolerate a pool short of its exact target). Raising these two constants covers
anything up to ~1400 participants first.

### B-53 — a bet can pay into the pool and still be left out of the draw

`app/bets/service.py:80-92`, `app/rounds/scheduler.py:101-129`.

`place_bet` commits its participant row as `building` *before* broadcasting
(the deliberate two-phase write of B-08). The scheduler counts in-flight
participants in one session and then reads the `confirmed` participants in a
second, separate session. A bet that passed `round_accepts_bets` just before the
deadline can commit its phase-1 row *between* those two queries: the count saw
zero, so the round draws and pays out, while the new row — not yet `confirmed` —
is excluded from `participants`. The bet then confirms normally and its sats land
in the pool address, credited to no round and to no participant. There is no
refund path, and the money silently improves the *next* round's payout change.

The window is one task switch wide, but both queries do real DB I/O, so it is
reachable rather than theoretical.

Fix directions: re-check the in-flight count inside the same transaction that
snapshots the participants (and abort the close if it is non-zero), or make the
deadline authoritative at the row level so a bet cannot commit against a round
whose timer has expired.

### (not new) `drawing` does not resume after a restart

Already tracked as an accepted gap in CLAUDE.md's "Known gaps", not re-numbered
here. Worth restating in context: with `restart: unless-stopped` on the app
container, this is the one state that gets stuck with money in play, and it
remains the last prerequisite for running unattended.

---

## High — security

### B-54 — `X-Forwarded-For` is trusted blindly, so every IP-keyed control is bypassable

`app/api/client_ip.py:14-17`, `Caddyfile`.

`client_ip()` returns `xff.split(",")[0]`. Caddy's `reverse_proxy` *appends* the
real peer address to an incoming `X-Forwarded-For` rather than replacing it, so
the first element is whatever the client sent. Rotating a fake value per request
defeats `login_ip`, `register_ip` (B-33) and the per-IP SSE subscriber cap
(B-38) outright; only the per-username login bucket still bites.

Fix: take the *last* element of the header, or force it at the proxy with
`header_up X-Forwarded-For {remote_host}` and keep reading element 0. The proxy-side
fix is the more robust of the two, since it makes the app's assumption true.

### B-55 — Argon2 hashing runs on the event loop

`app/auth/security.py:20-39`, called from `app/auth/routes.py` and
`app/api/routes/users.py`.

`hash_password`/`verify_password` are synchronous and cost tens of milliseconds
each, so every login, registration and password change blocks the whole process —
including all six background tasks (scheduler, confirmation poller, RBF bumper,
listener, both reconcilers). A burst of unauthenticated login attempts (which,
per B-54, is not effectively throttled) is a cheap denial of service that also
delays draws and confirmations.

Fix: run both through `starlette.concurrency.run_in_threadpool`.

### B-56 — `RateLimiter._buckets` is never pruned

`app/auth/rate_limit.py:37`.

The dict grows without bound, keyed by attacker-chosen strings (arbitrary
usernames, and — via B-54 — arbitrary IPs). `decay_seconds` ages a bucket's
*counter* but never removes the entry.

Fix: evict entries whose last failure is older than `decay_seconds` (opportunistically
on `record_failure`, or on a periodic sweep), and cap the dict size.

### B-57 — username matching is case-sensitive while the login throttle key is not

`app/auth/routes.py:126` (`body.username.lower()`) vs `:132`
(`User.username == body.username`).

Two consequences: `Bob` and `bob` are separate accounts sharing a single rate-limit
bucket (one locks the other out), and registration happily accepts near-duplicate
usernames, which is an impersonation vector on a custodial system.

Fix: make username uniqueness case-insensitive (store a normalized form, or a
functional unique index) and key the throttle on the same normalized value.

### B-58 — the registration throttle counts successes as failures and is IP-only

`app/auth/routes.py:66-71`.

`record_failure` is called on every registration attempt, successful ones
included. Five legitimate signups from one shared/NAT address lock the sixth real
user out with exponential backoff up to 600 s — while an attacker skips the limiter
entirely through B-54. The intent (bounding accounts per source) is reasonable;
the current shape punishes only honest users. The inline comment also cites B-31
(the resubscribe finding) where it means B-33.

Fix: keep an accounts-per-IP quota if that is the goal, but express it as a quota
rather than as failure backoff, and fix the B-nn reference.

### B-59 — deposit crediting is not corroborated, unlike external-spend detection

`app/deposits/service.py:31-45`, `app/electrum/listener.py:401-433`.

A candidate external spend is corroborated across the other configured servers
before it can reduce a balance (B-29), but `value` and `height` for a *credit*
are taken from the single active connection with no cross-check. A hostile or
broken server can inflate a user's displayed balance with outpoints that do not
exist. The blast radius is bounded — a bet or withdrawal built on a phantom UTXO
is refused at broadcast and the rollback releases it — but it wedges the user's
balance display and burns build attempts.

Fix: either corroborate credits the same way (symmetry with B-29), or record the
asymmetry explicitly as an accepted risk in CLAUDE.md with its bound stated.

### B-60 — `POST /admin/bug-reports/{id}/status` writes no audit entry

`app/api/routes/admin.py:404-419`.

Every other admin mutation (config edit, pause/resume, privkey export, password
reset) is audit-logged. This one is not, so a report can be silently marked
`resolved` with no trace — and with one shared `ADMIN_TOKEN` and no per-admin
identity, the audit log is the only accountability there is.

---

## Medium — correctness and robustness

### B-61 — config edits apply retroactively to the round already in progress

`app/rounds/scheduler.py:71`, `app/rounds/service.py:51-61`,
`app/api/routes/admin.py:106-128`.

`round_duration_seconds` is read live on every tick and on every bet check, and the
deadline is computed as `opened_at + duration`. Lowering it from 600 to 60 while a
round is 300 s in closes that round instantly; raising it moves the `closes_at`
clients are already counting down to. `round_cooldown_seconds` has the same
property for the gap after a close.

B-11 fixed exactly this class of problem for `bet_amount_sats` (an in-progress
round's advertised jackpot must not move when an operator edits the bet amount);
the timing fields were left live.

Fix: snapshot the duration (and cooldown) onto the `Round` row when it opens and
read them from there, leaving the config row as the value for the *next* round.

### B-62 — "withdraw the full amount" reliably produces an unbumpable transaction

`app/static/app.js:798-807`, `app/wallet/psbt_builder.py:138-141`,
`app/tx/broadcast.py:150-152`.

The max-amount checkbox sends `amount_sats == myBalanceSats == total_in`, so
`change == 0`, the change output is dropped, and the tx has a single output. `bump_fee`
then finds no change output to absorb the increase and raises `RbfError` every 30 s
until the reconciler abandons the row six hours later. The RBF single-change-output
limitation is a documented gap, but the UI makes it the *default* withdrawal path
rather than a corner case (the same applies to an exact-amount bet).

Fix directions: leave a change output above `DUST_LIMIT_SATS` when the requested
amount would consume the whole input total (i.e. reserve a little), or warn in the
UI that a full-balance withdrawal cannot be fee-bumped, or implement the
extra-input RBF fallback.

### B-63 — `tip_height == 0` window right after connecting can seed a draw from a pre-close block

`app/electrum/listener.py:160-167`, `app/rounds/scheduler.py:153`.

`_run_once` assigns `self.client` *before* `subscribe_headers()` returns, so there
is a window in which the client looks alive while `tip_height` is still 0 and
`tip_header_hex` is `None`. A `_close_and_draw` entering that window records
`tip_at_close = 0`, and the first header applied — the current tip, a block mined
*before* the round closed — satisfies `tip_height > tip_at_close` and becomes the
draw's entropy. The draw must use a block that did not exist at close time; a block
whose hash was already public before betting closed is not the guarantee the
flowchart describes.

Fix: publish `self.client` only after the first header has been applied, or refuse
to draw while `tip_header_hex is None` / `tip_height == 0`.

### B-64 — `_apply_header` accepts a same-height header without the chaining check

`app/electrum/listener.py:271-296`.

The chain check only runs for `height == self.tip_height + 1`. A header at exactly
the current tip height replaces `tip_header_hex` after passing only the
self-target check — which, as the docstring of `header_meets_its_own_target`
already notes, a server can satisfy with a self-declared easy target. Separately,
a header with no `hex` field sets `tip_header_hex = None`, discarding a tip we
otherwise accepted.

Fix: treat a same-height header as either ignorable or as a reorg signal rather
than silently replacing the entropy source, and don't clear `tip_header_hex` on a
hex-less notification.

### B-65 — `/rounds/current` counts unconfirmed participants; the draw and payout do not

`app/api/routes/rounds.py:131-143` vs `app/rounds/scheduler.py:126`.

`participant_count` and `jackpot_sats` are computed over *all* `round_participants`
rows, while the draw and the payout only use `status == "confirmed"`. So the
advertised jackpot can exceed what is actually paid out, and a participant whose
bet is later abandoned appears in the count and then vanishes from it.

Fix: either count only `confirmed` (and accept that a fresh bet takes a block to
show up), or expose the two figures separately (confirmed vs in-flight) so the
number on screen and the number that gets paid agree by construction.

### B-66 — nothing stops rounds from opening with no `fee_address` configured

`app/rounds/config.py:12-17`, `app/rounds/scheduler.py:330-335`.

A fresh instance starts with `fee_address = ""`. Rounds open, bets are accepted and
confirm, and only then does the payout refuse to build — leaving the round in
`paying_out`, retrying every 60 s, with the audit log as the only signal.

Fix: refuse to open a round while `fee_address` is unset (and surface it on
`/admin` and as a maintenance-style banner), so the failure happens before anyone's
money is committed.

### B-67 — `/qr/{address}` is unauthenticated, synchronous and only shape-validated

`app/api/routes/qr.py:11-21`.

`qrcode.make` runs on the event loop, so the endpoint is a cheap CPU amplifier for
an unauthenticated caller, and the regex accepts any `plm1[a-z0-9]{10,90}` string
without validating the bech32 checksum — so it happily renders a QR for a
non-address.

Fix: validate with `is_valid_plm_address` (already used by withdrawals and by the
admin `fee_address` validator), and either offload the render or cache it per
address.

---

## Low — documentation and consistency drift

### B-68 — CLAUDE.md and README describe a state the code has moved past

- CLAUDE.md's tech-stack line still says JWT has **no revocation (B-34)**; `token_version`
  implements exactly that revocation (`app/db/models.py:29`, `app/auth/dependencies.py:26`).
- "Known gaps" still says **no rate limiting anywhere (B-33)**; login and registration
  are throttled (`app/auth/rate_limit.py`). What is genuinely still unthrottled is
  bets, withdrawals and the admin endpoints — that is the claim worth keeping.
- "Known gaps" still says **`/report-bug` is a placeholder**; it is fully implemented
  and translated, with admin triage. Only `/guida` is still a stub.
- Test counts are stale in three places: 264 actual, CLAUDE.md says 253 twice,
  README says 232.
- The code map omits `app/auth/rate_limit.py`, `app/api/client_ip.py` and
  `app/api/routes/bug_reports.py`.
- README links `flowchart.mmd`, which does not exist (the diagrams live in
  `flowchart/`), describes `docs/running-the-server.md` as covering "local venv vs.
  Docker" (that workflow was removed in B-44), and links the anchor
  `CLAUDE.md#tech-stack-mvp`, which no longer exists.

### B-69 — stale in-code comments

- `app/tx/reconcile.py:206` calls the payout retry "a future payout-retry routine —
  still an open gap"; it exists (B-26).
- `app/db/base.py:9` says "five concurrent background tasks"; there are six.
- `app/auth/routes.py:31` cites B-31 where it means B-33 (see B-58).

### B-70 — the flowchart's WITHDRAW precondition is not implemented as written

`flowchart/platform-overview.mmd:39` (node E1) states a withdrawal cannot happen
together with a bet in progress. The code only serializes the *builds* through the
per-user lock (`app/tx/locks.py`): a withdrawal is accepted while a bet is still
unconfirmed, as long as confirmed UTXOs cover it.

CLAUDE.md declares every node and edge label of the diagrams a behaviour that must
be implemented as described, so one of the two has to move — most likely the
diagram, since the lock already prevents the actual double-spend hazard, but that
is a decision, not a cleanup.

### B-71 — `.env` points `MASTER_KEY_PATH` at a second copy of the master key

CLAUDE.md's deployment section prescribes pointing `MASTER_KEY_PATH` at the
host-side `./data/keys/master.xprv.enc` so the venv scripts and the container read
one file. `.env` instead sets `./master.xprv.enc`, and both files now exist in the
working tree (both gitignored). They were verified during this audit to decrypt to
the *same* xprv, so nothing has diverged yet — but `scripts/decrypt_master_key.py`
reads a different file from the one the container uses, and a future
`generate_master_key.py --overwrite` would split them silently, with an ops
recovery path that then reports the wrong key.

The same duplication exists for the database (`./plm_lottery.db` next to
`data/db/plm_lottery.db`), which is less dangerous but equally confusing.

Fix: set `MASTER_KEY_PATH=./data/keys/master.xprv.enc` in `.env`, delete the stray
root copy once confirmed redundant, and state the same for `DATABASE_URL`.

### B-72 — `docs/setup.md` still frames setup as "locally or via Docker"

`docs/setup.md:1-10` lists Python as a prerequisite "for the local/venv workflow"
and describes the master-key step as local *or* Docker, while B-44 made Docker the
only supported way to run the server (`docs/running-the-server.md` and README were
updated, this file was not). The venv genuinely is needed for tests, migrations and
the key scripts — the wording just needs to say that instead of implying a second
way to run the server.