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

State Update After External Call in withdrawWinnings

fedelmar

Root + Impact

Description

  • The withdrawWinnings function allows users to withdraw their pending winnings. The expected behavior is that the contract first updates the user's state to reflect the withdrawal, and then sends the Ether to the user, following the Checks-Effects-Interactions pattern.

  • However, in the current implementation, the contract performs the external call (via call{value: amount}) before updating the internal state (pendingWinnings[msg.sender] = 0). This violates a fundamental Solidity security pattern and, while currently mitigated by the nonReentrant modifier, it increases the risk of reentrancy if that protection is ever removed, bypassed, or incorrectly reused elsewhere.

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}("");
@> require(success, "Game: Failed to withdraw winnings.");
​
@> pendingWinnings[msg.sender] = 0;
​
emit WinningsWithdrawn(msg.sender, amount);
}

Risk

Likelihood:

  • The issue will manifest any time this pattern is copied without a reentrancy guard.

  • Developers might mistakenly remove or forget the nonReentrant modifier in future refactors or similar functions.

Impact:

  • In the absence of nonReentrant, this could be exploited for fund theft via reentrancy.

  • It can lead to unsafe assumptions during auditing or review, affecting the system's long-term security posture.

Proof of Concept

A malicious contract could exploit the unsafe order of operations by triggering a reentrant call during the Ether transfer. If the nonReentrant modifier were absent or misapplied, the attacker could repeatedly call withdrawWinnings() before their balance is cleared, draining funds.

// Malicious contract sketch
contract Attacker {
Game game;
​
constructor(address _game) {
game = Game(_game);
}
​
function attack() external payable {
game.withdrawWinnings();
}
​
receive() external payable {
// Reentrancy attempt
if (address(game).balance >= 1 ether) {
game.withdrawWinnings(); // would work if nonReentrant wasn't present
}
}
}
​

Recommended Mitigation

Reorder the logic to update the user's state before making the external call. This follows the Checks-Effects-Interactions pattern and ensures safety even if the reentrancy guard is modified or removed in the future.

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}("");
- require(success, "Game: Failed to withdraw winnings.");
-
- pendingWinnings[msg.sender] = 0;
​
+ pendingWinnings[msg.sender] = 0;
+ (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 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.