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

L02-Missing Checks-Effects-Interactions Pattern in  amendSellOrder

accessdenied

Missing Checks-Effects-Interactions Pattern in amendSellOrder

Root + Impact

Description

The Checks-Effects-Interactions (CEI) pattern is a well-established defense against reentrancy. It ensures that all internal state is updated before making external calls. The function amendSellOrder() interacts with user-provided ERC-20 tokens before state updates.

While reentrancy is here mitigated by the use of ERC-20whitelisting tokens, the current implementation does not strictly follow CEI for the function amendSellOrder, leaving room for a potential edge-case exploit, especially if a malicious ERC-20 token (with a callback function in transfer) or an ERC-777 tokens (compatible with ERC-20 but add a callback function) is inadvertently whitelisted.

function amendSellOrder(
uint256 _orderId,
uint256 _newAmountToSell,
uint256 _newPriceInUSDC,
uint256 _newDeadlineDuration
) public {
// [...] Previous code
// 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 state after transfer
// Update order details
order.amountToSell = _newAmountToSell;
order.priceInUSDC = _newPriceInUSDC;
order.deadlineTimestamp = newDeadlineTimestamp;
​
emit OrderAmended(_orderId, _newAmountToSell, _newPriceInUSDC, newDeadlineTimestamp);
}

Risk

Likelihood: Low

  • Occurs if a malicious or poorly designed token is added to allowedSellToken (e.g., ERC-777 or tokens with non-standard behavior like hooks or deflation).

  • Requires a token to exploit safeTransfer or safeTransferFrom to call back into the contract and perform reentrant logic before the contract finishes state updates.

Impact: Low to Moderate

  • Could allow reentrant execution of amendSellOrder() or buyOrder() in edge cases, such as updating the same order multiple times before it’s finalized.

  • In the worst-case scenario, could result in inconsistent order state or duplicate transfers.

Proof of Concept

Scenario:

  1. A malicious token is added to the allowedSellToken mapping by governance or accident.

  2. An attacker creates a sell order using the malicious token.

  3. When someone calls buyOrder() to fulfill that order, the token's transfer() or transferFrom() triggers a tokensReceived() hook.

  4. Inside that hook, the attacker calls amendSellOrder() or another method on the contract.

  5. Because state changes happen after some external calls (or between them), the attacker can re-enter the same function or related ones with inconsistent state. If another seller has also performed a sell order of this token, the attacker can use amendto perform several transfers and take the amount of token put by the another seller.

Recommended Mitigation

Strictly apply the Checks-Effects-Interactions pattern in amendSellOrder

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(); // Cannot amend expired order
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);
// Update order details
++ uint256 previousAmountToSell = order.amountToSell;
++ order.amountToSell = _newAmountToSell;
++ order.priceInUSDC = _newPriceInUSDC;
++order.deadlineTimestamp = newDeadlineTimestamp;
​
// Handle token amount changes
++ if (_newAmountToSell > previousAmountToSell) {
// Increasing amount: Transfer additional tokens from seller
++ uint256 diff = _newAmountToSell - previousAmountToSell;
token.safeTransferFrom(msg.sender, address(this), diff);
++} else if (_newAmountToSell < previousAmountToSell) {
// Decreasing amount: Transfer excess tokens back to seller
++ uint256 diff = previousAmountToSell - _newAmountToSell;
token.safeTransfer(order.seller, diff);
}
--
--
--
emit OrderAmended(_orderId, _newAmountToSell, _newPriceInUSDC, newDeadlineTimestamp);
}

Updates

Lead Judging Commences

yeahchibyke Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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