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

Funds Sent Before State Update (Reentrancy Risk if Modifier Absent)

dumbbutt0

Root + Impact

Description

  • The withdrawWinnings() function allows a player to withdraw their earnings if they have any pending winnings. The current behavior sends ETH to the player before setting their pendingWinnings[msg.sender] balance to zero.

  • This is only secure because of the nonReentrant modifier. However, relying solely on a modifier creates a fragile trust assumption. The code would be vulnerable to reentrancy attacks if the modifier is removed or bypassed, and is non-idiomatic compared to secure withdrawal patterns.

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

Risk

Likelihood:

  • This risk materializes whenever someone forgets to apply the nonReentrant modifier, especially during future upgrades or copy-pasting this logic into new functions.

  • Contracts using this pattern without a modifier (or with one that’s bypassed via inheritance or proxy logic) will be vulnerable.

Impact:

  • An attacker can recursively re-enter the function before the balance is zeroed, draining the contract.

  • Even if not exploited, this pattern goes against Solidity best practices and is flagged in most static analysis tools, causing audit friction and lower trust in code quality.

Proof of Concept

Imagine a contract without the nonReentrant modifier or with an accidental omission:

contract MaliciousWinner {
Game public target;
​
constructor(address _target) {
target = Game(_target);
}
​
receive() external payable {
if (address(target).balance >= 1 ether) {
target.withdrawWinnings(); // Recursive call
}
}
​
function attack() external {
target.withdrawWinnings(); // Starts the reentrancy chain
}
}

Because pendingWinnings[msg.sender] is only zeroed after the external call, the attacker repeatedly withdraws funds.

Recommended Mitigation

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.");
}
Updates

Appeal created

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