c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L202 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L121 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L94

Vulnerability details

Impact

Previous strategy is overwritten and can cause tokens to be lost forever.

Proof of Concept

When the asset strategy is updated, it doesn't transfer the current strategy asset balance to the new one.

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

This futher translates to the getAssetBalance and getAssetBalances functions which inturn affects the returned value of the getTotalAssetDeposits and affects the rsETH pricing. See my other issues for more info.

 function getAssetBalance(address asset) external view override returns (uint256) {
        address strategy = lrtConfig.assetStrategy(asset);
        return IStrategy(strategy).userUnderlyingView(address(this)); //@note checks the for the exact strategy.
    }

Tools Used

Manual code review

Recommended Mitigation Steps

Use safeTransferFrom to send the asset balance to the new strategy.

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