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
10 days ago
yeahchibyke Lead Judge 10 days 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.