Possible to prevent triple deposits

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0x29f4b8880245c61f2acd22d93c9da5a02745d7cf7d76acdbac3a210c1cb03b7a Severity: medium

Description:

Impact

Denial of service for particular triples of the targeted user

Description

The depositTriple() function of EthMultiVault allows an user to deposit assets to a triple vault. However a malicious actor may prevent deposits (especially if large) if he spends generalConfig.minDeposit. This is because he can deposit to the triple's counter vault for the receiver who intends to deposit, which will make the initial user's deposit fail.

EthMultiVault.sol - depositTriple()

    function depositTriple(address receiver, uint256 id)
        external
        payable
        nonReentrant
        whenNotPaused
        returns (uint256)
    {
        if (!isTripleId(id)) {
            revert Errors.MultiVault_VaultNotTriple();
        }

        if (_hasCounterStake(id, receiver)) { 
            revert Errors.MultiVault_HasCounterStake();
        }

        if (msg.value < generalConfig.minDeposit) {
            revert Errors.MultiVault_MinimumDeposit();
        }

        uint256 protocolFees = protocolFeeAmount(msg.value, id);
        uint256 userDepositAfterProtocolFees = msg.value - protocolFees;

        // deposit eth into vault and mint shares for the receiver
        uint256 shares = _deposit(receiver, id, userDepositAfterProtocolFees);

        _transferFeesToProtocolVault(protocolFees);

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

        return shares;
    }

Attack scenario

1. User calls depositTriple() for himself 0x1 and id of 1 with 10 ETH
2. Attacker front-runs the call and deposits in the opposite vault of the id for the 0x1 user with the minimum deposit amount
3. User's TX executes and reverts because the _hasCounterStake() checker will fail

[0xfuje]

Recommendation and additional commentary on the issue will come in comments

[mihailo-maksa]

The reported issue concerning the potential to prevent deposits to a triple vault by exploiting the depositTriple function has been reviewed. Here is our detailed perspective:

Impact Assessment: While the issue highlights a denial-of-service (DoS) scenario for targeted users, it does not pose a security risk or result in the loss of funds. The affected user would still retain their assets and would effectively receive the minimum deposit amount from the attacker, leading to a minor inconvenience rather than a critical issue.

Severity Adjustment: Given that this issue does not lead to a security vulnerability or loss of funds, we consider it to be of low severity. The primary impact is on user experience, causing a failed transaction which can be retried by the user.

Mitigation Consideration: Since the reporter didn't add any additional commentary on the recommended fixes, in order to address this potential DoS scenario, we suggest implementing an approval process. This could involve:

• Introducing a mechanism where the receiver must approve the sender's deposit to the triple vault.
• If the sender and receiver are the same, no approval is needed.
• This approval process would only be required for triple deposits.

User Experience: The introduction of an approval process would enhance user control and prevent malicious actors from causing transaction failures. It ensures that only authorized deposits are accepted, thereby improving the overall user experience.

Conclusion: While the issue does introduce a minor DoS scenario, it does not pose significant risks and can be mitigated with the suggested approval process. Therefore, we consider this issue to be of low severity. We welcome feedback on our proposed fix to ensure it aligns with the expectations and needs of our users.

Status: This issue is accepted as low severity.

Suggested Fix:

// new global variable for triple deposit approvals
mapping(address receiver -> mapping(address sender -> bool approved)) public tripleDepositApprovals;

// a function to approve the triple deposit for a certain sender
function approveTripleDeposit(address sender) external {
    tripleDepositApprovals[msg.sender][sender] = true;
}

function depositTriple(address receiver, uint256 id)
    external
    payable
    nonReentrant
    whenNotPaused
    returns (uint256)
{
    if (!isTripleId(id)) {
        revert Errors.MultiVault_VaultNotTriple();
    }

    if (_hasCounterStake(id, receiver)) { 
        revert Errors.MultiVault_HasCounterStake();
    }

    // Implement approval check
    if (msg.sender != receiver) {
        require(tripleDepositApprovals[receiver][msg.sender], "Deposit not approved by receiver");
    }

    if (msg.value < generalConfig.minDeposit) {
        revert Errors.MultiVault_MinimumDeposit();
    }

    uint256 protocolFees = protocolFeeAmount(msg.value, id);
    uint256 userDepositAfterProtocolFees = msg.value - protocolFees;

    // Deposit eth into vault and mint shares for the receiver
    uint256 shares = _deposit(receiver, id, userDepositAfterProtocolFees);

    _transferFeesToProtocolVault(protocolFees);

    // Distribute atom shares for all 3 atoms that underly the triple
    uint256 atomDepositFraction = atomDepositFractionAmount(userDepositAfterProtocolFees, id);
    _depositAtomFraction(id, receiver, atomDepositFraction);

    return shares;
}