EigenLayer Contest - 0xnev's results

Issues Template

Low

Non-Critical

Summary

Eigenlayer Introduces restaking, a new primitive to enable reuse of ETH on consensus layer, allowing stakes to opt-in to many services simultaneously with already staked ETH reducing capital costs for staker to participate and increases trust guarantees to individual services by allowing them to tap in to pooled security of ethereum stakers.

Many core functions for entry (depositing) and exit (withdrawing and slashing) are protected against reentrancy using the non-reentrant modifier.

There is centralization risk regarding slashing, pausing, changing critical addresses such as BeaconChainOracle and adding/removing allowed tokens for restaking but protocol is trusted and assigns governance to a community trusted safety multisig for emergencies.

For now, only liquid staking tokens and native ether are allowed for restaking but in the future if other tokens are allowed, more support might be required for restaking other types of tokens such as tokens with low decimals (e.g. USDC), fee on transfer tokens (e.g. USDT), ERC721 etc..

[L-01] Missing modifier onlyInitializing

The internal function StrategyBase._initializeStrategyBase is missing the onlyInitializing modifier which can cause the StrategyBase.initialize function to always revert when trying to initialize a StrategyBase.sol contract.

StrategyBase.sol#L56

function _initializeStrategyBase(IERC20 _underlyingToken, IPauserRegistry _pauserRegistry) internal onlyInitializing {
    underlyingToken = _underlyingToken;
    _initializePauser(_pauserRegistry, UNPAUSE_ALL);
}

[L-02] Lack of access control for DelayedWithdrawalRouter.claimDelayedWithdrawals()

DelayedWithdrawalRouter.sol#L79

function claimDelayedWithdrawals(address recipient, uint256 maxNumberOfDelayedWithdrawalsToClaim)
    external
    nonReentrant
    onlyWhenNotPaused(PAUSED_DELAYED_WITHDRAWAL_CLAIMS)
{
    _claimDelayedWithdrawals(recipient, maxNumberOfDelayedWithdrawalsToClaim);
}

Due to a lack of access control, anybody can call DelayedWithdrawalRouter.claimDelayedWithdrawals() whenever a withdrawal is claimable after queuing is completed.

As noted by protocol, caller cannot control WHERE funds are sent since recipient address is checked in the internal _claimDelayedWithdrawals but can control WHEN. Recommend only to allow staker or operators to claim delayed withdrawals to prevent this.

[L-03] Front runnable initializers

StrategyManager.sol#L146 EigenPodManager.sol#L84 EigenPod.sol#L152 DelayedWithdrawalRouter.sol#L49

Due to a lack of access control for initializers, attackers could front-run intialization of core protocol contracts and force protocol to redeploy core contracts.

Recommendation: Implement valid access control on core contracts to ensure only the relevant deployer can initialize().

[L-04] MIN_NONZERO_TOTAL_SHARES of 1e9 could lead to stuck funds for underlying tokens with lower decimals in the future

StrategyBase.sol#L28

uint96 internal constant MIN_NONZERO_TOTAL_SHARES = 1e9;

In the future, to support restaking and withdrawing tokens of lower decimals (USDC), MIN_NONZERO_TOTAL_SHARES could be changed to prevent funds from being locked. For example, 1e9 is equivalent to 1000 USDC which is a significant minimum amount to be locked in contract.

Recommendation: Could be complex since MIN_NONZERO_TOTAL_SHARES is the value to prevent ERC-4626 related inflation attacks

Some ways I can think of is setting a value for MIN_NONZERO_TOTAL_SHARES based on token decimals or implement logic to allow recovery of funds.

[NC-01] Potential precision loss when withdrawing small amount of wei (less than 1e9 gwei)

EigenPod.sol#L445

function withdrawRestakedBeaconChainETH(
    address recipient,
    uint256 amountWei
)
    external
    onlyEigenPodManager
{
    // reduce the restakedExecutionLayerGwei
    restakedExecutionLayerGwei -= uint64(amountWei / GWEI_TO_WEI);

    emit RestakedBeaconChainETHWithdrawn(recipient, amountWei);

    // transfer ETH from pod to `recipient`
    _sendETH(recipient, amountWei);
}

In EigenPod.withdrawRestakedBeaconChainETH(), if EigenPodManager owner withdraws less than 1e9 wei worth of ETH, the restakedExecutionLayerGwei may not be correctly updated.

Recommendation: Consider adding a check such as require(amountwei >= 1e9) to only allow minimum withdrawals of 1e9 wei.

[NC-02] A minimum _REQUIRED_BALANCE_WEI that is restaked per validator can be set

EigenPod.sol#L145

constructor(
    IETHPOSDeposit _ethPOS,
    IDelayedWithdrawalRouter _delayedWithdrawalRouter,
    IEigenPodManager _eigenPodManager,
    uint256 _REQUIRED_BALANCE_WEI
) {
    ethPOS = _ethPOS;
    delayedWithdrawalRouter = _delayedWithdrawalRouter;
    eigenPodManager = _eigenPodManager;
    REQUIRED_BALANCE_WEI = _REQUIRED_BALANCE_WEI;
    REQUIRED_BALANCE_GWEI = uint64(_REQUIRED_BALANCE_WEI / GWEI_TO_WEI);
    require(_REQUIRED_BALANCE_WEI % GWEI_TO_WEI == 0, "EigenPod.contructor: _REQUIRED_BALANCE_WEI is not a whole number of gwei");
    _disableInitializers();
}

Since the required wei balance for each validator must be a minimum of 32 ETH for native ether restaked, checks can be implemented within the constructor of EigenPod.sol to ensure _REQUIRED_BALANCE_WEI is set to a minimum of 32 ether (32e18 wei) to prevent accidental errors when initializing REQUIRED_BALANCE_GWEI especially given REQUIRED_BALANCE_GWEI is immutable and cannot be changed once each EigenPod is deployed and as such forces redeployment.

Recommendation:

constructor(
    IETHPOSDeposit _ethPOS,
    IDelayedWithdrawalRouter _delayedWithdrawalRouter,
    IEigenPodManager _eigenPodManager,
    uint256 _REQUIRED_BALANCE_WEI
) {
    require(_REQUIRED_BALANCE_WEI >= 32e18, "EigenPod.contructor: _REQUIRED_BALANCE_WEI is not at least 32 ETH");    
    ethPOS = _ethPOS;
    delayedWithdrawalRouter = _delayedWithdrawalRouter;
    eigenPodManager = _eigenPodManager;
    REQUIRED_BALANCE_WEI = _REQUIRED_BALANCE_WEI;
    REQUIRED_BALANCE_GWEI = uint64(_REQUIRED_BALANCE_WEI / GWEI_TO_WEI);
    require(_REQUIRED_BALANCE_WEI % GWEI_TO_WEI == 0, "EigenPod.contructor: _REQUIRED_BALANCE_WEI is not a whole number of gwei");
    _disableInitializers();
}

[NC-03] Lack of zero address checks in constructors

StrategyManager.sol#L129 StrategyBase.sol#L46 EigenPodManager.sol#L76 EigenPod.sol#L136

Perform zero address checks for constructors similar to DelayedWithdrawalRouter.sol for all the other core contracts to prevent accidental initialization to zero address

[NC-04] Consider using delete instead of assigning default boolean value

StrategyManager.sol#L554 StrategyManager.sol#L612 StrategyManager.sol#L772

554:        withdrawalRootPending[withdrawalRoot] = false;

612:                strategyIsWhitelistedForDeposit[strategiesToRemoveFromWhitelist[i]] = false;

772:        withdrawalRootPending[withdrawalRoot] = false;

[NC-05] Missing events for important functions

StrategyManager.sol#L164 StrategyManager.sol#L182 StrategyManager.sol#L304

164:    function depositBeaconChainETH(address staker, uint256 amount)

182:    function recordOvercommittedBeaconChainETH(address overcommittedPodOwner, uint256 beaconChainETHStrategyIndex, uint256 amount)

304:    function undelegate() external

Consider adding events for important functions. For depositBeaconChainETH, the Deposit event can be used. New events for overcommited beacon chain ETH and undelegation can be created.

[NC-06] Unecessary explicit conversion of uint256

EigenPod.sol#L375 EigenPod.sol#L382 EigenPod.sol#L389 EigenPod.sol#L404 EigenPod.sol#L429

375:                amountToSend = uint256(withdrawalAmountGwei - REQUIRED_BALANCE_GWEI) * uint256(GWEI_TO_WEI);

382:                eigenPodManager.recordOvercommittedBeaconChainETH(podOwner, beaconChainETHStrategyIndex, uint256(REQUIRED_BALANCE_GWEI - withdrawalAmountGwei) * GWEI_TO_WEI);

389:                amountToSend = uint256(withdrawalAmountGwei - REQUIRED_BALANCE_GWEI) * uint256(GWEI_TO_WEI);

404:                eigenPodManager.restakeBeaconChainETH(podOwner, uint256(withdrawalAmountGwei) * GWEI_TO_WEI);

429:        _sendETH(recipient, uint256(partialWithdrawalAmountGwei) * uint256(GWEI_TO_WEI));

GWEI_TO_WEI is already initialized as a type uint256 constant and does not need to be type casted.

Explicit type conversion of uint64 to uint256 is not required since solidity allows implicit conversion as no information is lost.

[NC-07] Lack of zero value check for StrategyManager.queueWithdrawal

StrategyManager.sol#L332

function queueWithdrawal(
    uint256[] calldata strategyIndexes,
    IStrategy[] calldata strategies,
    uint256[] calldata shares,
    address withdrawer,
    bool undelegateIfPossible
)
    external
    onlyWhenNotPaused(PAUSED_WITHDRAWALS)
    onlyNotFrozen(msg.sender)
    nonReentrant
    returns (bytes32)

Consider adding a check to not allow queueing withdrawals of shares in StrategyManager.queueWithdrawal of zero amount since it acheives nothing for restakers and operators. Also prevents restakers/operators from queueing unlimited number of withdrawals of 0 value, creating alot of noise as it will still emit WithdrawalQueued() event.

[NC-08] Perform input validation first in EigenPod.sol constructor

EigenPod.sol#L147

constructor(
    IETHPOSDeposit _ethPOS,
    IDelayedWithdrawalRouter _delayedWithdrawalRouter,
    IEigenPodManager _eigenPodManager,
    uint256 _REQUIRED_BALANCE_WEI
) {
    ethPOS = _ethPOS;
    delayedWithdrawalRouter = _delayedWithdrawalRouter;
    eigenPodManager = _eigenPodManager;
    REQUIRED_BALANCE_WEI = _REQUIRED_BALANCE_WEI;
    REQUIRED_BALANCE_GWEI = uint64(_REQUIRED_BALANCE_WEI / GWEI_TO_WEI);
    require(_REQUIRED_BALANCE_WEI % GWEI_TO_WEI == 0, "EigenPod.contructor: _REQUIRED_BALANCE_WEI is not a whole number of gwei");
    _disableInitializers();
}

Performing input validation first for _REQUIRED_BALANCE_WEI can potentially revert faster without initializing variables first whenever a new EigenPod is created.

Recommendation:

constructor(
    IETHPOSDeposit _ethPOS,
    IDelayedWithdrawalRouter _delayedWithdrawalRouter,
    IEigenPodManager _eigenPodManager,
    uint256 _REQUIRED_BALANCE_WEI
) {
    require(_REQUIRED_BALANCE_WEI % GWEI_TO_WEI == 0, "EigenPod.contructor: _REQUIRED_BALANCE_WEI is not a whole number of gwei");    
    ethPOS = _ethPOS;
    delayedWithdrawalRouter = _delayedWithdrawalRouter;
    eigenPodManager = _eigenPodManager;
    REQUIRED_BALANCE_WEI = _REQUIRED_BALANCE_WEI;
    REQUIRED_BALANCE_GWEI = uint64(_REQUIRED_BALANCE_WEI / GWEI_TO_WEI);
    _disableInitializers();
}

[NC-09] Consider using get prefix for getter functions

StrategyManager.sol#L871 StrategyBase.sol#L172 StrategyBase.sol#L196 StrategyBase.sol#L219 StrategyBase.sol#L235 DelayedWithdrawalRouter.sol#L105 DelayedWithdrawalRouter.sol#L110 DelayedWithdrawalRouter.sol#L122 DelayedWithdrawalRouter.sol#L127

function stakerStrategyListLength(address staker) external view returns (uint256) {
    return stakerStrategyList[staker].length;
}

function sharesToUnderlyingView(uint256 amountShares) public view virtual override returns (uint256) {
    if (totalShares == 0) {
        return amountShares;
    } else {
        return (_tokenBalance() * amountShares) / totalShares;
    }
}


function underlyingToSharesView(uint256 amountUnderlying) public view virtual returns (uint256) {
    uint256 tokenBalance = _tokenBalance();
    if (tokenBalance == 0 || totalShares == 0) {
        return amountUnderlying;
    } else {
        return (amountUnderlying * totalShares) / tokenBalance;
    }
}


function userUnderlyingView(address user) external view virtual returns (uint256) {
    return sharesToUnderlyingView(shares(user));
}


function shares(address user) public view virtual returns (uint256) {
    return strategyManager.stakerStrategyShares(user, IStrategy(address(this)));
}


function userWithdrawals(address user) external view returns (UserDelayedWithdrawals memory) {
    return _userWithdrawals[user];
}

function claimableUserDelayedWithdrawals(address user) external view returns (DelayedWithdrawal[] memory) {
    uint256 delayedWithdrawalsCompleted = _userWithdrawals[user].delayedWithdrawalsCompleted;
    uint256 delayedWithdrawalsLength = _userWithdrawals[user].delayedWithdrawals.length;
    uint256 claimableDelayedWithdrawalsLength = delayedWithdrawalsLength - delayedWithdrawalsCompleted;
    DelayedWithdrawal[] memory claimableDelayedWithdrawals = new DelayedWithdrawal[](claimableDelayedWithdrawalsLength);
    for (uint256 i = 0; i < claimableDelayedWithdrawalsLength; i++) {
        claimableDelayedWithdrawals[i] = _userWithdrawals[user].delayedWithdrawals[delayedWithdrawalsCompleted + i];
    }
    return claimableDelayedWithdrawals;
}


function userDelayedWithdrawalByIndex(address user, uint256 index) external view returns (DelayedWithdrawal memory) {
    return _userWithdrawals[user].delayedWithdrawals[index];
}


function userWithdrawalsLength(address user) external view returns (uint256) {
    return _userWithdrawals[user].delayedWithdrawals.length;
}

Consider using get prefix for getter (view/pure) functions to improve readability and prevent confusion

[NC-10] Shift all constants in StrategyManager.sol to StrategyManagerStorage.sol

StrategyManager.sol#L35-L45

uint256 internal constant GWEI_TO_WEI = 1e9;

// index for flag that pauses deposits when set
uint8 internal constant PAUSED_DEPOSITS = 0;
// index for flag that pauses withdrawals when set
uint8 internal constant PAUSED_WITHDRAWALS = 1;

uint256 immutable ORIGINAL_CHAIN_ID;

// bytes4(keccak256("isValidSignature(bytes32,bytes)")
bytes4 constant internal ERC1271_MAGICVALUE = 0x1626ba7e;

Since there is already a dedicated contract for storing StrategeManager storage variables, we can simply store all storage variables in StrategyManager.sol in StrategyManagerStorage.sol

[NC-11] Use ternary operators to shorten if/else statements

StrategyBase.sol#L78

function deposit(IERC20 token, uint256 amount)
        ...
        uint256 priorTokenBalance = _tokenBalance() - amount;
        if (priorTokenBalance == 0) {
            newShares = amount;
        } else {
            newShares = (amount * totalShares) / priorTokenBalance;
        }
        ...

StrategyBase.sol#L121

function withdraw(address depositor, IERC20 token, uint256 amountShares)
    ...
    if (priorTotalShares == amountShares) {
        amountToSend = _tokenBalance();
    } else {
        amountToSend = (_tokenBalance() * amountShares) / priorTotalShares;
    }
    ...

StrategyBase.sol#L172

function sharesToUnderlyingView(uint256 amountShares) public view virtual override returns (uint256) {
    if (totalShares == 0) {
        return amountShares;
    } else {
        return (_tokenBalance() * amountShares) / totalShares;
    }
}

StrategyBase.sol#L196

function underlyingToSharesView(uint256 amountUnderlying) public view virtual returns (uint256) {
    uint256 tokenBalance = _tokenBalance();
    if (tokenBalance == 0 || totalShares == 0) {
        return amountUnderlying;
    } else {
        return (amountUnderlying * totalShares) / tokenBalance;
    }
}

Use ternary operators to shorten if/else statements to improve readability and shorten SLOC. Can also potentially reduce deployment size and deployment cost at the expense of execution cost

Recommendation:

function deposit(IERC20 token, uint256 amount)
        ...
        uint256 priorTokenBalance = _tokenBalance() - amount;
        newShares = priorTokenBalance == 0 ? amount : (amount * totalShares) / priorTokenBalance;
        ...

function withdraw(address depositor, IERC20 token, uint256 amountShares)
    ...
    amountToSend = priorTotalShares == amountShares ? _tokenBalance() : (_tokenBalance() * amountShares) / priorTotalShares;
    ...

function sharesToUnderlyingView(uint256 amountShares) public view virtual override returns (uint256) {
    return totalShares == 0 ? amountShares : (_tokenBalance() * amountShares) / totalShares;
}

function underlyingToSharesView(uint256 amountUnderlying) public view virtual returns (uint256) {
    uint256 tokenBalance = _tokenBalance();
    return (tokenBalance == 0 || totalShares == 0) ? amountUnderlying: (amountUnderlying * totalShares) / tokenBalance;
}

[G-01] Shift input validation checks to top of function for StrategyManager.queueWithdrawal

Checks that involve input validation should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting gas on external function calls that may ultimately revert if checks does not pass.

Reccomendation: StrategyManager.sol#L329-L429

function queueWithdrawal(
    uint256[] calldata strategyIndexes,
    IStrategy[] calldata strategies,
    uint256[] calldata shares,
    address withdrawer,
    bool undelegateIfPossible
)
    external
    onlyWhenNotPaused(PAUSED_WITHDRAWALS)
    onlyNotFrozen(msg.sender)
    nonReentrant
    returns (bytes32)
{   
    require(strategies.length == shares.length, "StrategyManager.queueWithdrawal: input length mismatch");
    require(withdrawer != address(0), "StrategyManager.queueWithdrawal: cannot withdraw to zero address");

    ...
    
    /// @audit for loop checks can be shifted on top
    for (uint256 i = 0; i < strategies.length;) {
        if (strategies[i] == beaconChainETHStrategy) {
            require(withdrawer == msg.sender,
                "StrategyManager.queueWithdrawal: cannot queue a withdrawal of Beacon Chain ETH to a different address");
            require(strategies.length == 1,
                "StrategyManager.queueWithdrawal: cannot queue a withdrawal including Beacon Chain ETH and other tokens");
            require(shares[i] % GWEI_TO_WEI == 0,
                "StrategyManager.queueWithdrawal: cannot queue a withdrawal of Beacon Chain ETH for an non-whole amount of gwei");
        }   

        // the internal function will return 'true' in the event the strategy was
        // removed from the depositor's array of strategies -- i.e. stakerStrategyList[depositor]
        if (_removeShares(msg.sender, strategyIndexes[strategyIndexIndex], strategies[i], shares[i])) {
            unchecked {
                ++strategyIndexIndex;
            }
        }

        emit ShareWithdrawalQueued(msg.sender, nonce, strategies[i], shares[i]);

        //increment the loop
        unchecked {
            ++i;
        }
    }
    ...

    /// @audit checks can be shifted on top
    if (undelegateIfPossible && stakerStrategyList[msg.sender].length == 0) {
        _undelegate(msg.sender);
    }
    ...

[G-02] Multiple access of mapping/array should use a local variable cache

Caching a mapping's value in a local storage variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time.

stakerStrategyList[depositor] in StrategyManager._removeStrategyFromStakerStrategyList

StrategyManager.sol#L715-L740

function _removeStrategyFromStakerStrategyList(address depositor, uint256 strategyIndex, IStrategy strategy) internal {
    // if the strategy matches with the strategy index provided
    /// @audit stakerStrategyList[depositor] can be cached
    if (stakerStrategyList[depositor][strategyIndex] == strategy) {
        // replace the strategy with the last strategy in the list
        /// @audit stakerStrategyList[depositor] can be cached
        stakerStrategyList[depositor][strategyIndex] =
            stakerStrategyList[depositor][stakerStrategyList[depositor].length - 1];
    } else {
        //loop through all of the strategies, find the right one, then replace
        /// @audit stakerStrategyList[depositor] can be cached
        uint256 stratsLength = stakerStrategyList[depositor].length;
        uint256 j = 0;
        for (; j < stratsLength;) {
            /// @audit stakerStrategyList[depositor] can be cached
            if (stakerStrategyList[depositor][j] == strategy) {
                //replace the strategy with the last strategy in the list
                /// @audit stakerStrategyList[depositor] can be cached
                stakerStrategyList[depositor][j] = stakerStrategyList[depositor][stakerStrategyList[depositor].length - 1];
                break;
            }
            unchecked {
                ++j;
            }
        }
        // if we didn't find the strategy, revert
        require(j != stratsLength, "StrategyManager._removeStrategyFromStakerStrategyList: strategy not found");
    }
    // pop off the last entry in the list of strategies
    /// @audit stakerStrategyList[depositor] can be cached
    stakerStrategyList[depositor].pop();
}

stakerStrategyList[depositor] and stakerStrategyShares[depositor] in StrategyManager.getDeposits

StrategyManager.sol#L857-L868

function getDeposits(address depositor) external view returns (IStrategy[] memory, uint256[] memory) {
    uint256 strategiesLength = stakerStrategyList[depositor].length;
    uint256[] memory shares = new uint256[](strategiesLength);

    for (uint256 i = 0; i < strategiesLength;) {
        /// @audit stakerStrategyList[depositor] and stakerStrategyShares[depositor] can be cached
        shares[i] = stakerStrategyShares[depositor][stakerStrategyList[depositor][i]];
        unchecked {
            ++i;
        }
    }
    return (stakerStrategyList[depositor], shares);
}

[G-03] State variables should be cached in stack variables rather than re-reading them from storage

beaconChainETHStrategy in StrategyManager.queueWithdrawal

StrategyManager.sol#L359

function queueWithdrawal(
    uint256[] calldata strategyIndexes,
    IStrategy[] calldata strategies,
    uint256[] calldata shares,
    address withdrawer,
    bool undelegateIfPossible
)
    external
    onlyWhenNotPaused(PAUSED_WITHDRAWALS)
    onlyNotFrozen(msg.sender)
    nonReentrant
    returns (bytes32)
{    
    ...
    for (uint256 i = 0; i < strategies.length;) {
        /// @audit beaconChainETHStrategy can be cached
        if (strategies[i] == beaconChainETHStrategy) {
            require(withdrawer == msg.sender,
                "StrategyManager.queueWithdrawal: cannot queue a withdrawal of Beacon Chain ETH to a different address");
            require(strategies.length == 1,
                "StrategyManager.queueWithdrawal: cannot queue a withdrawal including Beacon Chain ETH and other tokens");
            require(shares[i] % GWEI_TO_WEI == 0,
                "StrategyManager.queueWithdrawal: cannot queue a withdrawal of Beacon Chain ETH for an non-whole amount of gwei");
        }   
    ...

[G-04] Unecessary explicit conversion of uint256

EigenPod.sol#L375 EigenPod.sol#L382 EigenPod.sol#L389 EigenPod.sol#L404 EigenPod.sol#L429

375:                amountToSend = uint256(withdrawalAmountGwei - REQUIRED_BALANCE_GWEI) * uint256(GWEI_TO_WEI);

382:                eigenPodManager.recordOvercommittedBeaconChainETH(podOwner, beaconChainETHStrategyIndex, uint256(REQUIRED_BALANCE_GWEI - withdrawalAmountGwei) * GWEI_TO_WEI);

389:                amountToSend = uint256(withdrawalAmountGwei - REQUIRED_BALANCE_GWEI) * uint256(GWEI_TO_WEI);

404:                eigenPodManager.restakeBeaconChainETH(podOwner, uint256(withdrawalAmountGwei) * GWEI_TO_WEI);

429:        _sendETH(recipient, uint256(partialWithdrawalAmountGwei) * uint256(GWEI_TO_WEI));

GWEI_TO_WEI is already initialized as a type uint256 constant and does not need to be type casted.

Explicit type conversion of uint64 to uint256 is not required since solidity allows implicit conversion as no information is lost.