Wrong Implementation ChainLinkPriceFeed leads to DOS of getOracleAssetPrice() function

[hats-bug-reporter[bot]]

Github username: @pavankv241 Submission hash (on-chain): 0x1fcc7602dddfa8a8f612375182276a1e37fc0937ecc9c576f4d8b03380f5d23c Severity: low severity

Description: Description: Wrong implementation of ChainLinkPriceFeed leads to DOS

Attack scenario: getOracleAssetPrice() function will calls Chainlink's latestRoundData API to get price of asset but it's not follows proper structure leads to Denial of service and chance of getting stale price . According to openzeppelin there’s no whitelisting mechanism to allow or disallow contracts from reading prices, powerful multisigs can tighten these access controls. In other words, the multisigs can immediately block access to price feeds at will. Therefore, to prevent denial of service scenarios, it is recommended to query ChainLink price feeds using a defensive approach with Solidity’s try/catch structure. In this way, if the call to the price feed fails, the caller contract is still in control and can handle any errors safely and explicitly.

Impact getOracleAssetPrice() may get dos and retruns stale price due to wrong implementation of ChainLinkPriceFeed.

Code snippet https://github.com/VMEX-finance/vmex/blob/master/packages/contracts/contracts/protocol/oracles/VMEXOracle.sol#LL224C1-L230C63 https://github.com/VMEX-finance/vmex/blob/master/packages/contracts/contracts/protocol/oracles/VMEXOracle.sol#LL154C1-L159C45

Reference https://blog.openzeppelin.com/secure-smart-contract-guidelines-the-dangers-of-price-oracles

Recommendation

            try IChainlinkPriceFeed(source).latestRoundData() returns( 
             uint80 roundID ,
                int256 price,
                /*uint startedAt*/,
                uint256 updatedAt,
                uint80 answeredInRound){
                require(answeredInRound >= roundId, 'stale price');
                 require(price > 0, 'invalid price'); 
                 return price;      
                } catch Error(string memory) {            
            // handle failure here:
            // revert, call propietary fallback oracle, fetch from another 3rd-party oracle, etc.
        }

[ksyao2002]

Thanks for the report. I agree that this is a low severity concern. Looks like some protocols just trust chainlink and don't implement this check (aave: https://github.com/aave/aave-v3-core/blob/29ff9b9f89af7cd8255231bc5faf26c3ce0fb7ce/contracts/misc/AaveOracle.sol#L4), but one can never be too safe.

[ksyao2002]

Sorry, I didn't realize this issue was already reported previously here https://github.com/hats-finance/VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/issues/3, meaning that this submission is a duplicate