c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L121-L124 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L109-L122

Vulnerability details

Impact

Function getAssetBalance() is responsible for fetching the balance of an asset that was deposited into strategy.

The issue with this function is that it will return incorrect result, when the strategy is changed.

Proof of Concept

Function updateAssetStrategy() updates only assetStrategy mapping. Whenever we update the strategy, there's no check if asset is already deposited to the previous strategy.

File: LRTConfig.sol

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;
    }

Let's consider an asset X which is using strategy STRATEGY_1. We call depositAssetIntoStrategy() to deposit that asset into that strategy. Now, let's consider a scenario when updateAssetStrategy() is being called and asset X points to STRATEGY_2 now. We call depositAssetInfoStrategy() to deposit that asset into another strategy this time.

This constitutes the problem, when we call getAssetBalance():

File: NodeDelegator.sol

function getAssetBalance(address asset) external view override returns (uint256) {
        address strategy = lrtConfig.assetStrategy(asset);
        return IStrategy(strategy).userUnderlyingView(address(this));
    }

As demonstrates above, function getAssetBalance() returns userUnderlyingView() only for STRATEGY_2 (updated one), even though we deposited asset X into two strategies.

Function getAssetBalance() does not take into consideration old strategies for the asset. Whenever we deposit asset into strategy, then change that strategy, function getAssetBalance() won't count deposit related to the previous strategy.

Tools Used

Manual code review

Recommended Mitigation Steps

Whenever the strategy for the asset is being updated, the previous deposited amount for that strategy should be stored and taken into account when getAssetBalance() returns the balance for that asset.

Assessed type

Other

c4-pre-sort commented 11 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #197

c4-judge commented 10 months ago

fatherGoose1 marked the issue as unsatisfactory: Invalid