CollateralTracker::deposit() does not account for the maximum amount of shares that can be minted

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L417-L440

Vulnerability details

Description

The deposit() function can mint more shares than maxMint(). Aside from deviating the ERC4626 standard, it defeats the purpose of capping the shares to a limited number as calculated in maxMint().

• deposit()

  function deposit(uint256 assets, address receiver) external returns (uint256 shares) {
      if (assets > type(uint104).max) revert Errors.DepositTooLarge();
  
  @>  shares = previewDeposit(assets);
  
      // transfer assets (underlying token funds) from the user/the LP to the PanopticPool
      // in return for the shares to be minted
      SafeTransferLib.safeTransferFrom(
          s_underlyingToken,
          msg.sender,
          address(s_panopticPool),
          assets
      );
  
      // mint collateral shares of the Panoptic Pool funds (this ERC20 token)
      _mint(receiver, shares);
  
      // update tracked asset balance
      unchecked {
          s_poolAssets += uint128(assets);
      }
  
      emit Deposit(msg.sender, receiver, assets, shares);
      }

Even if there is a validation for depositing the maximum assets which is type(uint104).max, it does not prevent the minting of more shares than the maxMint().

Looking at the previewDeposit() function below (which is responsible for calculating the shares), the ratio of totalSupply and totalAssets() is in fact an "exchange rate" of assets to shares. When totalAssets() drops way below totalSupply, there is a possibility of minting shares more than its maximum limit. This scenario happens when the vault is gravely undercollateralized.

• previewDeposit()

  function previewDeposit(uint256 assets) public view returns (uint256 shares) {
      // compute the MEV tax, which is equal to a single payment of the commissionRate on the FINAL (post mev-tax) assets paid
      unchecked {
          shares = Math.mulDiv(
              assets * (DECIMALS - COMMISSION_FEE),
              totalSupply,
              totalAssets() * DECIMALS
          );
      }
  }

Impact

ERC4626 standard is aimed to create a consistent implementation of tokenized vaults. By not conforming to it, breaks composability.

Proof of Concept

Tools Used

Manual Review

Recommended Mitigation Steps

Add validation to the deposit() function and use maxMint() as the limit.

function deposit(uint256 assets, address receiver) external returns (uint256 shares) {
    if (assets > type(uint104).max) revert Errors.DepositTooLarge();

    shares = previewDeposit(assets);
+   if (shares > maxMint()) revert Errors.SharesBeyondMaxLimit();   
    ...
    // mint collateral shares of the Panoptic Pool funds (this ERC20 token)
    _mint(receiver, shares);
    ...
    emit Deposit(msg.sender, receiver, assets, shares);
}

Assessed type

ERC4626

[c4-judge]

Picodes marked the issue as duplicate of #553

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)