c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/oracles/ChainlinkPriceOracle.sol#L37-L39 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/oracles/ChainlinkPriceOracle.sol#L11-L13

Vulnerability details

Bug description

using the latestAnswer from chainlink Aggregator missed some sanity checks that should be added into the function getAssetPrice.

Impact

the getAssetPrice using the latestAnswer from chainlink Aggregator and it miss some important checks that should be added, these checks is added by the latestRoundData too and should be added here for the latestAnswer in getAssetPrice.

Proof of Concept

the function getAssetPrice and the AggregatorInterface :

interface AggregatorInterface {
    function latestAnswer() external view returns (uint256);
}

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

sanity checks should be added in the getAssetPrice function. this function is used in many places and its important to add these checks for it, it called in this line and this line and this line too

Tools Used

manual review

Recommended Mitigation Steps

add the checks for answeredInRound & price > 0 & updateTime != 0 :

if(answeredInRound < roundID){
    revert ERR()
}
if( price <= 0) {
    revert err2()
}
if (updateTime == 0) revert  err3()