Root + Impact
Description
-
The amendSellOrder() function violates the CEI (Checks-Effects-Interactions) pattern by performing external token transfers before updating the order state.
-
This creates a reentrancy window where malicious tokens can call back into the contract while order state is in an intermediate state.
function amendSellOrder(uint256 _orderId, uint256 _newAmountToSell, ) public {
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);
}
order.amountToSell = _newAmountToSell;
order.priceInUSDC = _newPriceInUSDC;
order.deadlineTimestamp = newDeadlineTimestamp;
Risk
Likelihood:
The owner can add any ERC20 token via setAllowedSellToken(), including tokens with transfer hooks or malicious implementations
Every call to amendSellOrder() that changes token amounts triggers external transfers before state updates, creating reentrancy windows
Impact:
Malicious tokens can reenter and manipulate order parameters multiple times during a single amendment operation
Order state inconsistencies can be exploited where the contract operates on stale data during reentrancy calls
Proof of Concept
The following demonstrates how a malicious token can exploit the reentrancy window to manipulate order state during amendment. The malicious token's transfer hook reenters the contract while order state still reflects pre-amendment values.
contract MaliciousToken {
OrderBook orderBook;
uint256 targetOrderId;
bool attacking;
function transferFrom(address from, address to, uint256 amount) external returns (bool) {
if (!attacking && to == address(orderBook)) {
attacking = true;
orderBook.amendSellOrder(targetOrderId, 1, 1 ether, 1 days);
attacking = false;
}
return true;
}
}
Recommended Mitigation
The fix involves reordering the function to follow the CEI pattern: perform all state updates before any external interactions.
This prevents reentrancy by ensuring that all state changes are completed before any external calls occur. Even if a malicious token attempts to reenter, it will see the updated state and cannot manipulate the order during an intermediate state.
The CEI pattern is a fundamental security practice that eliminates reentrancy vulnerabilities by removing the window where external code can execute while internal state is inconsistent.
function amendSellOrder(
uint256 _orderId,
uint256 _newAmountToSell,
uint256 _newPriceInUSDC,
uint256 _newDeadlineDuration
) public {
Order storage order = orders[_orderId];
// ...validation checks (unchanged)...
uint256 newDeadlineTimestamp = block.timestamp + _newDeadlineDuration;
IERC20 token = IERC20(order.tokenToSell);
+
+ // Store original amount for transfer calculations
+ uint256 originalAmount = order.amountToSell;
+
+ // EFFECTS: Update order state BEFORE external interactions
+ order.amountToSell = _newAmountToSell;
+ order.priceInUSDC = _newPriceInUSDC;
+ order.deadlineTimestamp = newDeadlineTimestamp;
- // Handle token amount changes
- if (_newAmountToSell > order.amountToSell) {
+ // INTERACTIONS: Handle token transfers AFTER state updates
+ if (_newAmountToSell > originalAmount) {
- uint256 diff = _newAmountToSell - order.amountToSell;
+ uint256 diff = _newAmountToSell - originalAmount;
token.safeTransferFrom(msg.sender, address(this), diff);
- } else if (_newAmountToSell < order.amountToSell) {
+ } else if (_newAmountToSell < originalAmount) {
- uint256 diff = order.amountToSell - _newAmountToSell;
+ uint256 diff = originalAmount - _newAmountToSell;
token.safeTransfer(order.seller, diff);
}
- // Update order details
- order.amountToSell = _newAmountToSell;
- order.priceInUSDC = _newPriceInUSDC;
- order.deadlineTimestamp = newDeadlineTimestamp;
emit OrderAmended(_orderId, _newAmountToSell, _newPriceInUSDC, newDeadlineTimestamp);
}
