Fee on transfer tokens will not behave as expected

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-numoen/blob/main/src/core/Lendgine.sol#L99

Vulnerability details

Impact

In Numoen, it does not specifically restrict the type of ERC20 collateral used for borrowing.

If fee on transfer token(s) is/are entailed, it will specifically make mint() revert in Lendgine.sol when checking if balanceAfter < balanceBefore + collateral.

Proof of Concept

File: Lendgine.sol#L71-L102

  function mint(
    address to,
    uint256 collateral,
    bytes calldata data
  )
    external
    override
    nonReentrant
    returns (uint256 shares)
  {
    _accrueInterest();

    uint256 liquidity = convertCollateralToLiquidity(collateral);
    shares = convertLiquidityToShare(liquidity);

    if (collateral == 0 || liquidity == 0 || shares == 0) revert InputError();
    if (liquidity > totalLiquidity) revert CompleteUtilizationError();
    // next check is for the case when liquidity is borrowed but then was completely accrued
    if (totalSupply > 0 && totalLiquidityBorrowed == 0) revert CompleteUtilizationError();

    totalLiquidityBorrowed += liquidity;
    (uint256 amount0, uint256 amount1) = burn(to, liquidity);
    _mint(to, shares);

    uint256 balanceBefore = Balance.balance(token1);
    IMintCallback(msg.sender).mintCallback(collateral, amount0, amount1, liquidity, data);
    uint256 balanceAfter = Balance.balance(token1);

99:    if (balanceAfter < balanceBefore + collateral) revert InsufficientInputError();

    emit Mint(msg.sender, collateral, shares, liquidity, to);
  }

As can be seen from the code block above, line 99 is meant to be reverting when balanceAfter < balanceBefore + collateral. So in the case of deflationary tokens, the error is going to be thrown even though the token amount has been received due to the fee factor.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider:

1. whitelisting token0 and token1 ensuring no fee-on-transfer token is allowed when a new instance of a market is created using the factory, or
2. calculating the balance before and after the transfer of token1 (collateral), and use the difference between those two balances as the amount received rather than using the input amount collateral if deflationary token is going to be allowed in the protocol.

[c4-judge]

berndartmueller marked the issue as primary issue

[kyscott18]

Can you give an example of a deflationary token? Does this mean that the balance goes down w.r.t. time or w.r.t being transferred.

[berndartmueller]

Can you give an example of a deflationary token? Does this mean that the balance goes down w.r.t. time or w.r.t being transferred.

@kyscott18 With regard to being transferred.

https://github.com/d-xo/weird-erc20#fee-on-transfer is a great resource on this topic.

[berndartmueller]

This finding and its dupes show a valid issue that prevents the use of rebase/FoT tokens with the protocol. As there is no clear mention of the support of non-standard ERC-20 tokens in the Numoen docs or contest README, I consider Medium the appropriate severity.

[c4-judge]

berndartmueller marked the issue as satisfactory

[c4-judge]

berndartmueller marked the issue as selected for report

[kyscott18]

How is this different from https://github.com/Uniswap/v3-core/blob/main/contracts/UniswapV3Pool.sol#L486-L490 ? If it isn't any different, which I don't think it is, then we will just acknowledge this and be mindful of which token we allow people to list.

[berndartmueller]

@kyscott18

In this specific case of the mint(..) function, there is no difference to Uniswap. Both implementations do not work properly for this kind of rebase/FoT tokens. Uniswap V3 is built on a setup of assumptions (see here), excluding rebase tokens.

It becomes a bigger issue if the use of rebase tokens can influence the token balance accounting of other regular ERC-20 token pairs, which is not the case for Numoen.

One of the other submissions presents further instances in the code which are potentially affected by incorrect token balance accounting caused by rebase/FoT token -> https://github.com/code-423n4/2023-01-numoen-findings/issues/272

[kyscott18]

Okay, thanks for clarify, I think we should mark this as noted by the team because we want to use the same assumptions as uniswap in this case