`OperatorDelegator#queueWithdrawals()` would assume the wrong share amount from some strategies when queuing withdrawals

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Delegation/OperatorDelegator.sol#L193-L257

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Delegation/OperatorDelegator.sol#L193-L257

    function queueWithdrawals(
        IERC20[] calldata tokens,
        uint256[] calldata tokenAmounts
    ) external nonReentrant onlyNativeEthRestakeAdmin returns (bytes32) {
        // record gas spent
        uint256 gasBefore = gasleft();
        if (tokens.length != tokenAmounts.length) revert MismatchedArrayLengths();
        IDelegationManager.QueuedWithdrawalParams[]
            memory queuedWithdrawalParams = new IDelegationManager.QueuedWithdrawalParams[](1);
        // set strategies legth for 0th index only
        queuedWithdrawalParams[0].strategies = new IStrategy[](tokens.length);
        queuedWithdrawalParams[0].shares = new uint256[](tokens.length);

        // Save the nonce before starting the withdrawal
        uint96 nonce = uint96(delegationManager.cumulativeWithdrawalsQueued(address(this)));

        for (uint256 i; i < tokens.length; ) {
            if (address(tokens[i]) == IS_NATIVE) {
            // @audit (...snip) since the bug idea is considering when the token to be withdrawn is not native ETH 
            } else {
                if (address(tokenStrategyMapping[tokens[i]]) == address(0))
                    revert InvalidZeroInput();

                // set the strategy of the token
                queuedWithdrawalParams[0].strategies[i] = tokenStrategyMapping[tokens[i]];

                // set the equivalent shares for tokenAmount
                //@audit
                queuedWithdrawalParams[0].shares[i] = tokenStrategyMapping[tokens[i]]
                    .underlyingToSharesView(tokenAmounts[i]);
            }

            // set withdrawer as this contract address
            queuedWithdrawalParams[0].withdrawer = address(this);

            // track shares of tokens withdraw for TVL
            queuedShares[address(tokens[i])] += queuedWithdrawalParams[0].shares[i];
            unchecked {
                ++i;
            }
        }

        // queue withdrawal in EigenLayer
        bytes32 withdrawalRoot = delegationManager.queueWithdrawals(queuedWithdrawalParams)[0];
        // (...snip)
        return withdrawalRoot;
    }

This function is used whenever there is a need to like start a withdrawal from specified tokens strategies for any given amounts, now as hinted by the "@audit" tag we can see that whenever the token is not native ETH, the logic is quite similar, however in this case the strategy is not set to beaconChainEthStrategy and instead it's directly gotten from the tokenStrategyMapping mapping, issue however is with the way the equivalent shares for tokenAmount is being calculated, we can see that the queueWithdrawals() function queries the IStrategyManager#.underlyingToSharesView() function, but going to this section of the inherited IStrategyManager interface from EigenLayer https://github.com/Layr-Labs/eigenlayer-contracts/blob/dbfa12128a41341b936f3e8da5d6da58c6233877/src/contracts/interfaces/IStrategy.sol#L74 we can see that the underlyingToSharesView() in some cases is not going to be executed with current state data, i.e quoting them:

     * @notice In contrast to `underlyingToShares`, this function guarantees no state modifications

Would be key to note that the OperatorDelegator#queueWithdrawals() function in scope is not a view function unlike OperatorDelegator#getTokenBalanceFromStrategy(), so there is no need to query the non-state modifying underlying tokens to shares converter, in contrast using the non-state modifying converter would mean that for some strategies the wrong rate is going to be used when converting the underlying to shares, note that this can be coined from common logic and even the inherited EigenLayer documentation which indicates that for different strategies the implementation of both the underlyingToSharesView() and the underlyingToShares() can be different and considering popular DEFI logics we know that the view functions for exchange rates work with stale data (or to be more accurate, already stored/lagging data, this can be coined from popular logic where exchangeRateStored() a view function uses already stored data, whereas exchangeRateCurrent would be a non-view function and actually modify state and use the right state data and current exchange rate)

Impact

Shares gotten for the tokenAmounts for some strategies would be wrong as the non-state modifying rate converter is being used.

Recommended Mitigation Steps

Since the non-view modified OperatorDelegator#queueWithdrawals() function as the name suggests is infact used in order to queue withdrawals then the right amount of equivalent shares needs to be attached to the tokenAmounts in each instance, so considering different strategies are planned to be integrated then this function should query the underlyingToShares() function to ensure the right state data is used in the conversion to get the equivalent shares.

Assessed type

Context

[jatinj615]

Currently there is not difference between two functions. underlyingToShare calls underlyingToSharesView itself. ![Uploading Screenshot 2024-05-22 at 3.16.19 PM.png…]()

[alcueca]

Since there is no proof of an actual strategy used by Renzo in which the two functions are different, this is preventative of a future error and QA.

[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