Inflation Attack is possible on CuratedVault

Summary

Attackers are able to execute a vault inflation attack on the curated vault to steal the first deposit

Vulnerability Detail

Before a new deposit is executed, the function _accrueFee() is called which basically goes through all the assossiated vaults and register the current balance of the vault in the supported pool. This new totalAssets is then used as the newTotalAssets in the curatedVault

function deposit(uint256 assets, address receiver) public override returns (uint256 shares) {
@>>    uint256 newTotalAssets = _accrueFee(); // NOTE: new total assets after accruing fees.

@>>    lastTotalAssets = newTotalAssets; 
    // Convert deposited assets to shares 
    shares = _convertToSharesWithTotals(assets, totalSupply(), newTotalAssets, MathUpgradeable.Rounding.Down);
    _deposit(_msgSender(), receiver, assets, shares);
}

  function _accruedFeeShares() internal view returns (uint256 feeShares, uint256 newTotalAssets) {
@>>    newTotalAssets = totalAssets();

    uint256 totalInterest = newTotalAssets.zeroFloorSub(lastTotalAssets);
    if (totalInterest != 0 && fee != 0) {
      // It is acknowledged that `feeAssets` may be rounded down to 0 if `totalInterest * fee < WAD`.
      uint256 feeAssets = totalInterest.mulDiv(fee, 1e18);
      // The fee assets is subtracted from the total assets in this calculation to compensate for the fact
      // that total assets is already increased by the total interest (including the fee assets).
      feeShares = _convertToSharesWithTotals(feeAssets, totalSupply(), newTotalAssets - feeAssets, MathUpgradeable.Rounding.Down);
    }

And also knowing that supplying assets to pools is permissionless (you don't need to be the position owner to deposit to it.) By depositing assets on the pool directly instead of the curated vault the attacker will be able to inflate the current total assets of the curated vault. Which could then be used along with rounding down to zero to execute the inflation attack on the vault.

  function supply(address asset, address to, uint256 amount, uint256 index, DataTypes.ExtraData memory data) public returns (DataTypes.SharesType memory) {
    return _supply(asset, amount, to.getPositionId(index), data);
  }

Proof Of Concept

The _convertToSharesWithTotals function:

function _convertToSharesWithTotals(uint256 assets, uint256 newTotalSupply, uint256 newTotalAssets, MathUpgradeable.Rounding rounding) internal view returns (uint256) {
    return assets.mulDiv(newTotalSupply + 10 ** _decimalsOffset(), newTotalAssets + 1, rounding);
}

Example Scenario: Attacker Alice Exploiting the First Depositor Bug

Let's consider a scenario where Alice, an attacker, exploits the inflation attack vulnerability on the curated vault. The scenario involves the following steps and calculations:

please note that the feeshares also can be skipped by the attacker , since max fee percentage is 50% , and calculating feeAssets rounds down , so feeShares will be 0 as well since : `feeAssets * 1 / (totalAsset - feeAsset) = 0 , for simplicity sake let's assume fee is 0 in interest accrual

Initial State:
- Total Assets = 0
- Total Supply = 0
- Decimals Offset = 0 (assuming 18 decimal places for the asset)
Alice see a deposit transaction of 10e18 from the first depositor ,so she Front Runs him to :
- deposits 1 wei to the curatedVault to mint 1 share: shares = 1 * (0 + 10^0) / (0 + 1) = 1 share
- Alice supply 100e18 directly into one of the pools of the curated Vault on behalf of the curated => when totalAssets() will be called in CuratedVaults it will be 100e18 +1
Victim tx now will execute as a deposit of 10e8 to the curated vault. he will recieve (shares = 10e18 * (1 + 1)/(100e18 +1 +1)) = 20e18/(100e18+1+1) = 0 shares
Alice (the attacker) now has 1 share that is equivalent to (10e18 + 100e18 + 1) 110e18 +1 underlying tokens and alice will lose all of its deposited token (she recieves 0 shares)

This scenario demonstrates how Alice's direct deposit to the pool can dilute the shares of the first depositor, leading to a loss of funds and attackers completely stealing the first deposit by using the rounding in favour of the protocol

Impact

by inflating the currant balance of the pool, the attackers are able to dilute the shares and steal the first depositor asset.

Code Snippet

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/vaults/CuratedVault.sol#L322-L330
Tool used

Manual Review

Recommendation

We recommend, adding a step in the pool deployment in the CuratedVaultFactory to either deposit some dead shares in order to make the donation attack unfeasible. Or to implement something like virtual shares as recomended by openzepplin

Duplicate of #141

sherlock-audit / 2024-06-new-scope-judging

A2-security - Inflation Attack is possible on CuratedVault #501