Tadle

Tadle
DeFiFoundry
27,750 USDC
View results
Submission Details
Severity: medium
Valid

DoS when creating an offer with fee on transfer token

Summary

When maker creates an offer with fee on transfer token exchanging, he will face TransferFailed revert, due to fee on transfer tokens token transfer's bad handle

Vulnerability Details

As we can see from the sponsor's provided compabilities, the protocol is expected co compile with weird ERC20s, that take fee on transfer: - ERC20 (any token that follows the ERC20 standard)

However let's see what happens in scenario where maker decides to call PreMarkets::createOffer with param CreateOfferParams.tokenAddress == address of ERC20Token that have fee on transfer:

  1. Bob calls PreMarkets::createOffer with CreateOfferParams.tokenAddress == fee on tansfer ERC20Token

  2. During the execution TokenManager::tillInis called and TokenManager::_transferis executed

    function createOffer(CreateOfferParams calldata params) external payable {
    ....
    {
    /// @dev transfer collateral from _msgSender() to capital pool
    uint256 transferAmount = OfferLibraries.getDepositAmount(
    params.offerType,
    params.collateralRate,
    params.amount,
    true,
    Math.Rounding.Ceil
    );
    ITokenManager tokenManager = tadleFactory.getTokenManager();
    -> tokenManager.tillIn{value: msg.value}(
    _msgSender(),
    params.tokenAddress,
    transferAmount,
    false
    );
    }
    ....
    }
    function tillIn(
    address _accountAddress,
    address _tokenAddress,
    uint256 _amount,
    bool _isPointToken
    )
    external
    payable
    onlyRelatedContracts(tadleFactory, _msgSender())
    onlyInTokenWhiteList(_isPointToken, _tokenAddress)
    {
    ....
    } else {
    /// @notice token is ERC20 token
    -> _transfer(
    _tokenAddress,
    _accountAddress,
    capitalPoolAddr,
    _amount,
    capitalPoolAddr
    );
    }
    emit TillIn(_accountAddress, _tokenAddress, _amount, _isPointToken);
    }
  3. As we can see from the last check in TokenManager::_transferthe function will revert because of toBalanceAft != toBalanceBef + amount_ (actual toBalanceAft = toBalanceBef +_ amount - transferFee)

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
uint256 fromBalanceBef = IERC20(_token).balanceOf(_from);
uint256 toBalanceBef = IERC20(_token).balanceOf(_to);
if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
ICapitalPool(_capitalPoolAddr).approve(address(this));
}
_safe_transfer_from(_token, _from, _to, _amount);
uint256 fromBalanceAft = IERC20(_token).balanceOf(_from);
uint256 toBalanceAft = IERC20(_token).balanceOf(_to);
if (fromBalanceAft != fromBalanceBef - _amount) {
revert TransferFailed();
}
-> if (toBalanceAft != toBalanceBef + _amount) {
revert TransferFailed();
}
}

Impact

DoS if createOffer is called with CreateOfferParams.tokenAddress that have fee on transfer

Tools Used

Manual review

Recommendations

There are 2 steps needed to solve the issue:

  1. Rewrite the _transfer function as shown below:

  2. Add more code logic to return the deposited value(toBalanceAft - toBalanceBef) and if it's < _amount, decrease the _amount and points value in the corresponding offerInfo and stockInfo

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
uint256 fromBalanceBef = IERC20(_token).balanceOf(_from);
uint256 toBalanceBef = IERC20(_token).balanceOf(_to);
if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
ICapitalPool(_capitalPoolAddr).approve(address(this));
}
_safe_transfer_from(_token, _from, _to, _amount);
uint256 fromBalanceAft = IERC20(_token).balanceOf(_from);
uint256 toBalanceAft = IERC20(_token).balanceOf(_to);
if (fromBalanceAft != fromBalanceBef - _amount) {
revert TransferFailed();
}
- if (toBalanceAft != toBalanceBef + _amount) {
+ if (toBalanceAft <= toBalanceBef) {
revert TransferFailed();
}
}
Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-TokenManager-FOT-Rebasing

Valid medium, there are disruptions to the ability to take market actions. The following functions will be disrupted without the possibiliy of reaching settlement, since the respective offers cannot be created/listed regardless of mode when transferring collateral token required to the CapitalPool contract or when refunding token from user to capital pool during relisting. So withdrawal is not an issue - `createOffer()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L96-L102) - `listOffer()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L355-L362) - `relistOffer()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L515-L521) - `createTaker()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L831-L836) I believe medium severity is appropriate although the likelihood is high and impact is medium (only some level of disruption i.e. FOT tokens not supported and no funds at risk)

Support

FAQs

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