Asset Price and Deposit Amount Scaling Could be Different Than 1e18

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The vulnerability identified concerns the assumption that both deposit amount and lrtOracle.getAssetPrice(asset) in LRTDepositPool.getRsETHAmountToMint() are scaled to 1e18 (Wei standard in Ethereum).

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

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

In scenarios involving multiple asset types, this assumption may not hold true, as different assets can have varying decimal places. For instance, many ERC-20 tokens use different scalings (e.g., 6, 8, or even above 18 decimal places).

Additionally, not all prices returned by Chainlink are 18 decimals. Apparently, asset prices returned by the following function could be 8 or 18 decimals per Chainlink documentation:

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

    /// @notice Fetches Asset/ETH exchange rate
    /// @param asset the asset for which exchange rate is required
    /// @return assetPrice exchange rate of asset
    function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) {
        return AggregatorInterface(assetPriceFeed[asset]).latestAnswer();
    }

Incorrectly assuming a uniform scale could lead to significant discrepancies in calculations, impacting the accuracy of minted RSETH tokens and the overall economic logic of the protocol. This issue poses a risk of financial inaccuracies, potential exploits, and could undermine the trust and integrity of the system.

Proof of Concept

Scenario 1:

• Token A decimal() equals 18 whereas token B decimal() is 6.
• Users depositing asset A will be minted more RSETH compared to users minting through asset B

Scenario 2:

• Supposing token A and token B each has its decimal() equal 18.
• However, getAssetPrice() returns price A with 8 decimals while returning price B with 18 decimals.
• Users depositing asset A will be minted less RSETH compared to users minting through asset B

Scenario 3:

• The combination of Scenario 1 and Scenario 2.

Tools Used

Manual

Recommended Mitigation Steps

Consider normalizing all asset amounts and Chainlink price feeds to 18 decimals. Unnormalize the asset amount when redeeming RSETH right after burning and prior to transferring asset to the staker.

Assessed type

Decimal

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Probably wouldn't apply to LST tokens, but Chainlink maybe.

[c4-pre-sort]

raymondfam marked the issue as duplicate of #479

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)