Some ETH amount can be leftover and locked in DepositQueue contract

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/RestakeManager.sol#L592-L616
https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Deposits/DepositQueue.sol#L123-L126
https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Deposits/DepositQueue.sol#L294-L306
https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Deposits/DepositQueue.sol#L187-L206
https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Deposits/DepositQueue.sol#L211-L250

Vulnerability details

Impact

Calling the following RestakeManager.depositETH function would call the DepositQueue.depositETHFromProtocol and DepositQueue._checkAndFillETHWithdrawBuffer functions below, which would fill the ETH withdraw buffer in the WithdrawQueue contract. When the ETH withdraw buffer becomes full, calling the RestakeManager.depositETH function would cause the part of msg.value that is not used for filling the ETH withdraw buffer to remain in the DepositQueue contract since bufferToFill would be 0 or less than msg.value when calling the DepositQueue._checkAndFillETHWithdrawBuffer function. Over time, the leftover ETH amount would accumulate in the DepositQueue contract.

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/RestakeManager.sol#L592-L616

    function depositETH(uint256 _referralId) public payable nonReentrant notPaused {
        ...

        // Deposit the remaining ETH into the DepositQueue
        depositQueue.depositETHFromProtocol{ value: msg.value }();

        ...
    }

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Deposits/DepositQueue.sol#L123-L126

    function depositETHFromProtocol() external payable onlyRestakeManager {
        _checkAndFillETHWithdrawBuffer(msg.value);
        ...
    }

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Deposits/DepositQueue.sol#L294-L306

    function _checkAndFillETHWithdrawBuffer(uint256 _amount) internal {
        // Check the withdraw buffer and fill if below buffer target
        uint256 bufferToFill = withdrawQueue.getBufferDeficit(IS_NATIVE);

        if (bufferToFill > 0) {
            bufferToFill = (_amount <= bufferToFill) ? _amount : bufferToFill;

            // fill withdraw buffer from received ETH
            withdrawQueue.fillEthWithdrawBuffer{ value: bufferToFill }();

            ...
        }
    }

When the leftover ETH amount does not accumulate to 32 ETH, calling the following DepositQueue.stakeEthFromQueue and DepositQueue.stakeEthFromQueueMulti functions would revert. When the leftover ETH amount accumulates to exceed 32 ETH, calling the DepositQueue.stakeEthFromQueue and DepositQueue.stakeEthFromQueueMulti functions would cause the surplus ETH amount after subtracting 32 ETH or multiples of 32 ETH to still be leftover. Since there is no guarantee that the leftover ETH amount would always accumulate to multiples of 32 ETH, it is likely that some ETH amount would remain in the DepositQueue contract.

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Deposits/DepositQueue.sol#L187-L206

    function stakeEthFromQueue(
        IOperatorDelegator operatorDelegator,
        bytes calldata pubkey,
        bytes calldata signature,
        bytes32 depositDataRoot
    ) external onlyNativeEthRestakeAdmin {
        uint256 gasBefore = gasleft();
        // Send the ETH and the params through to the restake manager
        restakeManager.stakeEthInOperatorDelegator{ value: 32 ether }(
            operatorDelegator,
            pubkey,
            signature,
            depositDataRoot
        );

        ...
    }

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Deposits/DepositQueue.sol#L211-L250

    function stakeEthFromQueueMulti(
        IOperatorDelegator[] calldata operatorDelegators,
        bytes[] calldata pubkeys,
        bytes[] calldata signatures,
        bytes32[] calldata depositDataRoots
    ) external onlyNativeEthRestakeAdmin nonReentrant {
        ...

        // Iterate through the arrays and stake each one
        uint256 arrayLength = operatorDelegators.length;
        for (uint256 i = 0; i < arrayLength; ) {
            // Send the ETH and the params through to the restake manager
            restakeManager.stakeEthInOperatorDelegator{ value: 32 ether }(
                operatorDelegators[i],
                pubkeys[i],
                signatures[i],
                depositDataRoots[i]
            );

            ...

            unchecked {
                ++i;
            }
        }

        ...
    }

Besides the DepositQueue.depositETHFromProtocol function, the following DepositQueue.forwardFullWithdrawalETH and DepositQueue.receive functions also call the DepositQueue._checkAndFillETHWithdrawBuffer function for filling the ETH withdraw buffer. However, these DepositQueue.depositETHFromProtocol, DepositQueue.forwardFullWithdrawalETH, and DepositQueue.receive functions can only fill the ETH withdraw buffer with all or part of msg.value. Hence, there is no way to use these leftover ETH of the DepositQueue contract for filling the ETH withdraw buffer, and such leftover ETH would continue to remain and be locked in the DepositQueue contract. While some ETH amount is leftover and locked in the DepositQueue contract, another consequence is that after some depositors have withdrawn some of the available ETH amount to withdraw, for an unpredictable duration, which can be very long, when no or an insufficient ETH amount is deposited through the restake manager, is withdrawn from an operator delegator, and is sent to the DepositQueue contract from outside the protocol, at least one depositor would be blocked from withdrawing all of her or his entitled ETH amount using her or his ezETH because the ETH withdraw buffer is not enough to cover her or his full ETH withdrawal.

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Deposits/DepositQueue.sol#L152-L156

    function forwardFullWithdrawalETH() external payable nonReentrant {
        // Check and fill ETH withdraw buffer if required
        _checkAndFillETHWithdrawBuffer(msg.value);
        ...
    }

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Deposits/DepositQueue.sol#L163-L183

    receive() external payable nonReentrant {
        uint256 feeAmount = 0;
        // Take protocol cut of rewards if enabled
        if (feeAddress != address(0x0) && feeBasisPoints > 0) {
            feeAmount = (msg.value * feeBasisPoints) / 10000;
            (bool success, ) = feeAddress.call{ value: feeAmount }("");
            if (!success) revert TransferFailed();

            ...
        }
        // update remaining rewards
        uint256 remainingRewards = msg.value - feeAmount;
        // Check and fill ETH withdraw buffer if required
        _checkAndFillETHWithdrawBuffer(remainingRewards);

        // Add to the total earned
        totalEarned[address(0x0)] = totalEarned[address(0x0)] + remainingRewards;

        ...
    }

Proof of Concept

Please add the following test file. Using forge, this test_ethCanBeLeftoverInDepositQueue test will pass to demonstrate the described scenario.

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.19;

import { Test } from "forge-std/Test.sol"; // solhint-disable-line no-unused-import

// mock contract for RestakeManager
contract RestakeManagerMock {
    // mock function for stakeEthInOperatorDelegator
    function stakeEthInOperatorDelegatorMock() external payable {}
}

// mock contract for WithdrawQueue
contract WithdrawQueueMock {
    // simulate withdrawalBufferTarget and claimReserve
    uint256 withdrawalBufferTarget = 5 ether;
    uint256 claimReserve;

    // mock function for fillEthWithdrawBuffer
    function fillEthWithdrawBufferMock() external payable {}

    // mock function for getBufferDeficit
    function getBufferDeficitMock() public view returns (uint256) {
        uint256 availableToWithdraw = getAvailableToWithdrawMock();
        return
            withdrawalBufferTarget > availableToWithdraw
                ? withdrawalBufferTarget - availableToWithdraw
                : 0;
    }

    // mock function for getAvailableToWithdraw
    function getAvailableToWithdrawMock() public view returns (uint256) {
        return address(this).balance - claimReserve;
    }

    // simulate withdraw function call for withdrawing all of available ETH amount to withdraw
    //   given that withdrawAll would only be called once in this test
    function withdrawAll() public {
        claimReserve += address(this).balance;
    }
}

// mock contract for DepositQueue
contract DepositQueueMock {
    // use mock contracts for WithdrawQueue and RestakeManager
    WithdrawQueueMock public withdrawQueueMock = new WithdrawQueueMock();
    RestakeManagerMock restakeManagerMock = new RestakeManagerMock();

    // mock function for depositETHFromProtocol
    function depositETHFromProtocolMock() external payable {
        _checkAndFillETHWithdrawBufferMock(msg.value);
    }

    // mock function for _checkAndFillETHWithdrawBuffer
    function _checkAndFillETHWithdrawBufferMock(uint256 _amount) internal {
        // Check the withdraw buffer and fill if below buffer target
        uint256 bufferToFill = withdrawQueueMock.getBufferDeficitMock();

        if (bufferToFill > 0) {
            bufferToFill = (_amount <= bufferToFill) ? _amount : bufferToFill;

            // fill withdraw buffer from received ETH
            withdrawQueueMock.fillEthWithdrawBufferMock{ value: bufferToFill }();
        }
    }

    // mock function for stakeEthFromQueue
    function stakeEthFromQueueMock() external {
        // Send the ETH and the params through to the restake manager
        restakeManagerMock.stakeEthInOperatorDelegatorMock{ value: 32 ether }();
    }
}

contract C4Test7 is Test {
    function test_ethCanBeLeftoverInDepositQueue() external {
        DepositQueueMock depositQueueMock = new DepositQueueMock();

        address Alice = address(6);
        address Bob = address(7);

        deal(Alice, 100 ether);
        deal(Bob, 100 ether);

        // Alice deposits 5 ETH
        vm.prank(Alice);
        depositQueueMock.depositETHFromProtocolMock{value: 5 ether}();

        // Alice's deposit fully filled Eth withdraw buffer
        //   so withdraw queue contract has 5 ETH and deposit queue contract has 0 ETH at this moment
        assertEq(address(depositQueueMock.withdrawQueueMock()).balance, 5 ether);
        assertEq(address(depositQueueMock).balance, 0);

        // Bob deposits 5 ETH
        vm.prank(Bob);
        depositQueueMock.depositETHFromProtocolMock{value: 5 ether}();

        // Bob's deposit did not increase ETH withdraw buffer anymore because ETH withdraw buffer is full already
        //   so withdraw queue contract still has 5 ETH and deposit queue contract's ETH amount is increased to 5 ETH at this moment
        assertEq(address(depositQueueMock.withdrawQueueMock()).balance, 5 ether);
        assertEq(address(depositQueueMock).balance, 5 ether);

        // Alice withdraws all of available ETH amount to withdraw, which clears ETH withdraw buffer
        vm.prank(Alice);
        depositQueueMock.withdrawQueueMock().withdrawAll();

        // Bob deposits 5 ETH again
        vm.prank(Bob);
        depositQueueMock.depositETHFromProtocolMock{value: 5 ether}();

        // Bob's new deposit fully filled ETH withdraw buffer again
        //   so withdraw queue contract's ETH amount is increased to 10 ETH and deposit queue contract still has 5 ETH at this moment
        assertEq(address(depositQueueMock.withdrawQueueMock()).balance, 10 ether);
        assertEq(address(depositQueueMock).balance, 5 ether);

        // Alice deposits 5 ETH again
        vm.prank(Alice);
        depositQueueMock.depositETHFromProtocolMock{value: 5 ether}();

        // Alice's new deposit did not increase ETH withdraw buffer anymore because ETH withdraw buffer is again full
        //   so withdraw queue contract still has 10 ETH and deposit queue contract's ETH amount is increased to 10 ETH at this moment
        assertEq(address(depositQueueMock.withdrawQueueMock()).balance, 10 ether);
        assertEq(address(depositQueueMock).balance, 10 ether);

        // since deposit queue contract's ETH amount is less than 32 ETH, these ETH amount cannot be staked
        vm.expectRevert();
        depositQueueMock.stakeEthFromQueueMock();

        // 10 ETH is leftover in deposit queue contract
        assertEq(address(depositQueueMock).balance, 10 ether);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

The DepositQueue contract can be updated to add a function, which is only callable by the trusted protocol admin, for transferring the DepositQueue contract's leftover ETH to fill the WithdrawQueue contract's ETH withdraw buffer.

Assessed type

ETH-Transfer

[jatinj615]

Expected Behaviour. The leftover ETH in deposit queue is not considered as bug in the protocol.

[alcueca]

Tbh, filling the WithdrawBuffer with the ETH balance of DepositQueue, instead of just with the last deposit, makes quite some sense.

It's not clear that the impact ot the protocol is significant, so downgrading to QA, but still it is a good idea.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a

[c4-judge]

alcueca marked the issue as grade-b

[rbserver]

Hi @alcueca, thanks for judging!

The following scenarios can occur in sequence for the issue described by this report.

1. WithdrawQueue.withdrawalBufferTarget for ETH is set to 30 ETH.
2. Alice deposits 30 ETH.
   • Alice's deposit fully fills the ETH withdraw buffer.
3. Bob deposits 30 ETH.
   • Bob's deposit increases the DepositQueue contract's ETH balance to 30 ETH because the ETH withdraw buffer has been filled.
4. Bob calls the WithdrawQueue.withdraw function to withdraw 30 ETH by sending all of his ezETH.
   • Bob's withdrawal clears the ETH withdraw buffer.
5. Alice cannot withdraw any of her deposit because the ETH withdraw buffer was cleared even though the protocol's DepositQueue contract does hold 30 ETH, which is less than the 32 ETH threshold for staking ETH.
   • If no user deposits anymore afterwards, the 30 ETH would be locked in the DepositQueue contract, and Alice loses the 30 ETH that is entitled to her.
   • If a user only deposits less than 30 ETH afterwards that partially fills the ETH withdraw buffer, Alice can only withdraw such deposited amount while the 30 ETH is still locked in the DepositQueue contract. Alice loses the difference, which she cannot withdraw, between 30 ETH and such deposited amount.
   • Comparing to Bob who successfully withdrew his 30 ETH deposit, this is unfair to Alice because she and Bob both deposited 30 ETH and she even deposited before Bob did.

Then, if continuing from Step 5:

6. Charlie deposits 30 ETH.
   • Charlie's deposit fully fills the ETH withdraw buffer.
7. Alice calls the WithdrawQueue.withdraw function to withdraw 30 ETH by sending all of her ezETH.
   • Alice's withdrawal clears the ETH withdraw buffer.
8. Charlie cannot withdraw any of his deposit because the ETH withdraw buffer was cleared again even though the protocol's DepositQueue contract still holds 30 ETH, which is less than the 32 ETH threshold for staking ETH.
   • In this case, Charlie faces the same issue that Alice faces in Step 5.
   • If no user deposits anymore afterwards, the 30 ETH would be locked in the DepositQueue contract, and Charlie loses the 30 ETH that is entitled to him.
   • If a user only deposits less than 30 ETH afterwards that partially fills the ETH withdraw buffer, Charlie can only withdraw such deposited amount while the 30 ETH is still locked in the DepositQueue contract. Charlie loses the difference, which he cannot withdraw, between 30 ETH and such deposited amount.

Furthermore, if continuing from Step 8:

9. Diana deposits 60 ETH.
   • Diana's deposit fully fills the ETH withdraw buffer and increases the DepositQueue contract's ETH balance from 30 ETH to 60 ETH.
10. Charlie calls the WithdrawQueue.withdraw function to withdraw 30 ETH by sending all of his ezETH.
    • Charlie's withdrawal clears the ETH withdraw buffer.
11. The DepositQueue.stakeEthFromQueue function is called to stake 32 ETH.
    • This action decreases the DepositQueue contract's ETH balance from 60 ETH to 28 ETH.
12. The staked 32 ETH is unstaked from the operator delegators after some time.
    • This action fully fills the ETH withdraw buffer and increases the DepositQueue contract's ETH balance from 28 ETH to 30 ETH.
13. Diana can call the WithdrawQueue.withdraw function to only withdraw 30 ETH that is the ETH withdraw buffer amount and cannot withdraw the other 30 ETH even though the protocol's DepositQueue contract still holds such 30 ETH.
    • In this case, Diana faces the same issue that Alice faces in Step 5 or Charlie faces in Step 8.
    • If no user deposits anymore afterwards, the 30 ETH would be locked in the DepositQueue contract, and Diana loses the 30 ETH that is entitled to her.
    • If a user only deposits less than 30 ETH afterwards that partially fills the ETH withdraw buffer, Diana can only withdraw such deposited amount while the 30 ETH is still locked in the DepositQueue contract. Diana loses the difference, which she cannot withdraw, between 30 ETH and such deposited amount.

Since there is no guarantee that the leftover ETH amount in the DepositQueue contract would always accumulate to multiples of 32 ETH, it is very likely that some ETH amount would be leftover in the DepositQueue contract. Users, who have deposited amounts that contribute to such leftover ETH amount, would face the same issue that Alice, Charlie, or Diana faces in Step 5, 8, or 13 in which:

• if no subsequent deposit takes place, these users lose such deposited amounts of theirs;
• if the subsequent deposits are not enough to cover such deposited amounts of the these users, these users lose the differences between such deposited amounts and such subsequent deposits;
• if the subsequent deposits exceed such deposited amounts of these users, the subsequent depositors then have their deposited amounts contribute to the DepositQueue contract's leftover ETH amount and face the same issue.

Historically, C4 issues, such as code-423n4/2023-01-ondo-findings#265, and external issues, such as sherlock-audit/2024-03-zivoe-judging#113, that show the possibility of funds being leftover in a contract were considered as medium risk issues. Similarly, this report also demonstrates such possibility that is a leak value case, which fits in the medium risk's criteria stated in https://docs.code4rena.com/awarding/judging-criteria/severity-categorization#estimating-risk that is Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

For the reasons above, would this report be re-evaluated to be a medium risk issue?

[alcueca]

While I agree with the warden that the implementation is not great (a single buffer for deposits and withdrawals might make more sense), none of the scenarios presented is severe enough, exploitable enough, or sustained enough to warrant a medium rating. Under normal operations, the protocol will work without excessive complaint from the users. No funds will be lost either.