hats-finance / Velvet-Capital-0x0bb0c08fd9eeaf190064f4c66f11d18182961f77

Core smart contracts of Velvet Capital
Other
0 stars 1 forks source link

Potential Asset Manager Fee Bypass #105

Open hats-bug-reporter[bot] opened 4 months ago

hats-bug-reporter[bot] commented 4 months ago

Github username: @olaoyesalem Twitter username: salthegeek1 Submission hash (on-chain): 0x06cb50e8b225f95a4fb970af72e465348e897fe64bd972a364f96c0fa3c7a41c Severity: high

Description: Description\ The _mintFees function contains logic that can be exploited to bypass the fee mechanism. This vulnerability allows an attacker to avoid minting protocol and management fees by ensuring the fee amount is always below the minimum threshold (MIN_MINT_FEE). Consequently, the protocol and management may lose significant revenue, impacting financial sustainability.

Attack Scenario\


/**
 * @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;
}

This function checks if the fee amount is greater than the minimum fee threshold (MIN_MINT_FEE). If the amount is less than the threshold, it returns 0 without minting any fees. This behavior allows transactions to succeed even when no fees are minted, enabling attackers to bypass the fee mechanism.

An attacker submits transactions with fee amounts just below the MIN_MINT_FEE. Each transaction is processed successfully, but no fees are minted. The attacker repeats this process to bypass paying any fees, resulting in cumulative revenue loss for the protocol.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional) To mitigate this vulnerability, ensure that the _mintFees function has consistent logic and consider enforcing a revert mechanism if the fee amount is below the minimum threshold:

/**
 * @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) {
  require(_amount >= MIN_MINT_FEE, "Fee amount is below the minimum threshold");

  portfolio.mintShares(_to, _amount);
  return _amount;
}
langnavina97 commented 4 months ago

98

olaoyesalem commented 4 months ago

98


/**
 * @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 please check

langnavina97 commented 4 months ago

If we let the transaction fail, two users wouldn't be able to deposit in the same block. Additionally, we don't mint dust, so the transaction should not fail due to insufficient time passing, which would lead to minting dust. @olaoyesalem

olaoyesalem commented 4 months ago

But the transaction will still go through and nothing will be minted to the protocol . Is that agreed by you?

langnavina97 commented 4 months ago

Yes that's correct. As it should be.

olaoyesalem commented 4 months ago

The function won't mint at all. So it won't mint dust. But the user's txn will still go through. Without minting anything at all to the protocol. That's what I'm pointing out.

langnavina97 commented 4 months ago

Yes correct, it should not mint if there is only a dust value.

olaoyesalem commented 4 months ago

Yes correct, it should not mint if there is only a dust value.

So the Vulnerability here is that the user txn will go through even without paying the protocol fees.

langnavina97 commented 4 months ago

We are aware of this, it's a decision we've made. Not a vulnerability.

olaoyesalem commented 4 months ago

Waoh

olaoyesalem commented 4 months ago

@fonstack

olaoyesalem commented 4 months ago

@langnavina97 I would like to as if it was by design ?

olaoyesalem commented 4 months ago

If accepted as a Vulnerability. I can Add recommendations

olaoyesalem commented 4 months ago

We are aware of this, it's a decision we've made. Not a vulnerability.

Also if it is a design why is the fee there

olaoyesalem commented 4 months ago

@fonstack @langnavina97

Please check this