Setting the invert flag in OracleRef might have unexpected consequences

[cleanunicorn]

Description

The method setDoInvert sets a storage flag that affects the value read from the oracle.

The value determines if it is inverted according to doInvert

$$ f(x) = \begin{cases} 1/x & \quad \text{when doInvert is }true\\ x & \quad \text{otherwise} \end{cases} $$

review-volt-core-2022-09/code/contracts/refs/OracleRef.sol

Lines 108 to 111 in e37f6c8

	// Invert the oracle price if necessary
	if (doInvert) {
	_peg = invert(_peg);
	}

The value then proceeds to be scaled before being returned:

review-volt-core-2022-09/code/contracts/refs/OracleRef.sol

Lines 113 to 123 in e37f6c8

	// Scale the oracle price by token decimals delta if necessary
	uint256 scalingFactor;
	if (decimalsNormalizer < 0) {
	scalingFactor = 10**(-1 * decimalsNormalizer).toUint256();
	_peg = _peg.div(scalingFactor);
	} else {
	scalingFactor = 10**decimalsNormalizer.toUint256();
	_peg = _peg.mul(scalingFactor);
	}
	return _peg;

The storage slot doInvert is updated by calling the external method:

review-volt-core-2022-09/code/contracts/refs/OracleRef.sol

Lines 54 to 58 in e37f6c8

	/// @notice sets the flag for whether to invert or not
	/// @param newDoInvert the new flag for whether to invert
	function setDoInvert(bool newDoInvert) external override onlyGovernor {
	_setDoInvert(newDoInvert);
	}

This proceeds to call internally _setDoInvert:

review-volt-core-2022-09/code/contracts/refs/OracleRef.sol

Lines 140 to 149 in e37f6c8

	function _setDoInvert(bool newDoInvert) internal {
	bool oldDoInvert = doInvert;
	doInvert = newDoInvert;
	if (oldDoInvert != newDoInvert) {
	_setDecimalsNormalizer(-1 * decimalsNormalizer);
	}
	emit InvertUpdate(oldDoInvert, newDoInvert);
	}

It is not obvious nor documented that changing the invert value will also update the decimals normalizer.

Consider the example where the governance wants to update the oracle's invert and decimal scaling. To do that the following methods need to be executed:

• OracleRef.setDoInvert(bool)
• OracleRef.setDecimalsNormalizer(int256)

Depending on the order of the called methods you will have different final values for the decimals normalizer.

1. Updating doInvert first and decimalsNormalizer second

Initial values

doInvert = false;
decimalsNormalizer = 18;

Update values

doInvert(true);
setDecimalsNormalizer(-12);

Final values

doInvert = true;
decimalsNormalizer = -12;

2. Updating decimalsNormalizer first and doInvert second

We start from the same initial values

doInvert = false;
decimalsNormalizer = 18;

We update the values, but in a different order

setDecimalsNormalizer(-12);
doInvert(true);

The final values can be unexpected for the governance

doInvert = true;
decimalsNormalizer = 12;

Even though we set the decimals to be -12, because we also updated the invert flag, we obtained a different value for our decimals scaling. The order of operations determines our final value for the decimals scaling.

This time the decimalsNormalizer is updated first. Thus, we have decimalsNormalizer = -12.

When the method doInvert(true) is executed, the old value is first saved, and the flag doInvert is set to true:

review-volt-core-2022-09/code/contracts/refs/OracleRef.sol

Lines 140 to 142 in e37f6c8

	function _setDoInvert(bool newDoInvert) internal {
	bool oldDoInvert = doInvert;
	doInvert = newDoInvert;

After this, because the old value is different from the current value, the decimals normalizer is also inverted:

review-volt-core-2022-09/code/contracts/refs/OracleRef.sol

Lines 144 to 146 in e37f6c8

	if (oldDoInvert != newDoInvert) {
	_setDecimalsNormalizer(-1 * decimalsNormalizer);
	}

This might be unexpected because it's not obvious, it's "hidden" in the internal method and an event is not emitted.

We can clearly see how changing the order of the operations can unexpectedly update decimalsNormalizer.

Recommendation

Consider removing the logic related to updating decimalsNormalizer in the internal method _setDoInvert :

review-volt-core-2022-09/code/contracts/refs/OracleRef.sol

Lines 144 to 146 in e37f6c8

	if (oldDoInvert != newDoInvert) {
	_setDecimalsNormalizer(-1 * decimalsNormalizer);
	}