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

Error in declareWinner() (LOC 234)

prajwalk1412

Logical error in the declareWinner() function + Critical Impact

Description

  • The function should be emitting how much the winner (the last king) gets at the GameEnded condition

  • The issue is that the amount shown will always be 0 since, the pot variable was reset to 0 by the declareWinner function

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, however this emits the wrong pot amount later on
​
@> emit GameEnded(currentKing, pot, block.timestamp, gameRound); // pot is now 0!
}

Risk

Likelihood:

  • This will occur everytime declareWinner() is called.

Impact:

  • Users and observers are misinformed of the winnings (the pot)

  • Analytics and reward tracking are broken

Proof of Concept

​
function testDeclareWinnerEvent() public {
// Setup: Alice is king, pot is 10 ether
game.currentKing = alice;
game.pot = 10 ether;
​
// Expect event with correct prize
vm.expectEmit(true, false, false, true);
emit GameEnded(alice, 10 ether, block.timestamp, game.gameRound());
​
// Call declareWinner
game.declareWinner();
​
// But actual event emits prizeAmount = 0 due to bug
}

Explanation: Sample Scenario

  1. Alice is currentKing and pot is 10 ether.

  2. declareWinner() is called.

  3. GameEnded event emits prizeAmount = 0 instead of 10 ether.

Recommended Mitigation

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);
+ emit GameEnded(currentKing, pot, block.timestamp, gameRound);
+ pot = 0; // Reset pot after assigning to winner's pending winnings
}
​

Explanation : emitting the event first then resetting the variable fixes our error

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.