EigenLayer Contest - QiuhaoLi's results

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L498-L505 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/pods/EigenPod.sol#L241

Vulnerability details

Impact

Malicious staker can avoid the service's slash on its restaked ETH shares.

Proof of Concept

When a service/middleware decides to slash a staker, it first calls slasher.freezeOperator() to freeze the operator/staker and then call slashShares(). The freeze and slash operations are two independent APIs (not atomic):

    /**
     * @notice Used for 'slashing' a certain operator.
     * @param toBeFrozen The operator to be frozen.
     * @dev Technically the operator is 'frozen' (hence the name of this function), **and then subject to slashing pending a decision by a human-in-the-loop.**
     * @dev The operator must have previously given the caller (which should be a contract) the ability to slash them, through a call to `optIntoSlashing`.
     */
    function freezeOperator(address toBeFrozen) external;

Although calling to queueWithdrawal() requires the staker not frozen, verifyOvercommittedStake(EigenPos.sol)--> recordOvercommittedBeaconChainETH (StrategyManager) is callable by a frozen staker.

So a malicious staker can do the following: 1.Plan a time to make it have less than REQUIRED_BALANCE_WEI on the beacon chain and monitor the beacon/execution chain. 2.When the time arrives, the staker does malicious things for service. 3.The malicious behaviors are found by someone and the attacker is frozen. 4.The service decides to slash the staker's shares, broadcast a tx calling slashShares(). 5.The staker found the tx and front-run the verifyOvercommittedStake() to remove all his shares (also the ETH strategy). 6.The slash fails because there are no shares to slash.

And there are no incentives for watchers to call verifyOvercommittedStake, which increases the possibility the attacker win the time window -- maybe another issue.

Tools Used

Manual Review.

Recommended Mitigation Steps

Add onlyNotFrozen modifier to make verifyOvercommittedStake(EigenPos.sol)/recordOvercommittedBeaconChainETH (StrategyManager) only callable from non-frozen podOwner.

Assessed type

MEV

[LOW] _removeStrategyFromStakerStrategyList() should check strategyIndex is not out-of-bounds:

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

    function _removeStrategyFromStakerStrategyList(address depositor, uint256 strategyIndex, IStrategy strategy) internal {
        // if the strategy matches with the strategy index provided
        if (stakerStrategyList[depositor][strategyIndex] == strategy) {
            // replace the strategy with the last strategy in the list
            stakerStrategyList[depositor][strategyIndex] =
                stakerStrategyList[depositor][stakerStrategyList[depositor].length - 1];
        } else {
            //loop through all of the strategies, find the right one, then replace
            uint256 stratsLength = stakerStrategyList[depositor].length;
            uint256 j = 0;
            for (; j < stratsLength;) {

The strategyIndex is used as an optimistic method here, but forget to check it against the stakerStrategyList[depositor].length, which may lead to an array OOB.

[LOW] No Incentives for watchers to call verifyOvercommittedStake()

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

There are no incentives for watchers to call verifyOvercommittedStake(), which may help MEV attackers in some situations.

[non-critical] completeQueuedWithdrawals() should check queuedWithdrawals.length == tokens.length == middlewareTimesIndexes.length == receiveAsTokens.length

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

    function completeQueuedWithdrawals(
        QueuedWithdrawal[] calldata queuedWithdrawals,
        IERC20[][] calldata tokens,
        uint256[] calldata middlewareTimesIndexes,
        bool[] calldata receiveAsTokens
    ) external
        onlyWhenNotPaused(PAUSED_WITHDRAWALS)
        // check that the address that the staker *was delegated to* – at the time that they queued the withdrawal – is not frozen
        nonReentrant
    {
        for(uint256 i = 0; i < queuedWithdrawals.length; i++) {
            _completeQueuedWithdrawal(queuedWithdrawals[i], tokens[i], middlewareTimesIndexes[i], receiveAsTokens[i]);
        }
    }

[non-critical] Wrong comment for depositBeaconChainETH in IStrategyManager

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

     * @dev Only callable by EigenPod for the staker.
     */
    function depositBeaconChainETH(address staker, uint256 amount) external;

The comment says depositBeaconChainETH is only callable by EigenPod, but in StrategyManager.sol#L164 it's onlyEigenPodManager:

    function depositBeaconChainETH(address staker, uint256 amount)
        external
        onlyEigenPodManager
        onlyWhenNotPaused(PAUSED_DEPOSITS)
        onlyNotFrozen(staker)
        nonReentrant
    {
        // add shares for the enshrined beacon chain ETH strategy
        _addShares(staker, beaconChainETHStrategy, amount);
    }

_claimDelayedWithdrawals doesn't reuse/delete the paid DelayedWithdrawal slot in _userWithdrawals

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

In createDelayedWithdrawal(), new DelayedWithdrawal is pushes into the _userWithdrawals array:

    function createDelayedWithdrawal(address podOwner, address recipient) external payable onlyEigenPod(podOwner) {
        uint224 withdrawalAmount = uint224(msg.value);
        if (withdrawalAmount != 0) {
            DelayedWithdrawal memory delayedWithdrawal = DelayedWithdrawal({
                amount: withdrawalAmount,
                blockCreated: uint32(block.number)
            });
            _userWithdrawals[recipient].delayedWithdrawals.push(delayedWithdrawal);
            emit DelayedWithdrawalCreated(podOwner, recipient, withdrawalAmount, _userWithdrawals[recipient].delayedWithdrawals.length - 1);
        }
    }

However, when these DelayedWithdrawals are paid/claimed, the useless slot is not reused.

    function _claimDelayedWithdrawals(address recipient, uint256 maxNumberOfDelayedWithdrawalsToClaim) internal {
        uint256 amountToSend = 0;
        uint256 delayedWithdrawalsCompletedBefore = _userWithdrawals[recipient].delayedWithdrawalsCompleted;
        uint256 _userWithdrawalsLength = _userWithdrawals[recipient].delayedWithdrawals.length;
        uint256 i = 0;
        while (i < maxNumberOfDelayedWithdrawalsToClaim && (delayedWithdrawalsCompletedBefore + i) < _userWithdrawalsLength) {
            // copy delayedWithdrawal from storage to memory
            DelayedWithdrawal memory delayedWithdrawal = _userWithdrawals[recipient].delayedWithdrawals[delayedWithdrawalsCompletedBefore + i];
            // check if delayedWithdrawal can be claimed. break the loop as soon as a delayedWithdrawal cannot be claimed
            if (block.number < delayedWithdrawal.blockCreated + withdrawalDelayBlocks) {
                break;
            }
            // otherwise, the delayedWithdrawal can be claimed, in which case we increase the amountToSend and increment i
            amountToSend += delayedWithdrawal.amount;
            // increment i to account for the delayedWithdrawal being claimed
            unchecked {
                ++i;
            }
        }
        // mark the i delayedWithdrawals as claimed
        _userWithdrawals[recipient].delayedWithdrawalsCompleted = delayedWithdrawalsCompletedBefore + i;
        // actually send the ETH
        if (amountToSend != 0) {
            AddressUpgradeable.sendValue(payable(recipient), amountToSend);
        }
        emit DelayedWithdrawalsClaimed(recipient, amountToSend, delayedWithdrawalsCompletedBefore + i);
    }

If we can reuse these slots (e.g. use a pre-allocated array as a circular buffer), we can make the storage cost from O(n) to a const number. This can be a big saving for a busy recipient.