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

Missing allowance and balanceOf check + CEI Pattern Violation in `OrderBook::amendSellOrder()` function

mozi

Missing allowance and balanceOf check + CEI Pattern Violation in OrderBook::amendSellOrder() function

Description

The amendSellOrder() function allows sellers to update the amount, price, and duration of their existing orders. If the new amount is greater than the current amount, the contract will transfer the difference from the seller. If it's less, it refunds the difference to the seller.

However, this function:

  • Performs a safeTransferFrom without checking if the caller (msg.sender) has:

    • Sufficient ERC20 allowance given to the contract.

    • Sufficient token balance to cover the delta.

  • Violates the Checks-Effects-Interactions (CEI) pattern by updating storage only after an external token transfer.

if (_newAmountToSell > order.amountToSell) {
uint256 diff = _newAmountToSell - order.amountToSell;
@> token.safeTransferFrom(msg.sender, address(this), diff); // ❌ external call
} else if (_newAmountToSell < order.amountToSell) {
uint256 diff = order.amountToSell - _newAmountToSell;
@> token.safeTransfer(order.seller, diff); // ❌ external call
}
​
@> // state update after external calls
order.amountToSell = _newAmountToSell;
order.priceInUSDC = _newPriceInUSDC;
order.deadlineTimestamp = newDeadlineTimestamp;

Risk

Likelihood:

  • This issue occurs whenever a seller calls amendSellOrder() without setting allowance or not holding sufficient token balance.

  • It leads to an immediate revert, potentially confusing the user or breaking integrations.

  • Malicious sellers may exploit this flow in the future if ERC20 behavior is extended (e.g. with fallback() enabled tokens or non-standard token mechanics).

Impact:

  • Sellers cannot update their orders without pre-approving allowance off-chain.

  • Poor UX and degraded protocol reliability.

  • Violating CEI opens theoretical reentrancy risk if non-standard tokens are allowed in the future.

Proof of Concept

A user without prior allowance calls amendSellOrder():

// Seller forgets to call approve()
orderBook.amendSellOrder(orderId, newAmount + 1e18, newPrice, newDuration);
​
// Will revert here due to missing allowance
// or if balance is insufficient

And if reentrancy was possible (non-ERC20 safe token):

// token.safeTransfer triggers fallback that calls amendSellOrder again

Recommended Mitigation

function amendSellOrder(...) public {
...
​
+ if (token.allowance(msg.sender, address(this)) < diff) revert InsufficientAllowance();
+ if (token.balanceOf(msg.sender) < diff) revert InsufficientBalance();
​
if (_newAmountToSell > order.amountToSell) {
uint256 diff = _newAmountToSell - order.amountToSell;
token.safeTransferFrom(msg.sender, address(this), diff);
} else if (_newAmountToSell < order.amountToSell) {
uint256 diff = order.amountToSell - _newAmountToSell;
token.safeTransfer(order.seller, diff);
}
​
+ // CEI fix: update state before transfer, if required
+ order.amountToSell = _newAmountToSell;
+ order.priceInUSDC = _newPriceInUSDC;
+ order.deadlineTimestamp = newDeadlineTimestamp;
emit OrderAmended(...);
}

Not exploitable in the current context due to standard ERC20 safety.
But degrades user experience, introduces functionality failure, and violates CEI best practices.

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.