Fall back oracle does not validate timestamp and price range

[hats-bug-reporter]

Github username: @ArnieGod
Submission hash (on-chain): 0xed24816d80fe74fb2141567dcaf466be4481730401c0cdc77b99f79334f24cec
Severity: high severity

Description:

Vulnerability Report

Description

/**
     * @dev Gets an asset price for an asset with a chainlink aggregator
     * @param asset The asset address
     **/
    function getOracleAssetPrice(address asset) internal returns (uint256){
        IChainlinkPriceFeed source = _assetsSources[asset].feed;
        if (address(source) == address(0)) {
            return _fallbackOracle.getAssetPrice(asset);
        } else {
            (
                /* uint80 roundID */,
                int256 price,
                /*uint startedAt*/,
                uint256 updatedAt,
                /*uint80 answeredInRound*/
            ) = IChainlinkPriceFeed(source).latestRoundData();
            IChainlinkAggregator aggregator = IChainlinkAggregator(IChainlinkPriceFeed(source).aggregator());
            if (price > int256(aggregator.minAnswer()) && 
                price < int256(aggregator.maxAnswer()) && 
                block.timestamp - updatedAt < _assetsSources[asset].heartbeat
            ) {
                return uint256(price);
            } else {
                return _fallbackOracle.getAssetPrice(asset);
            }
        }
    }

the fallback oracle is used in any case when the primary oracle cannot be used, and this happens frequently. The problem here is that the code does not validate the price range and the timestamp of the last updated time for the fallback oracle as it should and this causes many problems.

Impact
Users may be unfairly liquidated and those their funds because of lack of fallback oracle timestamp and price range validation

Attachments

1. Proof of Concept

1. there is problem with primary oracle so the fallback must be used at timestamp 1,000
2. fallback oracle is used and it has not been updated since timestamp 500
3. since there is no validation of timestamp and or last updated time, this will allow prices from timestamp 500 to be used.
4. Let us assume that the price of eth was 1000 usd at timestamp 500 and is now 1200 usd at timestamp 1000.
5. the lack of validation will cause old prices to be used and can cause unfair liquidations for a user.

code snippet

VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/packages/contracts/contracts/protocol/oracles/VMEXOracle.sol

Lines 208 to 230 in fb396a3

	function getOracleAssetPrice(address asset) internal returns (uint256){
	IChainlinkPriceFeed source = _assetsSources[asset].feed;
	if (address(source) == address(0)) {
	return _fallbackOracle.getAssetPrice(asset);
	} else {
	(
	/* uint80 roundID */,
	int256 price,
	/*uint startedAt*/,
	uint256 updatedAt,
	/*uint80 answeredInRound*/
	) = IChainlinkPriceFeed(source).latestRoundData();
	IChainlinkAggregator aggregator = IChainlinkAggregator(IChainlinkPriceFeed(source).aggregator());
	if (price > int256(aggregator.minAnswer()) &&
	price < int256(aggregator.maxAnswer()) &&
	block.timestamp - updatedAt < _assetsSources[asset].heartbeat
	) {
	return uint256(price);
	} else {
	return _fallbackOracle.getAssetPrice(asset);
	}
	}
	}

recommendation
i recommend the protocol add code to validate timestamp and price range for the fallback oracle.

[ksyao2002]

The fallback oracle is outside the scope of this audit (it will be developed similar to Aave's fallback oracle at a later time). We already have planned to perform its own bounds checking of the timestamp and price range inside the fallback oracle.

There is no vulnerability.

[ksyao2002]

Sorry if there was any confusion on this. The OP argues that this contract is in scope. VMEXOracle.sol is indeed in scope, but the fallback oracle is not in scope. OP says that everything under protocol/ is under scope, but fallback oracle is not under that directory. Also, if you take a look at Aave's code, there's no bounds checking before entering the fallback oracle, as the fallback oracle itself does the checking. We have accepted valid answers as high, medium, and low already; we aren't using the pretense of "we are working on it later" as an excuse.

Let's take the current state of the codebase. The fallback oracle for now is simply a contract that reverts every call (see BaseUniswapOracle.sol), since we have not implemented it yet. Does there need to be bounds checking on that?