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

Missing CEI Pattern in withdrawFees() Enables Potential Reentrancy Attack

abhishek2416

Root + Impact

Description

  • In Solidity, the Checks-Effects-Interactions (CEI) pattern is a security best practice used to prevent reentrancy attacks. The pattern dictates that:

    1. Validate inputs (Checks)

    2. Update contract state (Effects)

    3. Interact with external contracts (Interactions)

    In the original withdrawFees() function, the contract transferred funds via safeTransfer() before zeroing out totalFees. This exposed the contract to reentrancy attacks if the iUSDC token implemented malicious behavior (e.g., a fallback function that re-entered withdrawFees or another function accessing totalFees).

function withdrawFees(address _to) external onlyOwner {
if (totalFees == 0) revert InvalidAmount();
​
@> iUSDC.safeTransfer(_to, totalFees); // ⚠️ External call before state update
totalFees = 0;
}
​

Risk

Likelihood:

Likelihood: Medium

  • Can occur when:

    • A malicious ERC20 implements a reentrant transfer() or receive() hook.

    • The contract's logic depends on totalFees not being zeroed immediately.

Impact:

Impact: High

  • Could lead to repeated withdrawals before totalFees is set to zero.

  • Would result in fund loss and integrity failure of the contract’s accounting system.

Proof of Concept

Explanation:

  1. withdrawFees() initiates a transfer to MaliciousUSDC.

  2. The transfer() method in MaliciousUSDC re-enters withdrawFees().

  3. Since totalFees was not yet reset, the second call succeeds again.

  4. This can be looped to drain all collected protocol fees.

// Assume malicious ERC20 token calls back into the contract
contract MaliciousUSDC {
function transfer(address to, uint256 amount) public returns (bool) {
// Re-enter withdrawFees() here while totalFees is still set
OrderBook(msg.sender).withdrawFees(address(this));
return true;
}
}
​

Recommended Mitigation

This fix eliminates the vulnerability by:

  • Preventing reentrant calls from accessing stale state (totalFees > 0)

  • Ensuring that only one successful withdrawal can ever happen per cycle

  • Following the CEI pattern to avoid external interaction before internal state mutation

By decoupling the amount to transfer from totalFees, we ensure atomicity and consistency, even in the presence of external token hooks.

unction withdrawFees(address _to) external nonReentrant onlyOwner {
if (totalFees == 0) {
revert InvalidAmount();
}
if (_to == address(0)) {
revert InvalidAddress();
}
​
+ uint256 feesToWithdraw = totalFees;
+ totalFees = 0;
​
- iUSDC.safeTransfer(_to, totalFees);
+ iUSDC.safeTransfer(_to, feesToWithdraw);
​
emit FeesWithdrawn(_to);
}
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.