Open the 2026-08-03 audit list (B-52 … B-72)
Third full-codebase audit, after the two earlier lists (B-01 … B-24 and B-25 … B-49) were emptied and BUGS.md deleted. Analysis only — nothing is fixed yet; each entry gets its own commit with its own regression test. Branched off feature/bug-reports rather than main on purpose: the audit describes this tree, /report-bug included (see B-68). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -0,0 +1,348 @@
|
||||
# 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
|
||||
|
||||
`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.
|
||||
|
||||
Fix directions: consolidate the pool between rounds (a sweep tx from pool to pool),
|
||||
or let a payout span more than one transaction, or — as an interim guard — cap
|
||||
participants per round and reject bets past the cap with a translatable error.
|
||||
Whatever the choice, it needs a regression test at n = MAX_TX_INPUTS + 1.
|
||||
|
||||
### 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.
|
||||
Reference in New Issue
Block a user