code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/pods/EigenPod.sol#L157-L163 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/pods/EigenPod.sol#L217-L220 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/pods/EigenPod.sol#L453-L457

Vulnerability details

Impact

In EigenPod.sol, the state variable, withdrawBeforeRestaking() serves good only for the first ETH validator. As soon as its default value is set true in verifyWithdrawalCredentialsAndBalance(), withdrawBeforeRestaking() will be uncallable for the next ETH validator that has not been restaked.

Proof of Concept

Before eigenPodManager.restakeBeaconChainETH() is invoked in verifyWithdrawalCredentialsAndBalance(), hasRestaked == false is turned into hasRestaked == true.

File: EigenPod.sol#L217-L225

        // Sets "hasRestaked" to true if it hasn't been set yet. 
        if (!hasRestaked) {
            hasRestaked = true;
        }

        emit ValidatorRestaked(validatorIndex);

        // virtually deposit REQUIRED_BALANCE_WEI for new ETH validator
        eigenPodManager.restakeBeaconChainETH(podOwner, REQUIRED_BALANCE_WEI);

This in turn makes the modifier hasNeverRestaked readily revert from now on.

File: EigenPod.sol#L114-L117

    modifier hasNeverRestaked {
        require(!hasRestaked, "EigenPod.hasNeverRestaked: restaking is enabled");
        _;
    }

Consequently, withdrawBeforeRestaking() associated with hasNeverRestaked visibility will not be callable by the pod owner for all subsequent ETH validators created by the owner even though they have not been restaked yet. The only option to retrieve the rewards is via the restake path, i.e. first verifyWithdrawalCredentialsAndBalance() then verifyAndProcessWithdrawal().

File: EigenPod.sol#L453-L457

    /// @notice Called by the pod owner to withdraw the balance of the pod when `hasRestaked` is set to false
    function withdrawBeforeRestaking() external onlyEigenPodOwner hasNeverRestaked {
        mostRecentWithdrawalBlockNumber = uint32(block.number);
        _sendETH(podOwner, address(this).balance);
    }

Tools Used

Manual

Recommended Mitigation Steps

A better approach would be to maintain a separate hasRestaked mapping for each validator instead of using a single boolean for the entire contract. This way, you can track the restaking status for each validator individually.

Here's how you can modify the contract:

Replace the current bool public hasRestaked; declaration with a mapping. Additionally, introduce another mapping to track the balance of each validator:

    mapping(uint40 => bool) public hasRestaked;
    mapping(uint40 => uint256) public validatorBalances;

Modify verifyWithdrawalCredentialsAndBalance() to set the hasRestaked flag for the specific validator:

    function verifyWithdrawalCredentialsAndBalance(
        // ...
    ) external {
        // ...
        if (!hasRestaked[validatorIndex]) {
            hasRestaked[validatorIndex] = true;
        }
        // ...
    }

Modify the hasNeverRestaked modifier to use the mapping instead of the single boolean:

    modifier hasNeverRestaked(uint40 validatorIndex) {
        require(!hasRestaked[validatorIndex], "EigenPod.hasNeverRestaked: restaking is enabled for this 
    validator");
        _;
    }

Update withdrawBeforeRestaking() to take a validatorIndex parameter and use the hasNeverRestaked modifier with the provided index:

    function withdrawBeforeRestaking(uint40 validatorIndex) external onlyEigenPodOwner 
    hasNeverRestaked(validatorIndex) {

        // Include the verifyAndProcessWithdrawal() logic to update validatorBalances[validatorIndex] here.

        uint256 validatorBalance = validatorBalances[validatorIndex];
        require(validatorBalance > 0, "EigenPod.withdrawBeforeRestaking: no funds to withdraw for this validator");

        mostRecentWithdrawalBlockNumber = uint32(block.number);
        validatorBalances[validatorIndex] = 0;
        _sendETH(podOwner, validatorBalance);

    }

With these changes, the contract will maintain separate restaking statuses for each validator, allowing withdrawBeforeRestaking() to be called for each validator individually before they have been restaked.