Insufficient operator checks can lead to DOS of deposits and token rewards restaking

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

Improper check of the delegate operator when depositing can revert normal deposits and protocol reward deposits in StakeManager.

Vulnerability details

The StakeManager.chooseOperatorDelegatorForDeposit() function is used to determine the appropriate operator, where the token amounts should be deposited. It iterates through all operators and picks the first one that has not exceeded its tvl allocation. In case there is no such operator, the first one is returned by default:

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

 function chooseOperatorDelegatorForDeposit(
        uint256[] memory tvls,
        uint256 totalTVL
    ) public view returns (IOperatorDelegator) {

        // 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];
    }

Each operator is an individual instance of the OperatorDelegator contract. Each operator can handle multiple tokens. A token is enabled/disabled through the OperatorDelegator.setTokenStrategy() function:

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L105-L114

 /// @dev Sets the strategy for a given token - setting strategy to 0x0 removes the ability to deposit and withdraw token
    function setTokenStrategy(
        IERC20 _token,
        IStrategy _strategy
    ) external nonReentrant onlyOperatorDelegatorAdmin {
        if (address(_token) == address(0x0)) revert InvalidZeroInput();

        tokenStrategyMapping[_token] = _strategy;
        emit TokenStrategyUpdated(_token, _strategy);
    }

The problem is that StakeManager.chooseOperatorDelegatorForDeposit() does not check if the particular collateral token has been enabled for the chosen operator.

As a result the functions that use it - StakeManager.deposit() & StakeManager.depositTokenRewardsFromProtocol() will subsequently revert when calling operatorDelegator.deposit(_token, _amount), because the token is not enabled.

Proof of Concept

A simple scenario:

• Renzo has 10 operators
• Only operators 5,6,7 support DAI as collateral
• Alice decides to deposit DAI
• chooseOperatorDelegatorForDeposit() starts looping and returns operator 3 (the first one below its allocated tvl) / or returns operator 1 because all operators are above their allocations
• The deposit fails, even though there are operators that would support it

Tools Used

Manual Review

Recommended Mitigation Steps

Refactor RestakeManager.chooseOperatorDelegatorForDeposit() to additionally check if the operator has enabled the collateral:

function chooseOperatorDelegatorForDeposit(
        uint256[] memory tvls,
        uint256 totalTVL
    ) public view returns (IOperatorDelegator) {
        ....

        // Otherwise, find the operator delegator with TVL below the threshold
        uint256 tvlLength = tvls.length;

+        IOperatorDelegator validOperator;

        for (uint256 i = 0; i < tvlLength; ) {

+        boolean supportsToken = address(operatorDelegators[i].tokenStrategyMapping[token]) != address(0x0);

+        if(supportsToken) {
+           validOperator = operatorDelegators[i];
+        }

            if (
+               supportsToken &&
                tvls[i] <
                (operatorDelegatorAllocations[operatorDelegators[i]] * totalTVL) /
                    BASIS_POINTS /
                    BASIS_POINTS

            ) {
                return operatorDelegators[i];
            }

            unchecked {
                ++i;
            }
        }

+        if (address(validOperator) == address(0x0)) revert NoOperatorFound()

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

+        // Default to the first valid operator delegator
+        return validOperator;
    }

Assessed type

Invalid Validation

[DadeKuma]

@howlbot accept

[DadeKuma]

@howlbot reject

[DadeKuma]

/// @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

[CloudEllie]

Validator reported this was accidentally mis-labelled as sufficient quality report; the intended label was insufficient quality report, therefore staff have adjusted the labels.

[BogoCvetkov]

I'm escalating this because I think that the provided comment by the validator does not invalidate the argumentation provided in the submission.

The argument from my side is that :

• RestakeManager.chooseOperatorDelegatorForDeposit() always picks the first operator below its TVL without checking if it supports the deposited token, which will revert deposits even in cases where operators supporting that token exist

Currently the validator has provided a reference to the @dev comment of the function, which does not explain why this submission was rejected.

I'm allowing myself to summarize once more the issue with a bit more detail to make it clearer:

• RestakeManager.deposit() is called by providing _collateralToken to be deposited
• deposit() calls chooseOperatorDelegatorForDeposit() to determine the appropriate operator delegator to use for that _collateralToken

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

• chooseOperatorDelegatorForDeposit() will return either the 1st operator below its TVL threshold or the default 1st operator. However no check is made if any of the 2 operators actually support the _collateralToken being deposited, which would lead to revert.

The POC in the submission makes an example with a concrete scenario.

Important Note

The important thing to note here is that even if a valid operator EXISTS that can support that token deposit it still cannot be chosen because chooseOperatorDelegatorForDeposit() will break the loop early at the first operator below its TVL threshold

Currently the logic for choosing an operator for a token being deposited can be expressed like so:

Pick the 1st OperatorDelegator with TVL below the threshold or returns the first one in the list

The correct way to determine the operator should be like so:

Pick the 1st OperatorDelegator with TVL below the threshold that SUPPORTS THE TOKEN BEING DEPOSITED or return the first one in the list that SUPPORTS THE TOKEN BEING DEPOSITED

[DadeKuma]

Thank you for your comment @BogoCvetkov, I've re-checked it and it doesn't seem to be any checks where it's used: https://github.com/search?q=repo%3Acode-423n4%2F2024-04-renzo%20chooseOperatorDelegatorForDeposit(&type=code

So, the described scenario might happen: when we have an operator with low TVL that doesn't support a token, while other operators with higher TVL do support the token, the function will revert.

@c4-sponsor could you check this issue to confirm?

[s1n1st3r01]

Operator delegators are expected to support all collateral tokens that exist, pretty sure sponsor would say the same. Therefore this should remain as invalid.

[blckhv]

@s1n1st3r0

But more importantly ⬇️

[alcueca]

The "for now" is good enough, the code is fit for the current purpose.

[blckhv]

@alcueca With all due respect, given that most wardens break the PJQA rules, but that's part of the contests game, I'd like to add a final comment:

This report as well as ours - #503, clearly states that one of the most important features of the protocol (depositing) will be impacted temporarily, caused by its architecture - to allow different tokens to be supported across the OperatorDelegators.

For now is an abstract wording because Renzo will target assets with most APY and it is not guaranteed that assets used in the old OperatorDelegators will also be included in the newly added ones, because it doesn't make sense to add new OD for a token that is already being accumulating rewards in others.

Thanks!