NoOne - Share Inflation Vulnerability in `_convertToSharesWithTotals` and `_convertToAssetsWithTotals`

[sherlock-admin3]

NoOne

High

Share Inflation Vulnerability in _convertToSharesWithTotals and _convertToAssetsWithTotals

Summary

The vulnerability is located in the CuratedVaultGetters.sol .sol contract within the _convertToSharesWithTotals and _convertToAssetsWithTotals functions. The issue arises when the first user deposits into the vault while newTotalAssets and totalSupply are both zero. In this scenario, the function incorrectly multiplies by 10 ** _decimalsOffset(), leading to an overinflated calculation of shares and assets.

Impact

• Over-Inflation of Shares: The first depositor receives an excessive number of shares, giving them a disproportionate claim on the vault's assets. This can destabilize the vault's share distribution and undermine the fairness of the system.

• Collateral Exploitation: The inflated shares can be used as collateral in other contracts, allowing users to borrow more assets than they should be entitled to. This could lead to significant financial loss and drain the vault's assets, as the over-inflated shares enable users to manipulate the system.

Code Snippet

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

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

Poc

uint8 public DECIMALS_OFFSET;
  using MathUpgradeable for uint256;

  function setUp() public {
    _setUpVault();

    // Assuming the underlying asset has 6 decimals like USDC
    DECIMALS_OFFSET = uint8(zeroFloorSub(18, 6));
  }

  function testSharesCalculation() public {
    uint256 assets = 1e18;
    uint256 totalSupply = 0; // Starting with 0 supply
    uint256 totalAssets = 0; // Starting with 0 assets

    uint256 expectedShares = assets;

    uint256 calculatedShares = _convertToSharesWithTotals(
      assets,
      totalSupply,
      totalAssets,
      MathUpgradeable.Rounding.Down
    );

    console.log("Expected Shares:", expectedShares);
    console.log("Calculated Shares:", calculatedShares);

    // Assert that the calculated shares match the expected value
    assertTrue(calculatedShares > expectedShares, "Calculated shares should be inflated due to DECIMALS_OFFSET");
  }

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

  function zeroFloorSub(uint256 x, uint256 y) internal pure returns (uint256 z) {
    assembly {
      z := mul(gt(x, y), sub(x, y))
    }
  }

Logs:
  Expected Shares: 1000000000000000000
  Calculated Shares: 1000000000000000000000000000000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 25.78ms (1.94ms CPU time)

write this test in CuratedVaultFactoryTest.sol and run with forge test --mt testSharesCalculation -vv

Recommendation

Change _convertToSharesWithTotals and _convertToAssetsWithTotals to

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

 function _convertToAssetsWithTotals(
    uint256 shares,
    uint256 newTotalSupply,
    uint256 newTotalAssets,
    MathUpgradeable.Rounding rounding
  ) internal view returns (uint256) {
    return shares.mulDiv(newTotalAssets + 1, newTotalSupply + 1, rounding);
  }

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

Honour commented:

Invalid: design choice.

[nevillehuang]

This is invalid, the computations are correct, and misses the root cause of incorrect decimal offset and lack of virtual shares that allows first depositor inflation attacks

[Jelev123]

Escalate, This is dup of #141