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

Failure of all safeTransfer functions

neomartis

Failure of all safeTransfer functions

Description

  • All ERC20 tokens in this contract are casted to a SafeERC20 type, which is a wrapper around the ERC20 contract. This is to allow for the safe transfer of tokens via safeTransfer and safeTransferFrom methods.

  • However, this is not possible for already deployed tokens, that do not have these methods implemented. For interfaces, we need to define the exact methods that the deployed contracts have implemented. In this case WETH, WBTC, WSOL and USDC do not have these safe methods implemented. Meaning that the safeTransfer and safeTransferFrom methods do not exist and will revert.

@> using SafeERC20 for IERC20;

Risk

Likelihood: High

  • This "casting" of the SafeERC20 type is on ALL tokens in the contract. Including the core (WETH, WBTC, WSOL and USDC) token.

  • All transfers are using the safeTransfer and safeTransferFrom methods. Meaning that all transfers are affected.

Impact: High

  • Any transfer of tokens will revert, making the whole contract unusable.

  • There is no way to change this behaviour on the contract, or on the tokens, making this issue non-reparable.

Affected Code Locations

  • Line 24: using SafeERC20 for IERC20; - Global SafeERC20 usage

  • Line 121: safeTransferFrom in createSellOrder()

  • Line 162: safeTransferFrom in amendSellOrder()

  • Line 121: safeTransfer in amendSellOrder()

  • Line 189: safeTransfer in cancelSellOrder()

  • Line 206: safeTransferFrom in buyOrder()

  • Line 207: safeTransferFrom in buyOrder()

  • Line 208: safeTransfer in buyOrder()

  • Line 289: safeTransfer in emergencyWithdrawERC20()

  • Line 302: safeTransfer in withdrawFees()

Affected functions

Following functions will always revert:

  • createSellOrder()

  • amendSellOrder()

  • cancelSellOrder()

  • buyOrder()

  • emergencyWithdrawERC20()

  • withdrawFees()

Recommended Mitigation

  1. Remove the usage of SafeERC20 on all IERC20 tokens

  2. Change all implementations of safeTransfer and safeTransferFrom to transfer and transferFrom, and add additional checks to ensure the transfer is successful.

  3. OR Add internal functions:

function _safeTransfer(IERC20 token, address to, uint256 amount) internal {
require(to != address(0), "Transfer to zero address");
require(amount > 0, "Transfer amount must be positive");
bool success = token.transfer(to, amount);
require(success, "Transfer failed");
}
​
function _safeTransferFrom(IERC20 token, address from, address to, uint256 amount) internal {
require(from != address(0), "Transfer from zero address");
require(to != address(0), "Transfer to zero address");
require(amount > 0, "Transfer amount must be positive");
bool success = token.transferFrom(from, to, amount);
require(success, "TransferFrom failed");
}

Additional tips:

  • In tests do not use SafeERC20 for already deployed tokens. Use ERC20 and real contract interfaces for WETH, WBTC, WSOL and USDC.

Updates

Lead Judging Commences

yeahchibyke Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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