chooseOperatorDelegatorForDeposit does not provide fair assets to OperatorDelegator

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L392

Vulnerability details

RestakeManager::deposit() is used for user to deposit an ERC20 collateral token into the protocol. THe function uses the RestakeManager::chooseOperatorDelegatorForDeposit which picks the Operator Delegator (i.e. OD) with the TVL below the threshold or returns the first one in the list. This function is crucial for each OD, because it affects the yield generated for each of them. If all the ODs has a higher threshold, the first OD will always be picked for depositing LSTs into Eigenlayer.

Example Operators Running: 1) P2P.org, currently has almost 269,327e18 restaked 2) Luganodes, currently has almost 269,237e18 restaked 3) Figment, currently has almost 269,088e18 restaked

If assuming that all the operators are above the threshold, constantly. The P2P.org will have an unfair advantage as the first OD for Renzo. Meaning that any LSTs staked with Renzo will be delegated and used to restake into Eigenlayer by P2P.org.

Impact

Other OperatorDelegators will not get it's fair share of assets to be used and restake into Eigenlayer. Causing them to lose rewards compared to the first OperatorDelegator.

Proof of Concept

    /// @dev Picks the OperatorDelegator with the TVL below the threshold or returns the first one in the list
    /// @return The OperatorDelegator to use
    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]; //@audit
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a logic to deposit using the OD with the lowest TVL among all other ODs.

Assessed type

Context

[DadeKuma]

Invalid, it works according to this comment:

/// @dev Picks the OperatorDelegator with the TVL below the threshold or returns the first one in the list

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L360

[DadeKuma]

@howlbot reject

[goheesheng]

It wasn't stated in known issues via README that this was by design. It is not fair for other OperatorDelegators because they will lose the staked yield due to lesser assets than the first Operator Delegator. This means that all ODs other than the first will LOSE yield.

[alcueca]

If assuming that all the operators are above the threshold, constantly.

How would that happen? If the first operator gets all the deposits, his proportional allocation will grow, and the allocation of the other operators will decrease. Eventually, they will be below their threshold.

[goheesheng]

If assuming that all the operators are above the threshold, constantly.

How would that happen? If the first operator gets all the deposits, his proportional allocation will grow, and the allocation of the other operators will decrease. Eventually, they will be below their threshold.

Can you elaborate on the allocation of the other operators will decrease? Let say that there are only 2 ODs. The first OD will be used until it is above threshold, Afterwards, the second OD will then be used.

One by one each OD is above used hence above threshold.

Now, since all the OD is above threshold, the first OD will constantly be used causing unfair allocation per OD, which allows the first OD to profit more from the restaking, this would be unfair for the second OD onwards

[alcueca]

The threshold is a proportion of the total TVL, not a fixed amount of assets.

[goheesheng]

The threshold is a proportion of the total TVL, not a fixed amount of assets.

Correct, the OD proportion can be set, but it still prioritize the first OD than any other ODs.