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

Centralization : Owner can malicious STEAL non-core tokens that has not been filled or canceled yet via the :emergencyWithdrawal function

yemi

Root + Impact

Description

  • The owner can allow new (non-core) tokens to be sold in the contract via the setAllowedSellToken(...) function.

  • Users can list non-core tokens (like DAI, UNI, etc.) for sale via the createSellOrder(...) function. These tokens are temporarily stored in the contract until filled or canceled.

However, the owner can arbitrarily drain these tokens at any time using the emergencyWithdrawERC20(...) function — even if they don’t belong to them and while orders are still active

// Root cause in the codebase with @> marks
@> function emergencyWithdrawERC20(
@> address _tokenAddress,
@> uint256 _amount,
@> address _to
@> ) external onlyOwner {
if (
_tokenAddress == address(iWETH) ||
_tokenAddress == address(iWBTC) ||
_tokenAddress == address(iWSOL) ||
_tokenAddress == address(iUSDC)
) {
revert("Cannot withdraw core order book tokens via emergency function");
}
if (_to == address(0)) {
revert InvalidAddress();
}
​
@> IERC20 token = IERC20(_tokenAddress);
@> token.safeTransfer(_to, _amount);
emit EmergencyWithdrawal(_tokenAddress, _amount, _to);
}

  • The root issue lies in the fact that:

  • The contract allows the owner to transfer any non-core token balance even while they are being listed

  • And it doesn't ensure the tokens were originally deposited by the owner

  • This opens a backdoor for fund extraction from sellers.

Risk

High:

  • The owner has full control over the system due to onlyOwner modifier.
    Any seller listing a non-core token exposes their funds to potential owner withdrawal.
    No checks exist to prevent this during active orders.

High:

  • Users lose tokens locked in the contract

  • No way to recover stolen funds

  • Allows centralized manipulation of liquidity

Proof of Concept

function testEmergencyWithdrawExploit() public {
// Deploy mock token
MockERC20 dai = new MockERC20("DAI", "DAI", 18);
​
// Allow DAI as sellable token
vm.prank(owner);
orderBook.setAllowedSellToken(address(dai), true);
​
// Seller deposits DAI into the contract via createSellOrder
uint256 orderId = orderBook.createSellOrder{from: seller}(
address(dai),
100 ether,
300 * 1e6, // priceInUSDC
3 days
);
​
// Owner calls emergencyWithdrawERC20 before buyer fills or seller cancels
vm.prank(owner);
orderBook.emergencyWithdrawERC20(address(dai), 100 ether, owner);
​
// Now, seller tries to cancel but reverts — no tokens left!
vm.expectRevert("ERC20: transfer amount exceeds balance");
vm.prank(seller);
orderBook.cancelSellOrder(orderId);
}

  • The seller loses their tokens

  • The owner gets them for free

  • No transaction reverts — everything looks normal

Recommended Mitigation

  • Allow the owner to recover only tokens that were not part of any active order , and do not belong to the list of tokens allowed be sold— i.e., truly unintended deposits.


i. Add Mapping to Track Token Deposits from Sellers

mapping(address => mapping(address => uint256)) public userTokenDeposits;

  • userTokenDeposits[seller][token]: How much of a given token was deposited by users.

  • Helps distinguish between user-owned tokens and owner-owned/orphaned tokens.




    ii. Update createSellOrder(...) to Record Deposit

--- a/createSellOrder(...)
+++ b/createSellOrder(...)
@@ -1,4 +1,5 @@
IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell);
​
+userTokenDeposits[msg.sender][_tokenToSell] += _amountToSell;
​
orders[orderId] = Order({ ... });

  • This records how much of each token has been deposited by whom.




    iii. Clear Deposit Tracking When Order Is Filled or Canceled

Update both buyOrder(...) and cancelSellOrder(...) to reduce the deposit tracking when orders are closed.

In buyOrder(...)

--- a/buyOrder(...)
+++ b/buyOrder(...)
@@ -1,4 +1,5 @@
// Decrease user deposit after transfer
-userTokenDeposits[order.seller][order.tokenToSell] -= order.amountToSell;
+userTokenDeposits[order.seller][order.tokenToSell] -= order.amountToSell;

In cancelSellOrder(...)

--- a/cancelSellOrder(...)
+++ b/cancelSellOrder(...)
@@ -1,4 +1,5 @@
token.safeTransfer(order.seller, order.amountToSell);
+userTokenDeposits[order.seller][order.tokenToSell] -= order.amountToSell;

This keeps the mapping accurate and prevents false assumptions about ownership.


iv. Restrict emergencyWithdrawERC20(...) to Non-Allowed & Non-Active Tokens

function emergencyWithdrawERC20(
address _tokenAddress,
uint256 _amount,
address _to
) external onlyOwner {
// Disallow core token withdrawal
if (
_tokenAddress == address(iWETH) ||
_tokenAddress == address(iWBTC) ||
_tokenAddress == address(iWSOL) ||
_tokenAddress == address(iUSDC)
) {
revert("Cannot withdraw core order book tokens via emergency function");
}
​
if (_to == address(0)) {
revert InvalidAddress();
}
​
IERC20 token = IERC20(_tokenAddress);
​
// Prevent withdrawal of tokens currently allowed for sale
require(!allowedSellToken[_tokenAddress], "Token is allowed for sale");
​
// Prevent withdrawal of tokens in active orders
for (uint256 i = 1; i < _nextOrderId; ++i) {
Order storage order = orders[i];
if (
order.isActive &&
order.tokenToSell == _tokenAddress
) {
revert("Token is currently listed in an active order");
}
}
​
// Proceed with withdrawal
token.safeTransfer(_to, _amount);
emit EmergencyWithdrawal(_tokenAddress, _amount, _to);
}
Updates

Lead Judging Commences

yeahchibyke Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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