Tadle

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

Tokens with fee on transfer are not supported

Summary

The vulnerability identified in the TokenManager contract is related to the handling of ERC20 tokens that impose a fee on transfers. The current implementation of the _transfer() function does not account for tokens that charge a fee, which can result in incorrect balance calculations and transaction failures.

Vulnerability Details

The vulnerability arises from the assumption that the amount transferred is equal to the amount received by the recipient. However, certain ERC20 tokens charge a fee on every transfer() or transferFrom() call. In the TokenManager contract, the _transfer() function assumes the full transfer amount is received. For tokens with transfer fees, this assumption leads to inaccurate calculations, as the actual received amount is less than the intended transfer amount.

Code Snippet

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

The impact of this vulnerability is significant, as it can cause transactions to fail when interacting with tokens that impose a transfer fee. This includes popular tokens like STA and PAXG. If the transfer fee is toggled on in the future for tokens like USDT and USDC, users may find themselves unable to withdraw their funds due to the transfer failures.

Proof Of Concept

Some tokens take a transfer fee (STA, PAXG) and there are some that currently
do not but might do so in the future (USDT, USDC).
Let’s look at the following scenario:

  1. Alice tries to transfer tokens to capital pool.

  2. Some checks are executed in TokenManager.sol::_transfer():

if (fromBalanceAft != fromBalanceBef - _amount) {
revert TransferFailed();
}
if (toBalanceAft != toBalanceBef + _amount) {
revert TransferFailed();
}````

Transfer will always fail, since the transferredAmount will be less than amount due to the fee.

USDT is an ERC20 token that has togglable transfer fees, but for now the fee is set to 0 (see
the contract here: https://etherscan.io/address/0xdAC17F958D2ee523a2206206994597C13D831ec7#code ).
If user transfer USDT and later on they change the transfer fee, user will be not able to withdraw
their funds.

Tools Used

Manual review

Recommendations

Implement a logic that supports ERC20 tokens with fees.

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.