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

withdrawWinnings() Does Not Follow CEI Pattern

obeddev

Root + Impact

Description

  • The withdrawWinnings() function allows winners to withdraw their ETH safely using a secure withdrawal pattern with reentrancy protection.

  • The function makes an external call (call{value: amount}) to the user before updating the state variable pendingWinnings[msg.sender] = 0. This violates the Checks-Effects-Interactions (CEI) pattern, which is a well-known best practice for secure smart contract development.


    The contract uses a nonReentrant modifier, which protects against reentrancy attacks, but relying solely on this guard instead of also following CEI introduces unnecessary risk and reduces code resilience.

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 occur on every withdrawal, but not in a way that causes failure or loss due to the nonReentrant modifier.

Impact:

  • Not following CEI introduces a known anti-pattern, making the code harder to audit and less robust against changes or integrations with external contracts.

Proof of Concept

​

Recommended Mitigation

Follow CEI: update state before making external calls.

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

Appeal created

inallhonesty Lead Judge about 2 months 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.