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

State Updated After External Call (Reentrancy Risk)

gurmeetkalyan

Root + Impact

Description

  • Normal behavior: A withdrawal function should update internal state before interacting with an external address to avoid reentrancy.


  • Issue: In both withdrawWinnings() and withdrawPlatformFees(), the contract calls call{value: amount} before resetting the corresponding balances:

    (bool success, ) = payable(msg.sender).call{ value: amount }("");
    require(success, "Game: Failed to withdraw winnings.");
    pendingWinnings[msg.sender] = 0; // updated after call
    ​

    This violates the Checks-Effects-Interactions pattern

// Root cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

  • Although a custom nonReentrant modifier is used, if another function is added without the guard or if the guard is accidentally removed, reentrancy becomes possible.

  • The risk exists every time a user withdraws funds, so the likelihood is moderate.

Impact:

  • Inconsistent State or Theft: A malicious contract could reenter before the state is reset and attempt multiple withdrawals.


  • Future Extendibility: If new functions or external calls are added, forgetting the guard could reintroduce reentrancy.

Proof of Concept

// A malicious contract triggers reentrancy by calling withdrawWinnings() in its fallback.
// Because the state update happens after the external call, the malicious contract
// could reenter and withdraw multiple times if the reentrancy guard were omitted.
​

Recommended Mitigation

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;
+ uint256 amount = pendingWinnings[msg.sender];
+ require(amount > 0, "Game: No winnings to withdraw.");
+
+ // Update state before external call (checks-effects-interactions)
+ pendingWinnings[msg.sender] = 0;
+
+ (bool success, ) = payable(msg.sender).call{ value: amount }("");
+ require(success, "Game: Failed to withdraw winnings.");
}
​

.

Updates

Appeal created

inallhonesty Lead Judge 4 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.

Give us feedback!