diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 0000000..34db4f9 --- /dev/null +++ b/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>` 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 diff --git a/src/main.nr b/src/main.nr index 1f8904b..9623dc7 100644 --- a/src/main.nr +++ b/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()); + } } diff --git a/src/race.nr b/src/race.nr index f4cb34d..c80e9e3 100644 --- a/src/race.nr +++ b/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) // diff --git a/src/test/helpers.nr b/src/test/helpers.nr index b9fe552..2f8fd13 100644 --- a/src/test/helpers.nr +++ b/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 diff --git a/src/test/pod_racing.nr b/src/test/pod_racing.nr index d072741..2e726e6 100644 --- a/src/test/pod_racing.nr +++ b/src/test/pod_racing.nr @@ -1,5 +1,5 @@ use crate::test::{utils, helpers}; -use dep::aztec::protocol_types::storage::map::derive_storage_slot_in_map; +use dep::aztec::protocol_types::{address::AztecAddress, storage::map::derive_storage_slot_in_map}; use crate::PodRacing; // ============================================================================ @@ -383,3 +383,118 @@ unconstrained fn test_fail_double_reveal() { // Try to finish again (should fail) env.call_private(player1, PodRacing::at(contract_address).finish_game(game_id)); } + +// ============================================================================ +// FORFEIT TESTS +// ============================================================================ + +// Test: Player 1 can forfeit and player 2 wins +#[test] +unconstrained fn test_forfeit_game_player1_forfeits() { + let (mut env, contract_address, _) = utils::setup(); + let player1 = env.create_light_account(); + let player2 = env.create_light_account(); + let game_id = helpers::TEST_GAME_ID_11; + + // Setup game with both players + helpers::setup_two_player_game(&mut env, contract_address, player1, player2, game_id); + + // Player 1 forfeits + env.call_public(player1, PodRacing::at(contract_address).forfeit_game(game_id)); + + // Verify player 2's win count increased + env.public_context_at(contract_address, |context| { + let win_slot = derive_storage_slot_in_map(PodRacing::storage_layout().win_history.slot, player2); + let player2_wins = context.storage_read(win_slot); + assert_eq(player2_wins, 1); + }); + + // Verify game was reset (player1 should be zero address) + env.public_context_at(contract_address, |context| { + let race_slot = derive_storage_slot_in_map(PodRacing::storage_layout().races.slot, game_id); + let stored_player1 = context.storage_read(race_slot); + assert_eq(stored_player1, AztecAddress::zero()); + }); +} + +// Test: Player 2 can forfeit and player 1 wins +#[test] +unconstrained fn test_forfeit_game_player2_forfeits() { + let (mut env, contract_address, _) = utils::setup(); + let player1 = env.create_light_account(); + let player2 = env.create_light_account(); + let game_id = helpers::TEST_GAME_ID_12; + + // Setup game with both players + helpers::setup_two_player_game(&mut env, contract_address, player1, player2, game_id); + + // Player 2 forfeits + env.call_public(player2, PodRacing::at(contract_address).forfeit_game(game_id)); + + // Verify player 1's win count increased + env.public_context_at(contract_address, |context| { + let win_slot = derive_storage_slot_in_map(PodRacing::storage_layout().win_history.slot, player1); + let player1_wins = context.storage_read(win_slot); + assert_eq(player1_wins, 1); + }); +} + +// Test: Player can forfeit mid-game (after playing some rounds) +#[test] +unconstrained fn test_forfeit_game_mid_game() { + let (mut env, contract_address, _) = utils::setup(); + let player1 = env.create_light_account(); + let player2 = env.create_light_account(); + let game_id = helpers::TEST_GAME_ID_13; + + // Setup game + helpers::setup_two_player_game(&mut env, contract_address, player1, player2, game_id); + + // Both players play round 1 + helpers::play_round_with_allocation( + &mut env, contract_address, player1, game_id, 1, helpers::balanced_allocation() + ); + helpers::play_round_with_allocation( + &mut env, contract_address, player2, game_id, 1, helpers::balanced_allocation() + ); + + // Player 1 forfeits mid-game + env.call_public(player1, PodRacing::at(contract_address).forfeit_game(game_id)); + + // Verify player 2 wins + env.public_context_at(contract_address, |context| { + let win_slot = derive_storage_slot_in_map(PodRacing::storage_layout().win_history.slot, player2); + let player2_wins = context.storage_read(win_slot); + assert_eq(player2_wins, 1); + }); +} + +// Test: Cannot forfeit a game that hasn't started (no player2) +#[test(should_fail_with = "Game has not started yet")] +unconstrained fn test_fail_forfeit_game_not_started() { + let (mut env, contract_address, _) = utils::setup(); + let player1 = env.create_light_account(); + let game_id = helpers::TEST_GAME_ID_11; + + // Create game but don't join + env.call_public(player1, PodRacing::at(contract_address).create_game(game_id)); + + // Try to forfeit before player2 joins (should fail) + env.call_public(player1, PodRacing::at(contract_address).forfeit_game(game_id)); +} + +// Test: Non-player cannot forfeit a game +#[test(should_fail_with = "Caller is not a player in this game")] +unconstrained fn test_fail_forfeit_game_not_a_player() { + let (mut env, contract_address, _) = utils::setup(); + let player1 = env.create_light_account(); + let player2 = env.create_light_account(); + let outsider = env.create_light_account(); + let game_id = helpers::TEST_GAME_ID_12; + + // Setup game with both players + helpers::setup_two_player_game(&mut env, contract_address, player1, player2, game_id); + + // Outsider tries to forfeit (should fail) + env.call_public(outsider, PodRacing::at(contract_address).forfeit_game(game_id)); +}