The Standard

Summary

Many issues with non-stadard erc-20 tokens

Vulnerability Details

1. Tokens with fee-on-transfer will lead to incorrect rewards
   https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L227-L232

First we give a holder X rewards, then we transfer X - fee rewards on LiquidationPool. Let's say for a simplified example fee is 10%, 10 holders with equal stakes. 10X rewards are given, but the LiquidationPool only has 9X because of fees.
Then we try to transfer, 9 first holders will be able to withdraw, the last one won't. Because there are not enough funds on the LiquidationPool
https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L168-L175

2. Tokens with fee-on-transfer may lead to bad debt
   Depending on how big is the fee-on-transfer and how big is collateralRate it's possible that on liquidation stackers will receive less rewards than burned EUROs. Because on liquidation we first transfer from a vault to LiquidationPoolManager, then to LiquidationPool, then stackers will be able to withdraw. So 3 transfers. If fee is 3.5% and collateralRate is 110% stackers' losses will be 0.5%.

3. Rebasing or deflationary tokens can change the LiquidationPool balance, so internal rewards sum will be more than actual ERC20's balance. The same results as in point 1 in this issue, some stackers won't be able to get rewards

4. Tokens with >18 decimals will lock liquidation, underflow
   https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L220

The same in PriceCalculator.getTokenScaleDiff, but it's out of scope
5. ERC777 will be able to reenter, remove holders and deny rewards
E.g. array of holders = [a, b, c, d]
We are in for (uint256 j = 0; j < holders.length; j++) { loop
j = 2
Attacker removes c from holders on safeTransferFrom
https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L232

Next loop j = 3, 3 == holders.length, d is skipped

6. ERC777 will be able to call decreasePosition on safeTransferFrom, but it will be restored after the callback
   https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L232

Here an attacker can call decreasePosition in safeTransferFrom, leave only 1 wei of either TST or EUR, but then it will be restored on line 237. So the attacker made a withdrawal of TST and EUR but the balance on LiquidationPool stays the same
https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L226-L237

7. Rebasing or deflationary tokens can change the balance significantly in one transaction, which can lead to sudden bad debt. E.g. we had collateral of 2x, it suddenly drop to 1x because of rebasing. We should also take into account that if the price of asset does change accordingly, e.g. 2x before rebase cost the same as 1x rebase after the oracle feed may not be updated immediately. It may lead to a Vault marked as undercollateralised even so it's not, we just have not received a price update

8. Pausable tokens can create bad debt because we can not do transfers => can't liquidate

9. Some tokens revert on 0 transfers https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers
   https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L219-L232 . It's possible to have a small _positionStake, e.g. 1wei, which will lead to 0 _portion, which will lead to safeTransferFrom of amount 0

Also if uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC(); is 0 (small amount and/or swapFeeRate = 0) in SmartVaultV3::swap then executeERC20SwapAndFee will fail in IERC20(_params.tokenIn).safeTransfer(ISmartVaultManagerV3(manager).protocol(), _swapFee);
10. If a token has multiple addresses an attacker can withdraw the collateral using SmartVaultV3::removeAsset, even if they have a debt, because of different addresses. See https://github.com/d-xo/weird-erc20#multiple-token-addresses

Impact

Depending on token used it can be loss of rewards, lock of liquidation => bad debt, etc.

Tools Used

Manual review

Recommended Mitigation Steps

Depends on the token type. The simplest one is to add only "normal" ERC20 tokens