Open code423n4 opened 1 year ago
0xSorryNotSorry marked the issue as primary issue
Sidu28 requested judge review
Sidu28 marked the issue as disagree with severity
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
Sidu28 marked the issue as sponsor disputed
GalloDaSballo marked the issue as duplicate of #432
GalloDaSballo marked the issue as not a duplicate
GalloDaSballo marked the issue as primary issue
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
GalloDaSballo changed the severity to QA (Quality Assurance)
GalloDaSballo marked the issue as grade-b
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()
andrecordOvercommittedBeaconChainETH()
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
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, ifamount > amountToDecrement
, the staker's debt will be cleared with less ETH sent to the recipient at the end. Ifamount <= 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
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
Recommended Mitigation Steps
It is recommended allowing the sending of ETH to the recipient directly in
slashShares()
by skipping_withdrawBeaconChainETH()
.Assessed type
DoS