Root + Impact
Description
The buyOrder()
function should follow the Checks-Effects-Interactions (CEI) pattern to prevent reentrancy attacks and ensure state consistency. However, the function updates the critical totalFees
state variable after making external token transfers, violating this security pattern.
If a malicious seller contract implements a fallback function that triggers during the USDC transfer, it can reenter the OrderBook contract while totalFees
is still in an inconsistent state. This creates a window where the protocol fees have been collected but not yet recorded in the contract's accounting system.
function buyOrder(uint256 _orderId) public {
Order storage order = orders[_orderId];
if (order.seller == address(0)) revert OrderNotFound();
if (!order.isActive) revert OrderNotActive();
if (block.timestamp >= order.deadlineTimestamp) revert OrderExpired();
order.isActive = false;
uint256 protocolFee = (order.priceInUSDC * FEE) / PRECISION;
uint256 sellerReceives = order.priceInUSDC - protocolFee;
iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee);
iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives);
IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);
totalFees += protocolFee;
emit OrderFilled(_orderId, msg.sender, order.seller);
}
Risk
Likelihood:
Occurs when a seller is a malicious contract that implements fallback/receive functions
Standard ERC-20 tokens like USDC don't typically have hooks, but custom tokens or upgradeable contracts could introduce them
The violation exists in every order execution, creating a consistent attack surface
Impact:
Fee accounting corruption where fees are collected but not recorded in totalFees
Potential for complex reentrancy attacks that manipulate contract state during the inconsistent window
Protocol revenue loss due to incorrect fee tracking
Broken invariants that could affect other contract functions relying on totalFees
Proof of Concept
contract MaliciousSellerContract {
OrderBook public orderBook;
uint256 public attackOrderId;
function createMaliciousOrder(address tokenToSell, uint256 amount, uint256 price) external {
IERC20(tokenToSell).approve(address(orderBook), amount);
attackOrderId = orderBook.createSellOrder(tokenToSell, amount, price, 1 days);
}
fallback() external {
}
}
Recommended Mitigation
difffunction buyOrder(uint256 _orderId) public {
Order storage order = orders[_orderId];
// Checks
if (order.seller == address(0)) revert OrderNotFound();
if (!order.isActive) revert OrderNotActive();
if (block.timestamp >= order.deadlineTimestamp) revert OrderExpired();
// Effects - ALL state changes first
order.isActive = false;
uint256 protocolFee = (order.priceInUSDC * FEE) / PRECISION;
uint256 sellerReceives = order.priceInUSDC - protocolFee;
+ totalFees += protocolFee;
// Interactions - external calls last
iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee);
iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives);
IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);
- totalFees += protocolFee;
emit OrderFilled(_orderId, msg.sender, order.seller);
}