Merge pull request #195 from truthixify/fix-178

Fixes a race condition in trigger_trap() that allowed traps to be triggered multiple times simultaneously.

Author: KevinMB0220

contract/src/models/enemy.cairo

@@ -68,18 +68,20 @@ pub impl EnemyImpl of EnemySystem {
             return Result::Err(errors::InvalidSpeed);
         }
 
-        Result::Ok(Enemy {
-            id,
-            enemy_type,
-            health,
-            speed,
-            x,
-            y,
-            is_alive: true,
-            coin_reward,
-            xp_reward,
-            reward_claimed: false
-        })
+        Result::Ok(
+            Enemy {
+                id,
+                enemy_type,
+                health,
+                speed,
+                x,
+                y,
+                is_alive: true,
+                coin_reward,
+                xp_reward,
+                reward_claimed: false,
+            },
+        )
     }
 
     fn take_damage(self: @Enemy, amount: u32) -> Enemy {

contract/src/models/trap.cairo

@@ -59,11 +59,14 @@ pub impl TrapImpl of TrapTrait {
     }
 
     fn trigger(ref self: Trap) -> u16 {
+        // Check if the trap is active before modifying its state.
+        // If it's not active, we immediately return 0, preventing any side effects.
         if !self.is_active {
             return 0;
         }
 
-        self.is_active = false; // Trap is consumed after triggering
+        // If it is active, deactivate it and return its damage.
+        self.is_active = false;
         self.damage
     }
 

contract/src/store.cairo

@@ -294,7 +294,21 @@ pub impl StoreImpl of StoreTrait {
         let mut trap = self.read_trap(trap_id);
         assert(trap.is_active == true, 'Trap not active');
         assert(TrapImpl::check_trigger(@trap, enemy_pos) == true, 'Enemy not in range');
+
+        // The TrapImpl::trigger function will set is_active to false and return the damage.
+        // If it was already inactive (e.g., another transaction got there first), it returns 0.
         let damage = TrapImpl::trigger(ref trap);
+
+        // If damage is 0, it means the trap was not active when TrapImpl::trigger was called.
+        // This could happen if a concurrent transaction successfully triggered and updated
+        // the global state to inactive before this transaction's TrapImpl::trigger was executed.
+        // In this scenario, we just return 0, as no new damage should be dealt.
+        if damage == 0 {
+            return 0;
+        }
+
+        // This transaction successfully triggered the trap and received non-zero damage.
+        // Write the now-inactive trap state back to the world.
         self.write_trap(@trap);
         damage
     }

contract/src/tests/test_store.cairo

@@ -1474,5 +1474,60 @@ mod tests {
         // Attempt to spend 50 gems which is more than the balance; should panic
         store.spend_gems(player_address, 50_u64);
     }
-}
 
+    #[test]
+    #[should_panic(expected: 'Trap not active')]
+    fn test_store_trigger_trap_concurrent_access() {
+        let player_system_contract_address = create_test_player_system();
+        let world = create_test_world(player_system_contract_address);
+        let mut store_tx1: Store = StoreTrait::new(world.clone()); // Simulate transaction 1
+        let mut store_tx2: Store = StoreTrait::new(world.clone()); // Simulate transaction 2
+
+        let trap = create_sample_trap();
+        store_tx1.place_trap(trap); // Place trap using one store instance
+
+        let enemy_pos = Vec2 { x: 12_u32, y: 16_u32 }; // Within radius
+
+        // Scenario: Both transactions read the active trap, then try to trigger
+        // Transaction 1 triggers
+        let damage_tx1 = store_tx1.trigger_trap(1_u32, enemy_pos);
+
+        // Transaction 2 tries to trigger, but the trap is already inactive due to TX1
+        let damage_tx2 = store_tx2.trigger_trap(1_u32, enemy_pos);
+
+        // Assertions
+        assert(damage_tx1 == 50_u16, 'TX1 should deal 50 damage');
+        assert!(damage_tx2 == 0_u16, "TX2 should deal 0 damage due to race condition protection");
+
+        let final_trap_state = store_tx1.read_trap(1_u32);
+        assert!(final_trap_state.is_active == false, "Trap should be consumed after one trigger");
+
+        // Verify that the second store instance also reflects the inactive state
+        let final_trap_state_tx2 = store_tx2.read_trap(1_u32);
+        assert!(final_trap_state_tx2.is_active == false, "TX2 view should also show inactive trap");
+    }
+
+    #[test]
+    #[should_panic(expected: 'Trap not active')]
+    fn test_store_trigger_trap_already_inactive_returns_zero() {
+        let player_system_contract_address = create_test_player_system();
+        let world = create_test_world(player_system_contract_address);
+        let mut store: Store = StoreTrait::new(world);
+
+        let mut trap = create_sample_trap();
+        store.place_trap(trap);
+
+        let enemy_pos = Vec2 { x: 12_u32, y: 16_u32 }; // Within radius
+
+        // First trigger works
+        let damage1 = store.trigger_trap(1_u32, enemy_pos);
+        assert!(damage1 == 50_u16, "First trigger should deal damage");
+
+        // Second trigger (trap is already inactive) should return 0, no panic
+        let damage2 = store.trigger_trap(1_u32, enemy_pos);
+        assert!(damage2 == 0_u16, "Second trigger on inactive trap should return 0");
+
+        let final_trap = store.read_trap(1_u32);
+        assert(final_trap.is_active == false, 'Trap should remain inactive');
+    }
+}