davide
17c557b8a3
bump_fee computed fee_delta as new_fee - old_fee, falling back to a flat 1-satoshi bump whenever that came out zero or negative - which happened whenever old_fee (the actual fee paid, from real prevout amounts) already exceeded the naive target, e.g. because dust change had been folded into the original fee (psbt_builder.py's DUST_LIMIT_SATS handling). A 1-satoshi total increase is nowhere near BIP125 rule 4's minimum (the replacement must pay at least the incremental relay fee rate times its own vsize more than what it replaces), so the node rejected it every time - and since bump_fee raised before touching `pending`, the next tick retried with identical parameters every 30 seconds, forever. Separately, the fee rate climbed by 1 sat/vB every bump with no ceiling. fee_delta is now max(target_fee - old_fee, vsize * the incremental relay rate) - always at least the relay-mandated minimum regardless of what the naive arithmetic produces. pending.fee_rate_sat_vb is set to the actual resulting rate rather than the naive target, so a later bump's arithmetic starts from what's really being paid instead of drifting from it. Once a transaction reaches MAX_FEE_RATE_SAT_VB (a new constant, 10,000 sat/vB, shared with RoundConfig.fee_rate_sat_vb's existing admin-facing bound so the two can't drift apart - the same reason MIN_PASSWORD_LENGTH is shared elsewhere) bump_fee refuses to bump further; the reconciler abandons it if it never confirms (B-27) instead of this retrying forever. Suite grows from 185 to 187 tests. BUGS.md moves B-32 to Previously fixed.
242 lines
13 KiB
Markdown
242 lines
13 KiB
Markdown
# Known bugs
|
|
|
|
A second full-codebase audit on 2026-07-27 found **25 further issues** (4 critical, 6 high,
|
|
7 medium, 8 low), listed below as B-33 … B-49. B-25 through B-32 are fixed (see "Previously
|
|
fixed" below) — no Critical-severity finding remains open; the other 17 are High/Medium/Low.
|
|
The 139-test suite was green at the time of the audit, so none of these were caught by existing
|
|
coverage — every fix lands with a regression test (the eight fixes so far brought the suite
|
|
from 139 to 187).
|
|
|
|
The recurring pattern across the open findings is worth stating once: the code is rigorous
|
|
about the failure modes that have actually been hit, and silent about the ones that have not.
|
|
The payout phase is now fully recoverable; the "drawing" phase (waiting on a block) still has
|
|
no equivalent resume-after-restart or stall visibility (B-36).
|
|
|
|
For limitations that are accepted by design rather than bugs (single-shared-token admin auth,
|
|
single-process assumptions, no user-facing history, etc.), see "Known gaps / TODO" in
|
|
[CLAUDE.md](CLAUDE.md).
|
|
|
|
---
|
|
|
|
## High
|
|
|
|
### B-33 — No brute-force protection on a custodial wallet
|
|
|
|
`POST /auth/login` (`auth/routes.py:83`) has no rate limiting, no lockout, no delay and no
|
|
CAPTCHA, and the password minimum is 8 characters. Argon2 slows a single attempt but not a
|
|
patient distributed attack against an enumerable username list — and `409 username_taken` on
|
|
registration is a perfect enumeration oracle.
|
|
|
|
"No rate limiting" is listed as a generic known gap; on a system where guessing a password
|
|
means **withdrawing someone's funds**, it deserves to be tracked separately and treated as a
|
|
blocker.
|
|
|
|
**Proposed fix.** Per-username *and* per-IP throttling with exponential backoff on failed
|
|
logins (`slowapi`, or a small DB-backed counter — but note the in-process caveat if workers
|
|
are ever scaled). Return an identical response for unknown-user and wrong-password. Rate-limit
|
|
registration too, which also bounds B-31's attacker-controlled user count.
|
|
|
|
### B-34 — Password change and admin reset do not invalidate existing sessions
|
|
|
|
Neither `/users/me/change-password` nor `/admin/users/{id}/reset-password` invalidates
|
|
already-issued JWTs (24h default lifetime, no revocation, no `token_version` on the user). The
|
|
admin reset exists precisely for the "account compromised" case and **does not evict the
|
|
attacker**.
|
|
|
|
**Proposed fix.** Add a `token_version` (or `password_changed_at`) column on `User`, embed it
|
|
in the JWT claims, and reject any token whose value is stale in
|
|
`auth/dependencies.py:get_current_user`. Bump it on both endpoints.
|
|
|
|
---
|
|
|
|
## Medium
|
|
|
|
### B-35 — Every API timestamp is naive, so the frontend renders it in the wrong timezone
|
|
|
|
Verified empirically: the `DateTime` columns carry no timezone, so SQLite returns naive
|
|
datetimes and `.isoformat()` produces `2026-07-27T06:56:47.489110` — **no `Z`**. JavaScript's
|
|
`new Date()` parses that as **local time**, so every date in `/admin` (rounds, pending
|
|
transactions, audit log, via `fmtDate` in `app/static/admin.js:50`) and `created_at` in
|
|
`/users/me` display two hours off in Italy.
|
|
|
|
The codebase knows about this — `api/routes/rounds.py` calls `.replace(tzinfo=timezone.utc)`
|
|
on `opened_at` explicitly — but the fix was never applied systematically.
|
|
|
|
**Proposed fix.** Make the columns `DateTime(timezone=True)` (Alembic migration) so the value
|
|
round-trips as aware, rather than patching each call site. Until then, at minimum a shared
|
|
serialization helper that stamps UTC, used by every `.isoformat()` in the API layer.
|
|
|
|
### B-36 — `_wait_for_next_block` waits forever, with no timeout and no visibility
|
|
|
|
`rounds/scheduler.py:158-161` loops until a higher block arrives. No timeout, no log, no audit
|
|
entry. If the connection dies in a way that stops the tip advancing, the round sits in
|
|
`drawing` indefinitely and **the admin panel shows nothing at all** — just a frozen state with
|
|
no explanation.
|
|
|
|
**Proposed fix.** Log progress periodically while waiting, and past a threshold (a few
|
|
multiples of the 120s block time) write a `draw_stalled` audit entry so it surfaces in
|
|
`/admin`. Surface the wait in `GET /rounds/current` too (it already returns
|
|
`chain_tip_height`; `draw_waiting_since` would make the stall self-evident to users).
|
|
|
|
### B-37 — Displayed balance and spendable balance diverge, and the error does not explain it
|
|
|
|
After a bet the change is unconfirmed, so `cached_balance_sats` ≈ 0 while the UI shows
|
|
`pending_balance_sats` (the real figure). A withdrawal attempted right after validates against
|
|
**confirmed** UTXOs (`withdrawals/service.py:54-60`) and answers `insufficient_balance`.
|
|
|
|
The user sees "1.000 PLM" on screen and is told they have no funds. The mechanism is a
|
|
documented design decision, but the error does not distinguish "you don't have the money" from
|
|
"your money is waiting to confirm" — two very different situations for whoever reads it.
|
|
|
|
**Proposed fix.** A distinct error code (e.g. `balance_pending_confirmation`) raised when the
|
|
requested amount is covered by `pending_balance_sats` but not by the confirmed balance,
|
|
carrying the pending amount in `params`, plus its `error.*` entry in all 7 languages. The
|
|
withdrawal form should also cap/hint the max against the confirmed balance rather than the
|
|
displayed one.
|
|
|
|
### B-38 — The 500-subscriber SSE cap is a zero-cost DoS of the realtime feature
|
|
|
|
`GET /rounds/stream` requires no authentication and each connection takes a slot on a
|
|
**global** counter (`rounds/events.py:33-38`). Anyone opening 500 connections degrades every
|
|
real user to polling. The comment describes it as a defensive cap; it is in fact the vector,
|
|
not the defence.
|
|
|
|
**Proposed fix.** Cap per client IP (and, once available, per authenticated user) rather than
|
|
globally, and evict the oldest idle subscriber instead of refusing new ones. The reverse proxy
|
|
is the right place for the connection-count limit — Caddy can enforce it before the request
|
|
reaches the app.
|
|
|
|
### B-39 — SQLite with no WAL, no `busy_timeout`, and five concurrent writer tasks
|
|
|
|
`db/base.py:6` calls `create_async_engine(settings.database_url)` with no `connect_args`, and
|
|
there is no `PRAGMA` anywhere in the repo (verified by grep). Without `journal_mode=WAL`
|
|
readers block writers, and the concurrent writers are five background tasks plus every HTTP
|
|
handler. `database is locked` under load is realistic, and nothing handles it.
|
|
|
|
**Proposed fix.** Set `journal_mode=WAL`, `synchronous=NORMAL` and a `busy_timeout` of a few
|
|
seconds on connect (a `connect` event listener on the engine, applied only for the SQLite
|
|
dialect), and retry `OperationalError: database is locked` in the background loops. Longer
|
|
term this is an argument for PostgreSQL, which the single-process constraints in CLAUDE.md
|
|
also point at.
|
|
|
|
### B-40 — `bump_fee` holds a DB session open across N network calls
|
|
|
|
`tx/broadcast.py:80` issues one `get_transaction` **per input** (up to 15s each) and then a
|
|
`broadcast`, all with the session open. This is precisely the pattern B-18 removed from
|
|
`_trigger_payout` via its three-phase structure; it survives here.
|
|
|
|
Side note in the same function: `_prevout_amount` does `round(value_coins * 100_000_000)` on a
|
|
float from the server — acceptable at these magnitudes, but it is floating-point money
|
|
arithmetic in a codebase that is otherwise strictly integer-satoshi.
|
|
|
|
**Proposed fix.** Restructure into the same three phases: read what is needed and close the
|
|
session, do the chain work, then reopen to persist. For the float: prefer the raw (non-verbose)
|
|
transaction and parse the output value as an integer with `embit`, which is what
|
|
`reconcile.py:_release_inputs` already does for inputs.
|
|
|
|
### B-41 — All confirmation logic depends on `verbose=True`, which is not universally supported
|
|
|
|
`poll_once`, `reconcile._tx_exists_on_chain` and `bump_fee` all call
|
|
`blockchain.transaction.get(txid, True)`. Several Electrum server implementations and versions
|
|
reject the verbose flag ("verbose transactions are currently unsupported"). Falling back onto
|
|
such a server means **no confirmations, no reconciliation, no bumps** — and the code would read
|
|
that as a transport error and stay silent.
|
|
|
|
Related: `reconcile.py:83` decides whether to **abandon a transaction** by substring-matching
|
|
the error text (`"missing"`, `"not found"`, `"no such"`, `"unknown"`). It works against
|
|
ElectrumX; it is fragile as the basis for a decision that releases funds.
|
|
|
|
**Proposed fix.** Use `blockchain.transaction.get_merkle` (or the scripthash history) for
|
|
confirmation and existence checks — both are portable and give the confirming height directly.
|
|
Probe verbose support once at connect time and record it on the client, so an unsupported
|
|
server is detected loudly at session start rather than silently mid-operation.
|
|
|
|
---
|
|
|
|
## Low / hygiene
|
|
|
|
### B-42 — `/docs` exposed in production
|
|
|
|
FastAPI mounts Swagger by default, so the entire API surface — `/admin` included — is publicly
|
|
enumerable. The README advertises it.
|
|
**Fix:** `docs_url=None, redoc_url=None, openapi_url=None` in production (env-gated), or place
|
|
them behind `require_admin`.
|
|
|
|
### B-43 — No HTTP security headers
|
|
|
|
The [Caddyfile](Caddyfile) sets no CSP, no `X-Frame-Options`/`frame-ancestors`, and no HSTS
|
|
(Caddy does not add it on its own). The JWT lives in `localStorage`, so any XSS exfiltrates
|
|
it, and the page is iframeable.
|
|
**Fix:** a `header` block in the Caddyfile with `Strict-Transport-Security`,
|
|
`X-Content-Type-Options: nosniff`, `Referrer-Policy` and a CSP tight enough for two static
|
|
pages with no external assets (`default-src 'self'`).
|
|
|
|
### B-44 — README and CLAUDE.md contradict each other
|
|
|
|
The README says to run `uvicorn --reload` directly and
|
|
`docker compose run --rm app python scripts/generate_master_key.py`; CLAUDE.md says explicitly
|
|
that neither is supported. Whoever opens the repo reads the README first.
|
|
**Fix:** align the README's Quick start with the Docker-only workflow documented in
|
|
CLAUDE.md and `docs/setup.md`.
|
|
|
|
### B-45 — Unvalidated and unpaginated admin list endpoints
|
|
|
|
`limit: int = 50` on `/admin/rounds` and `/admin/audit-log` has no bounds (`-1` means
|
|
"everything" on SQLite), and `/admin/pending-transactions` has no limit at all — it grows
|
|
without end.
|
|
**Fix:** `Query(default=50, ge=1, le=500)` on both, and the same treatment plus a status filter
|
|
on the pending-transaction list.
|
|
|
|
### B-46 — `secrets.compare_digest` on a `str` raises on non-ASCII input
|
|
|
|
`api/routes/admin.py:27` raises `TypeError` — a 500 instead of a 403 — when the header contains
|
|
non-ASCII characters.
|
|
**Fix:** compare the UTF-8 encoded bytes of both sides.
|
|
|
|
### B-47 — Unbounded `String` columns for large text
|
|
|
|
`raw_tx_hex` (`db/models.py:146`) and `payload_json` (`:178`) should be `Text`. It works on
|
|
SQLite and PostgreSQL and breaks elsewhere.
|
|
**Fix:** switch both to `Text` in a migration.
|
|
|
|
### B-48 — No cap on input count in `select_utxos`
|
|
|
|
A user with hundreds of small UTXOs builds a huge transaction whose fee — deducted from the bet
|
|
amount — materially erodes their contribution to the pool, and it can exceed standardness
|
|
limits.
|
|
**Fix:** cap the selected inputs (e.g. 50) and fail with a translatable error suggesting a
|
|
consolidation, or consolidate the address automatically when the count crosses a threshold.
|
|
|
|
### B-49 — Rollback paths do not publish an SSE update
|
|
|
|
`bets/service.py:_release_failed_bet` and `withdrawals/service.py:_release_failed_withdrawal`
|
|
restore the balance without calling `broadcaster.publish()`, so dashboards only find out on
|
|
their next poll.
|
|
**Fix:** one `broadcaster.publish()` at the end of each, as every other state-changing path
|
|
already does.
|
|
|
|
---
|
|
|
|
## Previously fixed
|
|
|
|
- **B-25** — the payout had no two-phase write, unlike bets and withdrawals
|
|
- **B-26** — a payout failure or a process restart could wedge a round in `paying_out` forever
|
|
- **B-27** — an RBF bump reset the reconciler's own abandon clock, so a repeatedly-bumped tx was never abandoned
|
|
- **B-28** — a hostile Electrum server (or a MITM) could single-handedly pick the round's winner
|
|
- **B-29** — a UTXO absent from one server's `listunspent` was marked spent immediately, irreversibly, on a single unauthenticated reply
|
|
- **B-30** — a lost scripthash subscription meant a user's deposits were never credited, with no periodic safety net
|
|
- **B-31** — resubscribing on reconnect ran serially before anything else started, freezing the chain tip (and so an in-flight draw) for the whole sweep
|
|
- **B-32** — an RBF bump's fee delta could fall below BIP125's relay-mandated minimum, so the node rejected it and the same tick retried identically forever; also had no ceiling on how high the fee rate could climb
|
|
|
|
See git history for the fix-by-fix breakdown (commits `f13f685`, `50a43ae`, `933760e`, and the
|
|
B-28/B-29/B-30/B-31/B-32 fixes). Suite grew from 139 to 187 tests over the eight.
|
|
|
|
A full-codebase audit on 2026-07-26 (commit `d4e0974`) found 24 bugs across every Python
|
|
module under `app/`, both static frontends, and the Docker/Caddy deployment — 5 critical,
|
|
7 high, 7 medium, 5 low. All 24 were fixed and verified against the current code on
|
|
2026-07-27; the fixes are covered by the regression suite (grew from 79 to 139 tests) and
|
|
five of them were additionally confirmed against a real mainnet deployment (see git history
|
|
between `fb734bb` (documenting the findings) and `845ba98` (recording the audit outcome) for
|
|
the fix-by-fix breakdown — each commit message names the bugs it closes and where their
|
|
tests live).
|