Submitted by juancito, also found by yjrwkk, pontifex, evmboi32, bin2chen, sashik_eth, Ruhum, MiloTruck, SpicyMeatball, and volodya.
StrategyManager::slashQueuedWithdrawal() contains an indicesToSkip parameter to skip malicious strategies, as documented in the function definition:
The problem is, the function does not work as expected, and indicesToSkip is ignored. If the queued withdrawal contains a malicious strategy, it will make the slash always revert.
Owners wont be able to slash queued withdrawals that contain a malicious strategy.
An adversary can take advantage of this and create withdrawal queues that wont be able to be slashed, completely defeating the slash system. The adversary can later complete the withdrawal.
The ++i; statement in StrategyManager::slashQueuedWithdrawal() is misplaced. It is only executed on the else statement:
Link to code
Lets suppose that the owner tries to slash a queued withdrawal, and wants to skip the first strategy (index 0) because it is malicious and makes the whole transaction revert.
1 . It defines indicesToSkipIndex = 0.
2 . It enters the for loop starting at i = 0.
3 . if (indicesToSkipIndex < indicesToSkip.length && indicesToSkip[indicesToSkipIndex] == i) will be true: 0 < 1 && 0 == 0.
4 . It increments ++indicesToSkipIndex; to skip the malicious strategy, so now indicesToSkipIndex = 1.
5 . It goes back to the for loop. But i hasnt been modified, so still i = 0.
6 . if (indicesToSkipIndex < indicesToSkip.length && indicesToSkip[indicesToSkipIndex] == i) will be false now: 1 < 1 && 0 == 0.
7 . It will enter the else statement and attempt to slash the strategy anyway.
8 . If the strategy is malicious, it will revert, making it impossible to slash.
9 . The adversary can later complete the withdrawal.
This test shows how the indicesToSkip parameter is completely ignored.
For the sake of simplicity of the test, it uses a normal strategy; which will be slashed, proving that it ignores the indicesToSkip parameter and it indeed calls queuedWithdrawal.strategies[i].withdraw().
A malicious strategy that makes withdraw() revert, would be to make the whole transaction revert (not shown on this test but easily checkable as the function wont catch it).
Add this test to src/tests/StrategyManagerUnit.t.sol and run forge test -m "testSlashQueuedWithdrawal_IgnoresIndicesToSkip".
Place the ++i outside of the if/else statement. This way, it will increment each time the loop runs.
Loop
sorrynotsorry (lookout) commented:
Sidu28 (EigenLayer) confirmed
Alex the Entreprenerd (judge) commented:
