Kelp DAO | rsETH - btk's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L79 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L83

Vulnerability details

Impact

Deriving the price with balanceOf() is dangerous as it can easily be manipulated by direct transfers.

Proof of Concept

In the getAssetDistributionData() function, the asset lying in the LRTDepositPool is retrieved using balanceOf(address(this)):

        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));

Similarly, the assets in :NodeDelegator are obtained through balanceOf(nodeDelegator):

assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);

Deriving the price with balanceOf is dangerous as balanceOf may be gamed. Consider univ2 as an example; Exploiters can send tokens into the pool and pump the price, successfully manipulating the price.

Similar finding: https://solodit.xyz/issues/m-05-pair-price-may-be-manipulated-by-direct-transfers-code4rena-caviar-caviar-contest-git

Tools Used

Manual review

Recommended Mitigation Steps

Consider tracking balances, using state variables, similarly to how Uniswap V2.

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L119-L144

Vulnerability details

Impact

There is no slippage control on depositAsset() of LRTDepositPool, which expose user to sandwich attack.

Proof of Concept

Any deposit can be sandwiched in LRTDepositPool, especially when the pool is not balanced.

Exploit Scenario:

Bob, a normal user, calls depositAsset(). Since there is no minAmountOut, which means that the deposit can be executed at any price. As a result, when Eve sandwiches the deposit , Bob deposit the tokens without minting any, effectively giving away tokens for free.

A Detailed Guide To Sandwich Attacks In DeFi.

Tools Used

Recommended Mitigation Steps

Add a minAmountOut in depositAsset()

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L73-L89

Vulnerability details

Impact

The inability of LRTConfig to remove old assets can result in a potential DoS on LRTOracle.

Proof of Concept

The Manager can introduce a new asset to LRTConfig using:

    function _addNewSupportedAsset(address asset, uint256 depositLimit) private {
        UtilLib.checkNonZeroAddress(asset);
        if (isSupportedAsset[asset]) {
            revert AssetAlreadySupported();
        }
        isSupportedAsset[asset] = true;
        supportedAssetList.push(asset);
        depositLimitByAsset[asset] = depositLimit;
        emit AddedNewSupportedAsset(asset, depositLimit);
    }

LRTOracle's getRSETHPrice() function iterates through all assets to retrieve prices. If one of these assets, especially an upgradeable contract, is compromised, an attacker can DoS on LRTOracle by manipulating certain view functions to consistently revert, such as in the example below:

assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));

Tools Used

Manual review

Recommended Mitigation Steps

Add a function to allow the manager to remove an asset.

Assessed type

Other