Security Fix: Wave Timing Manipulation Through Unvalidated Tick Parameters #196
Conversation
WalkthroughShifts wave timing from caller-provided ticks to contract-managed timestamps. Adds WaveTimer for per-wave timestamp validation, updates Wave and Store APIs to remove tick parameters, enforces monotonic progression and max advancement, and expands tests accordingly. Minor test assertion formatting updates in coins and gems. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant Store as StoreImpl
participant Wave as WaveImpl
participant Timer as WaveTimerImpl
participant Chain as StarkNet
rect rgba(230,245,255,0.5)
note over C,Store: Start Wave (timestamp-based)
C->>Store: start_wave(wave_id)
Store->>Wave: read_wave(wave_id)
Store->>Timer: WaveTimerImpl::new(wave_id)
Timer->>Chain: get_block_timestamp()
Chain-->>Timer: current_ts
Timer-->>Store: WaveTimer{...}
Store->>Wave: Wave.start()
Wave->>Chain: get_block_timestamp()
Chain-->>Wave: current_ts
Wave-->>Store: Wave{started_at=current_ts,...}
Store->>Store: write_wave_timer(timer)
Store->>Store: write_wave(wave)
Store-->>C: ok
end
sequenceDiagram
autonumber
participant C as Caller
participant Store as StoreImpl
participant Wave as WaveImpl
participant Timer as WaveTimerImpl
participant Chain as StarkNet
rect rgba(240,255,230,0.5)
note over C,Store: Register Enemy Spawn (validated)
C->>Store: register_enemy_spawn(wave_id)
Store->>Wave: read_wave(wave_id)
Store->>Wave: Wave.should_spawn()
Wave->>Chain: get_block_timestamp()
Chain-->>Wave: now_ts
alt allowed to spawn
Store->>Store: read_wave_timer(wave_id)
Store->>Chain: get_block_timestamp()
Chain-->>Store: now_ts
Store->>Timer: update_timestamp(now_ts)
Timer-->>Store: updated_timer
Store->>Store: write_wave_timer(updated_timer)
Store->>Wave: Wave.register_spawn()
Wave->>Chain: get_block_timestamp()
Chain-->>Wave: now_ts
Wave-->>Store: updated_wave
Store-->>C: ok
else not allowed
Store-->>C: revert (timing error)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Out-of-scope changes
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contract/src/models/wave.cairo (1)
250-264: Consider adding logging for rejected timestamp advancements.While the validation logic is sound, rejected attempts might indicate attack attempts and should be logged for monitoring.
Consider emitting events when validation fails to aid in detecting potential attacks:
fn validate_tick_advancement(self: @WaveTimer, new_timestamp: u64) -> bool { // In test environment, be more lenient if *self.last_validated_timestamp == 0 || new_timestamp == 0 { return true; } // Ensure monotonic progression if new_timestamp < *self.last_validated_timestamp { + // Consider logging: InvalidTimestampRejected event return false; } // Check advancement limits let advancement = new_timestamp - *self.last_validated_timestamp; - advancement <= *self.max_tick_advancement_per_tx + let is_valid = advancement <= *self.max_tick_advancement_per_tx; + if !is_valid { + // Consider logging: ExcessiveAdvancementRejected event + } + is_valid }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
contract/src/models/coins.cairo(1 hunks)contract/src/models/gems.cairo(1 hunks)contract/src/models/wave.cairo(11 hunks)contract/src/store.cairo(2 hunks)contract/src/tests/test_store.cairo(15 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
🔇 Additional comments (14)
contract/src/models/coins.cairo (1)
162-162: Test formatting change only, no security implications.The single-line assertion format is consistent with the gems.cairo change and maintains the same test logic.
contract/src/models/gems.cairo (1)
158-158: Test formatting change only, no security implications.The single-line assertion format is consistent with the coins.cairo change and maintains the same test logic.
contract/src/models/wave.cairo (4)
2-2: Excellent use of StarkNet's block timestamp as the canonical time source.The implementation correctly sources timestamps from
get_block_timestamp()instead of accepting external tick parameters, which was the root cause of the vulnerability. The fallback to 1000 for test environments is appropriate.Also applies to: 204-212
214-237: Strong monotonic time validation with bounded advancement.The
validate_timestamp_progressionfunction properly enforces:
- Monotonic progression (no backwards time travel)
- Maximum 1-hour advancement per transaction
- Flexible handling for test environments
This effectively prevents the attack vectors mentioned in issue #179.
241-248: WaveTimer implementation correctly enforces timing constraints.The timer initialization with a 3600-second (1 hour) max advancement limit is appropriate for preventing timing manipulation attacks.
408-429: Comprehensive timestamp validation tests.The tests properly verify both valid progressions and attack scenarios including backward time travel and excessive advancement attempts.
contract/src/store.cairo (4)
10-12: Secure wave start implementation with timer initialization.The removal of
current_tickparameter and creation ofWaveTimerproperly implements the centralized timing system as required.Also applies to: 159-165
168-180: Proper timer-based spawn validation.The spawn registration correctly validates timing through the
WaveTimersystem and updates the timer state atomically.
183-192: Timer accessor methods properly integrated.The read/write operations for
WaveTimerare correctly implemented with appropriate existence checks.
170-170: Resolved – no changes needed. Theshould_spawnimplementation invokesvalidate_timestamp_progressionand enforcescurrent_timestamp >= last_spawn_tick + tick_interval, so timing constraints are already respected.contract/src/tests/test_store.cairo (4)
108-109: Test configuration uses realistic 1-second intervals.The reduced tick interval speeds up tests while maintaining realistic timing validation scenarios.
1579-1835: Excellent comprehensive security test coverage!The security tests thoroughly validate:
- Timestamp monotonicity enforcement
- Maximum advancement per transaction limits
- Protection against extreme values (u64::MAX attack)
- Timer creation and validation
- Multi-spawn timing constraints
- Wave completion integrity
These tests directly address all attack vectors mentioned in issue #179.
1689-1693: Critical test for u64::MAX attack vector.This test specifically validates protection against the attack scenario mentioned in the PR description where an attacker could pass
u64::MAXto immediately trigger all spawns.
125-131: Tests are deterministic despite block timestamps.
get_current_timestamp()falls back to1000_u64whenget_block_timestamp()returns 0, andshould_spawnexplicitly allows same-timestamp spawns in tests (current_timestamp == last_spawn_tick), guaranteeing deterministic behavior inshould_spawnandregister_spawn—no change needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
contract/src/models/wave.cairo (1)
167-177: Enforce interval in register_spawn and remove empty allowance block.The nested condition with an empty block explicitly permits zero-diff spawns, enabling multi-spawn-in-same-block exploits. Also previously flagged in an older review.
Apply this diff to make the rule explicit and safe:
- // Check spawn timing - // Only validate timing if this is not the first spawn and we have a valid interval - if *self.last_spawn_tick > 0 && *self.tick_interval > 0 { - let required_interval = (*self.tick_interval).into(); - if current_timestamp < *self.last_spawn_tick + required_interval { - let time_diff = current_timestamp - *self.last_spawn_tick; - if time_diff == 0 - && current_timestamp > 0 {} else { - assert(false, errors::InvalidSpawnTick); - } - } - } + // Enforce spawn interval strictly for all non-zero intervals + if *self.tick_interval > 0 { + let required_interval: u64 = (*self.tick_interval).into(); + let time_since_last: u64 = current_timestamp - *self.last_spawn_tick; + assert(time_since_last >= required_interval, errors::InvalidSpawnTick); + }
🧹 Nitpick comments (6)
contract/src/models/wave.cairo (6)
84-91: If immediate-first-spawn is desired, backdate last_spawn_tick at start instead of allowing zero-diff spawns.This preserves pacing without reintroducing the vulnerability.
assert( Self::validate_timestamp_progression(*self.created_at, current_timestamp), errors::InvalidTimestamp, ); + // Backdate last_spawn_tick by the interval to allow an immediate first spawn safely. + let required_interval: u64 = (*self.tick_interval).into(); + let initial_last: u64 = if current_timestamp >= required_interval { + current_timestamp - required_interval + } else { + 0_u64 + }; @@ - last_spawn_tick: current_timestamp, + last_spawn_tick: initial_last, is_active: true, is_completed: false, created_at: *self.created_at, started_at: current_timestamp,Question: Should first spawn be immediate by design? If not, set last_spawn_tick: current_timestamp and keep interval enforcement strict.
Also applies to: 98-103
30-41: Deduplicate hardcoded “3600s” into a single constant to prevent drift.Use one constant for both WaveImpl and WaveTimerImpl.
@@ pub mod errors { @@ pub const InvalidTimestamp: felt252 = 'Invalid timestamp'; } + +// Max per-transaction timestamp advancement (seconds) +const MAX_TICK_ADVANCEMENT_PER_TX: u64 = 3600_u64; @@ - let max_advancement = 3600_u64; // 1 hour in seconds + let max_advancement = MAX_TICK_ADVANCEMENT_PER_TX; // 1 hour in seconds @@ - max_tick_advancement_per_tx: 3600_u64 // 1 hour max advancement + max_tick_advancement_per_tx: MAX_TICK_ADVANCEMENT_PER_TX // 1 hour max advancementAlso applies to: 227-231, 241-245
201-209: Avoid production fallback to a magic timestamp (1000).Returning 1000 on timestamp == 0 can mask bugs and create inconsistent behavior. Prefer gating any fallback behind tests or removing it.
Option A (preferred): remove fallback and fix tests to set non-zero timestamps.
Option B: gate fallback behind tests (only if Cairo cfg supports it in your setup).
211-235: Unify progression validation with WaveTimer or return specific errors.You define TimestampTooOld/TimestampTooFuture/ExcessiveTickAdvancement but only return a generic InvalidTimestamp here. Either (a) delegate to WaveTimer-style logic or (b) assert with specific errors to aid debugging and on-chain telemetry.
428-445: Add an update_timestamp negative-path test.Validate that update_timestamp enforces MAX_TICK_ADVANCEMENT_PER_TX.
#[test] #[should_panic(expected: ('Excessive tick advancement',))] fn test_wave_timer_update_enforces_limit() { let timer = WaveTimerImpl::new(1_u64); let too_far = timer.last_validated_timestamp + 7200_u64; let _ = WaveTimerImpl::update_timestamp(@timer, too_far); }
36-39: Unused error codes.TimestampTooOld and TimestampTooFuture are defined but never used. Either wire them into validation or remove them to reduce noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
contract/src/models/wave.cairo(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
🔇 Additional comments (3)
contract/src/models/wave.cairo (3)
42-59: Good shift to contract-managed time and centralized validation hooks.Removing caller-provided tick parameters and adding get_current_timestamp/validate_timestamp_progression and the WaveTimer API aligns with the PR’s security objectives.
333-343: Start flow looks good.created_at and started_at initialization with monotonicity checks are sound.
237-271: WaveTimer enforcement confirmed in start and register flows. Store.cairo creates and writes the timer on start (lines 160–162) and reads, validates, updates, and writes it before register_spawn (lines 173–177), so no additional enforcement is needed.Likely an incorrect or invalid review comment.
contract/src/models/wave.cairo
Outdated
| // In test environment, allow spawns if timestamps are the same or progressed | ||
| if current_timestamp == *self.last_spawn_tick { | ||
| return true; // Allow same timestamp spawns in tests | ||
| } | ||
|
|
||
| current_timestamp >= *self.last_spawn_tick + (*self.tick_interval).into() |
There was a problem hiding this comment.
Remove same-timestamp spawn bypass in should_spawn.
Allowing spawns when current_timestamp == last_spawn_tick lets attackers chain multiple spawns within a single block (shared timestamp), bypassing tick_interval. This re-opens the original timing-manipulation class of bug.
Apply this diff to enforce the interval strictly:
- // Check if enough time has passed since the last spawn
- // In test environment, be more lenient with timing
- if *self.last_spawn_tick == 0 {
- return true; // First spawn is always allowed
- }
-
- // In test environment, allow spawns if timestamps are the same or progressed
- if current_timestamp == *self.last_spawn_tick {
- return true; // Allow same timestamp spawns in tests
- }
-
- current_timestamp >= *self.last_spawn_tick + (*self.tick_interval).into()
+ // Enforce spawn interval strictly (no same-timestamp bypass)
+ current_timestamp >= *self.last_spawn_tick + (*self.tick_interval).into()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contract/src/models/wave.cairo around lines 146 to 151, remove the test-only
same-timestamp bypass: delete the if block that returns true when
current_timestamp == *self.last_spawn_tick so the function only uses the strict
interval check. Replace that section with a single return that evaluates
current_timestamp >= *self.last_spawn_tick + (*self.tick_interval).into(),
ensuring spawns require the configured tick_interval to have progressed.
| fn test_should_spawn_timing() { | ||
| let wave = sample_wave(); | ||
| let active_wave = WaveImpl::start(@wave, 500_u64); | ||
| let active_wave = WaveImpl::start(@wave); | ||
|
|
||
| // Not enough time has passed | ||
| assert(WaveImpl::should_spawn(@active_wave, 550_u64) == false, 'Should not spawn yet'); | ||
|
|
||
| // Enough time has passed | ||
| assert(WaveImpl::should_spawn(@active_wave, 600_u64) == true, 'Should spawn now'); | ||
| // Should spawn is now based on block timestamp, not user input | ||
| // This test verifies the function works with secure timing | ||
| let can_spawn = WaveImpl::should_spawn(@active_wave); | ||
| // The result depends on actual block timestamp vs wave timing | ||
| assert(can_spawn == true || can_spawn == false, 'Should return boolean'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen should_spawn test; current assertion is vacuous.
Add a test that proves interval enforcement by rejecting a second spawn within the same block timestamp.
Add this test:
#[test]
#[should_panic(expected: ('Invalid spawn tick',))]
fn test_register_spawn_rejects_same_timestamp_second_spawn() {
let wave = WaveImpl::new(2_u64, 5_u32, 3_u32, 100_u32);
let active = WaveImpl::start(@wave);
let w1 = WaveImpl::register_spawn(@active);
// Second spawn in same block timestamp must fail
let _ = WaveImpl::register_spawn(@w1);
}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
contract/src/models/wave.cairo (3)
124-151: Remove same-timestamp spawn bypass in should_spawn (security regression)Allowing spawns when current_timestamp == last_spawn_tick reopens interval bypass within a single block. Permit exactly one immediate spawn (first spawn) and otherwise enforce the interval strictly. Also fix the “Aallow” typo. Mirrors a prior review concern.
Apply:
- // Check if enough time has passed since the last spawn - if *self.last_spawn_tick == 0 { - return true; // First spawn is always allowed - } - - // Aallow spawns if timestamps are the same or progressed - if current_timestamp == *self.last_spawn_tick { - return true; - } - - current_timestamp >= *self.last_spawn_tick + (*self.tick_interval).into() + // Allow exactly one immediate spawn (first spawn). Afterwards, enforce interval strictly. + if *self.enemies_spawned == 0 { + return true; + } + current_timestamp >= *self.last_spawn_tick + (*self.tick_interval).into()
153-177: Strip test-only leniency and empty block from register_spawn; enforce interval after first spawnThe nested block silently allows unlimited same-timestamp spawns; attackers can chain within one block. Replace with a single assert that gates only post-first spawns. Mirrors a prior review concern.
- // Check spawn timing - // Only validate timing if this is not the first spawn and we have a valid interval - if *self.last_spawn_tick > 0 && *self.tick_interval > 0 { - let required_interval = (*self.tick_interval).into(); - if current_timestamp < *self.last_spawn_tick + required_interval { - let time_diff = current_timestamp - *self.last_spawn_tick; - if time_diff == 0 - && current_timestamp > 0 {} else { - assert(false, errors::InvalidSpawnTick); - } - } - } + // Enforce spawn interval for all spawns after the first (first spawn may be immediate) + if *self.enemies_spawned > 0 && *self.tick_interval > 0 { + let required_interval = (*self.tick_interval).into(); + assert( + current_timestamp >= *self.last_spawn_tick + required_interval, + errors::InvalidSpawnTick, + ); + }
349-358: Strengthen should_spawn test; current assertion is vacuousAssert first spawn is allowed, and second spawn in the same timestamp is denied.
- // Should spawn is now based on block timestamp, not user input - // This test verifies the function works with secure timing - let can_spawn = WaveImpl::should_spawn(@active_wave); - // The result depends on actual block timestamp vs wave timing - assert(can_spawn == true || can_spawn == false, 'Should return boolean'); + // First spawn should be allowed immediately after start + assert(WaveImpl::should_spawn(@active_wave) == true, 'First spawn must be allowed'); + // After performing the first spawn in the same timestamp, the next should be blocked + let w1 = WaveImpl::register_spawn(@active_wave); + assert(WaveImpl::should_spawn(@w1) == false, 'Second spawn must be delayed by tick_interval');If you prefer a panic-style test, I can add a dedicated #[should_panic] case.
🧹 Nitpick comments (5)
contract/src/models/wave.cairo (5)
20-28: WaveTimer model is a good foundation; avoid logic drift with WaveImplYou now have two parallel timing validators (WaveImpl::validate_timestamp_progression and WaveTimerImpl::validate_tick_advancement). Consider delegating WaveImpl timing checks to WaveTimer (or extracting a single shared helper) so max-advancement policy can’t diverge.
36-40: Tighten error set or wire them inTimestampTooOld/TimestampTooFuture aren’t used. Either use them in validation branches or drop them to avoid dead-code drift.
200-207: Avoid magic fallback timestampHardcoding 1000 can mask timing bugs in tests and create cross-test coupling. Prefer returning the chain value as-is.
- let timestamp = get_block_timestamp(); - if timestamp == 0 { - 1000_u64 - } else { - timestamp - } + get_block_timestamp()
209-230: Unify advancement policy with WaveTimerThis duplicates the 3600s cap defined in WaveTimer. Drive both through one source (e.g., WaveTimer field) or a shared const to prevent drift.
233-267: WaveTimer API — LGTM; consider wiring into WaveImplLooks sound. Next step: have WaveImpl use a persisted WaveTimer per wave rather than ad-hoc checks, so stateful advancement is enforced uniformly across modules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
contract/src/models/wave.cairo(11 hunks)
🔇 Additional comments (7)
contract/src/models/wave.cairo (7)
16-18: New audit fields (created_at, started_at) — LGTMGood addition for provenance and lifecycle auditing. They’re correctly set in new() and start().
64-77: Initialize created_at from chain time — LGTMGood use of the canonical source; zero start_at is sane prior to activation.
80-104: start(): secure activation and provenance — LGTMMonotonicity check and seeding started_at/last_spawn_tick with current chain time are consistent.
188-193: Timestamp update on spawn — LGTMlast_spawn_tick moves forward on each successful spawn; consistent with enforcement above.
294-306: Zero impls — LGTMZero states are coherent for model defaults and test setup.
331-337: start() test updates — LGTMAsserts on started_at and last_spawn_tick are appropriate.
400-439: Timestamp validation tests — LGTMGood coverage for monotonicity and max-advancement bounds; complements WaveTimer tests.
| fn start(self: @Wave) -> Wave; | ||
| fn complete(self: @Wave) -> Wave; | ||
| fn should_spawn(self: @Wave, current_tick: u64) -> bool; | ||
| fn register_spawn(self: @Wave, current_tick: u64) -> Wave; | ||
| fn should_spawn(self: @Wave) -> bool; | ||
| fn register_spawn(self: @Wave) -> Wave; | ||
| fn is_wave_completed(self: @Wave) -> bool; | ||
| fn get_current_timestamp() -> u64; | ||
| fn validate_timestamp_progression(last_timestamp: u64, current_timestamp: u64) -> bool; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
API shift to tickless entrypoints — LGTM; verify all call sites updated
Removal of caller-supplied ticks matches the threat model fix. Please verify no lingering usages remain.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# No external tick parameters left
rg -nP '\bcurrent_tick\b|tick:\s*u(32|64)'
# Callers now use zero-arg timing methods
rg -nP '\bshould_spawn\s*\(' -n -C2
rg -nP '\bregister_spawn\s*\(' -n -C2
rg -nP '\bstart\s*\(' -n -C2
# No same-timestamp bypass remnants
rg -nP 'current_timestamp\s*==\s*\*?self\.last_spawn_tick' -n
# Canonical time source usage
rg -nP '\bget_block_timestamp\s*\(' -nLength of output: 9499
Remove residual tick parameters and bypass code
- contract/src/store.cairo (around line 291):
can_tower_attackstill acceptscurrent_tick→ refactor to useget_block_timestamp()only. - contract/src/models/tower.cairo (lines 15–27, 51–55):
can_attackAPIs andlast_attack_tickfield remain → drop caller-supplied tick args. - contract/src/models/wave.cairo (line 146): Remove the same-timestamp bypass (
if current_timestamp == *self.last_spawn_tick) to enforce secure progression.
| fn test_register_spawn_secure() { | ||
| let wave = sample_wave(); | ||
| let active_wave = WaveImpl::start(@wave, 500_u64); | ||
| let updated_wave = WaveImpl::register_spawn(@active_wave, 600_u64); | ||
| let active_wave = WaveImpl::start(@wave); | ||
|
|
||
| let updated_wave = WaveImpl::register_spawn(@active_wave); | ||
|
|
||
| assert(updated_wave.enemies_spawned == 1_u32, 'Should have 1 spawned'); | ||
| assert(updated_wave.last_spawn_tick == 600_u64, 'Should update last spawn tick'); | ||
| assert( | ||
| updated_wave.last_spawn_tick >= active_wave.last_spawn_tick, 'Should update timestamp', | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also cover second spawn within same block should fail
Add a test that attempts a second register_spawn without time progression and expects InvalidSpawnTick.
Add alongside existing tests:
#[test]
#[should_panic(expected: ('Invalid spawn tick',))]
fn test_register_spawn_rejects_same_timestamp_second_spawn() {
let wave = WaveImpl::new(2_u64, 5_u32, 3_u32, 100_u32);
let active = WaveImpl::start(@wave);
let w1 = WaveImpl::register_spawn(@active);
// Second spawn in same block timestamp must fail
let _ = WaveImpl::register_spawn(@w1);
}🤖 Prompt for AI Agents
In contract/src/models/wave.cairo around lines 361 to 371, add a new unit test
that ensures a second register_spawn called in the same block (without advancing
time) reverts with InvalidSpawnTick; create a test similar to the snippet in the
review: construct a WaveImpl with capacity >1, call WaveImpl::start, call
WaveImpl::register_spawn once (w1), then call WaveImpl::register_spawn(&w1)
again and mark the test with #[should_panic(expected: ('Invalid spawn tick',))]
so the second call fails as expected; place the test adjacent to the existing
spawn tests and use WaveImpl::new/start/register_spawn APIs consistently.
2 participants
This PR fixes HIGH severity vulnerability where wave system accepted unvalidated tick parameters from external callers, allowing manipulation of wave timing mechanics.
Closes #179
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests