Lack of access controls on `_chargeProtocolAndManagementFees()` allows an attacker to disrupt whole protocol tokenomics, and cause inflation, rounding issues and permanent locking of user deposits

[hats-bug-reporter[bot]]

Github username: @burhankhaja Twitter username: imaybeghost Submission hash (on-chain): 0xc9ff27d7dff353b4d936f4853610cedaec523dae9f03cda55820541bfd3741b8 Severity: high

Description: Description\ _chargeProtocolAndManagementFees() in FeeModule.sol charges protocol and management fees by minting precisely calculated portfolio tokens to assetManagerTreasury() and velvetTreasury()

Note that portfolio tokens (shares) mainly represent underlying portfolio token deposits of users in the portfolio.

So everytime _chargeProtocolAndManagementFees() is called the totalsupply() of Portfolio tokens (shares) increases. link

/**@auditor Notice this call trace: */
// _chargeProtocolAndManagementFees() -->_mintProtocolAndManagementFees() --> _mintFees() --> portfolio.mintShares() 
  function mintShares(address _to, uint256 _amount) external onlyMinter {
        _mint(_to, _amount);
    }

Since There are no access controls that prevent who can call this function,

Malicious actor can call this repeatedly using bots in different blocks which inflate the totalSupply() of portfolio tokens and break all the protocol calculations that depend on totalSupply()

Remember, this function can't be called in the same block due to this check

/**@auditor : notice this calltrace
** _chargeProtocolAndManagementFees --> _calculateProtocolAndManagementFeesToMint() ---> _calculateMintAmountForStreamingFees()
**/
   function _calculateMintAmountForStreamingFees(
        uint256 _totalSupply,
        uint256 _lastChargedTime,
        uint256 _feePercentage,
        uint256 _currentTime
    ) internal pure returns (uint256 tokensToMint) {
        if (_lastChargedTime >= _currentTime) {
            return 0;
        }
  ...
  ...
}

But nothing prevents an attacker to repeatedly call this in different blocks. And that breaks the whole tokenomic invariants and calculations depending on totalSupply() variable.

Unfortunately, besides the withdrawal burn function, there is no extra burn function controlled by priviledged roles that could have been helpful in similar situations, Even if it was Implemented, nothing stops the attacker from Re-Attacking the system.

Attack Scenario\ There are dozens of ways an attacker can abuse this unprotected functionality:

• Inflating portfolio token supply, which is the root cause of all the exploits
• causing DOS on withdrawl functions, since _multiTokenWithdrawal() calculates tokenbalance by tokenBalance = (tokenBalance * _portfolioTokenAmount) / totalSupplyPortfolio; and reverts if (tokenBalance == 0) . since we know that the attacker can inflate the totalSupply(), all an attacker has to do is to keep calling _chargeProtocolAndManagementFees() until totalSupply() surpasses (tokenBalance * _portfolioTokenAmount) such that the statement returns 0 and triggers WithdrawalAmountIsSmall() revert.
  • therefore users won't be able to withdraw their funds from the vault
• Remember if the user deposited after the attacker had inflated totalsupply(), in that case even if underlying portfolio token are removed by rebalancing contract , still user won't be able to receive his tokens via claimRemovedTokens() because in the attempclaim() calculations will also return 0 as the inflated totalsupply is recorded in user's snapshot

//@auditor : calltrace := claimRemovedTokens() --> attemptClaim()
   function attemptClaim(
        address _removedToken,
        address _user,
        uint256 _portfolioTokenBalance,
        uint256 _balanceAtRemoval,
        uint256 _totalSupply
    ) private {
        if (_portfolioTokenBalance > 0) {
            // Calculate the user's share of the removed token
            uint256 balance = (_balanceAtRemoval * _portfolioTokenBalance) /
                _totalSupply;
            // Transfer the calculated share to the user
            TransferHelper.safeTransfer(_removedToken, _user, balance);
        }
    }

• Therefore, user deposits are permanently lost and locked either in vault or in tokenExclusiveManager

• Similarly, attacker can inflate his deposits as the similar calculations happen while minting that depend on totalSupply()

• In short, The attacker can disrupt whole Velvet protocol from head to toe and break the core tokenomic invariants.

Recommendation\ since only Feemanager calls this function which is ultimately inherited by Portfolio contract, therefore allow only portfolio contract to call it.

create a modifer that only allows calls from portfolio contract.

- function _chargeProtocolAndManagementFees() external nonReentrant {
+ function _chargeProtocolAndManagementFees() external nonReentrant onlyPortfolioContract {
    uint256 _managementFee = assetManagementConfig.managementFee();
    uint256 _protocolFee = protocolConfig.protocolFee();
    uint256 _protocolStreamingFee = protocolConfig.protocolStreamingFee();
    uint256 _totalSupply 
.....
.....
.....
}

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

[langnavina97]

Our fees are streaming fees, charged per second by minting a fair amount of tokens representing the fee share. @burhankhaja

[burhankhaja]

Im sorry but i dont think u have read the report,

No matter what the fee structure is. Fee module has external function _chargeProtocolAndManagermentFee() without any access controls that mints the same portfoliotokens to the treasury that are also minted on user deposits

And the attacker can abuse this, by repeatingly calling this function in order to inflate totalSupply() and cause rounding issues

This is a High severity issue that can badly damage velvet protocol, kindly take it seriously

[langnavina97]

Please provide us a PoC @burhankhaja

[burhankhaja]

can i do it at the end of contest? till then, can u please remove invalid label, it hurts feelings :smile:

[langnavina97]

We would appreciate if you could provide the PoC asap. If you can convince us it's a valid issue, we'll remove the invalid label. @burhankhaja

There is a check here and fee is minted per second.

https://github.com/hats-finance/Velvet-Capital-0x0bb0c08fd9eeaf190064f4c66f11d18182961f77/blob/aa47c9ff85bcc2bede62978c3895668b549da125/contracts/fee/FeeCalculations.sol#L99-L100

[burhankhaja]

since _currentTime is always equal to block.timestamp and _lastChargeTime is always equal to lastChargedManagementFee which is updated in the same vulnerable function via _setLastFeeCharged() and all the _setLastFeeCharged() does is it sets lastChargedManagementFee to the current block.timestamp

Now coming back to the check you mentioned, first of all i already mentioned in my report that attacker can't perform this attack in single block because of this check but i also mentioned that nothing prevents him from attacking in different blocks

since in the different blocks the currentTime {block.timestamp} will always be greater than lastChargedManagementFee {block.timestamp - 1 }, as a result this check is bypassed, there is no other timelocking mechanisms applied that prevents it.

Thanks for being responsive, i will add POC at the end of the contest

[burhankhaja]

is there any other confusions, please point out i want to give you clear details.

i am working on other high severity reports for velvet that is why i can't add full fledged POC right now, as it is too complex protocol and second of all how do i simulate txns in different blocks in foundry (i have to figure that part out also :smiling_face_with_tear: )

[kakarottosama]

I think submitter missing the point of sponsor comments about streaming fee

[burhankhaja]

@kakarottosama brother, can u please explain a bit?

[kakarottosama]

@burhankhaja streaming fee is accrual, every second it will accrue but not transferred yet (or minted yet) until it being triggered. when the _chargeProtocolAndManagementFees is being called (by anyone), it will then apply the accrued fees by minting it to the fee receiver.

As you already know, totalSupply will increase after there is a mint. Therefore, it's normal this totalSupply increased everytime there is a call to _chargeProtocolAndManagementFees (in assumption last charged time is differ than current time). if the diff between last charged time and current time is about 2 seconds, the totalSupply will increase proportionally as streaming fee calculated (small time diff resulting a tiny fractional increment of totalSupply).

charging fee 10 times every 2 seconds resulting equal increment amount of totalSupply compare to charging fee after 20 seconds. So, the totalSupply increase is considerable.

[burhankhaja]

which function in the protocol updates streaming fee after every second?

[burhankhaja]

@langnavina97 @kakarottosama please wait for the end of the contest, i will be adding POC here

[kakarottosama]

@burhankhaja

  function _calculateStreamingFee(
    uint256 _totalSupply,
    uint256 _lastChargedTime,
    uint256 _feePercentage,
    uint256 _currentTime
  ) internal pure returns (uint256 streamingFee) {
    uint256 timeElapsed = _currentTime - _lastChargedTime;
    streamingFee =
      (_totalSupply * _feePercentage * timeElapsed) /
      ONE_YEAR_IN_SECONDS /
      TOTAL_WEIGHT;
  }

[burhankhaja]

@langnavina97 @kakarottosama Finally, Here is the POC, i told you this is a high severity issue confirmed

• Remember this part
• Here are the instructions for the setup
• And here are the attack instructions

Finally, Here is the secret gist that you need to copy and paste inside test/POC.sol

[kakarottosama]

I didn't run the POC, but skimming your code, there is a flaw:

1. Alice deposit right after initial deposit, resulting 9500000000000000
2. Alice deposit after attack, which is warp-ing into the future and increase totalSupply through _chargeProtocolAndManagementFees mints (no deposits), resulting different than 9500000000000000

Comparing the first and second Alice balance is not correct, and it will NORMALLY resulting different result. Why? The attack phase you mentioned, is actually minting fees thus increasing totalSupply (which is normal in streaming fee). As totalSupply grows, the ratio of deposit to portfolio token also increase. Thus when Alice deposit (equal amount like the first test), it will get more portfolio token, BUT the share percentage is still the same.

So, actually Alice share is still same, in both Normal and afterAttack. Just check the totalSupply and calculate Alice share percentage in normal and afterAttack. (or maybe there will be a slightly lower share percentage, since shares goes to assetManager and protocol)

I respect your fighting spirit, but Sponsor told you this is invalid I even add some comments but you ignore it without trying to actually understand it. I wont respond to any of your argument after this, since it's useless.

---

I don't know if you read above paragraphs @burhankhaja, but in short, INVALID.

[langnavina97]

Thank you for providing the PoC @burhankhaja

But as @kakarottosama already pointed out there is no impact on the users share except for deducting the fees.