LST with different decimals for Chainlink Price Feeds will mint less Amount of RSETH to users.

[c4-submissions]

Lines of code

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

Vulnerability details

Chainlink Price Feeds does not maintain a consistent pattern for the number of decimals in returned prices. While some feeds provide token prices with 8 decimals, others may have 18 decimals or more. However, the contracts assume that prices will always be provided with 18 decimals. This could lead to discrepancies in the amount of tokens minted to users, potentially resulting in losses for them. While the price feeds for the initially supported token rETH, cbETH and stETH does have 18 decimals price feeds, but there are chances that in future new tokens are added with different decimals price feeds.

Impact

Some Chainlink price feeds return prices in different decimal places, such as 8 or 18, which may not align with the token's decimal representation. For example, USDC follows 6 decimal places, but the price feeds for USDC/USD return prices in 8 decimal places. You can check the information using the following links:

• Chainlink Price Feeds for USDC/USD: https://docs.chain.link/data-feeds/price-feeds/addresses?network=ethereum&page=1&search=usdc%2Fusd
• USDC Contract on Etherscan: https://etherscan.io/token/0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48#code

So there are good chances that the price feed for the newly added LST follows different decimal representation than the decimal representation of the LST itself.

This inconsistency can result in the amount of tokens minted to the user being less than the expected amount. The incorrect result is calculated by the following functions:

File: 2023-11-kelp/src/LRTDepositPool.sol

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

Github: 109

File: 2023-11-kelp/src/LRTOracle.sol

 function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

        if (rsEthSupply == 0) {
            return 1 ether;
        }

        uint256 totalETHInPool;
        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        uint256 supportedAssetCount = supportedAssets.length;

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

Github: 71

Proof of Concept

Here is a test for PoC

    function test_ProtocolWillNotWorkWithChainlinkPriceFeedsWithDifferentDecimalValues() public {
        // adding new LSToken
        addNewLST();

        uint256 amount = 500 ether;
        // updating decimals of the mock price aggregator.
        // this is not possible to do in original price aggregator. It's just a mock funciton to
        // simulate the condition. This stil represents 1 ETH : 1 LST
        mockPriceAggregator.updateDecimals(1e8);

        // checking if the decimals are updated
        assertEq(mockPriceAggregator.decimals(), 1e8, "decimals are not same");

        // getting the RSETH amount to be minted for `amount` of tokens
        // the actual amount minted will be less than `rsEthMintedForAmount` because there is
        // another bug in the code that will make the amount less than `rsEthMintedForAmount`
        uint256 rsEthMintedForAmount = lrtDepositPoolP.getRsETHAmountToMint(address(mLst), amount);

        console2.log("> Details for the Amount to be Mint for LST deposit");
        console2.log("\tAmount To Deposit: %s", amount);
        console2.log("\tAmount of RSETH Tokens to Recieve: %s\n", rsEthMintedForAmount);
    }

Link to the Original Test: Test

Outupt:

Running 1 test for test/Integration.t.sol:BaseIntegrationTest
[PASS] test_ProtocolWillNotWorkWithChainlinkPriceFeedsWithDifferentDecimalValues() (gas: 425074)
Logs:
  > Details for the Amount to be Mint for LST deposit
        Amount To Deposit: 500000000000000000000
        Amount of RSETH Tokens to Recieve: 50000000000


Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.15ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Note
The actual amount minted will be different as there is another vulnerability in the function

As you can see in the output, for 500e18 LST tokens we are only receiving 0.00000005 RSETH.

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

To address the issue of inconsistent decimals in the RSETH minting calculation, it is recommended to do the following changes in the contracts:

1. Add a mapping in the contracts to represent the price feeds decimals corresponding to each asset.
2. Modify the LRTDepositPool::getRsETHAmountToMint(...) and LRTOracle::getRSETHPrice(...) functions to use the price feed decimals for the asset when calculating the amount of RSETH to be minted.

Here's an example of how the changes could be implemented:

File: 2023-11-kelp/src/LRTDepositPool.sol

+    mapping(address asset => uint256 priceFeedDecimals) public priceFeedDecimals

    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) * (18 - priceFeedDecimals(asset))) / lrtOracle.getRSETHPrice();
    }

File: 2023-11-kelp/src/LRTOracle.sol
    
    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

        if (rsEthSupply == 0) {
            return 1 ether;
        }

        uint256 totalETHInPool;
        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        uint256 supportedAssetCount = supportedAssets.length;

        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;
+            totalETHInPool += totalAssetAmt * assetER * (18 - ILRTDepositPool(lrtDepositPoolAddr).priceFeedDecimals(asset));

            unchecked {
                ++asset_idx;
            }
        }
        // @audit can we manipulate the price and get more rsEth? or can we manipulate the price and make the rsEth more
        // expensive?
        return totalETHInPool / rsEthSupply;
    }

By adding the priceFeedDecimals mapping and modifying the calculation, we ensure that the RSETH amount is calculated correctly based on the decimals provided by the price feeds. This mitigates the potential issue of inconsistent decimals and ensures that users receive the correct amount of RSETH for their deposits.

The changes also needs to be done in other functions as well that are using this data.

Assessed type

Decimal

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #97

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as primary issue

[fatherGoose1]

Confirming as Medium. This issue highlights the potential for miscalculation for LSTs added in the future.

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 marked the issue as selected for report

[thangtranth]

@fatherGoose1 :
The price feeds with ETH as quote tokens (X/ETH) always have 18 decimals. It would be valid if the warden can show a not-yet-supported LST/ETH priceFeed with with a decimal count differing from 18.

The example in the issue is incorrect (USDC/USD pricefeed). The issue is purely speculative and has not yet been observed in practice, so it is appropriate to classify as QA.

[TradMod]

I agree @thangtranth. Actually, I think this is not even a QA finding. USDC example is completely out of the box. If it will be added in the future then it should be in-scope of the future audits. This is clearly OOS. The fix will only gonna increase code complexity unnecessarily.

[AtanasDimulski]

I completely agree with @thangtranth and @DevABDee, I would also like to add that tokens that can interact with the Kelp DAO, are first whitelisted by the team. First, there is no guarantee that new tokens will be added at all, secondly, the possibility of Chainlink deciding to add a price feed for new LST/ETH with different decimals is highly unlikely. Most importantly issues of possible future upgrades have never been accepted, otherwise saying that the protocol may add a function that transfers all funds to my address should also be considered a valid finding. In my opinion, this is NC, Low at best.

[manoj9april]

Yes I agree with other wardens. We can assign it a low severity.

[PlamenTSV]

This is one of the reasons I didn't submit this - I couldn't find an LST with different than 18 decimals and it would be highly unlikely that one would arise and gain enough popularity. So I agree this is QA

[radeveth]

#122 (comment)

[fatherGoose1]

Thank you @thangtranth. This will be downgraded to QA despite sponsor's agreement with medium severity. After looking at Chainlink's available price feeds, I confirm that 18 decimals are the standard and it would not make sense for them to support an LST price feed with other than 18 decimals.

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[c4-judge]

fatherGoose1 marked the issue as grade-b

[c4-judge]

fatherGoose1 marked the issue as not selected for report