Unchecking passed value in `setAtomDepositFractionForTriple()` to feeDenominator

[hats-bug-reporter[bot]]

Github username: @Al-Qa-qa Twitter username: al_qa_qa Submission hash (on-chain): 0xf9e251f7efb47b738f17b5d90747e2f2b97025d00a6ca288b2f93481a97aa244 Severity: low

Description: Description\ There are two types of fees that can be taken when deploying, depositing or redeeming from vaults.

• Static Fees
• % of fees relative to the deposited/redeemed amount

When setting the % of fees, the team checks that they do not exceeds feeDenominator. this can be seen in setEntryFee(), setExitFee(), and setProtocolFee().

The problem lies in setAtomDepositFractionForTriple(), where there is no check for this value relative to the feeDenominator.

EthMultiVault.sol#L270-L272

    function setAtomDepositFractionForTriple(uint256 atomDepositFractionForTriple) external onlyAdmin {
        tripleConfig.atomDepositFractionForTriple = atomDepositFractionForTriple;
    }

This fees is a percentage fees taken from the amount deposited to TripleVaults, and goes to the Atom Vaults this TripleVault refers too.

EthMultiVault.sol#L1210-L1213

    function atomDepositFractionAmount(uint256 assets, uint256 id) public view returns (uint256) {
        uint256 feeAmount = isTripleId(id) ? _feeOnRaw(assets, tripleConfig.atomDepositFractionForTriple) : 0;
        return feeAmount;
    }

As we can see it is a % with respect to the total deposited. The problem will occur if there is no prevention from setting atomDepositFractionForTriple with value >= feeDenominator.

This will make the process revert because of underflow error, as fees is expected to be smaller than the real value.

Recommendations\

Check that the value do not exceeds to equal feeDenominator. And it can get capped to a given value like feeDenominator / 2 to something.

EthMultiVault::setAtomDepositFractionForTriple()

function setAtomDepositFractionForTriple(uint256 atomDepositFractionForTriple) external onlyAdmin {
+       require(atomDepositFractionForTriple < generalConfig.feeDenominator, "EthMultiVault: exceeds Denominator");
tripleConfig.atomDepositFractionForTriple = atomDepositFractionForTriple;
}

[mihailo-maksa]

The reported issue concerning the lack of a check for the value of atomDepositFractionForTriple relative to generalConfig.feeDenominator has been reviewed. Here is our detailed perspective:

Enhancement Suggestion: Adding a check to ensure that atomDepositFractionForTriple does not exceed feeDenominator is a useful enhancement. It would prevent the admin from setting an unreasonable value that could cause the transaction to revert.

Current Functionality: The atomDepositFractionAmount is not a fee. It represents the amount allocated for purchasing underlying atoms' shares when users deposit into triple vaults. The initial value is set in the deploy script (1500, or 15%), which is within a reasonable range. Even if the admin sets an unreasonable value, deposits would simply fail rather than causing any economic loss to the users.

Severity Assessment: Since this issue does not introduce any vulnerabilities or risks to user funds and is mainly about preventing administrative errors, it is classified as a low severity enhancement.

Conclusion: Adding a check to ensure that atomDepositFractionForTriple does not exceed feeDenominator is a nice-to-have safety feature but is not a security vulnerability. The current design ensures that no funds are lost, as deposits would fail if an unreasonable value is set.

Status: This issue is an enhancement.

Suggested Fix: The suggested fix is correct and can be accepted.

Extra Considerations:

• The atomDepositFractionAmount is intended for purchasing underlying atoms’ shares and is not a fee, so it does not directly affect user funds.
• The initial value set in the deploy script is reasonable (1500 or 15%).
• If the admin sets an unreasonable value, the transaction will fail, and no user funds will be lost.
• This check is a preventive measure to avoid administrative errors, ensuring smoother operation.

Comment for the Reporter: Thank you for highlighting this potential issue. While atomDepositFractionAmount is not a fee and poses no risk to user funds, adding an upper limit check is a useful enhancement to prevent administrative errors. This is considered an enhancement rather than a low severity vulnerability. We can still consider a lower payout for this valid suggestion.

[mihailo-maksa]

Added a low label, primarily due to the fact that some functions may get DOS'd if the atomDepositFraction is higher than generalConfig.feeDenominator, i.e. higher than 100%.