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

## [H-#] Owner Can Allow New Token and Emergency-Withdraw It (Owner Privilege + Asset Drainage Risk)

mcqueengodice

[H-#] Owner Can Allow New Token and Emergency-Withdraw It (Owner Privilege + Asset Drainage Risk)

Description:

The OrderBook::setAllowedSellToken function allows the owner to add a new token to OrderBook::allowedSellToken mapping variable, enabling it for trading in the order book. Subsequently, the OrderBook::emergencyWithdrawERC20 function permits the owner to withdraw any token except the core tokens (wETH, wBTC, wSOL, USDC). However, a newly allowed token can be withdrawn via OrderBook::emergencyWithdrawERC20 function, potentially allowing the owner to drain user-deposited tokens that were added to the order book.

Impact:

This vulnerability enables the owner to add a new token, allow users to deposit it for sell orders, and then withdraw those tokens via OrderBook::emergencyWithdrawERC20, effectively stealing user funds. This undermines the trust and integrity of the order book, as users expect their deposited tokens to be secure unless used in valid trades.

Proof of Concept:

  1. Deploy the OrderBook contract with valid addresses for wETH, wBTC, wSOL, and USDC.

  2. Owner calls setAllowedSellToken(address(newToken), true) to allow a new token (newToken).

  3. Alice creates a sell order for newToken via createSellOrder(address(newToken), 100e18, 1000e6, 1 days), transferring 100 newToken to the contract.

  4. Owner calls emergencyWithdrawERC20(address(newToken), 100e18, owner), transferring Alice’s 100 newToken to themselves.

  5. Alice’s sell order remains active but cannot be fulfilled as the tokens are no longer in the contract, leading to potential loss of funds.

Create a file in test/mocks/ named MockNewToken.sol for this contract:

// SPDX-License-Identifier: SEE LICENSE IN LICENSE
pragma solidity 0.8.26;
​
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract MockNewToken is ERC20 {
uint8 tokenDecimals;
​
constructor(uint8 _tokenDecimals) ERC20("MockNewToken", "mNTK") {
tokenDecimals = _tokenDecimals;
}
​
function decimals() public view override returns (uint8) {
return tokenDecimals;
}
​
function mint(address to, uint256 value) public {
uint256 updateDecimals = uint256(tokenDecimals);
_mint(to, (value * 10 ** updateDecimals));
}
}
​

Add this to test/TestOrderBook.t.sol

PoC Test Code:
// Add this at the top
import {MockNewToken} from "./mocks/MockNewToken.sol";
​
function test_ownerCanAllowAndWithdrawNewToken() public {
// Deploy a new mock token
MockNewToken newToken = new MockNewToken(18);
address newTokenAddr = address(newToken);
​
// Mint 100 tokens to Alice
newToken.mint(alice, 100);
assertEq(newToken.balanceOf(alice), 100e18, "Alice should have 100 newToken just minted");
​
// Owner allows new token
vm.prank(owner);
book.setAllowedSellToken(newTokenAddr, true);
assertTrue(book.allowedSellToken(newTokenAddr), "New token should be allowed");
​
// Alice creates a sell order for new token
vm.startPrank(alice);
newToken.approve(address(book), 100e18);
uint256 orderId = book.createSellOrder(newTokenAddr, 100e18, 250_000e6, 1 days);
vm.stopPrank();
assertEq(newToken.balanceOf(address(book)), 100e18, "Contract should hold 100 newToken");
assertEq(newToken.balanceOf(alice), 0, "Alice should have 0 newToken");
​
// Owner performs emergency withdrawal of new token
vm.prank(owner);
book.emergencyWithdrawERC20(newTokenAddr, 100e18, owner);
assertEq(newToken.balanceOf(address(book)), 0, "Contract should have 0 newToken");
assertEq(newToken.balanceOf(owner), 100e18, "Owner should have 100 newToken");
​
// Verify order still exists but cannot be fulfilled
OrderBook.Order memory order = book.getOrder(orderId);
assertTrue(order.isActive, "Order should still be active");
}

Recommended Mitigation:

Restrict OrderBook::emergencyWithdrawERC20 function to only allow withdrawal of tokens that are not in OrderBook::allowedSellToken mapping variable. This ensures that tokens used in active sell orders cannot be withdrawn by the owner, protecting user funds.:

Recommended solution:
function emergencyWithdrawERC20(address _tokenAddress, uint256 _amount, address _to) external onlyOwner {
if (
_tokenAddress == address(iWETH) ||
_tokenAddress == address(iWBTC) ||
_tokenAddress == address(iWSOL) ||
_tokenAddress == address(iUSDC) ||
+ allowedSellToken[_tokenAddress]
) {
revert("Cannot withdraw core or allowed order book tokens");
}
if (_to == address(0)) {
revert InvalidAddress();
}
IERC20 token = IERC20(_tokenAddress);
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: Design choice

Support

FAQs

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