Asset manager can be exempted paying fee on withdrawal by assigning their address as `assetManagerTreasury`

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x8e9a311a50998bc8e7adecc06c1d5cb4536ab172f3cea7f4e81ce516d12d85f5 Severity: medium

Description: Description

In Velvet, fee can be charged on Deposit and Withdrawal, via _chargeFees call. This fee being charged is for protocol and management fees. Protocol fee is for Velvet, and management fee is for Asset manager.

If we look at following _chargeFees implementation, the fee is exempted on internal transfer, which the address are assetManagerTreasury or velvetTreasury. Meaning, any Deposit or Withdrawal related with this address are not going to be charged.

  function _chargeFees(address _user) internal {
    // Check if the sender is not a treasury account to avoid charging fees on internal transfers.
    if (
      !(_user == assetManagementConfig().assetManagerTreasury() ||
        _user == protocolConfig().velvetTreasury())
    ) {
      // Invoke the fee module to charge both protocol and management fees.
      feeModule()._chargeProtocolAndManagementFees();
    }
  }

Asset manager treasury can't deposit due to _beforeDepositCheck, we may think this intended check which exist to prevent asset manager to abuse assetManagerTreasury as a way to skip fees.

  function _beforeDepositCheck(address _user, uint256 _tokensLength) internal {
    if (
      !(assetManagementConfig().publicPortfolio() ||
        assetManagementConfig().whitelistedUsers(_user)) ||
      _user == assetManagementConfig().assetManagerTreasury() ||    // assetManagerTreasury is forbidden
      _user == protocolConfig().velvetTreasury()
    ) {
      revert ErrorLibrary.UserNotAllowedToDeposit();
    }

But that's not effective, as Asset manager can easily update this asset manager treasury address.

  function updateAssetManagerTreasury(
    address _newAssetManagerTreasury
  ) external onlyAssetManager {
    // Checks that the new treasury address is not the zero address.
    if (_newAssetManagerTreasury == address(0))
      revert ErrorLibrary.InvalidAddress();

    // Updates the treasury address and emits an event with the new address.
    assetManagerTreasury = _newAssetManagerTreasury;
    emit TreasuryUpdated(_newAssetManagerTreasury);
  }

They can simply change the address, to escape from paying fee on withdrawal, and may revert back if needed.

This fee exemption is not going to be detected by public user, thus the asset manager didn't do any malicious activity to public / users. But, here protocol is being tricked, they are not having the fees they supposed to have.

Attack Scenario

Asset manager (example using an address 0xBEEF) buy portfolio token, when when they want to withdraw, they temporarily set the 0xBEEF as assetManagerTreasury, then revert back if necessary. Here asset manager being exempt from withdrawal fees (especially the protocol fee). Protocol lose potential fee from this malicious activity.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Recommendation

There might be some other way, but one way is, instead of sending portfolio token as fee distribution to assetManagerTreasury, why not swap it to a token (eq. USDC) first, then transfer. Then, assetManagerTreasury should be prevented to do any transfer of portfolio token by adding restriction from & to in _beforeTokenTransfer

[langnavina97]

Asset managers are receiving the fees, so there is no motivation for them to avoid paying the fees.

[chainNue]

As written, the fees are for Asset Manager AND Protocol (velvet), the motivation is to skip paying fees for Protocol. @langnavina97

[kakarottosama]

Asset manager doesn't pay fee to protocol, instead protocol will receive a minted share portion. But I agree there will be some potential loss of share for protocol when asset manager did this, but this is just a low issue as it only impact asset manager and the potential loss is small