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

`withdrawWinnings` Function Violates Check-Effects-Interactions Pattern

farrellhauzan

Description

  • Functions that perform external calls should follow the Check-Effects-Interactions (CEI) pattern to prevent reentrancy vulnerabilities and improve code clarity.

  • The withdrawWinnings function performs external calls before updating state variables, violating the CEI pattern despite having reentrancy protection.

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender]; // Check
require(amount > 0, "Game: No winnings to withdraw."); // Check
// @> External interaction happens before state update (violates CEI)
(bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Game: Failed to withdraw winnings.");
// @> Effects come after interaction
pendingWinnings[msg.sender] = 0;
}

Risk

Likelihood:

  • Pattern violation exists in current implementation

  • While nonReentrant modifier prevents exploitation, pattern should still be followed

Impact:

  • Code violates security best practices

  • Reduced defense-in-depth approach

  • Less clear code structure for auditing

  • Potential confusion about intended execution order

Proof of Concept

// Current order: Check -> Interaction -> Effects (violates CEI)
// Proper order should be: Check -> Effects -> Interactions

Recommended Mitigation

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender]; // Check
require(amount > 0, "Game: No winnings to withdraw."); // Check
+ pendingWinnings[msg.sender] = 0; // Effects - update state first
(bool success, ) = payable(msg.sender).call{value: amount}(""); // Interactions
require(success, "Game: Failed to withdraw winnings.");
- pendingWinnings[msg.sender] = 0; // Remove from here
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.