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

Incorrect CEI Pattern Implementation in Withdrawal Function

luckyivanius

Incorrect CEI Pattern Implementation in Withdrawal Function

Description

  • The withdrawWinnings() function violates the Checks-Effects-Interactions (CEI) pattern by updating state after the external call instead of before

  • While the function has a nonReentrant modifier that prevents reentrancy, the pattern violation represents poor security practices and could cause issues if the modifier is ever removed or bypassed

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender]; // @> CHECK: Read state
require(amount > 0, "Game: No winnings to withdraw."); // @> CHECK: Validation
​
(bool success, ) = payable(msg.sender).call{value: amount}(""); // @> INTERACTION: 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

Likelihood: High

  • Every withdrawal transaction violates the CEI pattern

  • Pattern violation is consistent and always present

Impact: Low

  • Currently mitigated by nonReentrant modifier

  • Represents poor security practices and code quality

  • Could become vulnerable if modifier is removed or has bugs

  • Makes code review and audit more difficult

Proof of Concept

The test demonstrates the correct CEI pattern implementation:

function testWithdrawWinnings_CorrectCEIPattern() public {
// Setup: Player1 wins the game
vm.prank(player1);
game.claimThrone{value: INITIAL_CLAIM_FEE}();
vm.warp(block.timestamp + GRACE_PERIOD + 1);
game.declareWinner();
uint256 winningsAmount = game.pendingWinnings(player1);
assertGt(winningsAmount, 0);
// With correct CEI pattern, state should be updated before external call
uint256 balanceBefore = player1.balance;
vm.prank(player1);
game.withdrawWinnings();
// Verify state was updated and withdrawal succeeded
assertEq(game.pendingWinnings(player1), 0);
assertEq(player1.balance, balanceBefore + winningsAmount);
}

Explanation: The test verifies that the withdrawal works correctly and demonstrates what the proper CEI pattern should achieve.

Recommended Mitigation

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender]; // CHECK
require(amount > 0, "Game: No winnings to withdraw."); // CHECK
​
+ pendingWinnings[msg.sender] = 0; // EFFECT: Update state before external call
​
(bool success, ) = payable(msg.sender).call{value: amount}(""); // INTERACTION
require(success, "Game: Failed to withdraw winnings.");
​
- pendingWinnings[msg.sender] = 0; // Remove incorrect state update
​
emit WinningsWithdrawn(msg.sender, amount);
}

Explanation: Follow the proper CEI pattern by updating state before making external calls, even when protected by reentrancy guards, to maintain best security practices.

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.