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

Incorrect Pot Value in GameEnded Event

samuelsmith442

Description

The Last Man Standing game has a vulnerability in its declareWinner() function where the GameEnded event is emitted with an incorrect pot value. The event is emitted after the pot value has been reset to zero, which means that all event logs will show a pot value of 0 regardless of the actual prize amount won.

Expected Behavior

The GameEnded event should accurately reflect the prize amount won by the king when the game ends. This information is important for off-chain applications, analytics, and historical record-keeping.

Actual Behavior

The GameEnded event is emitted with a pot value of 0 because the event is emitted after the pot has been reset. This makes it impossible to determine the actual prize amount from the event logs.

Root Cause

The vulnerability exists in the declareWinner() function where the pot is reset before emitting 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);
}

The issue is that the pot variable is set to 0 before the GameEnded event is emitted, causing the event to always show a pot value of 0.

Risk Assessment

Impact

The impact is medium because:

  1. It doesn't affect the actual functionality of the contract (the winner still gets the correct amount)

  2. However, it breaks the event log's integrity, which is important for:

    • Off-chain applications that rely on events for data

    • Historical record-keeping and analytics

    • Transparency for users and stakeholders

Likelihood

The likelihood is high as this issue will occur every time a winner is declared. It's not an edge case but affects a core event in the contract.

Proof of Concept

The issue can be verified by examining the declareWinner() function in Game.sol. The pot variable is reset to 0 before the GameEnded event is emitted.

Recommended Mitigation

Store the pot value in a temporary variable before resetting it, and use this temporary variable in the event:

// Before: Vulnerable code
function declareWinner() external gameNotEnded {
// ... existing code ...
pendingWinnings[currentKing] = pendingWinnings[currentKing] + pot;
pot = 0; // Reset pot after assigning to winner's pending winnings
​
emit GameEnded(currentKing, pot, block.timestamp, gameRound);
}
​
// After: Fixed code
function declareWinner() external gameNotEnded {
// ... existing code ...
uint256 winningAmount = pot; // Store pot value before resetting
pendingWinnings[currentKing] = pendingWinnings[currentKing] + pot;
pot = 0; // Reset pot after assigning to winner's pending winnings
​
emit GameEnded(currentKing, winningAmount, block.timestamp, gameRound);
}

Explanation

The fix stores the pot value in a temporary variable winningAmount before resetting it, and then uses this variable in the GameEnded event. This ensures that the event accurately reflects the actual prize amount won by the king, maintaining the integrity of the event logs.

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.