venusprotocol/oracle

Venus oracle repo

oracle's Issues

Nitpicks for the oracle

I have reviewed the code carefully, and I'm really impressed by the code quality and consistency. @RoseRompkxm thank you for doing the great job here! Here are a couple of nitpicks I have, let's discuss them and address the issues. After that I'd say we're good to go with the audits.

1. We should handle reverts gracefully

Oracles may sometimes revert, e.g. when Chainlink drops the feeds support, they remove the implementation entirely and the call to functions like decimals() revert. Ideally, we should, ofc, monitor these events, but since we're working with external parties, sometimes things out of our control happen. I propose we handle such cases with try-catch blocks (i.e. try getting the underlying price and return INVALID_PRICE if the call reverts).

https://github.com/VenusProtocol/oracle/blob/develop/contracts/ResilientOracle.sol#L203
https://github.com/VenusProtocol/oracle/blob/develop/contracts/ResilientOracle.sol#L176

2. Incompatibilities with v0.8

These lines in PancakeLibrary suggest that the overflow is desired. However, 0.8 would throw an error in case of overflow:

oracle/contracts/libraries/PancakeLibrary.sol

Lines 49 to 55 in 5fb8988

	// subtraction overflow is desired
	uint32 timeElapsed = blockTimestamp - blockTimestampLast;
	// addition overflow is desired
	// counterfactual
	price0Cumulative += uint(FixedPoint.fraction(reserve1, reserve0)._x) * timeElapsed;
	// counterfactual
	price1Cumulative += uint(FixedPoint.fraction(reserve0, reserve1)._x) * timeElapsed;

If the overflow is desired, let's use an unchecked { ... } block here.

3. Let SafeMath die finally?

We're using solidity >= 0.8 here, so I propose we remove the dependency on SafeMath and use arithmetic operators directly.

4. Remove ancient code

        // @TODO: This is some history code, keep it here in case of messing up 
        } else if (_compareStrings(symbol, "XVS")) {
            return prices[address(vToken)];
        }

I don't know why we have it. Do we even have a vToken with "XVS" as a symbol? getUnderlyingPrice(XVS.address) returns zero now, assetPrices(XVS.address) returns zero as well 🤷

5. Typo :)

oracle/contracts/oracles/TwapOracle.sol

Line 157 in 5fb8988

require(tokenConfigs[vToken].vToken != address(0), "vTokne not exist");