Reentrancy Vulnerability in createSellOrder Function
Description
The createSellOrder function allows users to create sell orders by transferring tokens to the contract. However, the function performs the token transfer before updating the contract state, which violates the CEI (Checks-Effects-Interactions) pattern.
The function calls safeTransferFrom before storing the order details in the contract state. If the token being transferred has callbacks (like ERC-777 tokens), it could trigger a reentrancy attack that could manipulate the order creation process or drain funds from the contract.
function createSellOrder(
address _tokenToSell,
uint256 _amountToSell,
uint256 _priceInUSDC,
uint256 _deadlineDuration
) public returns (uint256) {
if (!allowedSellToken[_tokenToSell]) revert InvalidToken();
if (_amountToSell == 0) revert InvalidAmount();
if (_priceInUSDC == 0) revert InvalidPrice();
if (_deadlineDuration == 0 || _deadlineDuration > MAX_DEADLINE_DURATION) revert InvalidDeadline();
uint256 deadlineTimestamp = block.timestamp + _deadlineDuration;
uint256 orderId = _nextOrderId++;
@> IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell);
orders[orderId] = Order({
id: orderId,
seller: msg.sender,
tokenToSell: _tokenToSell,
amountToSell: _amountToSell,
priceInUSDC: _priceInUSDC,
deadlineTimestamp: deadlineTimestamp,
isActive: true
});
emit OrderCreated(orderId, msg.sender, _tokenToSell, _amountToSell, _priceInUSDC, deadlineTimestamp);
return orderId;
}
Risk
Likelihood:
-
The owner could add ERC-777 tokens or other tokens with callbacks to the whitelist via setAllowedSellToken
-
Users could create orders with malicious tokens that implement callback mechanisms in their transfer functions
-
The reentrancy could occur during the safeTransferFrom call before the order state is properly updated
Impact:
-
Reentrancy attacks could allow attackers to create multiple orders with the same tokens, potentially draining the contract
-
The attacker could manipulate order IDs and state variables during the reentrant call
-
Funds could be lost if the reentrant call interferes with the order creation logic
Proof of Concept
The following example demonstrates how a malicious token with callbacks could exploit the reentrancy vulnerability. The malicious token implements a callback in its transferFrom function that reenters the createSellOrder function.
contract MaliciousToken is IERC20 {
bool public callbackTriggered = false;
OrderBook public orderBook;
constructor(address _orderBook) {
orderBook = OrderBook(_orderBook);
}
function transferFrom(address from, address to, uint256 amount) external override returns (bool) {
if (!callbackTriggered && to == address(orderBook)) {
callbackTriggered = true;
orderBook.createSellOrder(address(this), amount, 1000, 1 days);
}
return true;
}
}
orderBook.setAllowedSellToken(address(maliciousToken), true);
orderBook.createSellOrder(address(maliciousToken), 1000, 1000, 1 days);
Recommended Mitigation
To prevent reentrancy attacks, the function should follow the CEI pattern by updating the contract state before making external calls. Additionally, implementing a reentrancy guard would provide extra protection.
- function createSellOrder(
- address _tokenToSell,
- uint256 _amountToSell,
- uint256 _priceInUSDC,
- uint256 _deadlineDuration
- ) public returns (uint256) {
- if (!allowedSellToken[_tokenToSell]) revert InvalidToken();
- if (_amountToSell == 0) revert InvalidAmount();
- if (_priceInUSDC == 0) revert InvalidPrice();
- if (_deadlineDuration == 0 || _deadlineDuration > MAX_DEADLINE_DURATION) revert InvalidDeadline();
- uint256 deadlineTimestamp = block.timestamp + _deadlineDuration;
- uint256 orderId = _nextOrderId++;
- IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell);
-
- // Store the order
- orders[orderId] = Order({
- id: orderId,
- seller: msg.sender,
- tokenToSell: _tokenToSell,
- amountToSell: _amountToSell,
- priceInUSDC: _priceInUSDC,
- deadlineTimestamp: deadlineTimestamp,
- isActive: true
- });
-
- emit OrderCreated(orderId, msg.sender, _tokenToSell, _amountToSell, _priceInUSDC, deadlineTimestamp);
- return orderId;
- }
+ function createSellOrder(
+ address _tokenToSell,
+ uint256 _amountToSell,
+ uint256 _priceInUSDC,
+ uint256 _deadlineDuration
+ ) public nonReentrant returns (uint256) {
+ if (!allowedSellToken[_tokenToSell]) revert InvalidToken();
+ if (_amountToSell == 0) revert InvalidAmount();
+ if (_priceInUSDC == 0) revert InvalidPrice();
+ if (_deadlineDuration == 0 || _deadlineDuration > MAX_DEADLINE_DURATION) revert InvalidDeadline();
uint256 deadlineTimestamp = block.timestamp + _deadlineDuration;
uint256 orderId = _nextOrderId++;
// Update state BEFORE external call (CEI pattern)
orders[orderId] = Order({
id: orderId,
seller: msg.sender,
tokenToSell: _tokenToSell,
amountToSell: _amountToSell,
priceInUSDC: _priceInUSDC,
deadlineTimestamp: deadlineTimestamp,
isActive: true
});
// External call AFTER state changes
IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell);
emit OrderCreated(orderId, msg.sender, _tokenToSell, _amountToSell, _priceInUSDC, deadlineTimestamp);
return orderId;
}
