EigenLayer Contest - sashik_eth's results

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L536-L579

Vulnerability details

The slashQueuedWithdrawal() function enables the owner to slash withdrawals that have already been queued by a 'frozen' operator (or a staker delegated to one). This function includes a parameter called 'indicesToSkip', which should contain the indexes of strategies that need to be skipped during slashing. Developers decided it necessary to implement this functionality due to the possibility of encountering malicious strategies in queued withdrawals:

/**
     * @notice Slashes an existing queued withdrawal that was created by a 'frozen' operator (or a staker delegated to one)
     * @param recipient The funds in the slashed withdrawal are withdrawn as tokens to this address.
     * @param queuedWithdrawal The previously queued withdrawal to be slashed
     * @param tokens Array in which the i-th entry specifies the `token` input to the 'withdraw' function of the i-th Strategy in the `strategies`
     * array of the `queuedWithdrawal`.
     * @param indicesToSkip Optional input parameter -- indices in the `strategies` array to skip (i.e. not call the 'withdraw' function on). This input exists
     * so that, e.g., if the slashed QueuedWithdrawal contains a malicious strategy in the `strategies` array which always reverts on calls to its 'withdraw' function,
     * then the malicious strategy can be skipped (with the shares in effect "burned"), while the non-malicious strategies are still called as normal.
     */
    function slashQueuedWithdrawal(address recipient, QueuedWithdrawal calldata queuedWithdrawal, IERC20[] calldata tokens, uint256[] calldata indicesToSkip)

However, upon examination of the function itself, it becomes apparent that the loop counter is not properly handled in situations where the loop encounters an index that should be skipped:

File: StrategyManager.sol
560:         for (uint256 i = 0; i < strategiesLength;) {
561:             // check if the index i matches one of the indices specified in the `indicesToSkip` array
562:             if (indicesToSkipIndex < indicesToSkip.length && indicesToSkip[indicesToSkipIndex] == i) { 
563:                 unchecked {
564:                     ++indicesToSkipIndex;
565:                 }
566:             } else {
567:                 if (queuedWithdrawal.strategies[i] == beaconChainETHStrategy){
568:                      //withdraw the beaconChainETH to the recipient
569:                     _withdrawBeaconChainETH(queuedWithdrawal.depositor, recipient, queuedWithdrawal.shares[i]);
570:                 } else {
571:                     // tell the strategy to send the appropriate amount of funds to the recipient
572:                     queuedWithdrawal.strategies[i].withdraw(recipient, tokens[i], queuedWithdrawal.shares[i]);
573:                 }
574:                 unchecked {
575:                     ++i;
576:                 }
577:             }
578:         }

Each time the loop counter 'i' reaches the same value as the index that needs to be skipped, the index counter value increases while the loop counter value remains the same. This means that the same index will be used again in the next iteration, potentially allowing a malicious strategy to block slashing.

Impact

The functionality designed to enable protocol owners to skip malicious strategies during slashing is not functioning properly.

Proof of Concept

The addition of the following test to src/test/unit/StrategyManagerUnit.t.sol could demonstrate a scenario where the owner attempts to skip one of the strategies in a queued withdrawal, but the skipping functionality fails to work:

    function testIndicesToSkipNotWorking() external {
        address recipient = address(333);
        uint256 withdrawalAmount = 1e18;

        IStrategyManager.QueuedWithdrawal memory queuedWithdrawal =
            QueueWithdrawal_ToSelf_MultipleStrategies(withdrawalAmount);

        uint256 balanceBefore = dummyToken.balanceOf(address(recipient));

        // slash the delegatedOperator
        slasherMock.freezeOperator(queuedWithdrawal.delegatedAddress);

        cheats.startPrank(strategyManager.owner());
        uint256[] memory indicesToSkip = new uint256[](1);
        indicesToSkip[0] = 0;
        IERC20[] memory tokenArray = new IERC20[](2);
        tokenArray[0] = dummyToken;
        tokenArray[1] = dummyToken;
        strategyManager.slashQueuedWithdrawal(recipient, queuedWithdrawal, tokenArray, indicesToSkip);
        cheats.stopPrank();

        uint256 balanceAfter = dummyToken.balanceOf(address(recipient));
        // balance should change only for half of withdrawalAmount, since one of strategies should be skipped
        require(balanceAfter == balanceBefore + withdrawalAmount/2, "balanceAfter != balanceBefore + withdrawalAmount / 2");
    }

    function QueueWithdrawal_ToSelf_MultipleStrategies(uint256 withdrawalAmount) public
        returns (IStrategyManager.QueuedWithdrawal memory /* queuedWithdrawal */)
    {
        StrategyWrapper maliciousStrat = new StrategyWrapper(strategyManager, dummyToken);
        // adding second strategy to the protocol 
        cheats.startPrank(strategyManager.owner());
        IStrategy[] memory _strategy = new IStrategy[](1);
        _strategy[0] = maliciousStrat;
        strategyManager.addStrategiesToDepositWhitelist(_strategy);
        cheats.stopPrank();
        // depositing to both strategies half of withdrawalAmount
        IStrategy[] memory strategies = new IStrategy[](2);
        strategies[0] = dummyStrat;
        strategies[1] = maliciousStrat;

        uint256 shares1 = strategyManager.depositIntoStrategy(strategies[0], dummyToken, withdrawalAmount/2);
        uint256 shares2 = strategyManager.depositIntoStrategy(strategies[1], dummyToken, withdrawalAmount/2);

        uint256[] memory shareAmounts = new uint256[](2);
        shareAmounts[0] = shares1;
        shareAmounts[1] = shares2;
        // creating Queued Withdrawal with both strategies
        (IStrategyManager.QueuedWithdrawal memory queuedWithdrawal) =
            _setUpQueuedWithdrawalStructMultipleStrategies(/*staker*/ address(this), /*withdrawer*/ address(this), strategies, shareAmounts);

        {
            uint256[] memory strategyIndexes = new uint256[](2);
            strategyIndexes[0] = 1;
            strategyIndexes[1] = 0;
            strategyManager.queueWithdrawal(
                strategyIndexes,
                queuedWithdrawal.strategies,
                queuedWithdrawal.shares,
                /*withdrawer*/ address(this),
                false
            );
        }
        return queuedWithdrawal;
    }

    function _setUpQueuedWithdrawalStructMultipleStrategies(address staker, address withdrawer, IStrategy[] memory strategies, uint256[] memory shareAmounts)
        internal view returns (IStrategyManager.QueuedWithdrawal memory queuedWithdrawal)
    {
        IStrategyManager.WithdrawerAndNonce memory withdrawerAndNonce = IStrategyManager.WithdrawerAndNonce({
            withdrawer: withdrawer,
            nonce: uint96(strategyManager.numWithdrawalsQueued(staker))
        });
        queuedWithdrawal = 
            IStrategyManager.QueuedWithdrawal({
                strategies: strategies,
                shares: shareAmounts,
                depositor: staker,
                withdrawerAndNonce: withdrawerAndNonce,
                withdrawalStartBlock: uint32(block.number),
                delegatedAddress: strategyManager.delegation().delegatedTo(staker)
            }
        );
        return (queuedWithdrawal);
    }

Recommended Mitigation Steps

Consider updating the slashQueuedWithdrawal() function to increment the loop counter 'i' with each iteration.

Assessed type

Other

Summary

Low Risk Issues List

Non-Critical Issues List

---

L-01 Possible to spam DelayedWithdrawalRouter with dust values

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/pods/EigenPod.sol#L454

    function withdrawBeforeRestaking() external onlyEigenPodOwner hasNeverRestaked {
        mostRecentWithdrawalBlockNumber = uint32(block.number);
        _sendETH(podOwner, address(this).balance);
    }

It is possible to create an empty pod and use the 'withdrawBeforeRestaking()' function on the EigenPod contract to spam arbitrary addresses on the DelayedWithdrawalRouter with dust values. This can lead to users having to claim their withdrawals multiple times until a withdrawal with a real value is claimed. Additionally, this can result in a DOS attack on the 'claimableUserDelayedWithdrawals()' view function due to the large number of dusted withdrawals. To prevent such dust attacks, it is advisable to consider adding a minimum threshold for the 'withdrawBeforeRestaking()' function. This would ensure that only withdrawals with a certain minimum value are processed, thereby avoiding the spamming of arbitrary addresses with dust values.

L-02 Empty withdrawal could be created and withdrawed

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L329 'queueWithdrawal()' function allows the creation of an empty queued withdrawal. This could potentially mislead external viewers who are monitoring contract events. To avoid confusion and potential misinterpretation, it may be advisable to add a check that the strategies array parameter in 'queueWithdrawal()' is not empty.

L-03 Missing emmiting event

Consider adding events to the following functions, since analogous functions in the code emit appropriate events:

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/pods/EigenPodManager.sol#L139

    function recordOvercommittedBeaconChainETH(address podOwner, uint256 beaconChainETHStrategyIndex, uint256 amount) external onlyEigenPod(podOwner) {
        strategyManager.recordOvercommittedBeaconChainETH(podOwner, beaconChainETHStrategyIndex, amount);
    } 

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/pods/EigenPodManager.sol#L150 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L164 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L182

N-01 Redundant check

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L196

    if (amount != 0) { 
        _removeShares(overcommittedPodOwner, beaconChainETHStrategyIndex, beaconChainETHStrategy, amount);            
    }

No need to check for 0 amount here, since _removeShares() function already include such check.

N-02 Redundant check

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L422

    if (undelegateIfPossible && stakerStrategyList[msg.sender].length == 0) { 
        _undelegate(msg.sender);
    }

No need to check staker strategy list length since _undelegate() function include such check.

N-03 No needed calculation

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L193

    if (amount > userShares) {
        uint256 debt = amount - userShares;
        beaconChainETHSharesToDecrementOnWithdrawal[overcommittedPodOwner] += debt;
        amount -= debt;
    }

Last line in this block could be substituted with more simple:

    amount = userShares

Due to math equivalence:

1. amount -= debt
   debt = amount - userShares

2. amount = amount - (amount - userShares)

3. amount = userShares

N-04 Wrong comment

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L161

    /**
     * @notice Deposits `amount` of beaconchain ETH into this contract on behalf of `staker`
     * @param staker is the entity that is restaking in eigenlayer,
     * @param amount is the amount of beaconchain ETH being restaked,
     * @param amount is the amount of token to be deposited in the strategy by the depositor 
     * @dev Only callable by EigenPodManager.
     */
    function depositBeaconChainETH(address staker, uint256 amount)

Second comment on @param amount wrong here and should be removed

N-05 Wrong comment

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L786

    // tell the strategy to send the appropriate amount of funds to the depositor 
    queuedWithdrawal.strategies[i].withdraw(
        msg.sender, tokens[i], queuedWithdrawal.shares[i]
    );

Comment here stands that funds would be withdrawed to the depositor address, while funds would be sent to the withdrawer address, which is msg.sender here.

N-06 Wrong variable naming

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L460

    function completeQueuedWithdrawals(
        QueuedWithdrawal[] calldata queuedWithdrawals,
        IERC20[][] calldata tokens,
        uint256[] calldata middlewareTimesIndexes,
        bool[] calldata receiveAsTokens 
    ) external

In contract code last parameters in completeQueuedWithdrawals() function named as receiveAsTokens while docs stand on naming withdrawAsTokens: https://github.com/code-423n4/2023-04-eigenlayer/blob/main/docs/EigenLayer-withdrawal-flow.md Consider updating naming in code or documentation to remove inconsistency.