Tadle

Tadle
DeFi
30,000 USDC
View results
Submission Details
Severity: medium
Valid

`TokenManager:_transfer()` function will get failed for some `ERC20` tokens.

Summary

Protocol does not support fee-on-transfer and rebasing tokens.

Vulnerability Details

As the protocol is going to deal with many ERC20, so it's execution will get failed for fee on transfer tokens and rebasing tokens.

Some tokens take a transfer fee (e.g. STA, PAXG), some do not currently charge a fee but may do so in the future (e.g. USDT, USDC).

TokenManager:_transfer() is used to transfer funds from One Account to other, It has the following implementation

if ( _from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
ICapitalPool(_capitalPoolAddr).approve(address(this));
}
_safe_transfer_from(_token, _from, _to, _amount);

Also before/after it is called we have code like this:

uint256 fromBalanceBef = IERC20(_token).balanceOf(_from);
uint256 toBalanceBef = IERC20(_token).balanceOf(_to);

and this

uint256 fromBalanceAft = IERC20(_token).balanceOf(_from);
uint256 toBalanceAft = IERC20(_token).balanceOf(_to);

This code does not account for tokens that have a fee-on-transfer or a rebasing (token balance going up/down without transfers) mechanisms. By caching (or removing) the amount given to the transfer or transferFrom methods of the ERC20 token, this implies that this will be the actual received/sent out amount by the protocol and that it will be static, but that is not guaranteed to be the case. If fee-on-transfer tokens are used, on deposit action the actual received amount will be less, so withdrawing the same balance won't be possible. For rebasing tokens it is also possible that the contract's balance decreases over time, which will lead to the same problem as with the fee-on-transfer tokens, and if the balance increases then the reward will be stuck in the AssetManager contract.

Impact

High, as this can leave tokens stuck in the protocol

Tools Used

Manual

Recommendations

For fee-on-transfer tokens, check the balance before and after the transfer and validate it is the same as the amount argument provided. For rebasing tokens, when they go down in value, you should have a method to update the cached reserves accordingly, based on the balance held. This is a complex solution. For rebasing tokens, when they go up in value, you should add a method to actually transfer the excess tokens out of the protocol (possibly directly to users).

Updates

Lead Judging Commences

0xnevi Lead Judge 10 months 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.