Kelp DAO | rsETH - critical-or-high's results

Lines of code

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

Vulnerability details

Impact

An attacker could skew the RSETH price by donating LSTs (stETH, rETH, cbETH) to LTRDepositPool. The getRSETHPrice() calls ILRTDepositPool.getTotalAssetDeposits(asset) in a loop for each supported asset as follows.

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        ...
        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            ...
            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER;

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;
    }

Inside getTotalAssetDeposits(), the call to getAssetDistributionData(asset) returns the current balance of LST of LTRDepositPool inside the assetLyingInDepositPool variable.

    function getTotalAssetDeposits(address asset) public view override returns (uint256     totalAssetDeposit) {
        (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
            getAssetDistributionData(asset);
        return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer);
    }

    function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
        // Question: is here the right place to have this? Could it be in LRTConfig?
        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));
        ...
    }

Therefore, the RSETH price returned from getRSETHPrice() could be dramatically increased by a donation attack.

And an attacker, as a first depositor, can block further deposits to LTRDepositPool by forcing subsequent depositors to mint 0 RSETH tokens.

The attack scenario is the following:

1. The attacker, as the first depositor, calls depositAsset() with 1 wei of stETH as depositAmount and mints 1 wei of RSETH.
2. The attacker donates 1 stETH to LTRDepositPool.
3. So the price returned from getRSETHPrice() will be 1 ether * 10^18 instead of 1 ether.
4. A benign depositor will mint 0 RSETH because denominator is huge in the formula (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice().

Proof of Concept

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

Tools Used

Manual review.

Recommended Mitigation Steps

Use ERC4626.sol as a base contract for the LTRDepositPool which has built-in protection.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/oracles/ChainlinkPriceOracle.sol#L37

Vulnerability details

Impact

The getAssetPrice() function of ChainlinkPriceOracle uses the latestAnswer() call to get the price of an asset. LRTOracle in turn may use price returned from ChainlinkPriceOracle for LSTs (stETH, rETH, cbETH).

    function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) {
        return AggregatorInterface(assetPriceFeed[asset]).latestAnswer();
    }

Yet there are two problems with latestAnswer() usage inside getAssetPrice():

1. It can return 0 value in case of an issue with the price feed. Moreover, there are no checks for that scenario inside ChainlinkPriceOracle and other contracts of the protocol.
2. latestAnswer() is marked as deprecated and latestRoundData() should be used instead.

In case latestAnswer() returns 0 value for one asset (for example, for only stETH), it leads to a situation when a user may mint 0 RSETH when calling depositAsset().

    function getRsETHAmountToMint(
        address asset,
        uint256 amount
    )
        public
        view
        override
        returns (uint256 rsethAmountToMint)
    {
        // setup oracle contract
        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
        ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);

        // calculate rseth amount to mint based on asset amount and asset exchange rate
        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
    }

Where lrtOracle.getAssetPrice(asset) returns 0 for stETH. But lrtOracle.getRSETHPrice() is non-zero because 0 value is produced only for one asset - stETH.

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        ...

        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];
            uint256 assetER = getAssetPrice(asset);

            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER;

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;
    }

Proof of Concept

• https://github.com/code-423n4/2023-11-kelp/blob/main/src/oracles/ChainlinkPriceOracle.sol#L37

Tools Used

Manual review.

Recommended Mitigation Steps

Use latestRoundData() instead to retrieve the price. Perform checks that the returned price isn't stale and has non-zero value.

Assessed type

Oracle