leak of value due to false reporting of funds in specific case

[c4-submissions]

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#L17 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L109-L122

Vulnerability details

Vulnerability details:

Details:

the protocol uses the amount of assets held by in either LRTDepositPool, NodeDelegator, eigenLayerStrategies. it gets the amount held by eigenLayer through the function:

NodeDelegator

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

which basically gets how much of an asset is held by a eigenLayer strategy plus reward ans slashing. but the strategy is gotten from a mapping in LRTConfig which maps each asset to a strategy.

mapping(address token => address strategy) public override assetStrategy;

this mapping can be changed by admin:

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

so if an admin changes the asset strategy, the reported amount that is invested in eigenlayer would be one of the new strategy and not the old one.

currently eigenLayer supports one strategy per token, but it can change in the future and the protcol has the functionality to change the strategy of each token.

Impact:

if an admin changes a strategy to another , the reported funds held by the protocol would be lower meaning that the exchange rate of asset/RSETH would also be lower or even 0, meaning the next person to deposit will get minted 0(SEE POC), because the likelyhood is low but the impact is high this should get a medium severity.

Proof of Concept:

lets imagine this scenario: their are already depositors and the vault has 100 stETH given to eigenLayer strategy A

• eigenlayer deploys second strategy B which is better.
• admin changes the strategy of stETH to strategy B
• alice comes to deposit 10 stETH
• the protocol calculates exchangeRate=totalAssetsDeposited/rsETHMinted
• the protocol would look for how much is deposited in eigenlayer strategy , but because admin changed it to B, it would return 0 because no deposits in strategy B.
• which would mean alice gets minted 0.
• basically DOS the protocol until somone donates stETH to the pool. or admin changes strategy back.

this POC shows that if the admin changes the strategy of an asset , it will create real problems with accounting, this could be mitigated easily (see mitigation).

Tools Used:

vscode

Recommended Mitigation Steps:

should use the same functionality of getAssetBalances because it checks all the strategies that the nodeDelegator has deposited into and gets the amount deposited , but instead of returning for all assets should return only one like so :

function getAssetBalance(address asset)
        external
        view
        override
        returns (uint256 assetBalance)
    {
        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);

        (IStrategy[] memory strategies,) =
            IEigenStrategyManager(eigenlayerStrategyManagerAddress).getDeposits(address(this));

        uint256 strategiesLength = strategies.length;
        uint assetBalance;

        for (uint256 i = 0; i < strategiesLength;) {
            assets[i] = address(IStrategy(strategies[i]).underlyingToken());
            if (assets[i] == asset){
            assetBalance += IStrategy(strategies[i]).userUnderlyingView(address(this));
            }
            unchecked {
                ++i;
            }
        }
    }

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #111

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid