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.