c4-bot-7 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L160-L181 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L193-L324

Vulnerability details

Impact

Completely removing an operator delegator can be impossible because if the completed withdrawal amount is bigger than the deficit then the excess will be redeposited which in case of a migration or off boarding an operator delegator this behaviour would not be correct. The excess being redeposited to the operator delegator makes the restaking manager admin removing the operator not possible. If the admin does that regardless, the excess TVL will also be scraped hence, the TVL will change and exchange rate will drop significantly.

Proof of Concept

There is a function in restaking manager that can be used to remove an operator delegator:

function removeOperatorDelegator(
        IOperatorDelegator _operatorDelegatorToRemove
    ) external onlyRestakeManagerAdmin {
        // Remove it from the list
        uint256 odLength = operatorDelegators.length;
        for (uint256 i = 0; i < odLength; ) {
            if (address(operatorDelegators[i]) == address(_operatorDelegatorToRemove)) {
                // Clear the allocation
                operatorDelegatorAllocations[_operatorDelegatorToRemove] = 0;
                emit OperatorDelegatorAllocationUpdated(_operatorDelegatorToRemove, 0);

                // Remove from list
                operatorDelegators[i] = operatorDelegators[operatorDelegators.length - 1];
                operatorDelegators.pop();
                emit OperatorDelegatorRemoved(_operatorDelegatorToRemove);
                return;
            }
            unchecked {
                ++i;
            }
        }

This function can be used to off board an operator. When this function will be used, all the funds has to be withdrawn from the operator delegator to make sure the TVL will not change. Thus, the admin has to queue and complete all the shares operator delegator has. However, if the withdrawn funds are bigger than the buffer deficit the excess will be redeposited in single tx hence, the off boarding can be impossible without leaving any TVL behind.

function completeQueuedWithdrawal(
        IDelegationManager.Withdrawal calldata withdrawal,
        IERC20[] calldata tokens,
        uint256 middlewareTimesIndex
    ) external nonReentrant onlyNativeEthRestakeAdmin {
        .
        .
        delegationManager.completeQueuedWithdrawal(withdrawal, tokens, middlewareTimesIndex, true);

        IWithdrawQueue withdrawQueue = restakeManager.depositQueue().withdrawQueue();
        for (uint256 i; i < tokens.length; ) {
            .
            .
            if (address(tokens[i]) != IS_NATIVE) {
                // Check the withdraw buffer and fill if below buffer target
                uint256 bufferToFill = withdrawQueue.getBufferDeficit(address(tokens[i]));

                // get balance of this contract
                uint256 balanceOfToken = tokens[i].balanceOf(address(this));
                -> if (bufferToFill > 0) {
                    bufferToFill = (balanceOfToken <= bufferToFill) ? balanceOfToken : bufferToFill;

                    // update amount to send to the operator Delegator
                    balanceOfToken -= bufferToFill;

                    // safe Approve for depositQueue
                    tokens[i].safeApprove(address(restakeManager.depositQueue()), bufferToFill);

                    // fill Withdraw Buffer via depositQueue
                    restakeManager.depositQueue().fillERC20withdrawBuffer(
                        address(tokens[i]),
                        bufferToFill
                    );
                }

                // Deposit remaining tokens back to eigenLayer
                -> if (balanceOfToken > 0) {
                ->    _deposit(tokens[i], balanceOfToken);
                -> }
            }
            unchecked {
                ++i;
            }
        }
        .
        .
    }

Considering the buffer is relatively a small amount than an operator delegators balance the excess is almost always will be exists. If the admin increases buffer just to be sure that there won't be any excess amount, then all the funds will go to withdraw queue. There is no way to put funds from withdraw queue back to operator delegators hence, the funds are dedicated to withdrawals only. If the operator delegator has significant TVL then all these funds are forced to be withdrawn and slowing down the ezETH yield immensely. Also, depending on the amount, someone can deposit lots of funds to fill the buffer such that the funds will be excess again and redeposited, making the off boarding without changing the TVL impossible.

Overall, this is an important functionality to have and currently there is no healthy/guaranteed way to manage this.

Tools Used

Recommended Mitigation Steps

Make an extra function just for off boarding an operator delegator which queues all the shares operator has and sends it back to deposit queue + withdraw queue

Assessed type

Other

raymondfam commented 5 months ago

@howlbot reject