Skip to content

🛡️ Fix: Path System Array Index Out of Bounds Vulnerability #193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sotoJ24 wants to merge 4 commits into from
Closed

🛡️ Fix: Path System Array Index Out of Bounds Vulnerability #193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4cd38c7
Feat: Path System Array Index Out of Bounds Vulnerability
sotoJ24 Aug 24, 2025
51bebeb
Fix: test_path_system.cairo and game.cairo functions
sotoJ24 Sep 3, 2025
9ada55e
Merge branch 'SunsetLabs-Game:main' into main
sotoJ24 Sep 9, 2025
78b896e
Fix: refactor advance_enemy_position fn and get_step_from_array fn
sotoJ24 Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 113 additions & 35 deletions contract/src/systems/game.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,11 +222,90 @@ 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),
}
}
}
}

Comment on lines +225 to +260
Copy link

@coderabbitai coderabbitai bot Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Add/verify tests for edge cases and legacy API behavior

Given the high-severity bug being addressed, please ensure tests cover:

  • Invalid path_id for all public helpers (expect None/InvalidPath, no panics).
  • Index exactly at length (expect PathCompleted from get_path_step_result, no movement in advance_enemy_position).
  • Index > length (expect None/IndexOutOfBounds, no movement).
  • Last valid step followed by call to advance (enemy stays put; path considered completed).
  • Legacy is_path_completed no longer panics (after the suggested change), and callers are updated accordingly.

I can draft these tests if helpful.

Run this script to find potential call-site breakages due to get_path_step signature change:

Also applies to: 248-276, 278-294, 295-301, 351-358

🏁 Script executed:

#!/bin/bash
# Look for calls expecting a tuple instead of Option
rg -nP -C2 '\bget_path_step\s*\('

# Ensure no panics remain in the path system APIs
rg -nP -C2 '\bpanic!\(|\bassert\s*\(' contract/src/systems/game.cairo

Length of output: 4227

🏁 Script executed:

#!/bin/bash
# Search for get_path_step_result in tests
rg -nP -C2 'get_path_step_result' contract/src/tests || echo "No matches for get_path_step_result"

# Search for is_path_completed in tests
rg -nP -C2 'is_path_completed' contract/src/tests || echo "No matches for is_path_completed"

Length of output: 3349

Critical fixes required: update tests and align legacy API behavior

  • Tests in contract/src/tests/test_path_system.cairo must be updated to match the new get_path_step signature and behavior:
    • test_get_path_step_valid_paths
      • Change from let (x, y) = PathSystemImpl::get_path_step(...) to 
        let coords = PathSystemImpl::get_path_step(...);
assert!(coords == Option::Some((expected_x, expected_y)), "…");
    • test_get_path_step_invalid_path and test_get_path_step_index_out_of_bounds
      • Remove #[should_panic] and change to 
        assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None);
  • Add new tests for get_path_step_result covering:
    • InvalidPath when path_id not found
    • IndexOutOfBounds when index > last step
    • PathCompleted when index == length
    • Success((x, y)) for valid steps
  • In contract/src/systems/game.cairo, the old is_path_completed still panics on invalid path_id. Per the original suggestion, it should return a safe result (e.g., false or Option<bool>) and callers updated; either remove the panic! or wrap it in a safe variant, then adjust or remove the #[should_panic] test accordingly.

I can draft the updated tests and refactoring if that would help.

🤖 Prompt for AI Agents 
In contract/src/systems/game.cairo around lines 212 to 247 the new get_path_step
signature returns Option<(u32,u32)> and get_path_step_result returns PathResult,
but tests and the legacy is_path_completed behavior weren't updated; update
tests in contract/src/tests/test_path_system.cairo to: replace destructuring let
(x,y) = PathSystemImpl::get_path_step(...) with let coords =
PathSystemImpl::get_path_step(...); assert that coords ==
Option::Some((expected_x, expected_y)) for valid cases, remove #[should_panic]
on invalid-path and out-of-bounds tests and
assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None), add
new tests for get_path_step_result asserting InvalidPath, IndexOutOfBounds,
PathCompleted (when index == length) and Success((x,y)) for valid indices; also
change is_path_completed implementation to not panic on missing path (return a
safe value or Option<bool>) and update any callers/tests to use the safe variant
or adjust assertions accordingly.

fn advance_enemy_position(ref enemy: Enemy, path_id: u64, current_index: u32) -> Enemy {
if !enemy.is_alive {
return enemy;
}

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),
}
}
}
}

/// 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<bool> {
// 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 => true,
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<Span<(u32, u32)>> {
match path_id {
0 => {
// Path 0: Simple straight line from left to right
Expand All @@ -238,7 +317,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
Expand All @@ -251,7 +330,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
Expand All @@ -266,46 +345,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
/// 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 {
// 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(i))
}
}

/// 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<u32> {
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,
}
}
}

Expand Down Expand Up @@ -368,4 +446,4 @@ pub mod brawl_game {
}
}
}
}
}
42 changes: 19 additions & 23 deletions contract/src/tests/test_path_system.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Comment on lines +39 to 40
Copy link

@coderabbitai coderabbitai bot Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix undefined variables in test (invalid_path, idx)

This test won’t compile; use concrete literals or define the variables.

Apply this diff:

-        assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None);
+        assert_eq!(PathSystemImpl::get_path_step(99_u64, 0_u32), Option::None);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None);
}
assert_eq!(PathSystemImpl::get_path_step(99_u64, 0_u32), Option::None);
}
🤖 Prompt for AI Agents 
In contract/src/tests/test_path_system.cairo around lines 39 to 40, the test
references undefined variables invalid_path and idx which causes a compile
error; replace them with concrete literals or add local let bindings before use
(e.g., define invalid_path with an appropriate Path value and idx with a
usize/u128 literal) and then call PathSystemImpl::get_path_step(invalid_path,
idx) asserting Option::None; ensure the types match the function signature and
the test imports any needed constructors or constants.


#[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);
}
Comment on lines +44 to 45
Copy link

@coderabbitai coderabbitai bot Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct the “index out of bounds” test to use a valid path with an OOB index

Currently reuses the invalid path variables; assert OOB on a valid path.

Apply this diff:

-        assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None);
+        // Path 0 has length 6 (valid indices 0..=5); 6 is out of bounds
+        assert_eq!(PathSystemImpl::get_path_step(0_u64, 6_u32), Option::None);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None);
}
// Path 0 has length 6 (valid indices 0..=5); 6 is out of bounds
assert_eq!(PathSystemImpl::get_path_step(0_u64, 6_u32), Option::None);
🤖 Prompt for AI Agents 
In contract/src/tests/test_path_system.cairo around lines 44 to 45, the test for
"index out of bounds" incorrectly reuses the invalid_path variable; update the
assertion to call PathSystemImpl::get_path_step with a valid path (e.g.,
valid_path) and an index that is out of bounds for that valid path (e.g., its
length or greater) so the test asserts Option::None on a valid path with an OOB
index.


#[test]
Expand Down Expand Up @@ -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]
Expand Down
Loading