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

Reentrancy Vulnerability in withdrawWinnings() Allows Repeated Withdrawals Before Balance Reset

na_ounnane

Root + Impact

Description

  • The withdrawWinnings() function is intended to allow users to securely withdraw their pending winnings. It checks that the user has a positive balance, sends them the funds, and then resets their pending winnings to zero.Explain the specific issue or problem in one or more sentences.

  • However, the contract first performs the external call to send ETH to the user before updating their internal balance. This exposes the contract to a reentrancy attack, especially if nonReentrant is removed or bypassed.

@> (bool success, ) = payable(msg.sender).call{value: amount}("");
@> require(success, "Game: Failed to withdraw winnings.");
@> pendingWinnings[msg.sender] = 0;

Risk

Likelihood:

  • The issue occurs every time a player with a malicious fallback function calls withdrawWinnings().

  • Although nonReentrant helps, good practice dictates using Checks-Effects-Interactions to avoid dependency on modifiers alone.

Impact:

  • An attacker could recursively withdraw winnings before their balance is reset.

  • May lead to draining the contract's ETH balance, affecting all honest participants.

Proof of Concept

contract Attacker {
Game game;
​
constructor(address _game) {
game = Game(_game);
}
​
fallback() external payable {
if (address(game).balance >= 1 ether) {
game.withdrawWinnings(); // reenter before pendingWinnings is set to 0
}
}
​
function attack() external payable {
// assume pendingWinnings[msg.sender] > 0
game.withdrawWinnings();
}
}
​

Recommended Mitigation

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

Appeal created

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