OrderBook
First Flight #43
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: medium
Invalid

Reentrancy Vulnerability in `amendSellOrder` Function

vicent00

Reentrancy Vulnerability in amendSellOrder Function

Description

The amendSellOrder function allows users to modify existing sell orders by changing the amount, price, and deadline. The function performs token transfers before updating the contract state, which violates the Checks-Effects-Interactions (CEI) pattern.

The function transfers tokens to/from the contract before updating the order state, allowing malicious tokens with callback mechanisms to reenter the function and manipulate the order state.

// Root cause in the codebase with @> marks to highlight the relevant section
function amendSellOrder(
uint256 _orderId,
uint256 _newAmountToSell,
uint256 _newPriceInUSDC,
uint256 _newDeadlineDuration
) public {
// ... validations ...
// @> PROBLEM: Transferencias ANTES de actualizar estado
if (_newAmountToSell > order.amountToSell) {
uint256 diff = _newAmountToSell - order.amountToSell;
token.safeTransferFrom(msg.sender, address(this), diff); // ← Puede reentrar
} else if (_newAmountToSell < order.amountToSell) {
uint256 diff = order.amountToSell - _newAmountToSell;
token.safeTransfer(order.seller, diff); // ← Puede reentrar
}
​
// @> Estado se actualiza DESPUÉS de transferencias
order.amountToSell = _newAmountToSell;
order.priceInUSDC = _newPriceInUSDC;
order.deadlineTimestamp = block.timestamp + _newDeadlineDuration;
}

Risk

Likelihood:

  • High - Any token with callback mechanisms (ERC-777, custom tokens with hooks) can trigger reentrancy when whitelisted via setAllowedSellToken

  • Medium - The function is publicly accessible and can be called by any user with an active order

Impact:

  • Double-spending of tokens through reentrancy attacks

  • Manipulation of order state leading to incorrect token transfers

  • Potential loss of user funds through state inconsistencies

Proof of Concept

This PoC demonstrates how a malicious token can exploit the reentrancy vulnerability in amendSellOrder. The malicious token implements callback mechanisms that trigger when tokens are transferred, allowing it to reenter the function before the state is updated. This can lead to double-spending and state manipulation.

// Malicious token with reentrancy capability
contract MaliciousToken {
OrderBook public orderBook;
bool public reentered = false;
constructor(address _orderBook) {
orderBook = OrderBook(_orderBook);
}
function transferFrom(address from, address to, uint256 amount) external returns (bool) {
if (!reentered && to == address(orderBook)) {
reentered = true;
// Reenter the amendSellOrder function
orderBook.amendSellOrder(1, 1000, 200000, 1 days);
}
return true;
}
function transfer(address to, uint256 amount) external returns (bool) {
if (!reentered && to == address(orderBook)) {
reentered = true;
// Reenter the amendSellOrder function
orderBook.amendSellOrder(1, 1000, 200000, 1 days);
}
return true;
}
}
​
// Test demonstrating the vulnerability
function test_amendSellOrder_reentrancy() public {
// Setup malicious token
MaliciousToken maliciousToken = new MaliciousToken(address(book));
// Whitelist the malicious token (this is the vulnerability in setAllowedSellToken)
vm.prank(owner);
book.setAllowedSellToken(address(maliciousToken), true);
// Create order with malicious token
vm.startPrank(alice);
uint256 orderId = book.createSellOrder(address(maliciousToken), 1000, 200000, 1 days);
vm.stopPrank();
// Attempt to amend the order - this will trigger reentrancy
vm.startPrank(alice);
book.amendSellOrder(orderId, 2000, 300000, 2 days);
vm.stopPrank();
// The malicious token can manipulate the order state during reentrancy
}

Recommended Mitigation

The fix follows the Checks-Effects-Interactions (CEI) pattern, which is a fundamental security principle in smart contract development. By updating the state before performing external calls, we prevent reentrancy attacks.

function amendSellOrder(
uint256 _orderId,
uint256 _newAmountToSell,
uint256 _newPriceInUSDC,
uint256 _newDeadlineDuration
) public {
// ... validations ...
- // Remove transfers before state updates
- 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 = block.timestamp + _newDeadlineDuration;
​
+ // Update state FIRST (Effects)
+ order.amountToSell = _newAmountToSell;
+ order.priceInUSDC = _newPriceInUSDC;
+ order.deadlineTimestamp = block.timestamp + _newDeadlineDuration;
+
+ // Then perform transfers (Interactions)
+ 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);
+ }
}
Updates

Lead Judging Commences

yeahchibyke Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.