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

Potential Re-entrancy and Wrong Event Emission in Game.sol::declareWinner()

legacy

Root + Impact

The event emission in the Game.sol::declareWinner function logs incorrectly as the lines of code do not follow CEI pattern.

Description

  • The declare::Winner function is expected to log the GameEnded event with the following data:

    currentKing, pot, block.timestamp, gameRound.

  • The potential re-entrancy issue is not clear in this case as there is no external interaction taking place here. However, this seemingly harmless pattern can be exploited in case of a code upgrade or integration where this function may be called by another function. It is thus recommended to follow the CEI pattern.

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

Risk

Likelihood:

  • The issue is bound to occur as the event would be emmitted whenever the winner is declared by a caller.

Impact:

  • This bug can lead to misleading logic when events are being relied upon due to the misleading emitted contract state.

  • Furthermore, the issue would be a re-entrancy attack vector if the contract should interact externally with others, or contract upgrades with other new functions.

Proof of Concept

The function is currently not vulnerable to re-entrancy as no external interaction currently takes place and no other cross-functional activity occurs based on the scope.
Furthermore, the issue with wrong event emission is evident since pot is alreadu set to 0, before event emission.

Recommended Mitigation

The recommended mitigation is to follow the CEI pattern for best practice and use a variable to store the pot as shown below:

function declareWinner() external {
require(currentKing != address(0), "No one has claimed the throne yet.");
require(block.timestamp > lastClaimTime + gracePeriod, "Grace not expired.");
require(!gameEnded, "Game already ended.");
​
gameEnded = true;
​
uint256 amount = pot;
pot = 0; // CEI pattern
​
(bool success, ) = currentKing.call{value: amount}("");
require(success, "Transfer failed");
​
emit GameEnded(currentKing, amount, block.timestamp, gameRound); // emit after all logic
}
​
Updates

Appeal created

inallhonesty Lead Judge 4 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.

Give us feedback!