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

Incorrect `prizeAmount` in event emitted when `declareWinner` function called

azriel20005

Root + Impact

Description

  • When the declareWinner function is called, the event that should be emitted should correspond to the following:

/**
* @dev Emitted when the game ends and a winner is declared.
* @param winner The address of the declared winner.
* @param prizeAmount The total prize amount won.
* @param timestamp The block timestamp when the winner was declared.
* @param round The game round that just ended.
*/
event GameEnded(
address indexed winner,
uint256 prizeAmount,
uint256 timestamp,
uint256 round
);

  • However due to incorrect usage of pot value, the prizeAmount emitted will always be 0 since and will not correctly tie to the total prize amount won

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 = true;
​
pendingWinnings[currentKing] = pendingWinnings[currentKing] + pot;
@> pot = 0; // Reset pot after assigning to winner's pending winnings
​
@> emit GameEnded(currentKing, pot, block.timestamp, gameRound); // Pot value just reset, will be 0
}

Risk

Likelihood: High

  • This will always occur when the declareWinner function is called, which is a core functionality of the contract.

Impact: Medium

  • The incorrect values emitted may cause confusion for users or front-end applications that rely on event tracking. This may break some of the functionality of applications that require accurate event values being emitted.

Proof of Concept

The following test code fails due to the logs being different

function testDeclareWinner() public {
vm.startPrank(player1);
vm.expectRevert("Game: No one has claimed the throne yet.");
game.declareWinner();
​
game.claimThrone{value: 1 ether}();
vm.expectRevert("Game: Grace period has not expired yet.");
game.declareWinner();
​
vm.warp(block.timestamp + GRACE_PERIOD + 1 days);
vm.expectEmit(true, true, false, true);
emit GameEnded(player1, game.pot(), block.timestamp, 1);
game.declareWinner();
vm.stopPrank();
​
uint256 winnings = game.pendingWinnings(player1);
assertEq(winnings, 0.95 ether);
assertEq(game.gameEnded(), true);
}

Recommended Mitigation

The value of the current prize amount should be saved before resetting the pot to 0, and this value should then be used instead.

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 = true;
+ uint256 prizeAmount = pot; // Store pot value before resetting
pendingWinnings[currentKing] = pendingWinnings[currentKing] + prizeAmount;
pot = 0; // Reset pot after assigning to winner's pending winnings
- emit GameEnded(currentKing, pot, block.timestamp, gameRound);
+ emit GameEnded(currentKing, prizeAmount, block.timestamp, gameRound);
}
Updates

Appeal created

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

Game::declareWinner emits GameEnded event with pot = 0 always

Support

FAQs

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