feat: harden game settlement and staking paths for issues 522-525#682
Merged
gabito1451 merged 1 commit intoApr 25, 2026
Conversation
Strengthen on-chain game settlement with replay-safe payout state transitions, admin-gated stake limits, safer timeout arithmetic, and strict puzzle reward amount validation. Add regression tests and updated snapshots to cover settlement replay, admin authorization, and invalid amount edge cases. Made-with: Cursor
Contributor
📝 WalkthroughWalkthroughThe PR introduces a terminal Changes
Sequence DiagramsequenceDiagram
participant Player
participant GameContract
participant TokenContract
Player->>GameContract: payout(game_id, player)
GameContract->>GameContract: Check game not Settled
GameContract->>GameContract: Validate authorization
alt Game is Settled
GameContract-->>Player: Error: AlreadySettled
else Game not Settled
GameContract->>TokenContract: Transfer winnings
TokenContract-->>GameContract: Success
GameContract->>GameContract: Set GameState::Settled
GameContract-->>Player: Success
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 14 UNAVAILABLE: read ECONNRESET 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. Comment |
|
@devsimze Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/game_contract/src/lib.rs (1)
196-204:⚠️ Potential issue | 🔴 CriticalCritical:
payout_tournament_optimizedis exploitable on canceled games — drains contract tokens.
cancel_game(line 556, unchanged) setsstate = GameState::Completedwhile leavingplayer2 = Noneandwinner = None. This function only gates onstate == Completedplusplayer1.require_auth()and then computestotal_pool = wager_amount(no player2) and physically transfers it viatoken_client.transfer(&contract_address, &winner, &amount)at line 241 — with no winner/player2/escrow sanity check.A malicious flow:
create_game(attacker, 100)— deposits 100; contract holds funds from other games too.cancel_game(game_id, attacker)— refunded 100, state →Completed.payout_tournament_optimized(game_id, [attacker], [100])— passes the state check, transfers anotherwager_amountfrom the contract toattacker.
payoutis safe because of thegame.winner.as_ref() == Some(&winner)check;payout_tournamentis mostly safe because the escrow balance is zeroed bycancel_game. Onlypayout_tournament_optimizedis unprotected — and the newSettledsemantics in this PR don't close it because cancellation never reaches a terminal payout-rejecting state.🛡️ Recommended fix — make cancellation terminal (preferred)
Treat
cancel_gameas a settlement: this leverages the newSettledstate and closes the lateral path uniformly across all three payout functions.- game.state = GameState::Completed; // Mark as completed to prevent joining + game.state = GameState::Settled; // Terminal: prevents joining and any payout path games.set(game_id, game);Alternatively (or in addition), defensively reject games that never reached a settleable shape inside
payout_tournament_optimized:if game.state != GameState::Completed { + if game.state == GameState::Settled { + return Err(ContractError::AlreadySettled); + } return Err(ContractError::GameNotInProgress); } + if game.player2.is_none() { + return Err(ContractError::GameNotInProgress); + } game.player1.require_auth();A regression test that asserts
payout_tournament_optimizedcannot be invoked aftercancel_gamewould lock this down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/lib.rs` around lines 196 - 204, The bug is that cancel_game sets game.state = GameState::Completed which lets payout_tournament_optimized (which only checks for GameState::Completed) be called on canceled games; change cancel_game to set game.state = GameState::Settled (use the new Settled terminal state) instead of Completed so canceled games are terminal and cannot be paid out, and add a regression test invoking cancel_game then asserting payout_tournament_optimized returns an error; touch the cancel_game function and reference GameState::Settled/GameState::Completed and payout_tournament_optimized in your change.
🧹 Nitpick comments (3)
Cargo.toml (1)
2-6: Confirm workspace narrowing is intentional and protected from drift.Line 2-Line 6 hard-codes workspace members. This can silently exclude newly added contract crates from
cargo test --workspaceunless the list is manually updated. Consider adding a lightweight CI check to keep[workspace].membersin sync withcontracts/*/Cargo.toml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 2 - 6, The [workspace].members list in Cargo.toml is hard-coded and can drift from actual crates under contracts/*, so add a CI check that enumerates contracts/*/Cargo.toml entries and fails if they don't match the members list; implement a small script (bash/python) that reads Cargo.toml's workspace.members, globs contracts/*/Cargo.toml to derive expected members, compares the two sets, and exits nonzero on mismatch (hook it into the existing CI workflow so PRs fail until synced), referencing the Cargo.toml [workspace].members key and the contracts/*/Cargo.toml pattern.contracts/game_contract/src/lib.rs (2)
2313-2352: Consider extending the double-call regression test to other settlement paths.
test_payout_cannot_be_called_twiceonly exercisespayout. The PR also transitionsclaim_win,forfeit,claim_timeout_win,payout_tournament, andpayout_tournament_optimizedtoSettled, but their replay rejection currently returns the genericGameNotInProgress(sincestate != InProgress/Completedis what they actually check). A small set of additional cases — e.g., callingforfeittwice, orpayout_tournament_optimizedtwice — would prove the terminal-state invariant holds across every settlement entrypoint and catch any future regression where someone forgets thestate = Settledwrite.Want me to draft the additional regression cases for the other settlement paths?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/lib.rs` around lines 2313 - 2352, The test only covers double-call rejection for payout; add analogous regression tests for each settlement entrypoint (claim_win, forfeit, claim_timeout_win, payout_tournament, payout_tournament_optimized) to ensure calling them twice returns AlreadySettled and the contract persists state = Settled; for each new test reuse the existing pattern (initialize contract, create/join game, set game state to Completed and winner if needed inside env.as_contract, call the settlement method once via the client, then call the corresponding try_* variant and assert Err(Ok(ContractError::AlreadySettled))) so any missing state write in functions like claim_win, forfeit, claim_timeout_win, payout_tournament, or payout_tournament_optimized is caught.
1294-1309: RedundantCompletedwrite beforeSettled.Line 1299 sets
game.state = GameState::Completedand line 1302 immediately overwrites it withGameState::Settled.process_payoutdoes not readgame.state, so the intermediate assignment is a dead store. Drop it for clarity and to keep the lifecycle linear (InProgress → Settled), matchingclaim_win/forfeit/payout.♻️ Proposed simplification
Some(ref winner_addr) => { if *winner_addr != game.player1 && Some(winner_addr.clone()) != game.player2 { return Err(ContractError::NotPlayer); } - game.state = GameState::Completed; game.winner = Some(winner_addr.clone()); Self::process_payout(&env, &game, winner_addr)?; game.state = GameState::Settled; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/lib.rs` around lines 1294 - 1309, Remove the redundant intermediate assignment to GameState::Completed in the winner branch: in the match handling where Some(ref winner_addr) is processed (checking winner_addr against game.player1 and game.player2 and calling Self::process_payout), drop the line setting game.state = GameState::Completed before calling Self::process_payout and directly set game.state = GameState::Settled after process_payout; this keeps the state transition linear (InProgress → Settled) and avoids the dead store since process_payout does not read game.state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/game_contract/src/test.rs`:
- Around line 252-254: The test currently only checks res.is_err() for
client.try_set_max_stake called by &attacker; replace this with asserting the
exact authorization failure by capturing the error (e.g., let err =
res.unwrap_err();) and then asserting it equals the unauthorized error variant
used by the contract (for example assert_eq!(err, ContractError::Unauthorized)
or match on Err(ContractError::Unauthorized) depending on the error type),
referencing client.try_set_max_stake, attacker and the contract's unauthorized
error variant so the test specifically verifies the permission check.
---
Outside diff comments:
In `@contracts/game_contract/src/lib.rs`:
- Around line 196-204: The bug is that cancel_game sets game.state =
GameState::Completed which lets payout_tournament_optimized (which only checks
for GameState::Completed) be called on canceled games; change cancel_game to set
game.state = GameState::Settled (use the new Settled terminal state) instead of
Completed so canceled games are terminal and cannot be paid out, and add a
regression test invoking cancel_game then asserting payout_tournament_optimized
returns an error; touch the cancel_game function and reference
GameState::Settled/GameState::Completed and payout_tournament_optimized in your
change.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 2-6: The [workspace].members list in Cargo.toml is hard-coded and
can drift from actual crates under contracts/*, so add a CI check that
enumerates contracts/*/Cargo.toml entries and fails if they don't match the
members list; implement a small script (bash/python) that reads Cargo.toml's
workspace.members, globs contracts/*/Cargo.toml to derive expected members,
compares the two sets, and exits nonzero on mismatch (hook it into the existing
CI workflow so PRs fail until synced), referencing the Cargo.toml
[workspace].members key and the contracts/*/Cargo.toml pattern.
In `@contracts/game_contract/src/lib.rs`:
- Around line 2313-2352: The test only covers double-call rejection for payout;
add analogous regression tests for each settlement entrypoint (claim_win,
forfeit, claim_timeout_win, payout_tournament, payout_tournament_optimized) to
ensure calling them twice returns AlreadySettled and the contract persists state
= Settled; for each new test reuse the existing pattern (initialize contract,
create/join game, set game state to Completed and winner if needed inside
env.as_contract, call the settlement method once via the client, then call the
corresponding try_* variant and assert Err(Ok(ContractError::AlreadySettled)))
so any missing state write in functions like claim_win, forfeit,
claim_timeout_win, payout_tournament, or payout_tournament_optimized is caught.
- Around line 1294-1309: Remove the redundant intermediate assignment to
GameState::Completed in the winner branch: in the match handling where Some(ref
winner_addr) is processed (checking winner_addr against game.player1 and
game.player2 and calling Self::process_payout), drop the line setting game.state
= GameState::Completed before calling Self::process_payout and directly set
game.state = GameState::Settled after process_payout; this keeps the state
transition linear (InProgress → Settled) and avoids the dead store since
process_payout does not read game.state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 871394f6-697f-4e7c-bcf0-0803bed2a56e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
Cargo.tomlcontracts/game_contract/src/lib.rscontracts/game_contract/src/test.rscontracts/game_contract/test_snapshots/test/test_payout_tournament.1.jsoncontracts/game_contract/test_snapshots/test/test_payout_tournament_dust.1.jsoncontracts/game_contract/test_snapshots/test/test_payout_with_fee.1.jsoncontracts/game_contract/test_snapshots/test/test_set_max_stake.1.jsoncontracts/game_contract/test_snapshots/test/test_set_max_stake_rejects_non_admin.1.jsoncontracts/game_contract/test_snapshots/tests/test_claim_puzzle_reward_invalid_amount_rejected.1.jsoncontracts/game_contract/test_snapshots/tests/test_claim_puzzle_reward_invalid_sig.1.jsoncontracts/game_contract/test_snapshots/tests/test_claim_puzzle_reward_replay_rejected.1.jsoncontracts/game_contract/test_snapshots/tests/test_claim_puzzle_reward_valid_sig.1.jsoncontracts/game_contract/test_snapshots/tests/test_claim_timeout_win_not_reached.1.jsoncontracts/game_contract/test_snapshots/tests/test_claim_timeout_win_rejects_current_turn_player.1.jsoncontracts/game_contract/test_snapshots/tests/test_claim_timeout_win_success.1.jsoncontracts/game_contract/test_snapshots/tests/test_fee_redirection_2_percent.1.jsoncontracts/game_contract/test_snapshots/tests/test_fee_redirection_2_percent_large.1.jsoncontracts/game_contract/test_snapshots/tests/test_file_dispute_rejects_settled_games.1.jsoncontracts/game_contract/test_snapshots/tests/test_file_dispute_success.1.jsoncontracts/game_contract/test_snapshots/tests/test_get_timeout_remaining.1.jsoncontracts/game_contract/test_snapshots/tests/test_payout_cannot_be_called_twice.1.jsoncontracts/game_contract/test_snapshots/tests/test_payout_tournament_optimized_splits_correctly.1.jsoncontracts/game_contract/test_snapshots/tests/test_reject_dispute_refund_fee.1.jsoncontracts/game_contract/test_snapshots/tests/test_resolve_dispute_rejects_already_settled_games.1.jsoncontracts/game_contract/test_snapshots/tests/test_resolve_dispute_winner_takes_all.1.jsoncontracts/game_contract/test_snapshots/tests/test_submit_move_rejects_out_of_turn_and_empty_moves.1.jsoncontracts/game_contract/test_snapshots/tests/test_submit_move_sequence_updates_turn_and_history.1.jsoncontracts/game_contract/test_snapshots/tests/test_usdc_staking_workflow.1.json
Comment on lines
+252
to
+254
| client.initialize_puzzle_rewards(&admin, &admin_key, &0i128, &0u32, &treasury_addr); | ||
| let res = client.try_set_max_stake(&attacker, &1_000i128); | ||
| assert!(res.is_err()); |
Contributor
There was a problem hiding this comment.
Make the non-admin test assert the specific authorization failure.
Line 254 currently checks only is_err(), so the test can pass on unrelated failures. Please assert the concrete unauthorized error code/variant to ensure this test truly guards the permission check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/game_contract/src/test.rs` around lines 252 - 254, The test
currently only checks res.is_err() for client.try_set_max_stake called by
&attacker; replace this with asserting the exact authorization failure by
capturing the error (e.g., let err = res.unwrap_err();) and then asserting it
equals the unauthorized error variant used by the contract (for example
assert_eq!(err, ContractError::Unauthorized) or match on
Err(ContractError::Unauthorized) depending on the error type), referencing
client.try_set_max_stake, attacker and the contract's unauthorized error variant
so the test specifically verifies the permission check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR delivers critical on-chain logic improvements for game settlement and staking in
contracts/game_contract/src, with a focus on correctness, replay safety, authorization, and resource-conscious execution patterns.#522: Context
This task involves critical on-chain logic for game settlement and staking on Stellar.
Implemented
Settled) to prevent replay payout paths.Acceptance
Closes #522
#523: Context
This task involves critical on-chain logic for game settlement and staking on Stellar.
Implemented
set_max_stakenow admin-authenticated).Acceptance
Closes #523
#524: Context
This task involves critical on-chain logic for game settlement and staking on Stellar.
Implemented
InvalidAmounterror handling for unsafe/invalid reward values.Acceptance
Closes #524
#525: Context
This task involves critical on-chain logic for game settlement and staking on Stellar.
Implemented
Acceptance
Closes #525
Test Plan
cargo test -p game_contract30 passed, 0 failedSummary by CodeRabbit
New Features
Bug Fixes
Security