OrderBook
First Flight #43
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

No Token Transfer Check in emergencyWithdrawERC20

evmninja

Summary

The emergencyWithdrawERC20 function does not check if the token transfer was successful, which could lead to inconsistent state if the transfer fails silently.

Description

The emergencyWithdrawERC20 function is designed to allow the contract owner to withdraw any non-core tokens that might have been sent to the contract by mistake. While the function correctly uses safeTransfer from OpenZeppelin's SafeERC20 library, which will revert if the transfer fails, there is still a potential issue with tokens that have unusual behaviour or non-standard implementations. Some ERC20 tokens, despite being compliant with the standard interface, might have custom behaviour that could cause transfers to fail silently or return false without reverting. In such cases, the contract would proceed as if the transfer was successful, emitting the EmergencyWithdrawal event. The tokens remain in the contract, but the contract and off-chain systems believe they have been withdrawn.

Step-by-step analysis

  1. A non-standard ERC20 token is accidentally sent to the contract.

  2. The owner calls emergencyWithdrawERC20 to recover these tokens.

  3. The token's transfer function silently fails without reverting.

  4. The contract proceeds as if the transfer was successful, emitting the EmergencyWithdrawal event.

  5. The tokens remain in the contract, but the contract and off-chain systems believe they have been withdrawn.

Severity classification

  • Impact: Low - The potential impact is limited to non-standard tokens and would only affect emergency withdrawals.

  • Likelihood: Low - Most ERC20 tokens follow the standard behaviour, and SafeERC20 mitigates many issues.

File name with issue

OrderBook.sol

Code with issue

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

Recommendation

Although SafeERC20's safeTransfer already provides significant protection, for maximum safety, consider checking the token balance before and after the transfer to ensure it was successful, especially for critical functions like emergency withdrawals.

Code to fix the problem

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);
uint256 balanceBefore = token.balanceOf(address(this));
if (balanceBefore < _amount) {
revert InvalidAmount();
}
token.safeTransfer(_to, _amount);
uint256 balanceAfter = token.balanceOf(address(this));
if (balanceBefore - balanceAfter != _amount) {
revert("Transfer failed or incorrect amount transferred");
}
emit EmergencyWithdrawal(_tokenAddress, _amount, _to);
}

The proposed fix adds balance checks before and after the token transfer to ensure that the expected amount was actually transferred. This provides an additional layer of safety beyond what SafeERC20 offers, ensuring that the contract's state accurately reflects the token balances.

Updates

Lead Judging Commences

yeahchibyke Lead Judge
4 months ago
yeahchibyke Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

No Token Transfer Check in `emergencyWithdrawERC20()`

The `emergencyWithdrawERC20()` function does not check if the token transfer was successful, which could lead to inconsistent state if the transfer fails silently.

Support

FAQs

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