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

Missing allowance and balance checks in `OrderBook::createSellOrder()` function, leading to confusion and poor UX.

mozi

Missing allowance and balance checks in OrderBook::createSellOrder() function

Description

Under normal behavior, the createSellOrder function allows a user (seller) to list a token for sale by specifying the token address, amount, price in USDC, and deadline. The contract performs validations on token address, amount, price, and deadline duration.

However, the function does not check the seller's balance or allowance for the _tokenToSell before calling safeTransferFrom(). This causes the transaction to revert if the seller has not approved the token or has insufficient balance — without giving a clear explanation to the user. In an on-chain only system with no frontend handling UX errors, this leads to poor user experience.

Additionally, the function violates the Checks-Effects-Interactions (CEI) pattern by performing an external token transfer before updating protocol state.

function createSellOrder(
address _tokenToSell,
uint256 _amountToSell,
uint256 _priceInUSDC,
uint256 _deadlineDuration
) public returns (uint256) {
if (!allowedSellToken[_tokenToSell]) revert InvalidToken();
if (_amountToSell == 0) revert InvalidAmount();
if (_priceInUSDC == 0) revert InvalidPrice();
if (_deadlineDuration == 0 || _deadlineDuration > MAX_DEADLINE_DURATION) revert InvalidDeadline();
​
uint256 deadlineTimestamp = block.timestamp + _deadlineDuration;
uint256 orderId = _nextOrderId++;
​
@> IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell);
@> // External call before updating state
​
orders[orderId] = Order({
id: orderId,
seller: msg.sender,
tokenToSell: _tokenToSell,
amountToSell: _amountToSell,
priceInUSDC: _priceInUSDC,
deadlineTimestamp: deadlineTimestamp,
isActive: true
});
​
emit OrderCreated(orderId, msg.sender, _tokenToSell, _amountToSell, _priceInUSDC, deadlineTimestamp);
return orderId;
}

Risk

Likelihood:

  • This will occur when the seller forgets to approve the token or doesn't have enough balance at the time of createSellOrder call.

  • The function will revert due to ERC20 safeTransferFrom() failure, without any custom error message, leading to UX failure.

Impact:

  • Users will be unable to create sell orders directly on-chain unless they manually approve tokens.

  • Frustration for users due to unexplained transaction reverts.

  • Protocol may be perceived as broken or unreliable when used without frontend abstraction.

Proof of Concept

// Assume user forgot to approve token or balance is not enough
IERC20(wETH).balanceOf(user); // 0
IERC20(wETH).allowance(user, orderBook); // 0
​
orderBook.createSellOrder(address(wETH), 1 ether, 1000e6, 3 days);
// ❌ Reverts at safeTransferFrom

Recommended Mitigation

function createSellOrder(...) public returns (uint256) {
+ if (IERC20(_tokenToSell).balanceOf(msg.sender) < _amountToSell) revert InsufficientBalance();
+ if (IERC20(_tokenToSell).allowance(msg.sender, address(this)) < _amountToSell) revert InsufficientAllowance();
​
...
- IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell);
+ IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell);
​
...
}

Additionally, move state changes (order registration) before external calls if reentrancy could become a concern in future token integrations.

Updates

Lead Judging Commences

yeahchibyke Lead Judge
about 1 month ago
yeahchibyke Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Too generic

Support

FAQs

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