An OperatorDelegator allocation limit can easily be exceeded by a very large arbitrarily amount

[c4-bot-1]

Lines of code

https://github.com/Renzo-Protocol/Contracts/blob/74a6c9963d4e9359c542901fdd2dd3f5a6864f16/contracts/RestakeManager.sol#L339-L370

Vulnerability details

Proof of Concept

NB: First, would be key to note that, this bug case was submitted here: https://github.com/code-423n4/2024-04-renzo-validation/issues/741 and didn't get through to sponsor review, cause the validator misunderstood the issue and thought it to be invalid. The scoping rules for this mitigation review, explicitly states that disputed issues are OOS, however this bug case never got to the Sponsors to review so it wasn't disputed by them which I assume confirms it as in-scope.

Now to dive into the issue, let's take a look at the RestakeManager's chooseOperatorDelegatorForDeposit() function here https://github.com/Renzo-Protocol/Contracts/blob/74a6c9963d4e9359c542901fdd2dd3f5a6864f16/contracts/RestakeManager.sol#L339-L370

    function chooseOperatorDelegatorForDeposit(
        uint256[] memory tvls,
        uint256 totalTVL
    ) public view returns (IOperatorDelegator) {
        // Ensure OperatorDelegator list is not empty
        if (operatorDelegators.length == 0) revert NotFound();

        // If there is only one operator delegator, return it
        if (operatorDelegators.length == 1) {
            return operatorDelegators[0];
        }

        // Otherwise, find the operator delegator with TVL below the threshold
        uint256 tvlLength = tvls.length;
        for (uint256 i = 0; i < tvlLength; ) {
            if (
                tvls[i] <
                (operatorDelegatorAllocations[operatorDelegators[i]] * totalTVL) /
                    BASIS_POINTS /
                    BASIS_POINTS
            ) {
                return operatorDelegators[i];
            }

            unchecked {
                ++i;
            }
        }

        // Default to the first operator delegator
        return operatorDelegators[0];
    }

We can see that this function picks the OperatorDelegator that's to be chosen for the deposit.

Now from the previous contest the trusted roles in the protocol have been listed, with one being the RESTAKE_MANAGER_ADMIN that could as well allocate a specific max tvl for an operatorDelegator, see https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/README.md#L343

| RESTAKE_MANAGER_ADMIN              | allows the admin to configure operatorDelegators(add/remove, allocations), supported collateral tokens and their respective TVL            |

We can consider this a subtle invariant, considering if a max allocation is placed on an operatorDelegator it shouldn't be able to hold more than this value, problem however is that with the way chooseOperatorDelegatorForDeposit has been coded, it could actually cause the deposit to exceed the operator delegator's allocation limit by an arbitrarily large amount.

This is because, when choosing the operatorDelegator the TVLs without adding the deposit are used for the calculation instead of TVLs as they would be after the deposit. So as long as the allocation is not 0 and does not previously exceed the limit, any amount will be deposited into the first operator delegator that was under its allocation before the new deposit.

A rough-worded POC:

• Say an operator delegator is added with a small allocation to limit the exposure and funds deposited into it, lets use random values and say the limit is 100.
• Now assume, there's been a previous deposit of 20.
• There is a request to deposit 100.
• If the chooseOperatorDelegatorForDeposit loop reaches the operator delegator, it will allow this deposit, which would then make the operator delegator more than it's allocation.
• Exarcebating the issue is the fact that the deposit could be of any size and still be accepted which immediately breaking the allocation limitation and safety mechanism, i.e in the rough example above, assume 90 was previously deposited, a new deposit of 1000 is going to be accepted, cause the tvl check does not take into account the value of what's to be deposited.
• Which ends up having the AVS now being over-allocated to, is insufficiently profitable, or puts staked funds in more risk than intended, causing potential loss of funds and yield.

Impact

When more than one operator delegator is used, the allocation limit is a safety mechanism that limits the exposure to a particular operator (and the AVSes it validates). Due to this issue, the allocation limit safety mechanism fails in limiting the exposure of the protocol since it can be arbitrarily surpassed. If the underlying AVS is not profitable enough, or justifies only a certain allocation due to risk management, this issue may cause loss of funds or loss of yield to ezETH holders.

When multiple operator delegators are utilized, the allocation limit serves as a safety measure to restrict exposure to a specific operator (and the AVSes it validates). However, this mechanism can fail because the allocation limit can be arbitrarily exceeded as explained in this report. Now if the underlying AVS is not sufficiently profitable or warrants only a certain allocation due to risk considerations, then this issue could lead to losses or reduced yields for ezETH holders.

Tools Used

• Manual Review
• Previous missed finding: https://github.com/code-423n4/2024-04-renzo-validation/issues/741

  Recommended Mitigation Steps

  As accurately suggested here, the tvl check should include the new deposit's value in the calculation to ensure the operator delegator never gets overallocated to, i.w

  
     // Determine which operator delegator to use
      IOperatorDelegator operatorDelegator = chooseOperatorDelegatorForDeposit(
          operatorDelegatorTVLs,

• totalTVL
• totalTVL,
• collateralTokenValue ); ..
  function chooseOperatorDelegatorForDeposit( uint256[] memory tvls,
• uint256 totalTVL
• uint256 totalTVL,
• uint256 depositValue ) public view returns (IOperatorDelegator) { ... if (
• tvls[i] <
• (operatorDelegatorAllocations[operatorDelegators[i]] * totalTVL) /
• tvls[i] + depositValue <=
• (operatorDelegatorAllocations[operatorDelegators[i]] * (totalTVL + depositValue)) / BASIS_POINTS / BASIS_POINTS ) { return operatorDelegators[i]; }

Assessed type

Context

[alcueca]

From the Guidelines:

Vulnerabilities submitted during the preceding audit should not be submitted as new HM issues during the mitigation review. They will be considered out of scope and ineligible for awards. If a warden feels an issue from the preceding audit was overlooked or undervalued, it is recommended to look into submitting it to the sponsor via other channels (e.g., a bug bounty program).

[c4-judge]

alcueca marked the issue as unsatisfactory: Out of scope