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

Analysis: Reentrancy Vulnerability in Game.withdrawWinnings()

mlvphani2582

Root + Impact

Description

Why This Is Vulnerable?

  1. External Call Before State Update: The function makes an external call to msg.sender before updating the pendingWinnings[msg.sender] state variable.

  2. Potential for Reentrancy: Even though the function has a nonReentrant modifier, a malicious contract receiving the ETH could:

    • Implement a receive() or fallback() function

    • Call back into the contract through other functions

    • Potentially manipulate state or cause unexpected behavior

  3. Reentrancy Guard Limitation: While the nonReentrant modifier prevents direct reentrancy into the same function, it doesn't prevent:

    • Cross-function reentrancy (calling other functions in the contract)

    • Complex attack vectors involving multiple transactions

    • State manipulation through other entry points

`withdrawWinnings()` function violates the CEI (Check-Effect-Interact) pattern and performs external calls before updating the internal state.
​
This can be exploited by a malicious contract using a fallback or receive function to re-enter and drain funds.
function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender]; // ✅ Check
require(amount > 0, "Game: No winnings to withdraw.");
​
(bool success, ) = payable(msg.sender).call{value: amount}(""); // ❌ Interact (EXTERNAL CALL)
require(success, "Game: Failed to withdraw winnings.");
​
pendingWinnings[msg.sender] = 0; // ❌ Effect (STATE UPDATE AFTER EXTERNAL CALL)
​
emit WinningsWithdrawn(msg.sender, amount);
}

Risk

Financial Risk: HIGH

  • Direct Loss: Potential theft of accumulated prize pools

  • Indirect Loss: Loss of user trust and platform reputation

  • Scale: Risk scales with the size of prize pools (could be substantial in popular games)

Operational Risk: MEDIUM

  • Game Disruption: Could break the game's economic model

  • Recovery Difficulty: May require contract redeployment and user migration

  • Regulatory Risk: Could expose platform to legal liability


Likelihood:

Overall Likelihood: MEDIUM-HIGH

Factors Increasing Likelihood:

  • Public Code: Contract code is likely public, making vulnerability analysis easier

  • Financial Incentive: Direct monetary gain motivates attackers

  • Low Barrier: Relatively simple to exploit once identified

  • Pattern Recognition: Well-known vulnerability pattern in DeFi

Factors Decreasing Likelihood:

  • Reentrancy Guard: The nonReentrant modifier provides some protection

  • Specific Conditions: Attacker must be a legitimate winner to exploit

  • Detection Risk: Suspicious activity could be detected and stopped

Impact:

Severity: HIGH

The reentrancy vulnerability in withdrawWinnings() has several critical impacts:

  1. Fund Drainage: Malicious actors can potentially drain the contract's ETH balance

  2. Game State Manipulation: Could disrupt the game's integrity and fairness

  3. Winner Privilege Abuse: Multiple withdrawals of the same prize amount

  4. Contract Insolvency: Could leave legitimate winners unable to withdraw their prizes

Proof of Concept

The vulnerability follows the classic pattern where:
​
External call is made before state update
Malicious contract receives ETH and can execute code
Window exists for state manipulation between ETH transfer and state update
Even with reentrancy guards, the CEI viola

Recommended Mitigation

Recommended Fix:
The withdrawWinnings() function should follow the CEI pattern by updating the state before making the external call:
​
​
function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender];
require(amount > 0, "Game: No winnings to withdraw.");
​
pendingWinnings[msg.sender] = 0; // ✅ Update state BEFORE external call
​
(bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Game: Failed to withdraw winnings.");
​
emit WinningsWithdrawn(msg.sender, amount);
}
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!