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

CEI (Checks-Effects-Interactions) Pattern Not Strictly Followed in `withdrawWinnings` function

azriel20005

Root + Impact

Description

  • withdrawWinnings function should follow CEI pattern, however it transfers ether before updating the value of pendingWinnings to 0. This exposes the function to re-entrancy risks and is not a secure withdraw pattern as opposed to what it states in the comments

/**
* @dev Allows the declared winner to withdraw their prize.
* Uses a secure withdraw pattern with a manual reentrancy guard.
*/
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}(""); // interactions called first to transfer ether to msg.sender
require(success, "Game: Failed to withdraw winnings.");
​
@> pendingWinnings[msg.sender] = 0; // then the pendingWinnings is zeroed out
​
emit WinningsWithdrawn(msg.sender, amount);
}

Risk

Likelihood: Low

  • The function is protected by a nonReentrant modifier as well, so the risk of re-entrancy attacks are low. But the function would solely rely on the modifier for protection.

Impact: High

  • If the manual re-entrancy guard could be bypassed, there is a potential for the entire contract to be drained.

Proof of Concept

NA

Recommended Mitigation

This could be easily solved by bringing the update of pending winnings forward before the transfer call.

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.