davideandClaude Opus 5
8dd913ec59
The max-amount checkbox sends amount_sats == the whole confirmed balance, so change came out at 0, the change output was dropped, and the transaction had a single output. bump_fee has nothing to shrink there: it raised RbfError every 30s until the reconciler abandoned the row six hours later. The RBF single-change-output limitation was a documented gap, but the UI made it the *default* withdrawal path. The extra-input fallback would not have helped this case: a transaction moving the entire balance already spends every UTXO the sender has. So the fix is at build time — build_signed_transaction never produces a change output below DUST_LIMIT_SATS, and never folds it into the fee either: - withdrawals pass reduce_amount_to_keep_change=True and move a dust limit less. The fee already comes out of the withdrawn amount by design, so this is the same rule applied a little harder, and Withdrawal.amount_requested_sats vs amount_sent_sats already existed to record the difference. - bets don't: the bet is a fixed price that can't be quietly reduced. A balance exactly equal to the bet is refused with balance_leaves_no_change (translated into all 7 languages, carrying required_extra_sats), which turns "a user's balance must never exactly equal the bet" from a documented assumption into an enforced one — and stops an unbumpable bet from holding a round open until the reconciler gives up on it. bump_fee's no-change guard stays: a single-output tx broadcast before this change can still be pending across the deploy, and it must fail loudly rather than start shrinking a recipient's output. Its test now hand-builds that shape, precisely because the builder no longer will. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8.7 KiB
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.
The list opened at B-52 … B-72 and holds only what is still open: a finding is
removed from this file once it is fixed, and is not listed here afterwards. 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' is the record of how any closed finding was closed.
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
(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.
Medium — correctness and robustness
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_versionimplements 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-bugis a placeholder; it is fully implemented and translated, with admin triage. Only/guidais 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.pyandapp/api/routes/bug_reports.py. - README links
flowchart.mmd, which does not exist (the diagrams live inflowchart/), describesdocs/running-the-server.mdas covering "local venv vs. Docker" (that workflow was removed in B-44), and links the anchorCLAUDE.md#tech-stack-mvp, which no longer exists.
B-69 — stale in-code comments
app/tx/reconcile.py:206calls the payout retry "a future payout-retry routine — still an open gap"; it exists (B-26).app/db/base.py:9says "five concurrent background tasks"; there are six.app/auth/routes.py:31cites 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.