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

Incomplete Reentrancy Protection in `withdrawWinnings` Enables Potential Multi-Withdraw via Indirect Vectors

thesandf

Description

  • Normally, the withdrawWinnings() function allows users to withdraw their earned ETH by reading their balance from pendingWinnings, transferring the funds, and then resetting the internal balance to zero. It uses a custom nonReentrant modifier to prevent reentry.

  • However, the function violates the Checks-Effects-Interactions pattern by performing the external call before clearing internal state. This creates a vulnerability surface for indirect reentrancy attacks, especially via fallback logic or in future additions to the contract that interact with shared storage (pendingWinnings). Delegatecalls and cross-function logic may also bypass the intended 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}(""); // External call
@> require(success, "Game: Failed to withdraw winnings.");
​
@> pendingWinnings[msg.sender] = 0; // Effect happens after interaction
​
emit WinningsWithdrawn(msg.sender, amount);
}

Risk

Likelihood: High

  • The function executes an external call (call{value: amount}) before clearing the user’s pendingWinnings, which opens the door for fallback-triggered logic to execute in the same transaction context.

  • In future contract versions or additions, cross-function or delegatecall-based logic could interact with pendingWinnings during a reentrant fallback, increasing exploit potential.

Impact: Medium

  • If exploited, an attacker could recursively call withdrawal mechanisms and drain more funds than allocated, leading to unauthorized withdrawals and financial loss.

  • Even if not exploitable today due to the nonReentrant modifier, the state mutation order creates fragile assumptions, exposing risk in upgradeable or modular deployments.

Proof of Concept

Simulated exploit contract:

contract MaliciousWinner {
Game public game;
bool public hasReentered;
​
constructor(address payable _game) {
game = Game(_game);
}
​
receive() external payable {
if (!hasReentered) {
hasReentered = true;
game.withdrawWinnings(); // Reentry attempt during fallback
}
}
​
function attack() external {
game.withdrawWinnings(); // Initial call triggers fallback
}
}

Foundry test:

function testMultiWithdrawExploit() public {
address attacker = address(new MaliciousWinner(payable(address(game))));
vm.deal(attacker, 1 ether);
game.join{value: 1 ether}(); // attacker wins prize
game.setPendingWinnings(attacker, 1 ether); // simulate win
​
vm.prank(attacker);
MaliciousWinner(attacker).attack(); // attempt reentrancy
​
// Expect only 1 withdrawal
assertEq(address(attacker).balance, 1 ether, "Should receive 1 ether only");
assertEq(game.pendingWinnings(attacker), 0, "Should reset winnings");
}

Result: This specific attack fails due to nonReentrant, but it proves the fallback is triggered and the logic is exposed.

Recommended Mitigation

Follow the Checks-Effects-Interactions pattern strictly. Update the internal state before making external calls:

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