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

External Call Before State Change in Fee Withdrawal

ryzen_xp

External Call Before State Change in Fee Withdrawal

Description

  • The withdrawFees function transfers accumulated protocol fees (totalFees) to the owner-specified address.

  • The function first performs an external call (iUSDC.safeTransfer) before updating internal state (totalFees = 0).

  • This is a violation of the "Checks-Effects-Interactions" pattern, a best practice in Solidity that prevents reentrancy and state inconsistency risks.

  • If the safeTransfer call fails or reverts, totalFees remains unchanged — which is acceptable. But if the transfer succeeds but a reentrancy or subsequent failure occurs, it could result in unintended behavior or double-withdrawals in more complex versions of the function.

function withdrawFees(address _to) external onlyOwner {
if (totalFees == 0) {
revert InvalidAmount();
}
if (_to == address(0)) {
revert InvalidAddress();
}
​
iUSDC.safeTransfer(_to, totalFees); // ❌ External call before state change
​
totalFees = 0;
​
emit FeesWithdrawn(_to);
}

Reference Files

File Function Lines Note
./src/OrderBook.sol withdrawFees L298–L313 External call before state mutation

Risk

Likelihood: Medium

  • The function is admin-only, reducing attack surface.

  • However, fee withdrawals are routine operations, and a failure in safeTransfer() (e.g. token misbehavior, non-standard return) could cause issues.

  • If the code evolves to include hooks, modifiers, or other logic after the transfer, risks would increase.

Impact: Medium

  • Currently low risk of exploitation due to simplicity, but:

    • Breaks Solidity best practices.

    • Can cause inconsistent contract state in the event of failed or malicious token transfers.

    • Future changes (e.g. multi-token support, callbacks) could make this a reentrancy vector.

Proof of Concept (PoC)

// Current behavior:
iUSDC.safeTransfer(_to, totalFees); // external call
totalFees = 0; // state change after
​
// If safeTransfer behaves unexpectedly or reverts internally:
// - state remains unchanged (acceptable)
// - but no effects rollback beyond revert (e.g., nested logic)
​
// Best practice:
uint256 fees = totalFees;
totalFees = 0; // state first
iUSDC.safeTransfer(_to, fees); // external call after

Recommended Mitigation

  1. Reorder operations to follow Checks-Effects-Interactions pattern:

function withdrawFees(address _to) external onlyOwner {
if (totalFees == 0) revert InvalidAmount();
if (_to == address(0)) revert InvalidAddress();
​
uint256 fees = totalFees;
totalFees = 0; // ✅ Update state first
​
iUSDC.safeTransfer(_to, fees); // ✅ External call after
​
emit FeesWithdrawn(_to);
}

  1. Document best-practice adherence
    Add comments to clarify why state change is done before transfer.

  2. Future-Proofing
    If supporting multiple tokens or callbacks later, this change reduces risk of reentrancy or inconsistencies.

Updates

Lead Judging Commences

yeahchibyke Lead Judge
3 months ago
yeahchibyke Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

CEI pattern not followed in `withdrawFees()` function

`withdrawFees()` function performs an external transfer using `iUSDC.safeTransfer()` before resetting totalFees. This breaks the `Checks-Effects-Interactions (CEI)` pattern and can lead to incorrect internal state if the transfer fails for any reason.

Support

FAQs

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