STRATEGYMANAGER.SOL CONTRACT OWNER COULD BE DEPRIVED OF A SLASH

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L189-L194
https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L821-L836
https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L501

Vulnerability details

Impact

Both slashShares() and recordOvercommittedBeaconChainETH() could possibly interfere each other such that the former could be deprived of sending ETH to the recipient.

Proof of Concept

A validator who has been slashed by the contract owner of StrategyManager.sol will incur a debt when he/she eventually over commits:

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

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

When the validator is slashed again in the Eigen Layer, calling slashShares() could likely sending less or no ETH the recipient. According to the code logic below, if amount > amountToDecrement, the staker's debt will be cleared with less ETH sent to the recipient at the end. If amount <= amountToDecrement, the function call ends prematurely with nothing sent to the recipient.

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

    function _withdrawBeaconChainETH(address staker, address recipient, uint256 amount) internal {
        uint256 amountToDecrement = beaconChainETHSharesToDecrementOnWithdrawal[staker];
        if (amountToDecrement != 0) {
            if (amount > amountToDecrement) {
                beaconChainETHSharesToDecrementOnWithdrawal[staker] = 0;
                // decrease `amount` appropriately, so less is sent at the end
                amount -= amountToDecrement;
            } else {
                beaconChainETHSharesToDecrementOnWithdrawal[staker] = (amountToDecrement - amount);
                // rather than setting `amount` to 0, just return early
                return;
            }
        }
        // withdraw the beaconChainETH to the recipient
        eigenPodManager.withdrawRestakedBeaconChainETH(staker, recipient, amount);
    }

The problem is that shares have been removed in slashShares(). Although the contract owner could still make up for the loss by slashing the staker again but I don't think this is the intended design. Moreover, doing this will severely tarnish the reputation of the protocol.

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

            if (_removeShares(slashedAddress, strategyIndexes[strategyIndexIndex], strategies[i], shareAmounts[i])) {

Recommended Mitigation Steps

It is recommended allowing the sending of ETH to the recipient directly in slashShares() by skipping _withdrawBeaconChainETH().

Assessed type

DoS

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

Sidu28 requested judge review

[c4-sponsor]

Sidu28 marked the issue as disagree with severity

[Sidu28]

We believe this is a duplicate of issue 259, as it appears to have the same root cause.

(Note to C4 admins: The disagree with severity tag and the sponsor disputed tag are accidental, please remove if possible)
@0xSorryNotSorry

[c4-sponsor]

Sidu28 marked the issue as sponsor disputed

[c4-judge]

GalloDaSballo marked the issue as duplicate of #432

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

Per the sponsors comment on #259

A slashed validator would be forced to repay the debt and withdraw

Meaning that the accounting error wouldn't happen in reality

That said, the onChain logic does provide a path in which this finding is valid, for this reason am downgrading to QA - Low Severity +3
L + 3

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-b