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

OrderBook State Update Order Optimization

yunohu

OrderBook State Update Order Optimization

Description

  • In the OrderBook contract, the buyOrder function processes a buy order by transferring USDC from the buyer to the contract (for fees) and the seller (for the order price minus fees), then transferring the seller’s tokens to the buyer. It updates totalFees after these transfers to track accumulated protocol fees. Similarly, the withdrawFees function transfers accumulated USDC fees to the owner and then resets totalFees to zero.

  • In buyOrder, the state update totalFees += protocolFee occurs after safeTransferFrom calls for USDC transfers. In withdrawFees, the state update totalFees = 0 occurs after the iUSDC.safeTransfer call. This deviates from the Checks-Effects-Interactions pattern, which recommends updating state before external calls to minimize risks like reentrancy or state inconsistencies in complex scenarios. While SafeERC20 mitigates reentrancy risks and transaction atomicity ensures state reversion on failure, reordering these updates would enhance robustness and align with best practices.


Risk

Likelihood: low

  • The issue occurs during every execution of buyOrder and withdrawFees, as the state updates (totalFees += protocolFee and totalFees = 0) are consistently placed after external SafeERC20 calls, deviating from the Checks-Effects-Interactions pattern.


Impact: low

  • The current order of operations is safe due to SafeERC20 and transaction atomicity, but it increases the risk of state inconsistencies and might allow reentrancy-prone tokens or complex interactions, potentially leading to incorrect fee tracking.

Proof of Concept

In buyOrder (lines 171-181):
​
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; // Should be moved before transfers
emit OrderFilled(_orderId, msg.sender, order.seller);
​
In withdrawFees (lines 249-257):
​
if (totalFees == 0) {
revert InvalidAmount();
}
if (_to == address(0)) {
revert InvalidAddress();
}
iUSDC.safeTransfer(_to, totalFees);
totalFees = 0; // Should be moved before transfer
emit FeesWithdrawn(_to);

Recommended Mitigation

//for buyOrder
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;
+ totalFees += protocolFee; // Update state before transfers
iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee);
iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives);
IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);
- totalFees += protocolFee; // Remove this line
emit OrderFilled(_orderId, msg.sender, order.seller);
}
​
//for withdraw fees
function withdrawFees(address _to) external onlyOwner {
if (totalFees == 0) {
revert InvalidAmount();
}
if (_to == address(0)) {
revert InvalidAddress();
}
+ uint256 amount = totalFees; // Store amount for transfer
+ totalFees = 0; // Update state before transfer
- iUSDC.safeTransfer(_to, totalFees);
+ iUSDC.safeTransfer(_to, amount);
- totalFees = 0; // Remove this line
emit FeesWithdrawn(_to);
}
​
Updates

Lead Judging Commences

yeahchibyke Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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