fix(economy): close SBC dup-card exploit + non-atomic economy races

Correctness fixes from docs/CORE_CORRECTNESS_ISSUES.md:

- Issue 3 (HIGH, exploit): submit_sbc dedups owned_card_ids (HashSet) and
  bounds the list (MAX_SBC_CARDS=30) before resolution. A repeated id resolved
  the same card N times, passed validation, and granted the reward while only
  one card was consumed -> any SBC satisfiable with one duplicated card = free
  reward. Now rejected with BadRequest. Regression test added.
- Issue 2 (HIGH): TOCTOU economy mutations closed with single-statement
  compare-and-swap (no transaction plumbing): club::spend_coins conditional
  debit (WHERE coins >= ?) + rows_affected, also rejects negative amounts;
  pack::open_pack claims the pack before minting; market::buy_listing claims
  the listing before charging and releases on debit failure; market::sell_card
  guards the DELETE with owner + rows_affected; checkin::claim uses a
  conditional INSERT ... WHERE NOT EXISTS (today) before paying out.
- Issue 4 (LOW): season.rs .expect() on missing rows -> graceful AppError;
  checkin index (streak-1) % 7 -> .rem_euclid(7) (guards negative index panic).
- Issue 1 (LOW): migration 0019 adds sbc_submissions.club_id + backfill;
  submit_sbc binds it so the MY CLUB milestone query stops silently reading 0.

Core suite 179 green + clippy clean.

migrations/0019_sbc_submissions_club_id.sql

@@ -0,0 +1,11 @@
+-- Issue 1: sbc_submissions was created (0001_initial.sql) without a club_id column,
+-- but the MY CLUB milestone query (routes/club.rs get_milestones) counts
+--   SELECT COUNT(*) FROM sbc_submissions WHERE club_id = ? AND passed = 1
+-- so SQLite errored on the unknown column and the error was swallowed by
+-- `.unwrap_or(0)` -> the `sbcs_completed` milestone always read 0. Add the column
+-- and backfill it from the profile's club so historical submissions count.
+ALTER TABLE sbc_submissions ADD COLUMN club_id TEXT;
+
+UPDATE sbc_submissions
+SET club_id = (SELECT c.id FROM clubs c WHERE c.profile_id = sbc_submissions.profile_id)
+WHERE club_id IS NULL;

src/services/checkin.rs

@@ -44,7 +44,7 @@ pub async fn get_status(pool: &Pool, profile_id: &str) -> AppResult<CheckinStatu
             let last_day = &last_at[..10]; // YYYY-MM-DD
             let available = last_day != today.as_str();
             let next_streak = compute_next_streak(last_streak, &last_at);
-            let idx = ((next_streak - 1) % 7) as usize;
+            let idx = (next_streak - 1).rem_euclid(7) as usize;
             Ok(CheckinStatus {
                 available,
                 streak_day: if available { next_streak } else { last_streak },
@@ -83,19 +83,17 @@ pub async fn claim(
     }
 
     let last_streak = row.as_ref().map(|(s, last_at)| compute_next_streak(*s, last_at)).unwrap_or(1);
-    let idx = ((last_streak - 1) % 7) as usize;
+    let idx = (last_streak - 1).rem_euclid(7) as usize;
     let coins = STREAK_COINS[idx];
     let pack_def = if idx == 6 { Some(STREAK_7_PACK) } else { None };
 
-    club::add_coins(pool, club_id, coins).await?;
-    if let Some(def) = pack_def {
-        let _ = pack::grant_pack(pool, club_id, def).await;
-    }
-
+    // Atomically claim today's check-in: the INSERT lands only if no row exists for
+    // today, so two concurrent claims cannot both pay out (was a check-then-act race).
     let now = chrono::Utc::now().to_rfc3339();
-    sqlx::query(
+    let inserted = sqlx::query(
         "INSERT INTO daily_checkins (id, profile_id, club_id, streak_day, coins_awarded, pack_granted, checked_in_at) \
-         VALUES (?, ?, ?, ?, ?, ?, ?)",
+         SELECT ?, ?, ?, ?, ?, ?, ? \
+         WHERE NOT EXISTS (SELECT 1 FROM daily_checkins WHERE profile_id = ? AND substr(checked_in_at, 1, 10) = ?)",
     )
     .bind(Uuid::new_v4().to_string())
     .bind(profile_id)
@@ -104,8 +102,26 @@ pub async fn claim(
     .bind(coins)
     .bind(pack_def)
     .bind(&now)
+    .bind(profile_id)
+    .bind(&today)
     .execute(pool)
-    .await?;
+    .await?
+    .rows_affected();
+
+    if inserted == 0 {
+        // A concurrent claim already recorded today's check-in — do not pay out again.
+        return Ok(CheckinResult {
+            coins_awarded: 0,
+            pack_awarded: None,
+            new_streak: last_streak,
+            already_claimed: true,
+        });
+    }
+
+    club::add_coins(pool, club_id, coins).await?;
+    if let Some(def) = pack_def {
+        let _ = pack::grant_pack(pool, club_id, def).await;
+    }
 
     Ok(CheckinResult {
         coins_awarded: coins,

src/services/club.rs

@@ -86,24 +86,43 @@ pub async fn add_coins(pool: &Pool, club_id: &str, amount: i64) -> AppResult<i64
 }
 
 pub async fn spend_coins(pool: &Pool, club_id: &str, amount: i64) -> AppResult<i64> {
-    let balance = sqlx::query_scalar::<_, i64>("SELECT coins FROM clubs WHERE id = ?")
-        .bind(club_id)
-        .fetch_one(pool)
-        .await?;
+    if amount < 0 {
+        return Err(AppError::BadRequest(format!(
+            "cannot spend a negative amount: {amount}"
+        )));
+    }
 
-    if balance < amount {
+    let now = Utc::now();
+    // Atomic compare-and-swap: the `coins >= ?` guard makes the debit conditional in a
+    // single statement, so two concurrent spends can never both pass a stale balance
+    // check and drive coins negative (the old SELECT-then-UPDATE was a TOCTOU race).
+    let affected = sqlx::query(
+        "UPDATE clubs SET coins = coins - ?, updated_at = ? WHERE id = ? AND coins >= ?",
+    )
+    .bind(amount)
+    .bind(now)
+    .bind(club_id)
+    .bind(amount)
+    .execute(pool)
+    .await?
+    .rows_affected();
+
+    if affected == 0 {
+        // No row updated: the club is missing, or it could not afford the debit.
+        // Disambiguate so callers keep the NotFound vs BadRequest distinction.
+        let balance = sqlx::query_scalar::<_, i64>("SELECT coins FROM clubs WHERE id = ?")
+            .bind(club_id)
+            .fetch_optional(pool)
+            .await?
+            .ok_or_else(|| AppError::NotFound(format!("club not found: {club_id}")))?;
         return Err(AppError::BadRequest(format!(
             "insufficient coins: have {balance}, need {amount}"
         )));
     }
 
-    let now = Utc::now();
-    sqlx::query("UPDATE clubs SET coins = coins - ?, updated_at = ? WHERE id = ?")
-        .bind(amount)
-        .bind(now)
+    let new_balance = sqlx::query_scalar::<_, i64>("SELECT coins FROM clubs WHERE id = ?")
         .bind(club_id)
-        .execute(pool)
+        .fetch_one(pool)
         .await?;
-
-    Ok(balance - amount)
+    Ok(new_balance)
 }

src/services/market.rs

@@ -141,12 +141,23 @@ pub async fn buy_listing(
     .await?
     .ok_or_else(|| AppError::NotFound("listing not found or already sold".into()))?;
 
-    club::spend_coins(pool, club_id, listing.price).await?;
-
-    sqlx::query("UPDATE market_listings SET sold = 1 WHERE id = ?")
+    // Atomically claim the listing (flip sold 0->1) before charging, so two concurrent
+    // buyers cannot both mint the same card. If the debit then fails, release the claim.
+    let claimed = sqlx::query("UPDATE market_listings SET sold = 1 WHERE id = ? AND sold = 0")
         .bind(&listing.id)
         .execute(pool)
-        .await?;
+        .await?
+        .rows_affected();
+    if claimed == 0 {
+        return Err(AppError::NotFound("listing not found or already sold".into()));
+    }
+    if let Err(e) = club::spend_coins(pool, club_id, listing.price).await {
+        let _ = sqlx::query("UPDATE market_listings SET sold = 0 WHERE id = ?")
+            .bind(&listing.id)
+            .execute(pool)
+            .await;
+        return Err(e);
+    }
 
     let owned_id = Uuid::new_v4().to_string();
     sqlx::query(
@@ -206,10 +217,17 @@ pub async fn sell_card(
     .await?
     .ok_or_else(|| AppError::NotFound("owned card not found".into()))?;
 
-    sqlx::query("DELETE FROM owned_cards WHERE id = ?")
+    // Atomically claim the card: guard the DELETE with the owner + rows_affected so two
+    // concurrent sells of the same card cannot both credit (double payout).
+    let deleted = sqlx::query("DELETE FROM owned_cards WHERE id = ? AND club_id = ?")
         .bind(&req.owned_card_id)
+        .bind(club_id)
         .execute(pool)
-        .await?;
+        .await?
+        .rows_affected();
+    if deleted == 0 {
+        return Err(AppError::NotFound("owned card not found".into()));
+    }
 
     let coins = (req.price as f64 * 0.4) as i64;
     let new_balance = club::add_coins(pool, club_id, coins).await?;

src/services/pack.rs

@@ -71,7 +71,17 @@ pub async fn open_pack(
     .await?
     .ok_or_else(|| AppError::NotFound(format!("pack {pack_id} not found")))?;
 
-    if pack.opened {
+    // Atomically claim the pack before minting any cards: only one concurrent opener
+    // flips opened 0->1, so a double-open cannot mint the reward twice (duplication).
+    let claimed = sqlx::query(
+        "UPDATE packs SET opened = 1 WHERE id = ? AND club_id = ? AND opened = 0",
+    )
+    .bind(pack_id)
+    .bind(club_id)
+    .execute(pool)
+    .await?
+    .rows_affected();
+    if claimed == 0 {
         return Err(AppError::BadRequest("pack already opened".into()));
     }
 
@@ -128,7 +138,7 @@ pub async fn open_pack(
         .unwrap_or_default();
     let now = chrono::Utc::now().to_rfc3339();
 
-    sqlx::query("UPDATE packs SET opened = 1, opened_cards = ?, opened_at = ? WHERE id = ?")
+    sqlx::query("UPDATE packs SET opened_cards = ?, opened_at = ? WHERE id = ?")
         .bind(&card_ids_json)
         .bind(&now)
         .bind(pack_id)

src/services/sbc.rs

@@ -12,6 +12,10 @@ use anyhow::Context;
 use std::path::Path;
 use uuid::Uuid;
 
+/// Upper bound on cards in one SBC submission (a real squad SBC is 11; consumables
+/// push it higher, but 30 is well beyond any legitimate challenge and caps a DoS).
+const MAX_SBC_CARDS: usize = 30;
+
 pub fn load_sbc_definitions(data_dir: &str) -> anyhow::Result<Vec<SbcDefinition>> {
     let dir = Path::new(data_dir).join("sbcs");
     let mut defs = Vec::new();
@@ -46,6 +50,26 @@ pub async fn submit_sbc(
         .find(|d| d.id == req.sbc_id)
         .ok_or_else(|| AppError::NotFound(format!("SBC {} not found", req.sbc_id)))?;
 
+    // Reject duplicate owned-card ids and bound the list length. A repeated id would
+    // resolve the SAME owned card N times (each fetch succeeds), so `validate_sbc`
+    // counts it toward the squad size and passes, while the DELETE loop removes it
+    // only once — i.e. any SBC satisfiable with a single duplicated card = free
+    // reward. An unbounded list is also a cheap DoS.
+    if req.owned_card_ids.len() > MAX_SBC_CARDS {
+        return Err(AppError::BadRequest(format!(
+            "too many cards in submission ({}, max {MAX_SBC_CARDS})",
+            req.owned_card_ids.len()
+        )));
+    }
+    {
+        let mut seen = std::collections::HashSet::with_capacity(req.owned_card_ids.len());
+        if let Some(dup) = req.owned_card_ids.iter().find(|id| !seen.insert(*id)) {
+            return Err(AppError::BadRequest(format!(
+                "duplicate card in submission: {dup}"
+            )));
+        }
+    }
+
     // Resolve cards from DB
     let mut cards: Vec<CardDefinition> = Vec::new();
     for owned_id in &req.owned_card_ids {
@@ -79,10 +103,11 @@ pub async fn submit_sbc(
         let sub_id = Uuid::new_v4().to_string();
         let card_ids_json = serde_json::to_string(&req.owned_card_ids)?;
         sqlx::query(
-            "INSERT INTO sbc_submissions (id, profile_id, sbc_id, submitted_card_ids, passed, submitted_at) VALUES (?, ?, ?, ?, 1, ?)"
+            "INSERT INTO sbc_submissions (id, profile_id, club_id, sbc_id, submitted_card_ids, passed, submitted_at) VALUES (?, ?, ?, ?, ?, 1, ?)"
         )
         .bind(&sub_id)
         .bind(profile_id)
+        .bind(club_id)
         .bind(&req.sbc_id)
         .bind(&card_ids_json)
         .bind(chrono::Utc::now().to_rfc3339())

src/services/season.rs

@@ -1,6 +1,6 @@
 use crate::{
     db::Pool,
-    error::AppResult,
+    error::{AppError, AppResult},
     models::season::{Season, SeasonEndSummary, SeasonHistoryEntry, SeasonResult},
     services::{club, pack},
 };
@@ -20,7 +20,9 @@ pub async fn get_or_create(pool: &Pool, profile_id: &str) -> AppResult<Season> {
     .bind(&now)
     .execute(pool)
     .await?;
-    Ok(fetch(pool, profile_id).await?.expect("just inserted"))
+    fetch(pool, profile_id)
+        .await?
+        .ok_or_else(|| AppError::Internal(anyhow::anyhow!("season row missing immediately after insert")))
 }
 
 async fn fetch(pool: &Pool, profile_id: &str) -> AppResult<Option<Season>> {
@@ -66,7 +68,9 @@ pub async fn record_match(
     .execute(pool)
     .await?;
 
-    let season = fetch(pool, profile_id).await?.expect("season must exist");
+    let season = fetch(pool, profile_id)
+        .await?
+        .ok_or_else(|| AppError::Internal(anyhow::anyhow!("season row missing after record_match update")))?;
 
     if !season.is_complete() {
         return Ok((season, None));
@@ -141,7 +145,9 @@ pub async fn record_match(
         pack_awarded: pack_id.map(String::from),
     };
 
-    let updated = fetch(pool, profile_id).await?.expect("season must exist");
+    let updated = fetch(pool, profile_id)
+        .await?
+        .ok_or_else(|| AppError::Internal(anyhow::anyhow!("season row missing after season rollover")))?;
     Ok((updated, Some(summary)))
 }
 

tests/integration_test.rs

@@ -269,6 +269,56 @@ async fn test_sbc_submit_with_bronze_cards() {
     assert!(result["reward"].is_object());
 }
 
+#[tokio::test]
+async fn test_sbc_rejects_duplicate_cards() {
+    // Regression: a single owned card repeated to fill an SBC must be rejected. Before
+    // the dedup guard the same id resolved N times, passed validation, and granted the
+    // reward while only one card was consumed (free-reward exploit).
+    let app = build_test_app().await;
+    auth(&app, "SBCDupePlayer").await;
+
+    let (s, _) = json_post(
+        &app,
+        "/packs/buy",
+        serde_json::json!({ "pack_definition_id": "bronze_pack" }),
+    )
+    .await;
+    assert_eq!(s, StatusCode::OK);
+    let (_, packs_json) = json_get(&app, "/packs").await;
+    let pack_id = packs_json["packs"]
+        .as_array()
+        .unwrap()
+        .iter()
+        .find(|p| p["definition_id"] == "bronze_pack")
+        .expect("bronze pack in inventory")["pack_id"]
+        .as_str()
+        .unwrap()
+        .to_string();
+    let (s, _) = json_post(&app, &format!("/packs/open/{pack_id}"), serde_json::json!({})).await;
+    assert_eq!(s, StatusCode::OK);
+
+    let (_, coll) = json_get(&app, "/collection").await;
+    let one_card = coll["collection"].as_array().unwrap()[0]["owned_card_id"]
+        .as_str()
+        .unwrap()
+        .to_string();
+
+    let (s, result) = json_post(
+        &app,
+        "/sbc/submit",
+        serde_json::json!({
+            "sbc_id": "sbc_bronze_upgrade",
+            "owned_card_ids": vec![one_card; 11]
+        }),
+    )
+    .await;
+    assert_eq!(s, StatusCode::BAD_REQUEST, "duplicate submission must be rejected: {result}");
+    assert!(
+        result["error"].as_str().unwrap_or_default().contains("duplicate"),
+        "expected a duplicate-card error, got: {result}"
+    );
+}
+
 #[tokio::test]
 async fn test_settings_read_write() {
     let app = build_test_app().await;