Tokens with a maximum transfer logic could cause accounting issues on minting portfolio shares

[hats-bug-reporter[bot]]

Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0x84da53824567246429629458bf0e7df20024abd583ace499d1fda300d74915f8 Severity: low

Description: Description\ Tokens with the special condition of doing a full balance transfer when transferring amount == type(uint256).max (most notably cUSDCv3) have the potential to brick accounting in edge-case portfolios or be weaponized by malicious managers.

Attack Scenario\ The problem occurs inside all types of deposits, be it in the bundle contracts or the vault manager, since the deposit value of type(uint256).max is a valid amount, a discrepency between the transferred amout of token and the specified amount parameter would allow for an extremely overinflated minting of portfolio shares.

function _handleTokenTransfer(
    address _from,
    uint256 amountLength,
    uint256[] calldata depositAmounts,
    address[] memory portfolioTokens,
    uint256[] memory tokenBalancesBefore,
    bool usePermit
  ) internal returns (uint256) {
    //Array to store deposited amouts of user
    uint256[] memory depositedAmounts = new uint256[](amountLength);

    // If the vault is empty, accept the deposits and return zero as the initial ratio
    if (totalSupply() == 0) {
      for (uint256 i; i < amountLength; i++) {
        uint256 depositAmount = depositAmounts[i];
        depositedAmounts[i] = depositAmount;
        if (depositAmount == 0) revert ErrorLibrary.AmountCannotBeZero();
        if (usePermit) {
          _transferToVaultWithPermit(_from, portfolioTokens[i], depositAmount);
        } else {
          _transferToVault(_from, portfolioTokens[i], depositAmount);
        }
      }
      return 0;
    }

    // Calculate the minimum ratio to maintain proportional token balances in the vault
    uint256 _minRatio = _getDepositToVaultBalanceRatio(
      depositAmounts[0],
      tokenBalancesBefore[0]
    );
    for (uint256 i = 1; i < amountLength; i++) {
      uint256 _currentRatio = _getDepositToVaultBalanceRatio(
        depositAmounts[i],
        tokenBalancesBefore[i]
      );
      _minRatio = MathUtils._min(_currentRatio, _minRatio);
    }

    uint256 transferAmount;
    uint256 _minRatioAfterTransfer = type(uint256).max;
    // Adjust token deposits to match the minimum ratio and update the vault balances
    for (uint256 i; i < amountLength; i++) {
      address token = portfolioTokens[i];
      uint256 tokenBalanceBefore = tokenBalancesBefore[i];
      transferAmount = (_minRatio * tokenBalanceBefore) / ONE_ETH_IN_WEI;
      depositedAmounts[i] = transferAmount;
      if (usePermit) {
        _transferToVaultWithPermit(_from, token, transferAmount);
      } else {
        _transferToVault(_from, token, transferAmount);
      }

      _minRatioAfterTransfer = _getMinDepositToVaultBalanceRatio(
        tokenBalanceBefore,
        _getTokenBalanceOf(token, vault),
        _minRatioAfterTransfer
      );
    }
    emit UserDepositedAmounts(depositedAmounts);
    return _minRatioAfterTransfer;
  }
}

As the minimum ratio above is calculated using the passed amount parameter, we can have the ratio be calculated with type(uint256).max, but the vault to only receive the balance of the sender which would be substancially lower. This is partially mitigated by using the minimum ratio since for multiple tokens we would have one with a lower amount than type(uint256).max. However in edge-case or intentionally made portfolios where cUSDCv3 or it's similiar tokens are currently the only token, such inflation can happen and impact the portfolio even if other tokens get added.

This can either be:

1. Accidental due to the manager not being aware of such functionality
2. Intentional - the manager creates the portfolio, mints the shares with the extreme inflation and then add more tokens to cover up his initial actions.

The scenario isn't so uncommon as cUSDCv3 and it's similiar tokens have a market cap of $62+m, so it's not a niche token and non-support for it is never stated. However it requires specific circumstances and malicious manager actions, which is why the marking is LOW

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Recommendation\ The simplest form of mitigation here is to sanity check the given deposit amounts and disallow the value type(uint256).max, since for any other ERC20s it would be impossible to pass it anyway.

[PlamenTSV]

Update: I want to add a few more exploit paths for portfolios containing such tokens and potentially pushing this to Medium severity.

As stated in the initial report, such tokens' transfer logic interprets type(uint256).max as the entire balance of the _from address, even if it is 0.

If a portfolio is created and even one of the tokens is such, like the most popular cUSDCv3, the first depositor would be able to bypass the if (depositAmount == 0) revert ErrorLibrary.AmountCannotBeZero(); check.

1. Be the first depositor and provide and any amounts, except for the cUSDCv3, for which you will make sure to have 0 balance of and pass type(uint256).max as the amount.
2. The if (depositAmount == 0) revert ErrorLibrary.AmountCannotBeZero(); clause will pass because the contract uses the provided amount parameter and not the funds actually received after the transfer
3. Since our balance is 0, but we exploit the weird behavior of the token, we manage to make a deposit of 0 but still minting portfolio shares, which means we successfully bricked the portfolio

Now no other users will be able to deposit, since the 0 balance of the cUSDCv3-like token would force the minimum ratio to always be 0. The only way to make the portfolio be non-insolvent is to forcefully transfer funds to it.

[langnavina97]

Could you please provide a Proof of Concept (POC)?

If the first depositor deposits a maximum amount or close to it, the subsequent transactions will most likely fail. There won't be any token amounts left, or the ratio will result in zero.

A critical issue arises when the first depositor deposits a substantial amount, and the second investor deposits a significantly higher amount. However, this scenario would require the second investor to own a very large portion of the deposited token. This could potentially lead to the withdrawal ratio of the first depositor becoming zero, allowing the second depositor to steal all the funds. This is most likely to happen with a self-created token where an individual can mint a very high amount to themselves.

@PlamenTSV

[PlamenTSV]

Could you please provide a Proof of Concept (POC)?

If the first depositor deposits a maximum amount or close to it, the subsequent transactions will most likely fail. There won't be any token amounts left, or the ratio will result in zero.

A critical issue arises when the first depositor deposits a substantial amount, and the second investor deposits a significantly higher amount. However, this scenario would require the second investor to own a very large portion of the deposited token. This could potentially lead to the withdrawal ratio of the first depositor becoming zero, allowing the second depositor to steal all the funds. This is most likely to happen with a self-created token where an individual can mint a very high amount to themselves.

@PlamenTSV

I am having issues on running the test suites due to yarn versions not allowing me to enable corepack. However I can assist in any questions you have. These specific ERC20s mechanism allow the depositAmount to be type(uint256).max, but transfer the user's balance which can be substantially lower. For e.g, like in my last comment, if the user has balanceOf = 0, but he attempts a transfer and specifies depositAmount = type(uint256).max, he can brick the contract by forcing subsequent deposits to round to 0. The scenario you describe is completely possible as well, since the user does not need to have an actual large balance of the token.

I will provide additional issues for context on these ERC20s for you to better understand their mechanism and potentially find another impact or exploit path I have missed Codehawks Sherlock Hats - most recent