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 10 days 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.