Asset manager can abuse minimum portfolio holding amount functionality

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0x6aac67f00d2c3d7d70f3cc7e0a7b6978bb07ad16f5410014feb0e7fa7d956e00 Severity: medium

Description:

Impact

Permanent or temporary denial of service of Portfolio deposits

Description

The asset manager can decide to update the minimum portfolio token holding amount any time by calling the updateMinPortfolioTokenHoldingAmount() function. The problem is, he can abuse this functionality to set the minimum portfolio token holding amount to an unreachable number.

PortfolioSettings.sol - updateMinPortfolioTokenHoldingAmount()

  function updateMinPortfolioTokenHoldingAmount(
    uint256 _minPortfolioTokenHoldingAmount
  ) external onlyAssetManager {
    if (
      _minPortfolioTokenHoldingAmount <
      protocolConfig.minPortfolioTokenHoldingAmount()
    ) revert ErrorLibrary.InvalidMinPortfolioTokenHoldingAmount();

    minPortfolioTokenHoldingAmount = _minPortfolioTokenHoldingAmount;
    emit MinPortfolioTokenHoldingAmountUpdated(_minPortfolioTokenHoldingAmount);
  }

This is because there is only a lower limit and no upper limit enforced for the the minPortfolioTokenHoldingAmount of the ProtocolConfig.

Proof of Concept

1. Users deposit to a Portfolio vault
2. When a good amount of liquidity is gathered the asset manager raises updateMinPortfolioTokenHoldingAmount() to type(uint256).max
3. No user will be able to deposit now due to verifications during deposits in _getTokenAmountToMint()

Recommendation

Consider to introduce a variable maximumMinPortfolioTokenHoldingAmount in ProtocolConfig that is settable by protocol admins and enforce the minPortfolioTokenHoldingAmount can never be set above this by the asset manager.

[langnavina97]

Users can withdraw their existing funds at any time and can decide whether they want to invest after the asset manager increases the minimum token amount. @0xfuje

[0xfuje]

Thanks for the quick judging everywhere @langnavina97. True, it only affects deposits, and that doesn't have much of an impact here.