EigenLayer Contest - RaymondFam's results

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L182-L206 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/DelegationManager.sol#L172-L196 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L304-L306 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/DelegationManager.sol#L136-L139

Vulnerability details

Impact

A staker could possibly dodge slashing when the delegated operator has been frozen due to the absence of onlyNotFrozen visibility on the chain of exit functions.

Proof of Concept

Here is a typical scenario:

1. Alice, the delegated operator of Bob, is frozen by Slasher.freezeOperator(().
2. Bob, who currently has beaconChainETHStrategy as the only strategy in his stakerStrategyList, happens to have overcommitted stake to EigenLayer as an ETH validator.
3. Bob calls EigenPod.verifyOvercommittedStake() to remove and undelegate shares in EigenLayer:

File: EigenPod.sol#L292-L293

        // remove and undelegate shares in EigenLayer
        eigenPodManager.recordOvercommittedBeaconChainETH(podOwner, beaconChainETHStrategyIndex, REQUIRED_BALANCE_WEI);

4. StrategyManager.recordOvercommittedBeaconChainETH(), which is devoid of onlyNotFrozen visibility, successfully removes shares for the enshrined beacon chain ETH strategy via _removeShares() on line 197, and modifies delegated shares accordingly on line 205:

File: StrategyManager.sol#L182-L206

    function recordOvercommittedBeaconChainETH(address overcommittedPodOwner, uint256 beaconChainETHStrategyIndex, uint256 amount)
        external
        onlyEigenPodManager
        nonReentrant
    {
        // get `overcommittedPodOwner`'s shares in the enshrined beacon chain ETH strategy
        uint256 userShares = stakerStrategyShares[overcommittedPodOwner][beaconChainETHStrategy];
        // if the amount exceeds the user's shares, then record it as an amount to be "paid off" when the user completes a withdrawal
        if (amount > userShares) {
            uint256 debt = amount - userShares;
            beaconChainETHSharesToDecrementOnWithdrawal[overcommittedPodOwner] += debt;
            amount -= debt;
        }
        // removes shares for the enshrined beacon chain ETH strategy
        if (amount != 0) {
197:            _removeShares(overcommittedPodOwner, beaconChainETHStrategyIndex, beaconChainETHStrategy, amount);            
        }
        // create array wrappers for call to DelegationManager
        IStrategy[] memory strategies = new IStrategy[](1);
        strategies[0] = beaconChainETHStrategy;
        uint256[] memory shareAmounts = new uint256[](1);
        shareAmounts[0] = amount;
        // modify delegated shares accordingly, if applicable
205:        delegation.decreaseDelegatedShares(overcommittedPodOwner, strategies, shareAmounts);
    }

5. Next, Bob calls StrategyManager.undelegate() which is again devoid of onlyNotFrozen visibility and successfully gets himself undelegated since he no longer has existing shares:

File: StrategyManager.sol#L304-L306

    function undelegate() external {
        _undelegate(msg.sender);
    }

File: StrategyManager.sol#L811-L814

    function _undelegate(address depositor) internal {
        require(stakerStrategyList[depositor].length == 0, "StrategyManager._undelegate: depositor has active deposits");
        delegation.undelegate(depositor);
    }

File: DelegationManager.sol#L136-L139

    function undelegate(address staker) external onlyStrategyManager {
        require(!isOperator(staker), "DelegationManager.undelegate: operators cannot undelegate from themselves");
        delegatedTo[staker] = address(0);
    }

6. After ensuring withdrawalAmountGwei >= REQUIRED_BALANCE_GWEI, Bob exits from Beacon Chain validation and calls EigenPod.verifyAndProcessWithdrawal() to invoke _processFullWithdrawal(). The full withdrawal amount is held for processing through EigenLayer's normal withdrawal path (line 396), and any excess 'beaconChainETH' shares in EigenLayer are immediately removed:

File: EigenPod.sol#L385-L396

        } else if (status == VALIDATOR_STATUS.OVERCOMMITTED) {
            // if the withdrawal amount is greater than the REQUIRED_BALANCE_GWEI (i.e. the amount restaked on EigenLayer, per ETH validator)
            if (withdrawalAmountGwei >= REQUIRED_BALANCE_GWEI) {
                // then the excess is immediately withdrawable
                amountToSend = uint256(withdrawalAmountGwei - REQUIRED_BALANCE_GWEI) * uint256(GWEI_TO_WEI);
                // and the extra execution layer ETH in the contract is REQUIRED_BALANCE_GWEI, which must be withdrawn through EigenLayer's normal withdrawal process
                restakedExecutionLayerGwei += REQUIRED_BALANCE_GWEI;
                /**
                 * since in `verifyOvercommittedStake` the podOwner's beaconChainETH shares are decremented by `REQUIRED_BALANCE_WEI`, we must reverse the process here,
                 * in order to allow the podOwner to complete their withdrawal through EigenLayer's normal withdrawal process
                 */
396:                eigenPodManager.restakeBeaconChainETH(podOwner, REQUIRED_BALANCE_WEI);

It is noteworthy that the onlyNotFrozen visibility of StrategyManager.depositBeaconChainETH() is no longer applicable to Bob since he has already undelegated Alice, his previous operator, and has refrained from calling DelegationManager.delegateTo() ever since.

File: StrategyManager.sol#L164-L173

    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);
    }

Tools Used

Manual

Recommended Mitigation Steps

Consider adding onlyNotFrozen visibility to the key leaked function, i.e., recordOvercommittedBeaconChainETH() and undelegate() in StrategyManager.sol.

Assessed type

Timing

Dummy returned values could break other function calls dependent on them

In StrategyBase.sol, sharesToUnderlyingView() returns amountShares when totalShares == 0 which is erroneous. If you were to call withdraw(), it would first run into an underflow error on line 137:

File: StrategyBase.sol#L121-L156

    function withdraw(address depositor, IERC20 token, uint256 amountShares)
        external
        virtual
        override
        onlyWhenNotPaused(PAUSED_WITHDRAWALS)
        onlyStrategyManager
    {
        require(token == underlyingToken, "StrategyBase.withdraw: Can only withdraw the strategy token");
        // copy `totalShares` value to memory, prior to any decrease
        uint256 priorTotalShares = totalShares;
        require(
            amountShares <= priorTotalShares,
            "StrategyBase.withdraw: amountShares must be less than or equal to totalShares"
        );

        // Calculate the value that `totalShares` will decrease to as a result of the withdrawal
137:        uint256 updatedTotalShares = priorTotalShares - amountShares;
        // check to avoid edge case where share rate can be massively inflated as a 'griefing' sort of attack
        require(updatedTotalShares >= MIN_NONZERO_TOTAL_SHARES || updatedTotalShares == 0,
            "StrategyBase.withdraw: updated totalShares amount would be nonzero but below MIN_NONZERO_TOTAL_SHARES");
        // Actually decrease the `totalShares` value
        totalShares = updatedTotalShares;

        /**
         * @notice calculation of amountToSend *mirrors* `sharesToUnderlying(amountShares)`, but is different since the `totalShares` has already
         * been decremented. Specifically, notice how we use `priorTotalShares` here instead of `totalShares`.
         */
        uint256 amountToSend;
        if (priorTotalShares == amountShares) {
            amountToSend = _tokenBalance();
        } else {
            amountToSend = (_tokenBalance() * amountShares) / priorTotalShares;
        }

        underlyingToken.safeTransfer(depositor, amountToSend);
    }

It is deemed non-critical for now since VoteWeigherBase.weightOfOperator() externally calling sharesToUnderlying() has sharesAmount > 0 check in the if statement. Elsewhere, it might return a wrong weight if cares have not been adequately put in place:

File: VoteWeigherBase.sol#L75-L82

                if (sharesAmount > 0) {
                    weight += uint96(
                        (
                            (strategyAndMultiplier.strategy).sharesToUnderlying(sharesAmount)
                                * strategyAndMultiplier.multiplier
                        ) / WEIGHTING_DIVISOR
                    );
                }

ETH restakers at a disadvantage when deciding to switch operator

In order to switch operator, undelegation requires stakerStrategyList[depositor].length == 0. Unlike all other liquid restaking where a staker can always make a full removal of shares via StrategyManager.queueWithdrawal(), the ETH restaker can only do so when exiting the Beacon Chain unless an over commitment has been made (deliberately or not) or calling queueWithdrawal() but not ignore completeQueuedWithdrawals() which is kind of irrational.

For this reason, it is advisable to keep beaconChainETHStrategy separate from stakerStrategyList by using a different account.

StrategyManager.slashShares() could easily run into DoS without first checking the contract balance of EigenPod

StrategyManager.slashShares() could easily run into DoS if the staker is still validating on Beacon Chain. Apparently, there is simply no ETH available in contract EigenPod to fulfill the sending of ETH to the recipient.

Consider adding the following check to the affected function:

File: StrategyManager.sol#L507-L510

_            if (strategies[i] == beaconChainETHStrategy) {
+            if (strategies[i] == beaconChainETHStrategy && address(eigenPodManager.ownerToPod(slashedAddressr).balance) >= shareAmounts[i]) {
                 //withdraw the beaconChainETH to the recipient
                _withdrawBeaconChainETH(slashedAddress, recipient, shareAmounts[i]);
            }

Inadequate check in the last if block of StrategyManager.queueWithdrawal()

The caller might be self delegated, making the function revert even more late in logic. Consider having the affected code line refactored as follows:

File: StrategyManager.sol#L422-L424

-        if (undelegateIfPossible && stakerStrategyList[msg.sender].length == 0) {
+        if (undelegateIfPossible && !delegation.isOperator(msg.sender) && stakerStrategyList[msg.sender].length == 0) {
            _undelegate(msg.sender);
        }

Comment and code mismatch

The function comment on StrategyManager.completeQueuedWithdrawal() is missing the struct withdrawerAndNonce instance. Consider having it rewritten as follows:

File: StrategyManager.sol#L432

-     * @notice Used to complete the specified `queuedWithdrawal`. The function caller must match `queuedWithdrawal.withdrawer`
+     * @notice Used to complete the specified `queuedWithdrawal`. The function caller must match `queuedWithdrawal.withdrawerAndNonce.withdrawer`

Typo mistakes

File: StrategyManager.sol#L287

-         * 2) if `staker` is a contract, then `signature` must will be checked according to EIP-1271
+         * 2) if `staker` is a contract, then `signature` must be checked according to EIP-1271

File: EigenPod.sol#L90

-    /// @notice Emitted when an ETH validator is prove to have withdrawn from the beacon chain
+    /// @notice Emitted when an ETH validator is proven to have withdrawn from the beacon chain

Events associated with setter functions

Consider having events associated with setter functions emit both the new and old values instead of just the new value.

Here is a specific instance entailed:

File: EigenPodManager.sol#L186-L189

    function _updateBeaconChainOracle(IBeaconChainOracle newBeaconChainOracle) internal {
        beaconChainOracle = newBeaconChainOracle;
        emit BeaconOracleUpdated(address(newBeaconChainOracle));
    }

Limiting 32 strategies

Hardcoding the upper limit of stakerStrategyList is limiting. In the event the protocol has over grown the 'whitelist' of strategies, users would have to resort to multiple accounts dealing with over 32 strategies.

File: StrategyManager.sol#L635-L641

        if (stakerStrategyShares[depositor][strategy] == 0) {
            require(
                stakerStrategyList[depositor].length < MAX_STAKER_STRATEGY_LIST_LENGTH,
                "StrategyManager._addShares: deposit would exceed MAX_STAKER_STRATEGY_LIST_LENGTH"
            );
            stakerStrategyList[depositor].push(strategy);
        }

Incorrect input of number in private __gap

In EigenPod.sol, there are total of 6 instead of 4 state variables to cater for future versions to add new variables without shifting down storage in the inheritance chain.

address public podOwner;
uint64 public mostRecentWithdrawalBlockNumber;
uint64 public restakedExecutionLayerGwei;
bool public hasRestaked;
mapping(uint40 => VALIDATOR_STATUS) public validatorStatus;
mapping(uint40 => mapping(uint64 => bool)) public provenPartialWithdrawal;

Consider having the affected code line refactored as follows:

File: EigenPod.sol#L473

-    uint256[46] private __gap;
+    uint256[44] private __gap;

Missing whistleblower method for other liquid restaking

A watcher can call EigenPod.verifyOvercommittedStake() to help enhance pooled security. However, this method is missing for non-beaconChainETHStrategy. Consider looking into implementing it where possible.

Missing check that leaves.length is a power of 2

According to the function NatSpec of Merkle.merkleizeSha256(), it requires the leaves.length is a power of 2. Consider adding the needed check where possible.

File: Merkle.sol#L123-L153

1. Implement isPowerOfTwo() that checks if a given number is a power of 2:

    function isPowerOfTwo(uint256 x) internal pure returns (bool) {
        return x != 0 && (x & (x - 1)) == 0;
    }

2. Refactor merkleizeSha256() to call isPowerOfTwo() and require the length of leaves to be a power of 2 as follows:

    function merkleizeSha256(
        bytes32[] memory leaves
    ) internal pure returns (bytes32) {
        require(
            isPowerOfTwo(leaves.length),
            "merkleizeSha256: The number of leaves must be a power of 2"
        );

        // ... rest of the function
    }

With this refactoring, merkleizeSha256() will revert the transaction if the number of leaves is not a power of 2.

Modularity on import usages

For cleaner Solidity code in conjunction with the rule of modularity and modular programming, use named imports with curly braces instead of adopting the global import approach.

For example, the import instances below could be refactored conforming to the suggested standards as follows:

File: StrategyManager.sol#L11-L13

- import "../interfaces/IEigenPodManager.sol";
+ import {IEigenPodManager} from "../interfaces/IEigenPodManager.sol";
- import "../permissions/Pausable.sol";
+ import {Pausable} from "../permissions/Pausable.sol";
- import "./StrategyManagerStorage.sol";
+ import {StrategyManagerStorage} from "./StrategyManagerStorage.sol";