From 4cd38c7b742ff6b0d12a0d902624da4657e73c06 Mon Sep 17 00:00:00 2001 From: Josue Soto Date: Sun, 24 Aug 2025 12:06:38 -0600 Subject: [PATCH 1/3] Feat: Path System Array Index Out of Bounds Vulnerability --- contract/src/systems/game.cairo | 156 +++++++++++++++++++++++++------- 1 file changed, 122 insertions(+), 34 deletions(-) diff --git a/contract/src/systems/game.cairo b/contract/src/systems/game.cairo index b1dafe2..f79cf87 100644 --- a/contract/src/systems/game.cairo +++ b/contract/src/systems/game.cairo @@ -209,11 +209,100 @@ pub mod brawl_game { } } + // Result type for path operations + #[derive(Copy, Drop, Serde)] + pub enum PathResult { + Success: (u32, u32), + InvalidPath, + IndexOutOfBounds, + PathCompleted, + } + #[generate_trait] pub impl PathSystemImpl of PathSystemTrait { /// Returns the (x, y) coordinates for the given path_id and step index - fn get_path_step(path_id: u64, index: u32) -> (u32, u32) { - // Predefined paths + fn get_path_step(path_id: u64, index: u32) -> Option<(u32, u32)> { + // Validate path_id first + let path_data = Self::get_path_data(path_id); + match path_data { + Option::None => Option::None, + Option::Some(steps) => Self::get_step_from_array(steps, index), + } + } + + /// Returns the (x, y) coordinates for the given path_id and step index with detailed result + fn get_path_step_result(path_id: u64, index: u32) -> PathResult { + // Validate path_id first + let path_data = Self::get_path_data(path_id); + match path_data { + Option::None => PathResult::InvalidPath, + Option::Some(steps) => { + match Self::get_step_from_array(steps, index) { + Option::None => PathResult::IndexOutOfBounds, + Option::Some(coords) => PathResult::Success(coords), + } + } + } + } + + /// Moves an enemy to the next valid step in its path + fn advance_enemy_position(ref enemy: Enemy, path_id: u64, current_index: u32) -> Enemy { + let next_index = current_index + 1; + + // Check if we've reached the end of the path + match Self::is_path_completed_safe(path_id, next_index) { + Option::None => { + // Invalid path_id, return enemy unchanged + enemy + }, + Option::Some(is_completed) => { + if is_completed { + // Enemy has completed the path + enemy + } else { + // Get the next position + match Self::get_path_step(path_id, next_index) { + Option::None => { + // Invalid index, return enemy unchanged + enemy + }, + Option::Some((next_x, next_y)) => { + EnemySystem::move_to(@enemy, next_x, next_y) + } + } + } + } + } + } + + /// Checks whether the enemy has reached the last step of its path + /// Returns None if path_id is invalid + fn is_path_completed_safe(path_id: u64, index: u32) -> Option { + // Get the path length for each predefined path + let path_length = match path_id { + 0 => Option::Some(6_u32), // Path 0 has 6 steps (indices 0-5) + 1 => Option::Some(7_u32), // Path 1 has 7 steps (indices 0-6) + 2 => Option::Some(9_u32), // Path 2 has 9 steps (indices 0-8) + _ => Option::None, + }; + + match path_length { + Option::None => Option::None, + Option::Some(length) => Option::Some(index >= length), + } + } + + /// Legacy function for backward compatibility - will panic if path completed + fn is_path_completed(path_id: u64, index: u32) -> bool { + match Self::is_path_completed_safe(path_id, index) { + Option::None => panic!("Invalid path_id"), + Option::Some(result) => result, + } + } + + /// Get path data for a given path_id + /// Returns None if path_id is invalid + fn get_path_data(path_id: u64) -> Option> { match path_id { 0 => { // Path 0: Simple straight line from left to right @@ -225,7 +314,7 @@ pub mod brawl_game { (4_u32, 5_u32), (5_u32, 5_u32) // End at (5, 5) ]; - Self::get_step_from_array(path_0_steps.span(), index) + Option::Some(path_0_steps.span()) }, 1 => { // Path 1: L-shaped path @@ -238,7 +327,7 @@ pub mod brawl_game { (2_u32, 3_u32), (3_u32, 3_u32) // End at (3, 3) ]; - Self::get_step_from_array(path_1_steps.span(), index) + Option::Some(path_1_steps.span()) }, 2 => { // Path 2: Zigzag pattern @@ -253,46 +342,45 @@ pub mod brawl_game { (4_u32, 1_u32), (4_u32, 2_u32) // End ]; - Self::get_step_from_array(path_2_steps.span(), index) + Option::Some(path_2_steps.span()) }, - _ => panic!("Invalid path_id"), + _ => Option::None, } } - /// Moves an enemy to the next valid step in its path - /// Returns an updated Enemy with new x, y coordinates - fn advance_enemy_position(ref enemy: Enemy, path_id: u64, current_index: u32) -> Enemy { - let next_index = current_index + 1; - - // Check if we've reached the end of the path - if Self::is_path_completed(path_id, next_index) { - // Enemy has completed the path - return current enemy unchanged - enemy + /// Safe version of get_step_from_array that returns Option + fn get_step_from_array(steps: Span<(u32, u32)>, index: u32) -> Option<(u32, u32)> { + if index >= steps.len() { + Option::None } else { - // Get the next position - let (next_x, next_y) = Self::get_path_step(path_id, next_index); - - EnemySystem::move_to(@enemy, next_x, next_y) + Option::Some(*steps.at(index.into())) } } - /// Checks whether the enemy has reached the last step of its path - fn is_path_completed(path_id: u64, index: u32) -> bool { - // Get the path length for each predefined path - let path_length = match path_id { - 0 => 6_u32, // Path 0 has 6 steps (indices 0-5) - 1 => 7_u32, // Path 1 has 7 steps (indices 0-6) - 2 => 9_u32, // Path 2 has 9 steps (indices 0-8) - _ => panic!("Invalid path_id"), - }; + /// Get the total length of a path + fn get_path_length(path_id: u64) -> Option { + match path_id { + 0 => Option::Some(6_u32), + 1 => Option::Some(7_u32), + 2 => Option::Some(9_u32), + _ => Option::None, + } + } - index >= path_length + /// Validate if a path_id exists + fn is_valid_path(path_id: u64) -> bool { + match path_id { + 0 | 1 | 2 => true, + _ => false, + } } - /// Get a step from a Span of coordinates - fn get_step_from_array(steps: Span<(u32, u32)>, index: u32) -> (u32, u32) { - assert(index < steps.len(), 'Index out of bounds'); - *steps.at(index.into()) + /// Validate if an index is valid for a given path + fn is_valid_index(path_id: u64, index: u32) -> bool { + match Self::get_path_length(path_id) { + Option::None => false, + Option::Some(length) => index < length, + } } } @@ -355,4 +443,4 @@ pub mod brawl_game { } } } -} +} \ No newline at end of file From 51bebeb949edd0887aed6d8ce208a4371470a986 Mon Sep 17 00:00:00 2001 From: Josue Soto Date: Wed, 3 Sep 2025 15:18:08 -0600 Subject: [PATCH 2/3] Fix: test_path_system.cairo and game.cairo functions --- contract/src/systems/game.cairo | 2 +- contract/src/tests/test_path_system.cairo | 42 ++++++++++------------- 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/contract/src/systems/game.cairo b/contract/src/systems/game.cairo index f79cf87..af53a37 100644 --- a/contract/src/systems/game.cairo +++ b/contract/src/systems/game.cairo @@ -295,7 +295,7 @@ pub mod brawl_game { /// Legacy function for backward compatibility - will panic if path completed fn is_path_completed(path_id: u64, index: u32) -> bool { match Self::is_path_completed_safe(path_id, index) { - Option::None => panic!("Invalid path_id"), + Option::None => true, Option::Some(result) => result, } } diff --git a/contract/src/tests/test_path_system.cairo b/contract/src/tests/test_path_system.cairo index a898819..5f3a997 100644 --- a/contract/src/tests/test_path_system.cairo +++ b/contract/src/tests/test_path_system.cairo @@ -15,39 +15,33 @@ mod path_system_tests { #[test] fn test_get_path_step_valid_paths() { - // Test Path 0 (straight line) - let (x, y) = PathSystemImpl::get_path_step(0_u64, 0_u32); - assert!(x == 0_u32 && y == 5_u32, "Path 0 start incorrect"); + let coords = PathSystemImpl::get_path_step(0_u64, 0_u32); + assert!(coords == Option::Some((0_u32, 5_u32)), "Path 0 start incorrect"); - let (x, y) = PathSystemImpl::get_path_step(0_u64, 5_u32); - assert!(x == 5_u32 && y == 5_u32, "Path 0 end incorrect"); + let coords = PathSystemImpl::get_path_step(0_u64, 5_u32); + assert!(coords == Option::Some((5_u32, 5_u32)), "Path 0 end incorrect"); - // Test Path 1 (L-shaped) - let (x, y) = PathSystemImpl::get_path_step(1_u64, 0_u32); - assert!(x == 0_u32 && y == 0_u32, "Path 1 start incorrect"); + let coords = PathSystemImpl::get_path_step(1_u64, 0_u32); + assert!(coords == Option::Some((0_u32, 0_u32)), "Path 1 start incorrect"); - let (x, y) = PathSystemImpl::get_path_step(1_u64, 6_u32); - assert!(x == 3_u32 && y == 3_u32, "Path 1 end incorrect"); + let coords = PathSystemImpl::get_path_step(1_u64, 6_u32); + assert!(coords == Option::Some((3_u32, 3_u32)), "Path 1 end incorrect"); - // Test Path 2 (zigzag) - let (x, y) = PathSystemImpl::get_path_step(2_u64, 0_u32); - assert!(x == 0_u32 && y == 2_u32, "Path 2 start incorrect"); + let coords = PathSystemImpl::get_path_step(2_u64, 0_u32); + assert!(coords == Option::Some((0_u32, 2_u32)), "Path 2 start incorrect"); - let (x, y) = PathSystemImpl::get_path_step(2_u64, 8_u32); - assert!(x == 4_u32 && y == 2_u32, "Path 2 end incorrect"); + let coords = PathSystemImpl::get_path_step(2_u64, 8_u32); + assert!(coords == Option::Some((4_u32, 2_u32)), "Path 2 end incorrect"); } #[test] - #[should_panic] fn test_get_path_step_invalid_path() { - let _ = PathSystemImpl::get_path_step(99_u64, 0_u32); + assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None); } #[test] - #[should_panic] fn test_get_path_step_index_out_of_bounds() { - // Path 0 only has indices 0-5, so index 6 should panic - let _ = PathSystemImpl::get_path_step(0_u64, 6_u32); + assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None); } #[test] @@ -88,9 +82,11 @@ mod path_system_tests { } #[test] - #[should_panic] - fn test_is_path_completed_invalid_path() { - let _ = PathSystemImpl::is_path_completed(99_u64, 0_u32); + fn test_is_path_completed_invalid_path_returns_true() { + assert!( + PathSystemImpl::is_path_completed(99_u64, 0_u32), + "Invalid path IDs should be treated as completed" + ); } #[test] From 78b896e97fac79a588c2baae328f183d9f11ebf8 Mon Sep 17 00:00:00 2001 From: Josue Soto Date: Mon, 8 Sep 2025 21:45:36 -0600 Subject: [PATCH 3/3] Fix: refactor advance_enemy_position fn and get_step_from_array fn --- contract/src/systems/game.cairo | 46 +++++++++++++-------------------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/contract/src/systems/game.cairo b/contract/src/systems/game.cairo index c5f8389..3bcf08e 100644 --- a/contract/src/systems/game.cairo +++ b/contract/src/systems/game.cairo @@ -258,31 +258,21 @@ pub mod brawl_game { } } - /// Moves an enemy to the next valid step in its path fn advance_enemy_position(ref enemy: Enemy, path_id: u64, current_index: u32) -> Enemy { - let next_index = current_index + 1; + if !enemy.is_alive { + return enemy; + } - // Check if we've reached the end of the path - match Self::is_path_completed_safe(path_id, next_index) { - Option::None => { - // Invalid path_id, return enemy unchanged - enemy - }, - Option::Some(is_completed) => { - if is_completed { - // Enemy has completed the path - enemy - } else { - // Get the next position - match Self::get_path_step(path_id, next_index) { - Option::None => { - // Invalid index, return enemy unchanged - enemy - }, - Option::Some((next_x, next_y)) => { - EnemySystem::move_to(@enemy, next_x, next_y) - } - } + match Self::get_path_length(path_id) { + Option::None => enemy, + Option::Some(length) => { + if length == 0_u32 || current_index >= length - 1_u32 { + return enemy; + } + let next_index = current_index + 1_u32; + match Self::get_path_step(path_id, next_index) { + Option::None => enemy, + Option::Some((next_x, next_y)) => EnemySystem::move_to(@enemy, next_x, next_y), } } } @@ -360,13 +350,13 @@ pub mod brawl_game { _ => Option::None, } } - - /// Safe version of get_step_from_array that returns Option - fn get_step_from_array(steps: Span<(u32, u32)>, index: u32) -> Option<(u32, u32)> { - if index >= steps.len() { + /// Helper to get a step from a path array safely + fn get_step_from_array(steps: Span<(u32, u32)>, index: u32) -> Option<(u32, u32)> { + let i: usize = index.into(); + if i >= steps.len() { Option::None } else { - Option::Some(*steps.at(index.into())) + Option::Some(*steps.at(i)) } }