davideandClaude Sonnet 5.5
db24287ea5
The seed was the hash of the first block seen after the close, so a miner holding tickets
could withhold blocks until one made them win, the active Electrum server could pick which
real block was used by not announcing another (a failed corroboration even moved the draw to
the next block), and the participant order was the server-only `broadcast_at` with no
tie-break, so nobody could recompute the result.
- Open: every round gets a 32-byte `server_seed`, stored only Fernet-encrypted with the
master-xprv key (`server_seed_enc`), with its published `server_seed_hash`;
`draw_confirmations` is copied from round_config. The plaintext column is written only by
the reveal.
- Entering `drawing` (one transaction, `draw_committed` audit row): participants ordered by
bet txid, each `draw_position`, `participants_hash` (sha256 of `txid:vout:address\n`
lines; new `bet_vout`), `draw_height = tip_at_close + 1`. A tip of 0 keeps the round
`closing` (`draw_baseline_tip_unknown`, once per round). All persisted, so a resumed draw
reads rather than recomputes (the `_tick` branch itself is B-81).
- `_complete_draw` / `_wait_for_draw_block` replace `_wait_for_next_block`: wait for
`draw_confirmations` blocks from `draw_height`, then `ElectrumListener.agreed_block_hash`
asks every server, the active one included, for the header at that exact height (valid
work, >= 2 other answers, strict majority of all); no agreement means the same height is
asked again and `draw_header_corroboration_failed` is written once per distinct reported
hash. `draw_stalled` now counts from the last tip advance, or from when the quorum started
to fail. Seed = sha256(server_seed || block_hash || participants_hash), index = seed mod n.
- The winner, the seed and the reveal (`server_seed`, `winner_drawn` payload) are written in
one transaction; a round closing with no participants reveals too. A round opened before
this change (no commitment) is given a secret as it enters `drawing`.
- `GET /rounds/{id}/proof` (public, 404 `round_not_found`, translated in all 7 languages) and
the new fields on `GET /rounds/current`; `draw_confirmations` in `/admin/config` and the
/admin form; `server_seed_hash`/`draw_height` in `/admin/rounds`, never the secret.
- Alembic d3e8b15a7c42 (plain add_column / create_index, expression indexes untouched).
- Docs: the B-73 `*Stato:*`/`*Oggi:*` lines are gone, guides and CLAUDE.md describe the new
draw. Ambiguities resolved in api.md: the proof's participants also carry `amount_sats`
(RND-37 lists it); `seed` is decimal in the proof and hex in the audit row, as specified.
Tests changed because the spec changed: ElectrumListener.corroborate_header (asked only the
other servers about the block the active one announced) is replaced by agreed_block_hash and
its quorum tests ported; draw_winner takes the secret and the list fingerprint; the four
_wait_for_next_block scheduler tests are replaced by _wait_for_draw_block ones (fixed height,
confirmations, same-height retry, stall clock). New: test_draw_proof.py recomputes the winner
independently from the public proof, checks the plaintext secret is nowhere in the database,
the API or the log before the reveal, and that a second pass over a `drawing` round yields
the same winner. Mutation-checked: ordering by broadcast_at, an early reveal and asking the
tip instead of the draw height each fail tests.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
261 lines
13 KiB
Markdown
261 lines
13 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-79 — A payout normally has no change output, so it cannot be fee-bumped
|
||
|
||
The pool normally holds exactly the round's bet outputs, the payout spends them all
|
||
for exactly `pool_amount_sats`, so there is no change and `bump_fee` can never help.
|
||
A payout stuck in a mempool is also never abandoned (the server still knows it): the
|
||
round stays in `paying_out` and no new round opens.
|
||
|
||
Fix: every payout carries a **bump reserve** output of
|
||
`max(DUST_LIMIT_SATS, estimated_vsize × 10 sat/vB)`, taken from the commission share
|
||
and paid to the pool reserve address (`m/84'/746'/0'/1/0`, the old pool address);
|
||
RBF shrinks that output. `rounds_can_open` refuses a configuration in which a
|
||
round of 1 or of `MAX_PARTICIPANTS_PER_ROUND` participants could not cover winner
|
||
fee + reserve + dust commission, and `PUT /admin/config` refuses a bet amount or
|
||
fee rate that would fail the same check (spec: RND-40).
|
||
|
||
### B-81 — `drawing` wedges on any exception, not only on restart
|
||
|
||
The participant snapshot and `tip_at_close` live only in memory; `_tick` skips
|
||
`drawing`. Any exception during the wait (`database is locked` writing an audit row,
|
||
a malformed header) leaves the round in `drawing` forever.
|
||
|
||
Fix: persist `draw_height`, `participants_hash` and each participant's
|
||
`draw_position` when entering `drawing`; `_tick` re-enters the wait for a round in
|
||
`drawing`, idempotently. Together with B-73's fixed height and committed seed, a
|
||
resumed draw always yields the same winner.
|
||
|
||
### 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.
|