EigenLayer Contest - Aymen0909's results

QA Report

Summary

Findings

1- claimableUserDelayedWithdrawals does not take into account withdrawalDelayBlocks :

Risk : Low

The claimableUserDelayedWithdrawals function from the DelayedWithdrawalRouter contract is used to get all the delayedWithdrawals that are currently claimable by the user, but the function does not take into account that delayedWithdrawals can only be claimed after withdrawalDelayBlocks from their creation time (blockCreated), the result of this error is that the function will return all the users's unclaimed delayedWithdrawals even if some of them are not yet claimable at that time.

The condition for a delayedWithdrawal to be claimable is highlighted in the canClaimDelayedWithdrawal function below :

 function canClaimDelayedWithdrawal(address user, uint256 index) external view returns (bool) {
    return ((index >= _userWithdrawals[user].delayedWithdrawalsCompleted) && (block.number >= _userWithdrawals[user].delayedWithdrawals[index].blockCreated + withdrawalDelayBlocks));
}

Basically there 2 condition : delayedWithdrawal index is greater than delayedWithdrawalsCompleted and withdrawalDelayBlocks has passed from the creation of the delayedWithdrawal.

As you can see in the claimableUserDelayedWithdrawals function below, the first condition is verified as the function finds only the users's unclaimed delayedWithdrawals (calculated with claimableDelayedWithdrawalsLength) but the second condition is not verified as there is no check against withdrawalDelayBlocks for each delayedWithdrawal.

function claimableUserDelayedWithdrawals(address user) external view returns (DelayedWithdrawal[] memory) {
    uint256 delayedWithdrawalsCompleted = _userWithdrawals[user].delayedWithdrawalsCompleted;
    uint256 delayedWithdrawalsLength = _userWithdrawals[user].delayedWithdrawals.length;
    // @audit get all unclaimed delayedWithdrawals without 
    uint256 claimableDelayedWithdrawalsLength = delayedWithdrawalsLength - delayedWithdrawalsCompleted;
    DelayedWithdrawal[] memory claimableDelayedWithdrawals = new DelayedWithdrawal[](claimableDelayedWithdrawalsLength);
    for (uint256 i = 0; i < claimableDelayedWithdrawalsLength; i++) {
        // @audit No check against `withdrawalDelayBlocks`
        claimableDelayedWithdrawals[i] = _userWithdrawals[user].delayedWithdrawals[delayedWithdrawalsCompleted + i];
    }
    // @audit will return all unclaimed delayedWithdrawals without checking against `withdrawalDelayBlocks`
    return claimableDelayedWithdrawals;
}

This issue does not currently have an impact on the protocol as the function is external and not used inside it, but it would if in the future other contracts rely on it, add to that this error will cause problems for users or other protocols when using this function in their logic.

Mitigation

I recommend to check if each DelayedWithdrawal is ready to be claimed in the claimableUserDelayedWithdrawals function, the function should be modified as follows :

function claimableUserDelayedWithdrawals(address user) external view returns (DelayedWithdrawal[] memory) {
    uint256 delayedWithdrawalsCompleted = _userWithdrawals[user].delayedWithdrawalsCompleted;
    uint256 delayedWithdrawalsLength = _userWithdrawals[user].delayedWithdrawals.length;
    uint256 claimableDelayedWithdrawalsLength = delayedWithdrawalsLength - delayedWithdrawalsCompleted;
    DelayedWithdrawal[] memory claimableDelayedWithdrawals = new DelayedWithdrawal[](claimableDelayedWithdrawalsLength);

    uint256 index = 0; // counter for really claimable DelayedWithdrawals
    for (uint256 i = 0; i < claimableDelayedWithdrawalsLength; i++) {
        // check if withdrawalDelayBlocks has passed for delayedWithdrawal creation
        if (block.number >= _userWithdrawals[user].delayedWithdrawals[i].blockCreated + withdrawalDelayBlocks){
            claimableDelayedWithdrawals[index] = _userWithdrawals[user].delayedWithdrawals[delayedWithdrawalsCompleted + i];
            index++;
        }
    }
    return claimableDelayedWithdrawals;
}

2- sharesToUnderlying and underlyingToShares functions marked as view but docs say they can make changes :

Risk : Low

The functions sharesToUnderlying and underlyingToShares are both marked as view but their respective comments say that they may make state modifications, because the functions are marked view in the StrategyBase contract any strategy that inherits from it will not be able to change the functions mutability and so those functions will never be able to make state modifications.

If this is just an error in the comments then it must be corrected but if the functions are really supposed to be able to make state modifications then the code must be reviewed to enable that.

Proof of Concept

Instances include:

File: StrategyBase.sol Line 180-188

/**
* @notice Used to convert a number of shares to the equivalent amount of underlying tokens for this strategy.
* @notice In contrast to `sharesToUnderlyingView`, this function **may** make state modifications
* @param amountShares is the amount of shares to calculate its conversion into the underlying token
* @dev Implementation for these functions in particular may vary signifcantly for different strategies
*/
function sharesToUnderlying(uint256 amountShares) public view virtual override returns (uint256) {
    return sharesToUnderlyingView(amountShares);
}

File: StrategyBase.sol Line 205-213

/**
* @notice Used to convert an amount of underlying tokens to the equivalent amount of shares in this strategy.
* @notice In contrast to `underlyingToSharesView`, this function **may** make state modifications
* @param amountUnderlying is the amount of `underlyingToken` to calculate its conversion into strategy shares
* @dev Implementation for these functions in particular may vary signifcantly for different strategies
*/
function underlyingToShares(uint256 amountUnderlying) external view virtual returns (uint256) {
    return underlyingToSharesView(amountUnderlying);
}

3- slashQueuedWithdrawal and slashShares functions should emit an event :

Risk : Low

The function slashShares is used to slash a given operator/staker shares and the function slashQueuedWithdrawal is used to slash the shares being withdrawen in the QueuedWithdrawal, both functions should emit an event to acknowledge stakers that there shares where slashed and this event can also be used by external dapps to triggers changes in their states.

Mitigation

Consider emitting events in both slashQueuedWithdrawal and slashShares functions.

4- Immutable state variables lack zero address checks :

Constructors should check the values written in an immutable state variables(address) is not the address(0).

Risk : Low

Proof of Concept

Instances include:

File: StrategyManagerStorage.sol Line 73-75

delegation = _delegation;
eigenPodManager = _eigenPodManager;
slasher = _slasher;

File: EigenPodManager.sol Line 77-80

ethPOS = _ethPOS;
eigenPodBeacon = _eigenPodBeacon;
strategyManager = _strategyManager;
slasher = _slasher;

File: StrategyBase.sol Line 47

strategyManager = _strategyManager;

Mitigation

Add non-zero address checks in the constructors for the instances aforementioned.

5- Adding a return statement when the function defines a named return variable is redundant :

Risk : Non critical

Proof of Concept

Instances include:

File: StrategyManager.sol Line 671

File: StrategyBase.sol Line 111

Mitigation

Either use the named return variables inplace of the return statement or remove them.

Gas Optimizations

Summary

Findings

1- Use uint96 for withdrawalDelayBlocks and pack its value with the address variable strategyWhitelister:

Because the maximum value allowed for withdrawalDelayBlocks variable is MAX_WITHDRAWAL_DELAY_BLOCKS = 50400 its respective value will not overflow 2^96, so the variable withdrawalDelayBlocks can be of type uint96 (or even less uint64, uint32) and its value can subsequently be packed with the storage variable (address) strategyWhitelister to save gas (Saves 1 SLOT: 2k gas).

The following line of codes must be changed :

File: StrategyManagerStorage.sol Line 36-44

/// @notice Permissioned role, which can be changed by the contract owner. Has the ability to edit the strategy whitelist
address public strategyWhitelister;

/**
* @notice Minimum delay enforced by this contract for completing queued withdrawals. Measured in blocks, and adjustable by this contract's owner,
* up to a maximum of `MAX_WITHDRAWAL_DELAY_BLOCKS`. Minimum value is 0 (i.e. no delay enforced).
* @dev Note that the withdrawal delay is not enforced on withdrawals of 'beaconChainETH', as the EigenPods have their own separate delay mechanic
* and we want to avoid stacking multiple enforced delays onto a single withdrawal.
*/
uint256 public withdrawalDelayBlocks;

The optimized code will be :

/// @notice Permissioned role, which can be changed by the contract owner. Has the ability to edit the strategy whitelist
address public strategyWhitelister;

/**
* @notice Minimum delay enforced by this contract for completing queued withdrawals. Measured in blocks, and adjustable by this contract's owner,
* up to a maximum of `MAX_WITHDRAWAL_DELAY_BLOCKS`. Minimum value is 0 (i.e. no delay enforced).
* @dev Note that the withdrawal delay is not enforced on withdrawals of 'beaconChainETH', as the EigenPods have their own separate delay mechanic
* and we want to avoid stacking multiple enforced delays onto a single withdrawal.
*/
uint96 public withdrawalDelayBlocks;

2- Multiple address/IDs mappings can be combined into a single mapping of an address/id to a struct :

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There are 5 instances of this issue :

File: StrategyManagerStorage.sol

25:     mapping(address => uint256) public nonces;
49:     mapping(address => mapping(IStrategy => uint256)) public stakerStrategyShares;
51:     mapping(address => IStrategy[]) public stakerStrategyList;
55:     mapping(address => uint256) public numWithdrawalsQueued;
68:     mapping(address => uint256) public beaconChainETHSharesToDecrementOnWithdrawal;

These mappings could be refactored into the following struct and mapping for example :

struct Staker {
    uint256 nonces;
    uint256 numWithdrawalsQueued;
    uint256 beaconChainETHSharesToDecrementOnWithdrawal;
    IStrategy[] stakerStrategyList;
    mapping(IStrategy => uint256) stakerStrategyShares;
}
    
mapping(uint256 => Staker) public stakers;

3- Reorder the check statements in the _completeQueuedWithdrawal function :

The checks statements at the start of the _completeQueuedWithdrawal function should be reodered to save gas when the call reverts, in fact as the slasher.canWithdraw check makes an external call to anther contract it should be the last check to allow the transaction to revert early without costing more gas (it doesn't make sense to call slasher.canWithdraw and then to check if the withdrawer address is correct for example), and its the same for the check on withdrawalRootPending which require the call to the calculateWithdrawalRoot function beforehand.

The issue occurs in :

File: StrategyManager.sol Line 746-769

// find the withdrawalRoot
bytes32 withdrawalRoot = calculateWithdrawalRoot(queuedWithdrawal);

require(
    withdrawalRootPending[withdrawalRoot],
    "StrategyManager.completeQueuedWithdrawal: withdrawal is not pending"
);

require(
    slasher.canWithdraw(queuedWithdrawal.delegatedAddress, queuedWithdrawal.withdrawalStartBlock, middlewareTimesIndex),
    "StrategyManager.completeQueuedWithdrawal: shares pending withdrawal are still slashable"
);

// enforce minimum delay lag (not applied to withdrawals of 'beaconChainETH', since the EigenPods enforce their own delay)
require(queuedWithdrawal.withdrawalStartBlock + withdrawalDelayBlocks <= block.number 
        || queuedWithdrawal.strategies[0] == beaconChainETHStrategy,
    "StrategyManager.completeQueuedWithdrawal: withdrawalDelayBlocks period has not yet passed"
);

require(
    msg.sender == queuedWithdrawal.withdrawerAndNonce.withdrawer,
    "StrategyManager.completeQueuedWithdrawal: only specified withdrawer can complete a queued withdrawal"
);

The check statements should be rearranged as follow to save gas :

require(
    msg.sender == queuedWithdrawal.withdrawerAndNonce.withdrawer,
    "StrategyManager.completeQueuedWithdrawal: only specified withdrawer can complete a queued withdrawal"
);

// enforce minimum delay lag (not applied to withdrawals of 'beaconChainETH', since the EigenPods enforce their own delay)
require(queuedWithdrawal.withdrawalStartBlock + withdrawalDelayBlocks <= block.number 
        || queuedWithdrawal.strategies[0] == beaconChainETHStrategy,
    "StrategyManager.completeQueuedWithdrawal: withdrawalDelayBlocks period has not yet passed"
);

// find the withdrawalRoot
bytes32 withdrawalRoot = calculateWithdrawalRoot(queuedWithdrawal);

require(
    withdrawalRootPending[withdrawalRoot],
    "StrategyManager.completeQueuedWithdrawal: withdrawal is not pending"
);

require(
    slasher.canWithdraw(queuedWithdrawal.delegatedAddress, queuedWithdrawal.withdrawalStartBlock, middlewareTimesIndex),
    "StrategyManager.completeQueuedWithdrawal: shares pending withdrawal are still slashable"
);

4- Emit events outside the for loops :

Putting an event inside a for loop means that the event will be emitted at each iteration which will cost a lot of gas in the end, you should consider emitting a global event outside the for loop when possible.

There is 1 instance of this issue :

File: StrategyManager.sol Line 376

emit ShareWithdrawalQueued(msg.sender, nonce, strategies[i], shares[i]);

The event in the code linked above is emitted at each iteration which can be avoided by emitting a final event after the for loop containing all the strategies and correspanding withdrawed shares as follows :

emit SharesWithdrawalQueued(msg.sender, nonce, strategies, shares);

5- storage variable should be cached into memory variables instead of re-reading them :

The instances below point to the second+ access of a state variable within a function, the caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read, thus saves 100gas for each instance.

There are 4 instances of this issue :

File: StrategyBase.sol Line 92-105

In the code linked above the value of totalShares is read multiple times (3) from storage and it's value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: StrategyBase.sol Line 173-176

In the code linked above the value of totalShares is read multiple times (2) from storage and it's value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: StrategyBase.sol Line 198-201

In the code linked above the value of totalShares is read multiple times (2) from storage and it's value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: EigenPod.sol Line 333-355

In the code linked above the value of validatorStatus[validatorIndex] is read multiple times (2) from storage and it's value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

6- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There are 3 instances of this issue:

File: StrategyManager.sol Line 193

amount -= debt;

In the code linked above the decrement of the amount value by debt cannot underflow because of the operation in line 190 which ensures that we always have amount >= debt, so the operation should be marked as unchecked to save gas.

File: StrategyManager.sol Line 827

amount -= amountToDecrement;

In the code linked above the decrement of the amount value by amountToDecrement cannot underflow because of the check in line 824 ensures that we always have amount > amountToDecrement, so the operation should be marked as unchecked to save gas.

File: StrategyManager.sol Line 829

beaconChainETHSharesToDecrementOnWithdrawal[staker] = (amountToDecrement - amount);

The operation in the code linked above cannot underflow because of the check in line 824, so it should be marked as unchecked to save gas.

7- Using delete statement can save gas :

When reseting a variable to its default value (0 for uint or false for boolean) it's cost less gas to use the delete operator rather than assigning the default value to the variable.

There are 4 instances of this issue :

File: StrategyManager.sol Line 554

withdrawalRootPending[withdrawalRoot] = false;

File: StrategyManager.sol Line 612

strategyIsWhitelistedForDeposit[strategiesToRemoveFromWhitelist[i]] = false;

File: StrategyManager.sol Line 772

withdrawalRootPending[withdrawalRoot] = false;

File: StrategyManager.sol Line 825

beaconChainETHSharesToDecrementOnWithdrawal[staker] = 0;