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

`Game::declareWinner` emit a wrong data, make it send a wrong data to frontEnd or blockchain explorer

vegazeronos

Game::GameEndedemit a wrong data, make it send a wrong data to frontEnd or blockchain explorer

Description

  • The pot will be reset after Game::declareWinner is called. This is good but it will give a wrong data when it emit the event.

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);
}

  • Not just that, pot = 0; is called again on Game::resetGame . It is redundant and bad practices

    function resetGame() external onlyOwner gameEndedOnly {
    currentKing = address(0);
    lastClaimTime = block.timestamp;
    @> pot = 0;

Risk

Likelihood:

  • Very likely to give a wrong data when emit when everytime Game::declareWinner called.


Impact:

  • Front end will get wrong data if they use it

Proof of Concept

  1. Player use Game::declareWinner to get the winner

  2. The contract send an Emit to the blockchain with the pot = 0

  3. Frontend will get the wrong value because its only 0


Recommended Mitigation

Please consider between this two suggestion:

  1. Please remove the first pot = 0; on Game::declareWinner because when the game reset, pot = 0 will be called on Game::resetGame which is mandatory to continue the game

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);
}
​

  1. If the protocol insist to use it on Game::declareWinner , consider to use a ghost variable to cache the data before it goes to pot = 0; and use the variable to emit the data.

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 potChache = pot;
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, potChache, block.timestamp, gameRound);
}
Updates

Appeal created

inallhonesty Lead Judge about 1 month 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.