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

`withdrawWinnings()` function does not follow the CEI pattern

vladich0x

Root + Impact

withdrawWinnings() function does not follow the CEI pattern

Description

The Game::withdrawWinnings() function, which implements the mechanism allowing regular players to withdraw their prize, does not follow the Checks-Effects-Interactions (CEI) pattern. The reason is that the contract’s state is updated only after the funds have been transferred to the player’s account. This behavior is incompatible with CEI and does not meet the requirements defined by the pattern. This makes the logic vulnerable and renders the function fragile, with the only reason it is not susceptible to reentrancy attacks being the presence of the nonReentrant modifier.

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: Low

Impact:

Proof of Concept

N/A

Tools Used

Manual review

Recommended Mitigation

Update function withdrawWinnings() as follows:

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 16 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.