OrderBook

First Flight #43
Beginner FriendlySolidity
100 EXP
View results
Submission Details
Impact: high
Likelihood: low
Invalid

CEI Pattern Violation in amendSellOrder Enables Reentrancy

The amendSellOrder function violates the Checks-Effects-Interactions (CEI) pattern by performing token transfers before updating order state. When decreasing order amounts, tokens are transferred back to the seller before order storage is updated. If the owner adds tokens with callback mechanisms, an attacker could exploit reentrancy during the safeTransfer to manipulate order state and potentially drain protocol tokens.

Description

  • The amendSellOrder function allows users to modify their existing sell orders, including decreasing the token amount, which triggers a token transfer back to the seller.

  • The function violates the Checks-Effects-Interactions (CEI) pattern by performing external token transfers before updating the order state in storage.

  • This is not a risk with the current tokens in the protocol, which do not have callbacks functions, but could become if new tokens are approved.

function amendSellOrder(
uint256 _orderId,
uint256 _newAmountToSell,
uint256 _newPriceInUSDC,
uint256 _newDeadlineDuration
) public {
Order storage order = orders[_orderId];
// Validation checks
if (order.seller == address(0)) revert OrderNotFound(); // Check if order exists
if (order.seller != msg.sender) revert NotOrderSeller();
if (!order.isActive) revert OrderAlreadyInactive();
if (block.timestamp >= order.deadlineTimestamp) revert OrderExpired();
if (_newAmountToSell == 0) revert InvalidAmount();
if (_newPriceInUSDC == 0) revert InvalidPrice();
if (
_newDeadlineDuration == 0 ||
_newDeadlineDuration > MAX_DEADLINE_DURATION
) revert InvalidDeadline();
uint256 newDeadlineTimestamp = block.timestamp + _newDeadlineDuration;
IERC20 token = IERC20(order.tokenToSell);
// Handle token amount changes
if (_newAmountToSell > order.amountToSell) {
// Increasing amount: Transfer additional tokens from seller
uint256 diff = _newAmountToSell - order.amountToSell;
token.safeTransferFrom(msg.sender, address(this), diff);
} else if (_newAmountToSell < order.amountToSell) {
// Decreasing amount: Transfer excess tokens back to seller
uint256 diff = order.amountToSell - _newAmountToSell;
@> token.safeTransfer(order.seller, diff);
}
// Update order details
order.amountToSell = _newAmountToSell;
order.priceInUSDC = _newPriceInUSDC;
order.deadlineTimestamp = newDeadlineTimestamp;
emit OrderAmended(
_orderId,
_newAmountToSell,
_newPriceInUSDC,
newDeadlineTimestamp
);
}

Risk

Likelihood: Low

  • The owner adds a token with callback functionality (hooks, rebasing tokens, or malicious tokens) to the allowed sell tokens list. Since only the owner can approve new tokens, this is not a likely outcome but perhaps enhances the centralization risk.

  • A user creates an order with such a token and later calls amendSellOrder to decrease the amount, triggering the vulnerable transfer.

Impact: High

  • Reentrancy during the token transfer can allow manipulation of order state before updates are committed

  • Attacker could potentially drain protocol tokens or manipulate multiple orders simultaneously through recursive calls

The proof of code is not provided since it is a plain reentrancy.

Recommended Mitigation

move the "// Handle token amount changes" with the if part after the order deatils have been updated, and for best practices put it after also the events emitted
Updates

Lead Judging Commences

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.