Last Man Standing
First Flight #45
Beginner FriendlyFoundrySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Grace period can be circumvented until `declareWinner` function is called

soupy

Summary

The claimThrone function remains accessible to players even after the grace period has elapsed, as long as no one calls the declareWinner function. This allows users to continue making throne claims beyond the intended game duration. The game is only effectively concluded, preventing further claims when declareWinner is explicitly invoked in which the global gameEnded flag is then set to true. Until then, the contract does not enforce the grace period, allowing unintended gameplay continuation.

Description

The claimThrone function uses the gameNotEnded modifier to restrict access based on the game's state. However, this modifier checks only the gameEnded boolean flag to determine whether the game has concluded, rather than verifying whether the grace period has elapsed. The gameEnded flag is only set to true when the declareWinner function is called. As a result, if no one invokes declareWinner, the game can continue indefinitely even after the grace period has expired, allowing players to keep claiming the throne beyond the intended end of the game.

// gameNotEnded modifier is used to restrict access
<@@> function claimThrone() external payable gameNotEnded nonReentrant {...}
​
// this modifier depends on `gameEnded` flag for game play duration control
modifier gameNotEnded() {
<@@> require(!gameEnded, "Game: Game has already ended. Reset to play again.");
_;
}
​
function declareWinner() external gameNotEnded {
require(currentKing != address(0), "Game: No one has claimed the throne yet.");
require(
block.timestamp > lastClaimTime + gracePeriod,
"Game: Grace period has not expired yet."
);
​
// gameEnded flag is only updated under `declareWinner` function
<@@> gameEnded = true;
​
pendingWinnings[currentKing] = pendingWinnings[currentKing] + pot;
pot = 0;
​
emit GameEnded(currentKing, pot, block.timestamp, gameRound);
}

Risk

Likelihood:

  • Depending on player behavior and familiarities of the contract’s game rules, the exploitation can be moderate to high. Since the grace period is not automatically enforced and requires a manual call to declareWinner, a player or group of players with knowledge of this design flaw could intentionally delay this call to continue participating in the game beyond the intended time limit to have bigger pot value cumulated towards the end. Furthermore, as there is no built-in mechanism to trigger declareWinner after the grace period, the game remains vulnerable to extended, unintended gameplay unless an external actor intervenes.

Impact:

  • This vulnerability allows players to continue claiming the throne even after the grace period has ended. As a result, the game duration can be extended indefinitely, which may lead to unfair gameplay advantages, disruption of expected game flow, and potential economic implications depending on the value associated with final winning pot. Additionally, it introduces uncertainty for participants expecting a definitive end to the game based on time constraints, which could erode trust in the contract's integrity.

Proof of Concept

In test/Game.t.sol, add the following test:

function test_audit_gracePeriodIssue() public {
vm.prank(player1);
game.claimThrone{value: INITIAL_CLAIM_FEE}();
​
// game should end at this stage
vm.warp(game.lastClaimTime() + GRACE_PERIOD + 1 minutes);
bool gameEnded = game.gameEnded();
// this assertion test shows that the game was not ended even though it should have ended
assertFalse(gameEnded);
​
// another player continue to claim throne
vm.prank(player2);
uint256 player2claimAmount = INITIAL_CLAIM_FEE * 110 / 100;
game.claimThrone{value: player2claimAmount}();
// this assertion test shows other player can still continue to claim throne and becomes the current king without issue
assertEq(game.currentKing(), player2);
​
vm.warp(game.lastClaimTime() + GRACE_PERIOD + 1 minutes);
// game should end at this stage, then someone else calls declareWinner
game.declareWinner();
// this assertion shows that the game state is changed to true and game has now ended
assertTrue(game.gameEnded());
​
// another player tries to claim throne after someone declareWinner
// revert is expected as the gameEnd flag is now set as True
vm.prank(player3);
vm.expectRevert("Game: Game has already ended. Reset to play again.");
game.claimThrone{value: player2claimAmount * 110 / 100}();
​
}

In terminal, run forge test --match-test test_audit_gracePeriodIssue -vvv will generate the following results:

$ forge test --match-test test_audit_gracePeriodIssue -vvv
...
​
Ran 1 test for test/Game.t.sol:GameTest
[PASS] test_audit_gracePeriodIssue() (gas: 244130)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 847.79µs (127.67µs CPU time)
​
Ran 1 test suite in 118.97ms (847.79µs CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

All assertion tests and expectRevert pass indicating that player can still claimThrone even though grace period elapsed and only failed to claim when someone called the declareWinner function.

Recommended Mitigation

To implement time constraints check in a new function isGameEnded, update/remove the gameEnd flag accordingly, so the isGameEnded function which replaces the gameEnd flag will be consistent across all calls without depending on either modifier or the declareWinner function:

​
// Game Core State
address public currentKing; // The address of the current "King"
...
- bool public gameEnded;
​
constructor(
uint256 _initialClaimFee,
uint256 _gracePeriod,
uint256 _feeIncreasePercentage,
uint256 _platformFeePercentage
) Ownable(msg.sender) { // Set deployer as owner
require(_initialClaimFee > 0, "Game: Initial claim fee must be greater than zero.");
...
​
// Initialize game state for the first round
claimFee = initialClaimFee;
gracePeriod = initialGracePeriod;
lastClaimTime = block.timestamp; // Game starts immediately upon deployment
gameRound = 1;
- gameEnded = false;
// currentKing starts as address(0) until first claim
}
​
+ function isGameEnded() public view returns (bool) {
+ return block.timestamp >= lastClaimTime + gracePeriod;
+ }
​
modifier gameNotEnded() {
- require(!gameEnded, "Game: Game has already ended. Reset to play again.");
+ require(!isGameEnded(), "Game: Game has already ended. Reset to play again.");
_;
}
​
modifier gameEndedOnly() {
- require(gameEnded, "Game: Game has not ended yet.");
+ require(isGameEnded(), "Game: Game has not ended yet.");
_;
}
​
...
​
- function declareWinner() external gameNotEnded {
+ function declareWinner() external gameEndedOnly {
require(currentKing != address(0), "Game: No one has claimed the throne yet.");
- require(
- block.timestamp > lastClaimTime + gracePeriod,
- "Game: Grace period has not expired yet."
- );
​
- gameEnded = true;
​
​
pendingWinnings[currentKing] = pendingWinnings[currentKing] + pot;
pot = 0; // Reset pot after assigning to winner's pending winnings
​
emit GameEnded(currentKing, pot, block.timestamp, gameRound);
}
​
function resetGame() external onlyOwner gameEndedOnly {
currentKing = address(0);
lastClaimTime = block.timestamp;
pot = 0;
claimFee = initialClaimFee;
gracePeriod = initialGracePeriod;
- gameEnded = false;
...
}
​
function getRemainingTime() public view returns (uint256) {
- if (gameEnded)
+ if (isGameEnded()) {
return 0; // Game has ended, no remaining time
}
​
- uint256 endTime = lastClaimTime + gracePeriod;
- if (block.timestamp >= endTime) {
- return 0; // Grace period has expired
- }
​
return endTime - block.timestamp;
}
Updates

Appeal created

inallhonesty Lead Judge about 2 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Game::claimThrone can still be called regardless of the grace period

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.