Upgraded Q -> 2 from #677 [1702060237936]

[c4-judge]

Judge has assessed an item in Issue #677 as 2 risk. The relevant finding follows:

[L-04] Deposited amounts in the EigenLayer strategy should be checked before updating the strategy for the asset Users deposit in this protocol and the protocol deposits these funds to EigenLayer strategy contracts. There are 3 supported assets (rETH, cbETH, stETH) at the moment and every asset has one strategy contract.

The protocol also has a maximum deposit limit for each asset. An asset’s total deposits are calculated by summing up the asset amounts in the pool, in node delegators and in the EigenLayer strategy.

The admin can update the strategy contract address for these assets. https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L109C1-L122C6

function updateAssetStrategy(
    address asset,
    address strategy
)
    external
    onlyRole(DEFAULT_ADMIN_ROLE)
    onlySupportedAsset(asset)
{
    UtilLib.checkNonZeroAddress(strategy);
    if (assetStrategy[asset] == strategy) {
        revert ValueAlreadyInUse();
    }
    assetStrategy[asset] = strategy;
}

As we can see above, this update is performed and the new strategy address is assigned without checking if there are already deposited assets in the previous strategy.

The biggest problem with that is the total asset deposits are calculated using the current EigenLayer strategy contract. https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L84 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L121C1-L124C6

file: NodeDelegator.sol //@audit Strategy contracts can be changed for assets. If the strategy is changed for an asset (before withdrawing the balance from previous strategy), // this function will return only the balance in the current strategy and the total deposits will be calculated lower than the actual. function getAssetBalance(address asset) external view override returns (uint256) { address strategy = lrtConfig.assetStrategy(asset); return IStrategy(strategy).userUnderlyingView(address(this)); } If the strategy address is updated without withdrawing deposits from the previous strategy, the totalAssetDeposits will be calculated lower than the actual deposit amount. This will lead to the getAssetCurrentLimit() function returning a bigger value than the actual limit and breaking the maximum deposit invariant.

Recommendation: I would recommend checking if there are already deposited assets in the EigenLayer strategy, and withdrawing them before updating the strategy (Or transferring deposits from the old strategy to the new strategy in EigenLayer, and updating the asset strategy in the same transaction with a multicall-like action).

[c4-judge]

fatherGoose1 marked the issue as duplicate of #197

[c4-judge]

fatherGoose1 marked the issue as satisfactory