`depositTriple()` will get `DOS'ed` if `atomDepositFraction` is set to zero

[hats-bug-reporter[bot]]

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

Description: Description\ When making a new deposit to Triple Vaults, a part of that share goes to Atoms Vaults that is linked to that Triple Vault.

EthMultiVault.sol#L778-L779

    function depositTriple( ... ) ... {
        ...

        // distribute atom shares for all 3 atoms that underly the triple
        uint256 atomDepositFraction = atomDepositFractionAmount(userDepositAfterprotocolFee, id);
@>   _depositAtomFraction(id, receiver, atomDepositFraction);

        return shares;
    }

The problem lies is that the protocol is not checking if this value is 0 or not (initialized to be 0), which will make the transaction revert from _depositAtomFraction() in _deposit() internal function as sharesForReceiver will be 0 in that case.

EthMultiVault.sol#L924-L926

    function _deposit(address receiver, uint256 id, uint256 value) internal returns (uint256) {
        if (previewDeposit(value, id) == 0) {
            revert Errors.MultiVault_DepositOrWithdrawZeroShares();
        }
        ...
}

This edge case is handled correctly in _createTriple().

EthMultiVault.sol#L638-L644

        if (atomDepositFraction > 0) {
            _depositAtomFraction(
                id,
                msg.sender, // receiver
                atomDepositFraction
            );
        }

Recommendations\ Skip depositing if the atomDepositFraction is 0.

EthMultiVault.sol#L778-L779

uint256 atomDepositFraction = atomDepositFractionAmount(userDepositAfterprotocolFee, id);
-       _depositAtomFraction(id, receiver, atomDepositFraction);
+       if (atomDepositFraction > 0) {
+           _depositAtomFraction(id, receiver, atomDepositFraction);
+       }

[mihailo-maksa]

The reported issue concerning the potential for a DoS attack on the depositTriple function if atomDepositFraction is set to zero has been reviewed. Here is our detailed perspective:

Implementation Details: The current implementation of the depositTriple function does not explicitly check if atomDepositFraction is zero before calling _depositAtomFraction. This could theoretically lead to a transaction revert if atomDepositFraction is zero, as the _deposit function would throw an error for attempting to deposit zero shares.

Deploy Script Design: However, in our protocol's deploy script, atomDepositFraction is designed such that it will never be zero. This design ensures that the scenario described in the report cannot occur under normal operation.

Mitigation Consideration: Although this specific edge case is already handled by our deploy script, adding an explicit check to ensure atomDepositFraction is not zero can further safeguard the function and improve code robustness. This additional validation can be considered an enhancement to the existing implementation.

Severity Assessment: Given that the current deploy script prevents atomDepositFraction from being zero, the described issue does not pose an immediate risk. Therefore, it is classified as a low severity enhancement rather than a medium severity vulnerability.

Conclusion: The suggested check for atomDepositFraction being greater than zero is a valid enhancement to further ensure robustness. However, since our deploy script already prevents this issue, it does not qualify as a security vulnerability.

Status: This issue is an enhancement.

Suggested Fix: We do not agree with the suggested fix, as we do not want the deposits to triples to be prevented in this situation, and instead we would suggest that modifying the adminOnly function setAtomDepositFractionForTriple to not allow atomDepositFraction to be set to 0 is the way to go. Together with this, it should also be modified to not allow the depositAtomFraction to be higher than 100% (i.e. the value of generalConfig.feeDenominator).

Comment for the Reporter: Thank you for highlighting this potential issue. While our deploy script ensures that atomDepositFraction will never be zero, adding an explicit check can enhance code robustness. We consider this a low severity enhancement and can still consider a lower payout for this valid suggestion.

[Al-Qa-qa]

Hi @mihailo-maksa,

You are right. MEDIUM severity may be too large for this issue. I see that it lies as a LOW issue, even if the deployment script checks for it, setAtomDepositFractionForTriple() can be called anytime after deploying. which will make this edge case occuar.