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

[I-4] `Game::withdrawWinnings` should follow CEI

thecil

Root + Impact

[I-4] Game::withdrawWinnings should follow CEI

Description

The Game::withdrawWinnings function does have the nonReentrant modifier to avoid reentrancy attacks, which provide security for the function execution.

Still, it's best to keep code clean and follow CEI (Checks, Effects, Interactions).

Risk

Likelihood: Low

Impact: None

Proof of Concept

The Game::withdrawWinnings function is currently written as follows:

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

Recommended Mitigation

Move the pendingWinnings[msg.sender] = 0; before transfering the winnings. This ensures that the pendingWinnings is updated before any further operations are performed.

function withdrawWinnings() external nonReentrant {
// Checks
uint256 amount = pendingWinnings[msg.sender];
require(amount > 0, "Game: No winnings to withdraw.");
// Effects
+ pendingWinnings[msg.sender] = 0;
// Interactions
(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.