Minimum portfolio holding check does not amount for token transfers

[hats-bug-reporter[bot]]

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

Description: Description\ The VaultManager.sol's deposit and withdraw operations go over a series of sanity checks to make sure the parameters are correct. The withdraw action's checks inside _beforeWithdrawCheck check for the cooldown, pause of the contract, valid amount and that the user's balance after the withdraw does not go below a predefined minimum amount. However transfers of portfolio tokens between user records is completely allowed and they do not do this sanity check, leading to discrepencies and the possibility of users going under the minimum

Attack Scenario\ The check I mention inside _beforeWithdrawCheck is:

if (
      balanceAfterRedemption != 0 &&
      balanceAfterRedemption < protocolConfig().minPortfolioTokenHoldingAmount()
    ) {
      revert ErrorLibrary.CallerNeedToMaintainMinTokenAmount();
    }

leads to the assumption that user balances at all times should hold either 0 portfolio tokens or the minimum amount. However this check is not enforced on transfers of the Portfolio token

Both transfer checks inside PortfolioToken.sol:

function _beforeTokenTransfer(
    address from,
    address to,
    uint256 amount
  ) internal override {
    super._beforeTokenTransfer(from, to, amount);
    if (from == address(0) || to == address(0)) {
      return;
    }
    if (
      !(assetManagementConfig().transferableToPublic() ||
        (assetManagementConfig().transferable() &&
          assetManagementConfig().whitelistedUsers(to)))
    ) {
      revert ErrorLibrary.Transferprohibited();
    }
    _checkCoolDownPeriod(from);
  }

function _afterTokenTransfer(
    address _from,
    address _to,
    uint256 _amount
  ) internal override {
    super._afterTokenTransfer(_from, _to, _amount);
    if (_from == address(0)) {
      _updateUserRecord(_to, balanceOf(_to));
    } else if (_to == address(0)) {
      _updateUserRecord(_from, balanceOf(_from));
    } else {
      _updateUserRecord(_from, balanceOf(_from));
      _updateUserRecord(_to, balanceOf(_to));
    }
  }

do not include any checks that make sure the sender and receiver's new balances are within the allowed range, allowing users to unexpectedly go under the allowed threshold. An unsuspecting user may end up being unable to burn his shares.

As the impact is only on the user themselves, and the issue is unharmful but unaccounted for behavior - Low

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Recommendation\ Implement the respective check that the new balances are both above the minimum required holdings.

[langnavina97]

The check is not required for token transfers. We will provide a warning on the front-end but won't restrict the transfer amounts. @PlamenTSV