OrderBook
First Flight #43
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: medium
Likelihood: medium
Invalid

Violation of `Checks-Effects-Interactions` Pattern in `OrderBook::withdrawFees()` May Pose Future Risk

mozi

Violation of Checks-Effects-Interactions Pattern in OrderBook::withdrawFees() May Pose Future Risk

Description

Under normal behavior, the OrderBook::withdrawFees() function allows the contract owner to withdraw accumulated protocol fees in USDC. The function performs access control via onlyOwner and emits a FeesWithdrawn event after the withdrawal.

However, this function performs an external call (iUSDC.safeTransfer) before updating internal state (totalFees = 0), violating the recommended Checks-Effects-Interactions (CEI) pattern. Although no immediate vulnerability exists due to USDC being a standard ERC20 token, this pattern can introduce fragility and risk if:

  • The USDC token is replaced with a non-standard token (e.g., ERC777 or malicious ERC20)

  • The function is extended or reused in an upgradable architecture

  • Ownership is transferred to a contract with unknown logic

function withdrawFees(address _to) external onlyOwner {
if (totalFees == 0) revert InvalidAmount();
if (_to == address(0)) revert InvalidAddress();
​
iUSDC.safeTransfer(_to, totalFees); // External call
@> // State not updated yet
​
totalFees = 0; // State change occurs after the external call
emit FeesWithdrawn(_to);
}

Risk

Likelihood:

  • Moderate likelihood under upgradable design or token change

  • May affect maintainability and extensibility

Impact:

  • Increased risk of reentrancy in future integrations

  • Unexpected behavior if token uses non-standard logic

  • Reduces auditability and developer confidence

Proof of Concept

Even though not exploitable today, here's a non-exploitable simulation to illustrate the CEI violation:

contract FakeUSDC is IERC20 {
bool public called;
function transfer(address to, uint256 amount) public returns (bool) {
called = true;
// Attempt to reenter withdrawFees (will fail unless owner is this contract)
OrderBook(msg.sender).withdrawFees(to);
return true;
}
}

However, in practice:

  • USDC doesn’t execute external calls on transfer or safeTransfer

  • So fallback or reentrancy from withdrawFees() using USDC is impossible

Recommended Mitigation

Apply the CEI pattern to eliminate unnecessary external call risks:

function withdrawFees(address _to) external onlyOwner {
if (totalFees == 0) revert InvalidAmount();
if (_to == address(0)) revert InvalidAddress();
​
+ uint256 fees = totalFees;
+ totalFees = 0;
​
- iUSDC.safeTransfer(_to, totalFees);
+ iUSDC.safeTransfer(_to, fees);
​
emit FeesWithdrawn(_to);
}

This change ensures the internal state is updated before any external call, minimizing reentrancy and edge-case risks in future versions.

Updates

Lead Judging Commences

yeahchibyke Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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