Last Man Standing
First Flight #45
Beginner FriendlyFoundrySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: medium
Likelihood: high
Invalid

Reentrancy in `Game.withdrawWinnings()`

sidrahh12

Reentrancy in Game.withdrawWinnings()

Description

  • Normally, withdrawWinnings() sends a user their accumulated winnings and resets their balance to 0 in pendingWinnings.

  • The issue is that the function updates pendingWinnings[msg.sender] after making an external call (call{value:}), which could allow a malicious contract to reenter and exploit the delayed state update.

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender];
require(amount > 0, "Game: No winnings to withdraw.");
​
(bool success, ) = payable(msg.sender).call{value: amount}("");
// @> Vulnerable external call before state update
require(success, "Game: Failed to withdraw winnings.");
​
pendingWinnings[msg.sender] = 0;
// @> State updated too late (after external interaction)
​
emit WinningsWithdrawn(msg.sender, amount);
}

Risk

Likelihood:

  • Medium: Reentrancy is possible if msg.sender is a malicious contract with a receive() or fallback() function that calls back into Game before pendingWinnings in `Game.sol` is cleared.

  • Cross-function reentrancy: The same pendingWinnings state variable is used in declareWinner(), meaning an attacker could manipulate winnings across multiple functions.

Impact:

  • Funds theft: An attacker could drain the contract by recursively calling withdrawWinnings() before pendingWinnings is set to 0.

  • State corruption: If pendingWinnings is used elsewhere (e.g., declareWinner()), inconsistent state could lead to incorrect payouts or logic errors.

Proof of Concept

Contract Attacker is a malicious contract, which reenters before `pendingWinnings` is cleared.

contract Attacker {
Game game;
uint256 reentrancyCount;
​
constructor(address _game) {
game = Game(_game);
}
​
receive() external payable {
if (reentrancyCount < 3) {
reentrancyCount++;
game.withdrawWinnings(); // Reenters before pendingWinnings is cleared
}
}
​
function attack() external {
game.withdrawWinnings(); // Starts the reentrancy loop
}
}

Recommended Mitigation

  1. Follow Checks-Effects-Interactions: Update state before external calls.

  2. Use OpenZeppelin’s ReentrancyGuard: Even though nonReentrant is present, ensure it’s applied consistently across all state-modifying functions.

  3. Consider Pull-over-Push: Let users withdraw funds themselves (e.g., via a claimWinnings() pattern) instead of the contract pushing ETH.

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender];
require(amount > 0, "Game: No winnings to withdraw.");
+ pendingWinnings[msg.sender] = 0; // @> State updated first
+ emit WinningsWithdrawn(msg.sender, amount);
​
+ (bool success, ) = payable(msg.sender).call{value: amount}(""); // @> Call after state update
+ require(success, "Game: Failed to withdraw winnings.");
}
Updates

Appeal created

inallhonesty Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Game::withdrawWinnings reentrancy

We only audit the current code in scope. We cannot make speculation with respect to how this codebase will evolve in the future. For now there is a nonReentrant modifier which mitigates any reentrancy. CEI is a good practice, but it's not mandatory. Informational

Support

FAQs

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