Description
-
The normal behavior should ensure that order state changes only occur after all token transfers have been successfully completed to maintain data consistency.
-
The buyOrder function modifies the order's active state before completing token transfers, which can result in orders being marked as inactive while tokens remain locked in the contract if any transfer fails.
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:
-
When buyer has insufficient USDC allowance or balance for the protocol fee transfer
-
When seller's address is a contract that rejects token transfers
-
When the token being sold has transfer restrictions or is paused
Impact:
-
Seller's tokens remain permanently locked in the contract with no way to retrieve them
-
Order becomes unfillable and uncancellable since it's marked as inactive
-
Protocol loses user trust due to funds being stuck
Proof of Concept
function testStateInconsistencyAttack() public {
vm.startPrank(alice);
wbtc.approve(address(book), 1e8);
uint256 orderId = book.createSellOrder(address(wbtc), 1e8, 50000e6, 1 days);
vm.stopPrank();
vm.startPrank(dan);
usdc.approve(address(book), 1000e6);
vm.expectRevert();
book.buyOrder(orderId);
vm.stopPrank();
vm.startPrank(alice);
vm.expectRevert(OrderBook.OrderAlreadyInactive.selector);
book.cancelSellOrder(orderId);
vm.stopPrank();
assertEq(wbtc.balanceOf(address(book)), 1e8);
assertEq(wbtc.balanceOf(alice), 0);
}
Recommended Mitigation
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; // Only mark as inactive after all transfers succeed
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);
}
