Submission Details
Impact:
Likelihood:
[L-2] `OrderBook::withdrawFees()` Does Not Follow CEI (Checks-Effects-Interactions) Pattern
Description
The `withdrawFees()` function in the `OrderBook` contract performs an external `token transfer` before updating internal state `totalFees`.
This violates the Checks-Effects-Interactions (CEI) pattern, a widely recommended best practice in Solidity smart contract development.
The CEI pattern mitigates risks such as:
1. Reentrancy attacks
2. State inconsistencies during external calls
3. Unexpected behavior when integrating with malicious or non-standard tokens
In this case, the contract calls `iUSDC.safeTransfer()` before resetting `totalFees` to zero.
While USDC is generally safe and not reentrant,
relying on the safety of external contracts (even trusted tokens) introduces composability risk in a DeFi context.
Risk
Impact:
Although the severity is low due to the assumption that USDC is a well-behaved ERC20 token, this pattern opens up the potential for:
1. Future vulnerabilities:
If a malicious token or upgradeable proxy replaces USDC (e.g., during testing or misconfiguration), it could exploit the state inconsistency.
2. Loss of funds / double-withdrawal:
A theoretical reentrant call into withdrawFees() before totalFees is set to zero could allow fees to be withdrawn multiple times,
resulting in a loss of funds from the protocol.
Security analysis & audit tooling failure:
Violating CEI may cause false positives or missed flags in formal analysis, verification, or static auditing tools.
3. Negative perception:
Security-conscious DeFi users and integrators expect CEI compliance as a basic security guarantee.
Proof of Concept
Here’s a theoretical PoC exploiting the CEI violation if USDC were a malicious ERC20 that reenters:
1. Malicious USDC Implementation
```javascript
contract FakeUSDC is IERC20 {
OrderBook public orderBook;
address public attacker;
constructor(address _orderBook) {
orderBook = OrderBook(_orderBook);
attacker = msg.sender;
}
function transfer(address recipient, uint256 amount) public override returns (bool) {
// Reenter the contract before totalFees is set to 0
if (msg.sender == address(orderBook)) {
orderBook.withdrawFees(attacker);
}
return true;
}
// Other ERC20 methods omitted for brevity...
}
```
2. Outcome
First call to `withdrawFees()` triggers `transfer()`.
`transfer()` calls `withdrawFees()` again before `totalFees` is reset.
This causes the same amount of fees to be transferred multiple times, draining the contract’s USDC balance.
Recommended Mitigation
Apply the Checks-Effects-Interactions (CEI) pattern properly in withdrawFees() by updating internal state before any external calls:
Fixed Code:
```diff
function withdrawFees(address _to) external onlyOwner {
if (totalFees == 0) {
revert InvalidAmount();
}
if (_to == address(0)) {
revert InvalidAddress();
}
- iUSDC.safeTransfer(_to, totalFees);
+ uint256 feesToWithdraw = totalFees;
totalFees = 0; // Effect before interaction
+ iUSDC.safeTransfer(_to, feesToWithdraw); // Interaction after state change
emit FeesWithdrawn(_to);
}
```
This fix ensures that even if a reentrant call occurs during `safeTransfer()`, the internal state `totalFees` has already been cleared, preventing any double-withdrawal or unexpected behavior.
Updates
Lead Judging Commences
yeahchibyke
about 1 month ago
yeahchibyke about 1 month ago
Submission Judgement Published 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.
Rewards breakdown
nSLOC
217This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.