OpenFUT/docs/CORE_CORRECTNESS_ISSUES.md

# openfut-core — Correctness Issues (audit 2026-08-17)

Read-only audit of `openfut-core` (game-agnostic axum + SQLite/sqlx economy authority).
Four reported issue classes confirmed with exact `file:line` evidence, **plus a bonus
HIGH-severity SBC duplicate-card economy exploit**. Nothing here is fixed yet —
fixing bumps Core off the frozen P1 reference (`fbb54ea`, the current known-good
production Core) and one item needs a DB migration + prod backfill, so this needs a
**go/no-go** before rebuild+redeploy.

> **Zero live users right now**, so the HIGH-severity economy exploits are not
> currently exploitable — but they are the exact class (coin overspend + card
> duplication) that crashed live clients earlier (project memory), so they should be
> fixed before any real play.

## Architecture context (why the bugs cluster)

Two generations of economy code coexist:
- **NEW** `services/economy.rs` + `routes/economy.rs` — **fully atomic + validated**:
  every compound op acquires a connection, `BEGIN IMMEDIATE`, composes
  transaction-scoped primitives (`debit`/`credit`/`add_item`/`remove_item`/
  `consume_entitlement`), commits/rolls back via `finish()`, rejects negative amounts,
  and has an extensive in-module test suite. **This is the reference fix pattern.**
- **OLD** per-feature services (`club`, `pack`, `market`, `sbc`, `checkin`, `upgrades`,
  `match_service`, `season`) — predate it, still do read/check/write directly against
  the `&Pool` with each statement on its own connection: no transaction. `economy.rs`'s
  own module doc explicitly warns these "cannot offer that guarantee".

Issues 2 and 3 live entirely in the OLD generation; the fix is to route them through
`economy.rs`'s proven atomic ops (or give the pool helpers `&mut SqliteConnection`
transactional variants). Request flow is `X-OpenFUT-Game` header → active profile →
club → service; clients never pass a `club_id`, so cross-club access is **not** a
vector — the risks are intra-club concurrency + unvalidated payloads.

---

## Issue 1 — `sbc_submissions` missing `club_id` (milestone always 0) · **LOW**

- **Root cause:** `migrations/0001_initial.sql:96-102` creates `sbc_submissions(id,
  profile_id, sbc_id, submitted_card_ids, passed, submitted_at)` — no `club_id`, and no
  later migration adds one. But `src/routes/club.rs:94-99` (`get_milestones`) runs
  `SELECT COUNT(*) FROM sbc_submissions WHERE club_id = ? AND passed = 1`. SQLite errors
  `no such column: club_id`; the error is swallowed by `.unwrap_or(0)` → the
  `sbcs_completed` milestone is **always 0**. Writer `services/sbc.rs:74-83` inserts
  `profile_id`, not `club_id`.
- **Impact:** wrong milestone stat only. No crash, no economy corruption.
- **Fix (matches the ticket — needs migration + backfill):** new migration `0019`:
  `ALTER TABLE sbc_submissions ADD COLUMN club_id TEXT;` then backfill
  `UPDATE sbc_submissions SET club_id = (SELECT c.id FROM clubs c WHERE c.profile_id =
  sbc_submissions.profile_id);` and bind `club_id` in `sbc.rs:submit_sbc`'s INSERT
  (`club_id` is already a param at `sbc.rs:41`).
- **Simpler alternative (no migration):** change the `club.rs:95` query to
  `WHERE profile_id = ?` (column already exists). Ticket asks for the column, so both
  are recorded.
- **Migration required:** YES (for the ticket's fix); NO (for the alternative).

## Issue 2 — Non-atomic check-then-act economy mutations (TOCTOU) · **HIGH**

Each does read → check → write across multiple pool round-trips with no
`BEGIN IMMEDIATE`, so concurrent requests race → overspend / duplication / double reward:

| Site | Race |
|---|---|
| `services/club.rs:88-115` `spend_coins` | SELECT coins → `if balance<amount` → UPDATE; two concurrent spends both pass → **overspend / negative balance**. Shared primitive used by all callers below. |
| `services/pack.rs:58-141` `open_pack` | read `pack.opened` → INSERT cards loop → UPDATE opened=1; concurrent double-open → **card duplication** |
| `services/pack.rs:144-160` `buy_pack` | `spend_coins` then `grant_pack`, separate ops → crash between = coins gone, no pack |
| `services/market.rs:129-186` `buy_listing` | SELECT sold=0 → spend → UPDATE sold=1 → INSERT; concurrent double-buy → **two cards minted** |
| `services/market.rs:188+` `sell_card` | SELECT owned → DELETE → add_coins; concurrent double-sell → **double credit** |
| `services/sbc.rs:35-107` `submit_sbc` | validate → DELETE cards → INSERT submission → add_coins/grant_pack; same cards to two SBCs → **double reward** |
| `services/checkin.rs:60-120` `claim` | SELECT last → same-day check → reward → INSERT; concurrent → **double claim** |
| `services/upgrades.rs:64-95` `change_position` | fetch → spend_coins → UPDATE |
| `services/match_service.rs:97-235` + `season.rs` reward | many sequential writes; partial failure leaves partial rewards |

- **Impact:** corrupts economy state (coin overspend + card duplication).
- **Fix:** wrap each compound op in one `BEGIN IMMEDIATE`…`finish()` transaction
  exactly as `services/economy.rs:214-236` already does; route pack/market/sbc/checkin/
  upgrades through `economy.rs`'s composed atomic ops (or add `&mut SqliteConnection`
  variants of `spend_coins`/`add_coins`/`grant_pack`/`add_item`).
- **Migration required:** NO (code-only).

## Issue 3 — Missing input validation: SBC duplicate-card exploit · **HIGH**

- **Root cause:** `services/sbc.rs:53-70` iterates `req.owned_card_ids` with **no dedup
  and no length bound**. The same `owned_card_id` repeated N times resolves the same card
  N times (each `fetch_optional` succeeds); `validate_sbc` (`sbc.rs:110-116`) counts it
  toward `squad_size` and passes; the DELETE loop deletes it once → **a user satisfies
  any SBC with ONE card duplicated → free rewards**. Entry point `routes/sbc.rs:30-49`
  forwards `req` unvalidated. Unbounded Vec length is also a DoS.
- **Fix:** in `submit_sbc`, reject duplicate ids (collect into a `HashSet`, compare
  `len`) and bound the list (e.g. ≤ 30) before resolving → `AppError::BadRequest`.
- **Already-good validation (no change):** `economy.rs:64-67,84-87` reject negative
  amounts; `match_service.rs:100-104` clamps goals 0..99; `upgrades.rs` validates
  boost 1..3 / positions. Minor: `routes/economy.rs post_grant_reward` forwards an
  unbounded `amount` (trusted host caller; add an upper-bound sanity guard).
- **Migration required:** NO.

## Issue 4 — `season.rs` panics + checkin index panic · **LOW**

- **Root cause:** `services/season.rs` `.expect()` on `fetch_optional` Options at
  `:23` (`get_or_create`), `:69` and `:144` (`record_match`) — panic if the `seasons`
  row is absent when expected. `seasons.profile_id` is PRIMARY KEY
  (`migrations/0006_seasons_loans_packs.sql:2`), so the concurrent-insert case surfaces
  as a UNIQUE error via `?` (not the panic), lowering probability — but it still
  panics-on-invariant, aborting that request (axum → 500 for the request; not a full
  server crash).
- **Secondary:** `services/checkin.rs:~52,~88` index `STREAK_COINS[idx]` with
  `idx = ((streak-1)%7) as usize`; Rust `%` can be negative → a corrupt/negative
  persisted `streak_day` yields a negative index → **panic**.
- **Fix:** replace each `.expect(...)` with
  `.ok_or_else(|| AppError::Internal("season row missing".into()))?`; guard the checkin
  index with `.rem_euclid(7)` (or clamp `streak_day >= 1` on read).
- **Migration required:** NO.

---

## Fix plan summary

| Issue | Severity | Migration | Files |
|---|---|---|---|
| 1 sbc_submissions club_id | LOW | YES (or none via alt) | `migrations/0001` (+new 0019), `routes/club.rs`, `services/sbc.rs` |
| 2 non-atomic mutations | HIGH | NO | `services/{club,pack,market,sbc,checkin,upgrades,match_service,season}.rs` → route through `services/economy.rs` |
| 3 SBC duplicate-card exploit | HIGH | NO | `services/sbc.rs`, `routes/sbc.rs` |
| 4 season/checkin panics | LOW | NO | `services/season.rs`, `services/checkin.rs` |

**Recommended order:** 3 (smallest, highest-value: kills the free-reward exploit) → 2
(the transactional refactor, largest) → 4 (defensive hygiene) → 1 (cosmetic; do with
the alt query unless the column is wanted).

**Deployment note:** all of these change `openfut-core`, which is currently frozen at
the P1 reference (`fbb54ea`) in production. Fixing + rebuilding + redeploying prod-core
is a deliberate step off that reference — get a go/no-go first. Only Issue 1's
column-add needs a migration + prod backfill; 2/3/4 are code-only.

---

## Resolution (2026-08-17, on `openfut-core` @ `fbb54ea` + these edits)

All four issue classes fixed in the canonical superproject submodule
`openfut-core`; full test suite green (179 tests) + clippy clean + a new
regression test `tests/integration_test.rs::test_sbc_rejects_duplicate_cards`.

| Issue | Fix | Files |
|---|---|---|
| 3 SBC dup-card exploit | dedup (`HashSet`) + `MAX_SBC_CARDS`=30 bound in `submit_sbc`, before card resolution → `BadRequest` | `services/sbc.rs` |
| 1 sbc_submissions club_id | migration `0019` adds `club_id` + backfills from `clubs`; `submit_sbc` INSERT now binds `club_id` | `migrations/0019_*.sql`, `services/sbc.rs` |
| 4 season/checkin panics | `.expect()` → `.ok_or_else(AppError::Internal)?` (3 sites); checkin index `% 7` → `.rem_euclid(7)` (2 sites) | `services/season.rs`, `services/checkin.rs` |
| 2 non-atomic mutations | statement-level compare-and-swap (see below) | `services/{club,pack,market,checkin}.rs` |

### Issue 2 — how it was fixed, and the residual

Rather than the full transaction refactor (threading `&mut SqliteConnection`
through every service), the concurrency-exploitable races were closed with
single-statement **compare-and-swap** — the atomic unit SQLite already gives us,
no transaction plumbing, minimal blast radius on the working prod economy path:

- `club::spend_coins` — `UPDATE … SET coins = coins - ? WHERE id = ? AND coins >= ?`
  + `rows_affected` guard; also rejects negative amounts. Kills **overspend** for
  every caller (the shared root primitive).
- `pack::open_pack` — claims the pack (`UPDATE … opened = 1 WHERE … AND opened = 0`)
  **before** minting cards; loser aborts. Kills **card duplication** via double-open.
- `market::buy_listing` — claims the listing (`sold 0→1`) before charging; releases
  the claim if the debit fails. Kills **double-mint**.
- `market::sell_card` — `DELETE … WHERE id = ? AND club_id = ?` + `rows_affected`
  guard before crediting. Kills **double-credit** via double-sell.
- `checkin::claim` — conditional `INSERT … SELECT … WHERE NOT EXISTS (today's row)`
  + `rows_affected` guard; pays out only if the claim landed. Kills **double-claim**.

**Residual (accepted, documented):** the *multi-statement all-or-nothing* edges that
need a real transaction to close — `pack::buy_pack` (spend then grant: a crash between
loses coins with no pack), `sbc::submit_sbc` (concurrent submits sharing cards could
double-consume mid-loop), and `match_service`/`season` reward chains (partial writes on
crash). These are **partial-failure durability edges, not statement-level races**, and
require concurrency that a single-player FIFA17 client does not generate. Closing them
is the `&mut SqliteConnection` transaction refactor originally proposed; deferred as
low-value for single-player. Overspend + duplication + double-credit — the vectors that
corrupt economy state — are all closed.