Description
The `amendSellOrder` function performs token transfers (external calls) before updating critical internal state variables (amountToSell, priceInUSDC, deadlineTimestamp). If the seller is a contract with malicious logic in its `receive()` or `fallback()` function, it could exploit this ordering to reenter amendSellOrder or other functions, resulting in unexpected behavior or double-withdrawal.
function amendSellOrder(
uint256 _orderId,
uint256 _newAmountToSell,
uint256 _newPriceInUSDC,
uint256 _newDeadlineDuration
) public {
Order storage order = orders[_orderId];
if (order.seller == address(0)) revert OrderNotFound();
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);
@> 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;
emit OrderAmended(_orderId, _newAmountToSell, _newPriceInUSDC, newDeadlineTimestamp);
}
Impact:
An attacker could recursively reenter the function before state is updated and manipulate order state or drain funds from the protocol. This violates the Checks-Effects-Interactions (CEI) pattern and poses a high risk when dealing with untrusted external contracts.
Proof of Conept
During decrease of amountToSell, this external call occurs before state update:
token.safeTransfer(order.seller, diff);
Recommended Mitigation
Refactor the function to follow the CEI pattern. Update the internal state first, then interact with external contracts:
function amendSellOrder(
uint256 _orderId,
uint256 _newAmountToSell,
uint256 _newPriceInUSDC,
uint256 _newDeadlineDuration
) public {
Order storage order = orders[_orderId];
if (order.seller == address(0)) revert OrderNotFound();
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);
@> if (_newAmountToSell > order.amountToSell) {
uint256 diff = _newAmountToSell - order.amountToSell;
+ order.amountToSell = _newAmountToSell;
+ order.priceInUSDC = _newPriceInUSDC;
+ order.deadlineTimestamp = newDeadlineTimestamp;
token.safeTransferFrom(msg.sender, address(this), diff);
} else if (_newAmountToSell < order.amountToSell) {
uint256 diff = order.amountToSell - _newAmountToSell;
+ order.amountToSell = _newAmountToSell;
+ order.priceInUSDC = _newPriceInUSDC;
+ order.deadlineTimestamp = newDeadlineTimestamp;
token.safeTransfer(order.seller, diff);
}
- order.amountToSell = _newAmountToSell;
- order.priceInUSDC = _newPriceInUSDC;
- order.deadlineTimestamp = newDeadlineTimestamp;
emit OrderAmended(_orderId, _newAmountToSell, _newPriceInUSDC, newDeadlineTimestamp);
}
