feat: add forfeit_game function and contract review

REVIEW.md

@@ -0,0 +1,186 @@
+# Aztec Contract Review: PodRacing
+
+## Overall Assessment: **Good** ✓
+
+The contract is well-structured with clear documentation and follows most Aztec best practices. However, there are several security concerns and improvements worth addressing.
+
+---
+
+## ✅ Strengths
+
+### Structure
+- Proper use of `#[aztec]` macro and storage attributes
+- Clean separation between contract (`main.nr`), data types (`race.nr`), and notes (`game_round_note.nr`)
+- Good use of constants (`TOTAL_ROUNDS`, `GAME_LENGTH`)
+- Excellent inline documentation explaining game flow
+
+### Function Design
+- Correct use of `#[external("private")]` for sensitive operations (`play_round`, `finish_game`)
+- Internal functions properly marked with `#[only_self]`
+- Good public/private split for the commit-reveal pattern
+
+### Privacy
+- Private notes correctly store player choices until reveal
+- `Owned<PrivateSet<...>>` ensures only the owner can read their notes
+- Point allocations hidden until `finish_game` is called
+
+---
+
+## ⚠️ Security Issues
+
+### 1. **CRITICAL: `finalize_game` can be called multiple times**
+**Location:** `main.nr:222-232`
+
+The `finalize_game` function doesn't prevent repeated calls. An attacker can call it multiple times to inflate their win count.
+
+```rust
+// Current code - no protection against re-finalization
+#[external("public")]
+fn finalize_game(game_id: Field) {
+    let game_in_progress = self.storage.races.at(game_id).read();
+    let winner = game_in_progress.calculate_winner(self.context.block_number());
+    let previous_wins = self.storage.win_history.at(winner).read();
+    self.storage.win_history.at(winner).write(previous_wins + 1);
+    // Game is NOT reset - can be finalized again!
+}
+```
+
+**Fix:** Reset the game after finalization:
+```rust
+fn finalize_game(game_id: Field) {
+    let game_in_progress = self.storage.races.at(game_id).read();
+    let winner = game_in_progress.calculate_winner(self.context.block_number());
+    let previous_wins = self.storage.win_history.at(winner).read();
+    self.storage.win_history.at(winner).write(previous_wins + 1);
+
+    // Reset the game to prevent re-finalization
+    self.storage.races.at(game_id).write(Race::empty());
+}
+```
+
+### 2. **MEDIUM: No validation that player2 has joined before playing rounds**
+**Location:** `main.nr:100-131`, `race.nr:116-167`
+
+A player can play rounds before player2 joins. The `increment_player_round` function doesn't check if the game has started.
+
+**Fix:** Add validation in `increment_player_round`:
+```rust
+pub fn increment_player_round(self, player: AztecAddress, round: u8) -> Race {
+    // Ensure game has started (player2 joined)
+    assert(!self.player2.eq(AztecAddress::zero()), "Game has not started");
+    assert(round < self.total_rounds + 1);
+    // ... rest of function
+}
+```
+
+### 3. **MEDIUM: No validation that player completed all rounds before finishing**
+**Location:** `main.nr:149-184`
+
+A player can call `finish_game` before completing all 3 rounds. The loop assumes exactly `TOTAL_ROUNDS` notes exist, but there's no validation.
+
+**Fix:** Add round completion check in `validate_finish_game_and_reveal`:
+```rust
+fn validate_finish_game_and_reveal(...) {
+    let game_in_progress = self.storage.races.at(game_id).read();
+
+    // Validate player completed all rounds
+    if player.eq(game_in_progress.player1) {
+        assert(game_in_progress.player1_round == game_in_progress.total_rounds, "Must complete all rounds");
+    } else {
+        assert(game_in_progress.player2_round == game_in_progress.total_rounds, "Must complete all rounds");
+    }
+    // ... rest of function
+}
+```
+
+### 4. **LOW: Tie-breaker always favors player2**
+**Location:** `race.nr:266-294`
+
+When scores are tied on a track, player2 always wins. This is documented but creates an inherent disadvantage for player1.
+
+**Consideration:** This may be intentional to offset first-mover advantage, but consider:
+- Making ties count for neither player
+- Using a different tie-breaker mechanism
+
+### 5. **LOW: No check for game expiration in `join_game`**
+**Location:** `main.nr:83-90`
+
+A player can join a game that has already expired (`end_block` passed).
+
+**Fix:**
+```rust
+fn join_game(game_id: Field) {
+    let maybe_existing_game = self.storage.races.at(game_id).read();
+    assert(self.context.block_number() < maybe_existing_game.end_block, "Game has expired");
+    let joined_game = maybe_existing_game.join(self.context.msg_sender().unwrap());
+    self.storage.races.at(game_id).write(joined_game);
+}
+```
+
+---
+
+## 🔧 Code Quality Improvements
+
+### 1. **Missing error messages on most assertions**
+Most `assert` statements lack descriptive error messages, making debugging difficult.
+
+```rust
+// Current
+assert(track1 + track2 + track3 + track4 + track5 < 10);
+
+// Better
+assert(track1 + track2 + track3 + track4 + track5 < 10, "Point allocation exceeds maximum of 9");
+```
+
+### 2. **Unused `admin` storage variable**
+**Location:** `main.nr:42`
+
+The `admin` is set in the constructor but never used. Either remove it or implement admin functions (pause, update settings, etc.).
+
+### 3. **Double-reveal check is fragile**
+**Location:** `race.nr:175-181`
+
+The check `sum of all tracks == 0` fails if a player legitimately allocates 0 points to all tracks in all rounds.
+
+**Fix:** Add a dedicated `revealed` flag or check round count:
+```rust
+// Better approach - check if player completed rounds but hasn't revealed
+if player.eq(self.player1) {
+    assert(self.player1_round == self.total_rounds, "Must complete all rounds first");
+    assert(
+        self.player1_track1_final == 0 &&
+        self.player1_track2_final == 0 &&
+        // ... etc (explicit zero check)
+    , "Already revealed");
+}
+```
+
+### 4. **Redundant `GameRoundNote::get()` method**
+**Location:** `game_round_note.nr:45-55`
+
+This method just returns a copy of itself and appears unused.
+
+---
+
+## 📋 Summary Table
+
+| Category | Issue | Severity | Status |
+|----------|-------|----------|--------|
+| Security | `finalize_game` re-entrancy | **Critical** | Needs fix |
+| Security | Play rounds before game starts | Medium | Needs fix |
+| Security | Finish without all rounds | Medium | Needs fix |
+| Security | Join expired game | Low | Consider |
+| Design | Tie always favors player2 | Low | Documented |
+| Quality | Missing error messages | Low | Improve |
+| Quality | Unused admin variable | Low | Remove/Use |
+| Quality | Fragile double-reveal check | Low | Improve |
+
+---
+
+## Recommendations Priority
+
+1. **Immediately fix** the `finalize_game` re-entrancy by resetting the game after determining winner
+2. **Add validation** that game has started before playing rounds
+3. **Add validation** that all rounds are completed before revealing
+4. **Add error messages** to all assertions for better debugging
+5. **Consider** removing unused `admin` or implementing admin functionality

src/main.nr

@@ -230,4 +230,55 @@ pub contract PodRacing {
         let previous_wins = self.storage.win_history.at(winner).read();
         self.storage.win_history.at(winner).write(previous_wins + 1);
     }
+
+    // Returns the current state of a game
+    // Useful for frontends to display game progress
+    #[external("utility")]
+    unconstrained fn get_game_state(game_id: Field) -> pub Race {
+        self.storage.races.at(game_id).read()
+    }
+
+    // Returns the total career wins for a player
+    #[external("utility")]
+    unconstrained fn get_player_wins(player: AztecAddress) -> pub u64 {
+        self.storage.win_history.at(player).read()
+    }
+
+    // Allows player1 to cancel a game if player2 hasn't joined yet
+    // Useful if no opponent shows up and player1 wants to reclaim the game slot
+    #[external("public")]
+    fn cancel_game(game_id: Field) {
+        let game = self.storage.races.at(game_id).read();
+
+        // Only player1 can cancel
+        assert(game.player1.eq(self.context.msg_sender().unwrap()));
+
+        // Can only cancel if no player2 has joined
+        assert(game.player2.eq(AztecAddress::zero()));
+
+        // Reset the game slot by writing a zeroed Race
+        self.storage.races.at(game_id).write(Race::empty());
+    }
+
+    // Allows a player to forfeit an in-progress game
+    // The opponent is automatically awarded the win
+    // Can only be called after player2 has joined (game has started)
+    #[external("public")]
+    fn forfeit_game(game_id: Field) {
+        let game = self.storage.races.at(game_id).read();
+        let caller = self.context.msg_sender().unwrap();
+
+        // Game must have started (player2 joined)
+        assert(!game.player2.eq(AztecAddress::zero()), "Game has not started yet");
+
+        // Get the opponent (also validates caller is a player)
+        let winner = game.get_opponent(caller);
+
+        // Award the win to the opponent
+        let previous_wins = self.storage.win_history.at(winner).read();
+        self.storage.win_history.at(winner).write(previous_wins + 1);
+
+        // Reset the game slot
+        self.storage.races.at(game_id).write(Race::empty());
+    }
 }

src/race.nr

@@ -38,6 +38,28 @@ pub struct Race {
 }
 
 impl Race {
+    // Creates an empty/zeroed Race for resetting game slots
+    pub fn empty() -> Race {
+        Self {
+            player1: AztecAddress::zero(),
+            player2: AztecAddress::zero(),
+            total_rounds: 0,
+            player1_round: 0,
+            player2_round: 0,
+            player1_track1_final: 0,
+            player1_track2_final: 0,
+            player1_track3_final: 0,
+            player1_track4_final: 0,
+            player1_track5_final: 0,
+            player2_track1_final: 0,
+            player2_track2_final: 0,
+            player2_track3_final: 0,
+            player2_track4_final: 0,
+            player2_track5_final: 0,
+            end_block: 0,
+        }
+    }
+
     // Creates a new game with player1 set, waiting for player2 to join
     // All track scores initialized to 0, rounds start at 0
     pub fn new(player1: AztecAddress, total_rounds: u8, end_block: u32) -> Race {
@@ -210,6 +232,19 @@ impl Race {
         ret.unwrap()
     }
 
+    // Returns the opponent's address for a given player
+    // Asserts if the caller is not a player in this game
+    pub fn get_opponent(self, player: AztecAddress) -> AztecAddress {
+        if player.eq(self.player1) {
+            self.player2
+        } else if player.eq(self.player2) {
+            self.player1
+        } else {
+            assert(false, "Caller is not a player in this game");
+            AztecAddress::zero() // unreachable, but needed for return type
+        }
+    }
+
     // Determines the game winner by comparing track scores
     // Winner is whoever won more tracks (best of 5)
     //

src/test/helpers.nr

@@ -14,6 +14,9 @@ pub global TEST_GAME_ID_7: Field = 7;
 pub global TEST_GAME_ID_8: Field = 8;
 pub global TEST_GAME_ID_9: Field = 9;
 pub global TEST_GAME_ID_10: Field = 10;
+pub global TEST_GAME_ID_11: Field = 11;
+pub global TEST_GAME_ID_12: Field = 12;
+pub global TEST_GAME_ID_13: Field = 13;
 
 // Common point allocations for testing
 // Balanced strategy: distribute points evenly