LRTOracle and ChainlinkPriceOracle contracts getAssetPrice function do not take stale prices into account

[c4-submissions]

Lines of code

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/LRTOracle.sol#L45-L47 https://github.com/code-423n4/2023-11-kelp/blob/main/src/oracles/ChainlinkPriceOracle.sol#L37-L39

Vulnerability details

Impact

LRTOracle calls ChainlinkPriceOracle getAssetPrice function that returns the latest answer provided by a Chainlink data feed aggregator. As shown below:

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

The issue is latestAnswer may be a value provided by an oracle that has been idle for longer than its heartbeat (Chainlink's defined maximum amount of time without aggregator updates before the answer is stale). This implies the LRTOracle may utilize old data and become a target for profitable arbitrage strategies.

Proof of Concept

According to Chainlink: check-the-timestamp-of-the-latest-answer: the aggregator updates its latestAnswer when the value deviates beyond a specified threshold or when the heartbeat idle time has passed.

This means a contract should not trust the validity of the data before checking it.

Also according to Chainlink: Your application should track the latestTimestamp variable or use the updatedAt value from the latestRoundData() function to make sure that the latest answer is recent enough for your application to use it. If your application detects that the reported answer is not updated within the heartbeat or within time limits that you determine are acceptable for your application, pause operation or switch to an alternate operation mode while identifying the cause of the delay.

Tools Used

Manual review

Recommended Mitigation Steps

Define a threshold for price staleness and apply it then getting the latest round data before utilizing it for monetary operations.

interface AggregatorInterface {

    function latestAnswer() external view returns (uint256);
    function latestRoundData() external view returns(
        uint80 id,
        int256 answer,
        uint256 startedAt,
        uint256 updatedAt,
        uint80 answeredInRound
    );
}

function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) {
    (uint80 roundId, 
    int256 answer, 
    , 
    uint256 updatedAt, 
    uint80 answeredInRound) = AggregatorInterface(assetPriceFeed[asset]).latestRoundData();

    require(block.timestamp <= lastUpdatedAt + stalePriceDelay, "Stale price");
    require(answer > 0,"Invalid price");
    return uint256(answer);

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