EigenLayer Contest - bughunter007's results

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/core/StrategyManager.sol#L526-L579 https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/core/StrategyManager.sol#L442-L449 https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/core/StrategyManager.sol#L456-L469

Vulnerability details

Impact

The function slashQueuedWithdrawal of StrategyManager.sol takes optional parameter of indicesToSkip. As per code comments, the purpose of this parameter is to skip malicious strategies so that function call will not revert. However same risk possesses for regular queued withdrawls i.e completeQueuedWithdrawal & completeQueuedWithdrawals functions. These two functions are missing indicesToSkip parameter to skip malicious strategies which may cause withdrawls to fail.

Also erroneous input to indicesToSkip parameter will permanently lock the tokens in the strategy. There is no mechanism to recover those tokens.

Proof of Concept

//Admin provides erroroneous indicesToSkip parameter to the following finction of StrategyManager.sol function slashQueuedWithdrawal(address recipient, QueuedWithdrawal calldata queuedWithdrawal, IERC20[] calldata tokens, uint256[] calldata indicesToSkip)

The following code disables calling slashQueuedWithdrawal function again with same withdrawalRoot

// reset the storage slot in mapping of queued withdrawals withdrawalRootPending[withdrawalRoot] = false;

//The following code skips the withdrawl. Any erroneous input to indicesToSkip parameter will permanently lock the tokens in the strategy as user will not get his burned shares back.

if (indicesToSkipIndex < indicesToSkip.length && indicesToSkip[indicesToSkipIndex] == i) { unchecked { ++indicesToSkipIndex; } }

Tools Used

VS Code

Recommended Mitigation Steps

Add indicesToSkip parameter to completeQueuedWithdrawal & completeQueuedWithdrawals functions. The skipped withdrawls must be recorded in unclaimedFunds data strucure that contains beneficiary address, strategy, and number of shares. Provide a function call for beneficiary to claim unclaimed funds from a strategy. Once funds are claimed, remove the information from unclaimedFunds data structure.

L1) The function claimableUserDelayedWithdrawals of DelayedWithDrawlRouter.sol is not functioning as per function specification comments.

https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/pods/DelayedWithdrawalRouter.sol#L109-L119

The comments section for claimableUserDelayedWithdrawals function says following //Getter function to get all delayedWithdrawals that are currently claimable by the user But the function doesn't actually returns currently claimable withdrawls as there is no block number verification. It actually returns all claimable withdrawls (withdrawls that can be claimed in current block + withdrawls that can be claimed in future blocks)

Mitigation: Add block number verification in the for loop as shown below. claimableDelayedWithdrawals array will have to be re-sized. if (block.number < claimableDelayedWithdrawals[i].blockCreated + withdrawalDelayBlocks) { break; }