getRSETHPrice can return wrong price and let users mint at a different rate.

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

getRSETHPrice can return wrong price and let users mint at a different rate.

Proof of Concept

• Let's take a look at the getRSETHPrice function

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

• Let's say that the currently supported assets are cbETH, stETH and rETH.
• Now let's look at the for-loop

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

• It goes through all the supportedAssests (which equals 3) and calculates the value of each asset based on the current ER for the asset and the amount of the assets deposits.
• Let's say that there are 10 ETH deposited for each asset, 10 for cbETH, 10 for stETH and 10 for rETH.
• The totalETHInPool should result in 30 assuming the assetER retruns 1.
• But what if assetER returns 0 for one asset due to oracle failure?
• Let's say it returns 0 for the cbETH. Then totalETHInPool will be equal to 20 instead of 30.
• In the end, we get the price as return totalETHInPool / rsEthSupply;
• Now rsEthSupply should be 30 as we deposited the 30 eth and the resulting price would be 20 / 30 ~= 0.666666 ETH which is obviously wrong. (it should be 1 or very close to 1)
• This happens due to insufficient checks for the return data of the oracle call.

Tools Used

VS Code

Recommended Mitigation Steps

Make sure to check that oracle call didn't fail and that the exchange rate is not 0

for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
    address asset = supportedAssets[asset_idx];
    uint256 assetER = getAssetPrice(asset);
+   require(assetER != 0, "Oracle failure");
    
    uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
    totalETHInPool += totalAssetAmt * assetER;
    
    unchecked {
        ++asset_idx;
    }
}

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #32

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as duplicate of #34

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as duplicate of #215

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory:
Invalid

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)