Missing Oracle Validation

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L95-L110 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L45-L47 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/oracles/ChainlinkPriceOracle.sol#L37-L39

Vulnerability details

Summary

A deprecated function from chainlink is used, and no price checks are implemented at all. This could lead to huge MEV opportunities on the cost of other users if chainlink returns a stale price.

Vulnerability Details

When a user deposits an asset, the depositAsset function is called which calls _mintRsETH:

function depositAsset(
    address asset,
    uint256 depositAmount
)
    external
    whenNotPaused
    nonReentrant
    onlySupportedAsset(asset)
{
    ...

    uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

        ...
}

_mintRsETH calculates the rsETH amount to mint depending on the current asset price. Here we can see the flow of requesting the current asset price from chainlink:

function _mintRsETH(address _asset, uint256 _amount) private returns (uint256 rsethAmountToMint) {
    (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount);

    address rsethToken = lrtConfig.rsETH();
    // mint rseth for user
    IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint);
}

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

function getAssetPrice(address asset) public view onlySupportedAsset(asset) returns (uint256) {
    return IPriceFetcher(assetPriceOracle[asset]).getAssetPrice(asset);
}

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

As we can see, there isn’t any price validation and the deprecated latestAnswer function is used. This function returns stale prices without any information to check if it is a stale price. If the given price feed returns a stale price which is bigger than the current price, MEV bots would be able to take advantage of this. They could buy big amounts of the given token cheap and deposit it into the system for more rsETH than they should receive. By withdrawing it in another token (as soon as this is implemented) they would make profit and leave the rest of the users with tokens which are less worth than they should and therefore drain value from the other users tokens.

This is only one example of missing oracle validation, all the best practices from chainlink should be implemented like for example checking min / max answers and checking if the chainlink sequencer is down if the protocol is used on L2 chains.

Impact

Huge MEV opportunities occur if the given chainlink feed returns a stale price. This could lead to a big loss for other users, as their rsETH tokens lose value.

Tools Used

Manual Review

Recommendations

Implement chainlinks current best practices when requesting prices.

Assessed type

Oracle

[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 not a duplicate

[c4-pre-sort]

raymondfam marked the issue as duplicate of #843

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid