Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential Asset Manager Fee Bypass #98

Open
olaoyesalem opened this issue Jul 3, 2024 · 4 comments
Open

Potential Asset Manager Fee Bypass #98

olaoyesalem opened this issue Jul 3, 2024 · 4 comments
Labels
invalid This doesn't seem right

Comments

@olaoyesalem
Copy link

olaoyesalem commented Jul 3, 2024

Vulnerability Description:

The _chargeProtocolAndManagementFees function in smart contract systemically calculates and mints fees for asset management and protocol fees. However, a critical vulnerability exists where the asset manager may potentially receive zero fees due to specific conditions in the fee calculation process.

https://github.com/Velvet-Capital/velvet-core/blob/849629b1aacf32d84634d8c4ef1378527bce3bb3/contracts/fee/FeeCalculations.sol#L128

https://github.com/Velvet-Capital/velvet-core/blob/849629b1aacf32d84634d8c4ef1378527bce3bb3/contracts/fee/FeeModule.sol

Vulnerability Details:

In the _calculateProtocolAndManagementFeesToMint function, the calculation for managementFeeToMint relies on the calculation of feeReceiverShare, which is derived from streamingFees * ONE_ETH_IN_WEI / _totalSupply. If _totalSupply is significantly greater than streamingFees * ONE_ETH_IN_WEI, feeReceiverShare can round down to zero, resulting in managementFeeToMint being zero.

Consequently, when _mintProtocolAndManagementFees is called in _chargeProtocolAndManagementFees, assetManagerFeeToMint can be zero, allowing the asset manager fee to be bypassed .

Severity Level: High

Explanation: This vulnerability can lead to a complete bypass of fees intended for the asset manager, undermining the expected revenue generation mechanism for the protocol and management fees. The impact could significantly affect the financial sustainability and operational integrity of the contract.

Recommendations:

Implement Minimum Fee Requirements: Consider implementing minimum fee requirements to ensure that even if the calculated fee is low, there's a base amount that must be paid to the asset manager to prevent fee bypassing.

@langnavina97 langnavina97 added the invalid This doesn't seem right label Jul 4, 2024
@langnavina97
Copy link

We do not mint small values. If a deposit is so small that the minting fee would be zero or negligible, no fees will be minted. @olaoyesalem

@olaoyesalem
Copy link
Author

olaoyesalem commented Jul 4, 2024

/**
 * @dev Mints both protocol and management fees based on their respective calculated amounts.
 * @param _assetManagerFeeToMint The amount of asset manager fees to mint.
 * @param _protocolFeeToMint The amount of protocol fees to mint.
 */
function _mintProtocolAndManagementFees(
  uint256 _assetManagerFeeToMint,
  uint256 _protocolFeeToMint
) internal {
  _mintFees(
    assetManagementConfig.assetManagerTreasury(),
    _assetManagerFeeToMint
  );
  _mintFees(protocolConfig.velvetTreasury(), _protocolFeeToMint);
}

The _mintProtocolAndManagementFees function mints both protocol and management fees based on the provided amounts. It calls the _mintFees function to handle the actual minting process.

/**
 * @dev Mints fees to a specified address if the amount is greater than the minimum fee threshold.
 * @param _to The address to mint the fees to.
 * @param _amount The amount of fees to mint.
 * @return The actual amount of fees minted.
 */
function _mintFees(address _to, uint256 _amount) internal returns (uint256) {
  if (_amount < MIN_MINT_FEE) return 0;

  portfolio.mintShares(_to, _amount);
  return _amount;
}

The _mintFees function checks if the amount is greater than the minimum fee threshold. If the amount is less than the threshold, it returns 0 without minting any fees. This behavior can lead to a successful transaction even when no fees are minted, effectively allowing an attacker to bypass the fee mechanism.

This vulnerability could allow an attacker to continuously execute transactions with amounts below the threshold, avoiding the minting of fees for the protocol and management. This can undermine the expected revenue generation for the protocol.

@langnavina97 @fonstack @

@kakarottosama
Copy link

@olaoyesalem your issues was submitted directly on github, not using Hats dapp.

According to rules, To participate, security researchers must submit their findings on-chain, and an automatic GitHub issue will be generated in the forked repository. , Submissions should be made using our Dapp, so, please submit it through dapp

@olaoyesalem
Copy link
Author

Thanks

@olaoyesalem your issues was submitted directly on github, not using Hats dapp.

According to rules, To participate, security researchers must submit their findings on-chain, and an automatic GitHub issue will be generated in the forked repository. , Submissions should be made using our Dapp, so, please submit it through dapp

Thanks I have done that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

3 participants