Description

The Checks-Effects-Interactions (CEI) pattern is a critical security best practice in smart contract development, dictating that all input validations (Checks), state modifications (Effects), and external calls (Interactions) should occur in that specific order.

​

In createSellOrder, the transfer of tokens from the seller to the contract (IERC20(_tokenToSell).safeTransferFrom(...)) occurs before the order's full state is completely stored in the orders mapping. Similarly, in amendSellOrder, when reducing the order amount, tokens are returned to the seller (token.safeTransfer(...)) before the order.amountToSell and other related state variables are updated.

​

Risk

Likelihood: High (deviation is always present in code)

The structural deviation from the CEI pattern is always present in the identified functions. However, successful exploitation is highly complex due to the use of SafeERC20, which includes re-entrancy guards and reverts on call failures, significantly mitigating direct re-entrancy risks.

Impact: Low

While theoretically, a highly sophisticated attacker leveraging a malicious token or complex interaction could exploit this, SafeERC20 makes direct fund loss via simple re-entrancy very difficult. The primary impact is a violation of a critical security best practice.

​

As it stands alone this is low impact but it would be more significant with a malicious token.

I'm probably going to make one as a Solana token ./#H4ck3d1t.exe was me so I know this can be done with a malicious file set as NFT.

Proof of Concept

Explanation:

A direct, executable Proof of Concept that results in fund loss is difficult to provide without a custom malicious ERC20 token designed to exploit very subtle state inconsistencies in a re-entrant call. However, the conceptual proof lies in the violated sequence of operations:

​

For createSellOrder:

​

Violation: The contract calls IERC20(_tokenToSell).safeTransferFrom on Line 59 (Interaction) before the Order struct, including amountToSell, priceInUSDC, and isActive status, is fully assigned to orders[orderId] on Line 60 (Effects).

Scenario: If _tokenToSell were a specially crafted malicious token, and it could somehow re-enter the OrderBook contract during its transferFrom call (Line 59), the order's state in orders[orderId] would still be incomplete or partially initialized. While SafeERC20 typically prevents a direct re-entrant call to the same function, this pattern deviation leaves a theoretical window where an inconsistent state could be observed or, in highly complex scenarios, potentially exploited if other functions are callable in this intermediate state.

​

For amendSellOrder (when decreasing amount):

​

Violation: When the _newAmountToSell is less than order.amountToSell, the excess tokens are transferred back to the seller via token.safeTransfer on Line 114 (Interaction) before the order.amountToSell, order.priceInUSDC, and order.deadlineTimestamp are updated on Lines 116-118 (Effects).

Scenario: If order.seller is a malicious contract, its receive() or fallback function could theoretically attempt to re-enter the OrderBook contract. At the point of re-entry (Line 114), the order in storage still holds its old amountToSell value. Although SafeERC20's re-entrancy guard makes a direct re-entry to amendSellOrder difficult, the violation of CEI means the contract briefly exists in an inconsistent state where external calls can occur while its internal logic has not fully reflected the current operation.

​

Explanation:

The recommended mitigation involves reordering the operations within both createSellOrder and amendSellOrder to strictly adhere to the Checks-Effects-Interactions (CEI) pattern.

For createSellOrder:

The Order struct assignment to orders[orderId] (Effect) is moved to occur before the IERC20(_tokenToSell).safeTransferFrom (Interaction). This ensures that by the time any external token transfer is initiated, the contract's internal state accurately reflects that the order has been created and its details are finalized in storage.

For amendSellOrder (when reducing amount):

The updates to order.amountToSell, order.priceInUSDC, and order.deadlineTimestamp (Effects) are moved to occur before the token.safeTransfer(order.seller, diff) (Interaction). This guarantees that if the order.seller contract attempts any re-entrant calls, the OrderBook contract's state (specifically the order details) is already updated to its new, consistent values, making it much harder to exploit any intermediate inconsistent state.

By implementing these changes, you eliminate the brief windows of inconsistent state during external calls, significantly improving the contract's security posture against sophisticated re-entrancy attempts, even with the protective measures offered by SafeERC20.

​

​

if (_priceInUSDC == 0) revert InvalidAmount();

orderId = _nextOrderId++;

- IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell); //@> Move this after state update

orders[orderId] = Order({

seller: msg.sender,

tokenToSell: _tokenToSell,

amountToSell: _amountToSell,

priceInUSDC: _priceInUSDC,

deadlineTimestamp: block.timestamp + _deadlineDuration,

isActive: true

});

+ IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell); //@> Interaction after state update

emit OrderCreated(orderId, msg.sender, _tokenToSell, _amountToSell, _priceInUSDC, block.timestamp + _deadlineDuration);

}

@@ -107,17 +107,17 @@

uint256 diff = _newAmountToSell - order.amountToSell;

IERC20 token = IERC20(order.tokenToSell);

token.safeTransfer(order.seller, diff); //@> Move this after state update

}

// Update the order details

- order.amountToSell = _newAmountToSell;

- order.priceInUSDC = _newPriceInUSDC;

- order.deadlineTimestamp = block.timestamp + _newDeadlineDuration;

+ // State updates moved before interaction for CEI adherence

+ order.amountToSell = _newAmountToSell;

+ order.priceInUSDC = _newPriceInUSDC;

+ order.deadlineTimestamp = block.timestamp + _newDeadlineDuration;

emit OrderAmended(

_orderId,

order.seller,

order.tokenToSell,

order.amountToSell,

order.priceInUSDC,