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

Preserve CEI and Avoid Future Risks

adiverse

order.isActive = false Should Be Moved After External Calls to Preserve CEI and Avoid Future Risks

Description

  • The current buyOrder() implementation sets order.isActive = false before any external token transfers are made.

  • This violates the Checks-Effects-Interactions (CEI) pattern

  • Although the function uses safeTransferFrom() (which reverts on failure, reverting all state changes including order.isActive = false), this pattern creates a fragile dependency on revert behavior and introduces latent risk if the code is refactored or if tokens do not conform to the ERC-20 standard.

  • If in the future safeTransferFrom() is replaced with transferFrom() and return values are ignored (common for gas savings), or wrapped in try/catch, the order could be marked inactive permanently, even if the payment fails — resulting in a denial-of-service (DoS) to the seller.

// Root cause in the codebase with @> marks to highlight the relevant section
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;
​
emit OrderFilled(_orderId, msg.sender, order.seller);
}
​

Risk

Likelihood:

  • Low currently — because safeTransferFrom() reverts on failure.

  • High in the future — if transfer logic is changed (e.g., try/catch, return-value checks), or with integration of non-standard ERC-20s (like USDT).

Impact:

  • High

  • If external transfers fail but order.isActive = false is already executed (and not reverted), the order becomes unusable.

  • Seller’s tokens remain locked in the contract, permanently.

  • Buyer’s failed transaction does not transfer funds, but still kills the order — resulting in DoS for the seller.

  • State inconsistency that is hard to debug and fix post-deployment.

Proof of Concept

  • Buyer initiates buyOrder() but has insufficient USDC allowance.

  • safeTransferFrom() reverts, and entire transaction is reverted — so today, this does not break the contract.

  • However, future versions might catch the revert or ignore return values, causing:

    • order.isActive = false to persist

    • USDC transfer to fail silently

    • Seller’s token never delivered

    • Order never fillable again

Recommended Mitigation

Move order.isActive = false after all safeTransferFrom() and safeTransfer() calls.

Ensures that the order is only marked inactive if and only if the entire exchange succeeds

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;
​
totalFees += protocolFee;
​
emit OrderFilled(_orderId, msg.sender, order.seller);
}
​
Updates

Lead Judging Commences

yeahchibyke Lead Judge 11 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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