OrderBook
First Flight #43
Beginner FriendlySolidity
100 EXP
View all submissions
Previous Next
Submission Details
Impact: high
Likelihood: high
Invalid

State Inconsistency in Order Execution + Funds Permanently Locked

Author Revealed upon completion

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];
​
// Validation checks
if (order.seller == address(0)) revert OrderNotFound();
if (!order.isActive) revert OrderNotActive();
if (block.timestamp >= order.deadlineTimestamp) revert OrderExpired();
​
order.isActive = false; // State changed before transfers
uint256 protocolFee = (order.priceInUSDC * FEE) / PRECISION;
uint256 sellerReceives = order.priceInUSDC - protocolFee;
​
iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee); // ← Transfer 1
iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives); // ← Transfer 2
IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell); // ← Transfer 3
​
totalFees += protocolFee; // ← State update after transfers
​
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

Attack Scenario: This demonstrates how a failed buyOrder transaction can permanently lock seller's tokens due to premature state changes.

function testStateInconsistencyAttack() public {
// Alice creates a sell order
vm.startPrank(alice);
wbtc.approve(address(book), 1e8);
uint256 orderId = book.createSellOrder(address(wbtc), 1e8, 50000e6, 1 days);
vm.stopPrank();
​
// Dan tries to buy but has insufficient USDC
vm.startPrank(dan);
usdc.approve(address(book), 1000e6); // Only approve small amount
// This will fail on second transfer but order.isActive is already false
vm.expectRevert();
book.buyOrder(orderId);
vm.stopPrank();
​
// Order is now inactive but Alice cannot cancel it
vm.startPrank(alice);
vm.expectRevert(OrderBook.OrderAlreadyInactive.selector);
book.cancelSellOrder(orderId);
vm.stopPrank();
​
// Alice's tokens are permanently locked
assertEq(wbtc.balanceOf(address(book)), 1e8);
assertEq(wbtc.balanceOf(alice), 0);
}

Explanation:

  1. Alice creates a valid sell order, transferring her wBTC to the contract

  2. Dan attempts to buy but only approves insufficient USDC (1000 vs 50000 needed)

  3. The buyOrder function sets order.isActive = false before transfers

  4. The first USDC transfer succeeds (protocol fee), but the second fails due to insufficient allowance

  5. The transaction reverts, but the order state remains isActive = false

  6. Alice cannot cancel the order because it's marked inactive, but she also didn't receive payment

  7. Her wBTC remains permanently locked in the contract with no recovery mechanism

Recommended Mitigation

Solution: Apply the Checks-Effects-Interactions pattern by moving state changes after all external calls.

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);
​
+ order.isActive = false; // Only mark as inactive after all transfers succeed
totalFees += protocolFee;
​
emit OrderFilled(_orderId, msg.sender, order.seller);
}

Why this works:

  • State changes (order.isActive = false) only occur after all external calls succeed

  • If any transfer fails, the entire transaction reverts and state remains unchanged

  • Sellers retain the ability to cancel orders that haven't been successfully purchased

  • Follows the secure Checks-Effects-Interactions pattern to prevent state inconsistencies

  • Additionally consider adding nonReentrant modifier for extra protection

Updates

Lead Judging Commences

yeahchibyke Lead Judge
6 days ago
yeahchibyke Lead Judge about 5 hours ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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