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

[H-01] Lack of checks in OrderBook::emergencyWithdrawERC20() function can cause broken orders and centralisation risks

soarinskysagar

Root + Impact

Description

  • emergencyWithdrawERC20()function is meant for emergency situations where funds need to be withdrawn from the contract when they get stuck in the contract due to accidental transfers.

  • However, a malicious contract owner may drain all the non-core tokens from the contract for their own benefits.

  • Even if tokens are withdrawn for emergency purposes, the withdraw may cause lack of funds for active orders and this breaks the contract

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);
}

Risk

Likelihood:

  • Whenever emergencyWithdrawERC20() function is called by the owner

Impact:

  • Insufficient funds for active orders

  • Malicious intent of contract owner

Proof of Concept

Place the following function into TestOrderBook.t.sol and run with forge test --mt test_insufficientFundsForActiveOrderAfterEmergencyWithdraw

function test_insufficientFundsForActiveOrderAfterEmergencyWithdraw() public {
// deploy a non-core token
MockWETH newMock = new MockWETH(18);
​
vm.prank(owner);
book.setAllowedSellToken(address(newMock), true);
​
newMock.mint(alice, 10e18);
​
vm.startPrank(alice);
newMock.approve(address(book), 10e18);
uint256 aliceId = book.createSellOrder(address(newMock), 8e18, 1e18, 3 days);
vm.stopPrank();
​
vm.prank(owner);
book.emergencyWithdrawERC20(address(newMock), 8e18, owner);
​
vm.prank(dan);
vm.expectRevert();
book.buyOrder(aliceId);
}

The test passes, which means that the buy function has been reverted due to insufficient balance.

Recommended Mitigation

  • Check for active orders on the token being withdrawn

+mapping(address=>bool) public isTokenActive; // keeps track of token to check if it has any active order
.
.
.
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();
}
+ assert(!isTokenActive[_tokenAddress]);
IERC20 token = IERC20(_tokenAddress);
token.safeTransfer(_to, _amount);
​
emit EmergencyWithdrawal(_tokenAddress, _amount, _to);
}

  • Use governance to prevent malicious owner

Updates

Lead Judging Commences

yeahchibyke Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Too generic

Support

FAQs

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