davideandClaude Sonnet 5.5
daa85cf4f0
The pool normally holds exactly the round's bet outputs and the payout spent them all for exactly the pool amount, so it had no change output: bump_fee could never accelerate it, and a payout stuck in a mempool (never abandoned while a server knows it) kept the round in `paying_out` and the whole lottery stopped. - build_payout_transaction always has three outputs: winner, fee address, and a bump reserve of max(DUST_LIMIT_SATS, estimate_vsize(inputs, 3) * 10 sat/vB) paid to the reserve address m/84'/746'/0'/1/0. The reserve comes out of the platform's 30%, the network fee still out of the winner's 70%. Input value beyond winner share + commission (only rounds opened before the per-round pool address) is folded into the reserve output if >= dust, left to the network fee otherwise. `change_address` is replaced by `reserve_address`. - `rounds.bump_reserve_sats` (Alembic e5a91c3d7b26, plain add_column) is written with the payout intent before the broadcast; /admin/rounds returns it. - bump_fee takes the increase from the reserve output for a payout (_find_reducible_output; a payout already in flight with a change to the pool still falls back to that), never from the winner or fee-address outputs. - payout_config_problem / rounds_can_open (RND-40): a bet amount and fee rate under which a round of 1 or of MAX_PARTICIPANTS_PER_ROUND participants could not cover the network fee, the reserve and dust outputs opens no round (lottery_configured=false); PUT /admin/config refuses such a bet amount or fee rate with 422 config_not_payable, judged on the resulting values, since the payout reads the fee rate live. Ambiguity resolved: the reserve output and any excess input are one output (RND-23 says one output to the reserve address), and bump_reserve_sats records that output's initial value. DAT-22's "abandon clears all amounts" stays with B-96. Tests changed because the spec changed: the four test_payout_builder tests asserted the old two/three-output shape with change to the pool and the fee address receiving the whole 30%; the whitespace fee_address test now spells out the amounts rounds_can_open reads. New: reserve sizing, payout with 1 and with 400 participants (inputs = outputs + fee to the satoshi), exact commission/reserve/dust boundary, excess folded or left to the fee, a real payout bumped out of its reserve with the other outputs byte-identical and bumped five times, the old reserve-less shape still raising RbfError, the scheduler output addresses and recorded amounts, payability boundaries found independently by bisection, no round / unconfigured flag for an unpayable config, a round in progress surviving it, PUT /admin/config refusals and the migration column. Mutation-checked: dropping the reserve lookup in bump_fee and the payability check in rounds_can_open each fail tests. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
235 lines
11 KiB
Markdown
235 lines
11 KiB
Markdown
# BUGS.md — open findings
|
|
|
|
Two audits currently have open entries: 2026-08-03 (B-71, B-72, below) and the
|
|
2026-09-28 specification review (B-73 … B-98, at the end of the file).
|
|
|
|
## 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.
|
|
|
|
---
|
|
|
|
## Low — documentation and consistency drift
|
|
|
|
### 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.
|
|
|
|
---
|
|
|
|
# Audit of 2026-09-28 — specification review (B-73 … B-98)
|
|
|
|
Fourth audit, run against the new specification in `docs/` rather than the code
|
|
alone: four independent reviews (coin/chain mechanics, money state machine,
|
|
DevOps, spec cross-validation), with the critical claims re-verified in code.
|
|
The target behaviour of every entry below is now written into `docs/` as
|
|
requirements marked `*Stato:* da implementare (B-nn)`; the Italian overview, the
|
|
design decisions and the implementation order are in
|
|
[docs/piano-correzioni.md](docs/piano-correzioni.md). The code has **not** been
|
|
changed yet. Same convention as above: one commit per entry, with its own
|
|
regression test, spec + code + test together.
|
|
|
|
Design decisions taken with the owner on 2026-09-28 (they change existing
|
|
decisions, see `docs/limiti-noti.md`):
|
|
|
|
- **Draw by commit-reveal.** The winner must not be computable by anyone who
|
|
controls block production, since PLM's hashrate (~22 TH/s) is rentable.
|
|
- **Never release funds on doubt.** A money-moving row is rolled back only on proof
|
|
(an explicit rejection, or its inputs confirmed-spent elsewhere), never on a
|
|
timeout or on one server's silence.
|
|
- **One pool address per round**, so a round's money, history and payout are
|
|
isolated from every other round's.
|
|
- **Deposits need 6 confirmations** (configurable), everything else keeps 1: the
|
|
deposit is the only input a user can double-spend.
|
|
- **Contain reorgs instead of pretending they don't happen:** detect them, pause
|
|
on deep ones, and pay out only what is verifiably on chain.
|
|
|
|
## Critical
|
|
|
|
### B-82 — A deposit reorged out after a bet leaves the round unpayable
|
|
|
|
Deposits are credited at 1 confirmation. A user can bet, then (if they lost)
|
|
double-spend the deposit the bet spent with a private chain: the bet disappears but
|
|
its participant stays `confirmed` and counted, and the payout fails "insufficient
|
|
pool UTXOs" forever with every other player's money stuck. Immature coinbase outputs
|
|
are also credited at 1 confirmation (maturity is 120).
|
|
|
|
Fix: deposits credited at `deposit_confirmations` (round_config, default 6),
|
|
coinbase outputs at 120. Before building a payout the round address's UTXOs are
|
|
verified with the quorum; participants whose bet outpoint is gone are marked
|
|
`reorged_out` and the 70/30 split is computed on what is actually there. If the
|
|
winner's own bet is gone the payout is held (`payout_blocked`) and the lottery pauses.
|
|
|
|
## High
|
|
|
|
### B-87 — The RBF bump can leave a dust change output
|
|
|
|
`bump_fee` only requires change > fee delta. A 294-sat change (full-balance
|
|
withdrawal) becomes ~153 sat after the minimum bump and every replacement is refused
|
|
by relay, every 30s.
|
|
|
|
Fix: when a bump would leave change below `DUST_LIMIT_SATS`, the change output is
|
|
dropped and its value goes to the fee (the replacement is then final); `RbfError`
|
|
and every refused bump write a `rbf_bump_failed` audit row.
|
|
|
|
### B-89 — Key problems surface only at first use
|
|
|
|
Startup checks only that the key file exists and `XPRV_ENCRYPTION_KEY` is non-empty;
|
|
a wrong key or a different xprv is found at the first registration or payout, and a
|
|
different xprv silently moves the pool address. The scripts accept any xprv and write
|
|
files `0644`.
|
|
|
|
Fix: at startup decrypt the key and check the pool reserve address and a sample of
|
|
stored user addresses; refuse to serve on mismatch. The scripts refuse an xprv whose
|
|
derived addresses differ from the DB's and write `0600`.
|
|
|
|
### B-90 — Restoring a backup is unsafe
|
|
|
|
A restored DB reopens rounds that were already drawn and paid, and reassigns the
|
|
derivation indexes of users registered after the backup to new users — who then
|
|
receive the old users' funds.
|
|
|
|
Fix: `START_PAUSED=true` starts with the lottery paused; at startup the next 20
|
|
indexes past the highest assigned are checked for on-chain history and, if any has
|
|
some, registration is refused until the operator reconciles (`restore_gap_detected`);
|
|
per-round addresses (B-80) and B-82's pre-payout check make a replayed round
|
|
unpayable instead of payable twice.
|
|
|
|
### B-91 — Host and supply-chain hardening
|
|
|
|
The container runs as root, `.env` and key files are world-readable, dependencies are
|
|
unpinned `>=` and base images float, secrets sit in env vars visible to
|
|
`docker inspect`.
|
|
|
|
Fix: non-root `USER`, `0600`/`0700` permissions, a hash-pinned lock file,
|
|
digest-pinned base images, the Fernet key mounted as a file rather than an env var.
|
|
|
|
## Medium
|
|
|
|
### B-92 — No health, readiness or real audit browsing
|
|
|
|
`/health` always answers ok; nothing exposes Electrum state; the audit endpoint
|
|
returns only the latest 500 rows unfiltered; registration writes no audit row; the
|
|
access log records Caddy's IP; app logs go only to the file.
|
|
|
|
Fix: `GET /health/ready` (Electrum connected, tip age < 360s, scheduler heartbeat <
|
|
30s) used as the container healthcheck; Electrum status in `/admin`; audit filters by
|
|
event type, round, user and time, with pagination; `user_registered` audit row with
|
|
index and address; uvicorn proxy headers from Caddy only; logs also to stdout.
|
|
|
|
### B-93 — Deploys and rollbacks
|
|
|
|
A deploy with an open SSE stream never shuts down gracefully (uvicorn waits, Docker
|
|
kills after 10s); an older image cannot start on a newer DB; images are untagged;
|
|
`SITE_ADDRESS` passed per command reverts to `localhost` on the next plain
|
|
`docker compose up`.
|
|
|
|
Fix: close SSE streams on shutdown, `stop_grace_period: 30s`, await task
|
|
cancellation; tag images per release; `SITE_ADDRESS` in `.env` with `${SITE_ADDRESS:?}`;
|
|
documented pre-deploy backup and downgrade procedure.
|
|
|
|
### B-94 — Account and endpoint abuse limits
|
|
|
|
`change-password` has no rate limit, no audit and no maximum length; login fields
|
|
have no length bounds; `POST /bug-reports` is anonymous and unlimited;
|
|
`GET /admin/users` has no pagination; admin, SSE and framework errors are bare
|
|
strings; static JS/CSS have no `Cache-Control`.
|
|
|
|
Fix: throttle and audit (`password_changed`) the password change, cap every
|
|
credential field at 256, rate-limit bug reports per IP, paginate the user list,
|
|
structured errors everywhere, `Cache-Control: no-cache` on static files.
|
|
|
|
### B-95 — Round parameters read live instead of snapshotted
|
|
|
|
Only duration and cooldown are snapshotted at open; bet amount, `fee_address` and the
|
|
new `draw_confirmations` are read live, so an edit mid-round changes the round in
|
|
progress (e.g. redirects its commission).
|
|
|
|
Fix: snapshot `bet_amount_sats`, `fee_address` and `draw_confirmations` on the round
|
|
at open. Optional per-round cap `max_round_pool_sats` (round_config, 0 = disabled):
|
|
once reached, further bets get `round_full`.
|
|
|
|
### B-96 — Smaller state inconsistencies
|
|
|
|
A reduced full-balance withdrawal is not re-checked against the minimum; an abandoned
|
|
payout leaves `winner_amount_sats`/`fee_amount_sats` set; a promoted payout writes no
|
|
`payout_sent` and a promoted bet no `bet_placed`; the reconciler pings SSE for every
|
|
row it checks; concurrent refreshes of one user can collide on the unique outpoint
|
|
and end the Electrum session; `listener.client` is read outside the per-user lock;
|
|
a bet covered only by pending change gets `insufficient_balance` instead of
|
|
`balance_pending_confirmation`. A round waiting in `closing` on a bet that never
|
|
leaves the mempool gives no signal at all.
|
|
|
|
Fix: each as described, one regression test per item; for the `closing` wait, a
|
|
`closing_stalled` audit row every 1800s past the deadline and `closing_waiting_since`
|
|
on `GET /rounds/current` (spec: TX-29).
|
|
|
|
### B-98 — Coin selection stops at the first covering UTXO, refusing a payable bet
|
|
|
|
Found while writing the spec on 2026-09-28 (medium). `select_utxos` returns as soon
|
|
as the selected total covers the target. When that happens exactly (a 10 PLM coin
|
|
for a 10 PLM bet), the change is 0: the bet is refused with
|
|
`balance_leaves_no_change` and a withdrawal is reduced by `DUST_LIMIT_SATS`, even
|
|
though a second coin would have left a proper change output.
|
|
|
|
Fix: select until the total covers `target + DUST_LIMIT_SATS` (still largest-first,
|
|
still capped at `max_inputs`); only when every allowed coin together cannot reach it
|
|
fall back to today's behaviour (refuse the bet, reduce the withdrawal). Spec:
|
|
`docs/scommesse-e-prelievi.md` BET-24.
|