Summary
Because stale data is preferred over a revert - if a primary oracle is disabled without backups getData would keep providing invalid data (0 value) to the rules until oracle is re-enabled.
Vulnerability Details
As the stated in the cyfrin report 7.2.4 in regards to oracles
A stale update is better than none because of the way the estimators encapsulate a weighted price and a certain level of smoothing
It currently returns optimized (main) oracle data even if it is stale. This makes the below comment left by the sponsors invalid
Return empty timestamp if oracle is no longer approved, result will be discarded
The result would not be discarded if the oracle is the main one and there are no backups.
function _getOracleData(OracleWrapper _oracle) private view returns (OracleData memory oracleResult) {
if (!approvedOracles[address(_oracle)]) return oracleResult;
(int216 data, uint40 timestamp) = _oracle.getData();
oracleResult.data = data;
oracleResult.timestamp = timestamp;
}
The consequence of this would be that 0 value data would be returned as a sucessful fetch and is not checked anywhere down the line apart from it's previously mentioned staleness.
Inside _getData oracle loop
for (uint i; i < oracleLength; ) {
OracleData memory oracleResult;
oracleResult = _getOracleData(OracleWrapper(optimisedOracles[i]));
if (oracleResult.timestamp > block.timestamp - oracleStalenessThreshold) {
outputData[i] = oracleResult.data;
} else {
unchecked {
numAssetOracles = poolBackupOracles[_pool][i].length;
}
for (uint j = 1 ; j < numAssetOracles; ) {
oracleResult = _getOracleData(
OracleWrapper(poolBackupOracles[_pool][i][j])
);
if (oracleResult.timestamp > block.timestamp - oracleStalenessThreshold) {
break;
} else if (j == numAssetOracles - 1) {
revert("No fresh oracle values available");
}
unchecked {
++j;
}
}
outputData[i] = oracleResult.data;
}
unchecked {
++i;
}
}
After _getData call finishes:
function _getUpdatedWeightsAndOracleData (...) {
data = _getData(_pool, true);
updatedWeights = rules[_pool].CalculateNewWeights(
_currentWeights,
data,
_pool,
_ruleSettings.ruleParameters,
_ruleSettings.lambda,
_ruleSettings.epsilonMax,
_ruleSettings.absoluteWeightGuardRail
);
...
After the above CalculateNewWeights call finishes the data is not used anywhere else.
The oracle approvals approvedOracles are fully controlled by the admins. But they can't control the oracle output. And in a case when a single main oracle goes rouge it would create a dillema where getData will always return invalid data - either by the rogue oracle returned data or by the functionality of a disabled oracle _getOracleData (0 value).
It would be a rare occurance where the main oracle of a pool to be purposefully turned off (removed from approvedOracles) but is possible.
A situation like this could also be introduced by aiming to disable a backup oracle of one pool, but missing that the same oracle is used as the main in another.
After the oracle is disabled and the performUpdate is called the weights will be updated to invalid ones.
Impact
getaData should never return invalid data. Only up to date data / stale data (special cases) / revert.
Any invalid weights would create new arbitrage opportunities as longs as invalid weights (0 data values) are continuesly submitted. And again once weights are fixed. But due to special conditions required - Medium.
Tools Used
Manual review + modified foundry MinVarianceUpdateRuleTest test, where the test passes with zero data value as the new data.
function testCalculateNewWeightsWith0NewData() public {
mockPool.setNumberOfAssets(2);
int256[][] memory parameters = new int256[][]();
parameters[0] = new int256[]();
parameters[0][0] = 0.5e18;
int256[] memory prevWeights = new int256[]();
prevWeights[0] = 0.5e18;
prevWeights[1] = 0.5e18;
int256[] memory data = new int256[]();
data[0] = 0;
data[1] = 0;
int256[] memory variances = new int256[]();
variances[0] = PRBMathSD59x18.fromInt(1);
variances[1] = PRBMathSD59x18.fromInt(1);
int256[] memory prevMovingAverages = new int256[]();
prevMovingAverages[0] = PRBMathSD59x18.fromInt(1);
prevMovingAverages[1] = PRBMathSD59x18.fromInt(1) + 0.5e18;
prevMovingAverages[2] = PRBMathSD59x18.fromInt(1);
prevMovingAverages[3] = PRBMathSD59x18.fromInt(1) + 0.5e18;
int128[] memory lambda = new int128[]();
lambda[0] = 0.7e18;
rule.initialisePoolRuleIntermediateValues(
address(mockPool),
prevMovingAverages,
variances,
mockPool.numAssets()
);
rule.CalculateUnguardedWeights(prevWeights, data, address(mockPool), parameters, lambda, prevMovingAverages);
int256[] memory res = rule.GetResultWeights();
int256[] memory ex = new int256[]();
ex[0] = 0.548283261802575107e18;
ex[1] = 0.451716738197424892e18;
console2.log("with zero weights", res[0]);
console2.log("with zero weights", res[1]);
checkResult(res, ex);
}
Recommendations
Oracles that are disabled should not return any result and be immediately discarded. A 0 value may be rare, but in theory is possible for extremly low value coins. So we check both data and timestamp for validity:
A solution would be to add an extra check at the end of the loop to see if the final result is not empty.
function _getData(address _pool, bool internalCall) private view returns (int256[] memory outputData) {
require(internalCall || (approvedPoolActions[_pool] & MASK_POOL_GET_DATA > 0), "Not allowed to get data");
address[] memory optimisedOracles = poolOracles[_pool];
uint oracleLength = optimisedOracles.length;
uint numAssetOracles;
outputData = new int256[](oracleLength);
uint oracleStalenessThreshold = IQuantAMMWeightedPool(_pool).getOracleStalenessThreshold();
for (uint i; i < oracleLength; ) {
OracleData memory oracleResult;
oracleResult = _getOracleData(OracleWrapper(optimisedOracles[i]));
if (oracleResult.timestamp > block.timestamp - oracleStalenessThreshold) {
outputData[i] = oracleResult.data;
} else {
unchecked {
numAssetOracles = poolBackupOracles[_pool][i].length;
}
for (uint j = 1 ; j < numAssetOracles;) {
oracleResult = _getOracleData(
OracleWrapper(poolBackupOracles[_pool][i][j])
);
if (oracleResult.timestamp > block.timestamp - oracleStalenessThreshold) {
break;
} else if (j == numAssetOracles - 1) {
revert("No fresh oracle values available");
}
unchecked {
++j;
}
}
if (oracleResult.data == 0 && oracleResult.timestamp == 0) {
revert("No oracles approved or returned no values");
}
outputData[i] = oracleResult.data;
}
unchecked {
++i;
}
}
}
This is similar to what OracleWrapper already checks inside if the oracle is approved:
function getData() public view returns (int216 data, uint40 timestamp) {
(data, timestamp) = _getData();
require(timestamp > 0, "INVORCLVAL");
}
