Christmas Dinner
First Flight #31
Beginner FriendlyFoundrySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

CEI Pattern Violation in _refundERC20 Function

0xmaxi

Summary

The _refundERC20 function in the ChristmasDinner contract violates the Check-Effects-Interactions (CEI) pattern by performing external calls before state changes, creating potential reentrancy vulnerabilities despite the presence of a reentrancy guard.

Vulnerability Details

The current implementation performs operations in this order:

  1. Makes external token transfers via safeTransfer (Interactions)

  2. Updates user balances to zero (Effects)

function refundERC20(address _to) internal {
// Interactions first (incorrect)
i_WETH.safeTransfer(to, balances[_to][address(i_WETH)]);
i_WBTC.safeTransfer(to, balances[_to][address(i_WBTC)]);
i_USDC.safeTransfer(to, balances[_to][address(i_USDC)]);
// Effects last (should be first)
balances[to][address(i_USDC)] = 0;
balances[to][address(i_WBTC)] = 0;
balances[to][address(i_WETH)] = 0;
}

Impact

While the parent refund() function uses a nonReentrant modifier, violating CEI:

  • Makes the code more vulnerable if the reentrancy guard is removed/modified

  • Could lead to state inconsistencies if token transfers fail

  • Makes the code less maintainable and harder to audit

  • Increases gas costs by potentially reading state variables multiple times

Tools Used

  • Manual Review

  • Solidity Visual Developer

Recommendations

Restructure the function to follow CEI pattern:

function refundERC20(address _to) internal {
// Effects (state changes first)
uint256 wethAmount = balances[to][address(i_WETH)];
uint256 wbtcAmount = balances[to][address(i_WBTC)];
uint256 usdcAmount = balances[to][address(i_USDC)];
balances[to][address(i_WETH)] = 0;
balances[to][address(i_WBTC)] = 0;
balances[to][address(i_USDC)] = 0;
// Interactions (external calls last)
i_WETH.safeTransfer(to, wethAmount);
i_WBTC.safeTransfer(to, wbtcAmount);
i_USDC.safeTransfer(to, usdcAmount);
}

This implementation:

  1. Follows CEI pattern

  2. Uses local variables to cache state reads

  3. Updates state before external calls

  4. Is more gas efficient

  5. Maintains better state consistency

Updates

Lead Judging Commences

0xtimefliez Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.