Summary
Rules that require the previous average will suffer a permanent DoS upon performing any update
Vulnerability Details
When a pool is created, the _setInitialMovingAverages is executed with _initialMovingAverages having the double of elements as tokens registered in the pool. We can see that in the tests for the only rule that require previous averages: QuantAMMMinVariance.t.sol
function testCorrectUpdateWithLambdaPointFiveAndTwoWeights() public {
mockPool.setNumberOfAssets(2);
...
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()
);
...
}
In this test we can see that there are 2 tokens in the pool and the movingAverages will store 4 elements, the double.
The function that stores the data in the movingAverages storage variable is the following:
function _setInitialMovingAverages(
address _poolAddress,
int256[] memory _initialMovingAverages,
uint _numberOfAssets
) internal {
uint movingAverageLength = movingAverages[_poolAddress].length;
if (movingAverageLength == 0 || _initialMovingAverages.length == _numberOfAssets) {
movingAverages[_poolAddress] = _quantAMMPack128Array(_initialMovingAverages);
} else {
revert("Invalid set moving avg");
}
}
This function will essentially pack 2 subsequent elements into a single slot and then create an array of these packed slots.
This data is accessed and modified during CalculateNewWeights function that is executed during each update from the updateWeightRunner.
function CalculateNewWeights(
int256[] calldata _prevWeights,
int256[] calldata _data,
address _pool,
int256[][] calldata _parameters,
uint64[] calldata _lambdaStore,
uint64 _epsilonMax,
uint64 _absoluteWeightGuardRail
) external returns (int256[] memory updatedWeights) {
...
locals.currMovingAverage = _quantAMMUnpack128Array(movingAverages[_pool], locals.numberOfAssets);
...
}
In it, we can see that to retrieve the data from the movingAverages storage variable, it uses the _quantAMMUnpack128Array by passing the number of assets of the pool.
However, let's see what happens when there is the double of data stored:
function _quantAMMUnpack128Array(
int256[] memory _sourceArray,
uint _targetArrayLength
) internal pure returns (int256[] memory targetArray) {
require(_sourceArray.length * 2 >= _targetArrayLength, "SRC!=TGT");
targetArray = new int256[](_targetArrayLength);
uint targetIndex;
uint sourceArrayLengthMinusOne = _sourceArray.length - 1;
bool divisibleByTwo = _targetArrayLength % 2 == 0;
for (uint i; i < _sourceArray.length; ) {
targetArray[targetIndex] = _sourceArray[i] >> 128;
unchecked {
++targetIndex;
}
if ((!divisibleByTwo && i < sourceArrayLengthMinusOne) || divisibleByTwo) {
targetArray[targetIndex] = int256(int128(_sourceArray[i]));
}
unchecked {
++i;
++targetIndex;
}
}
...
}
Continuing with the previous example of the test where there are 2 tokens in the pool and 4 moving averages stored, the array of moving averages would look like this:
0 1
[ 1e18 | 1.5e18, 1e18 | 1.5e18]
In this function the new targetArray will be of 2 elements.
The loop will sweep with i values 0, 1 because the _sourceArray has also 2 elements.
In the first iteration when i starts being 0, it assigns the targetArray[0] to 1e18 and targetArray[1] to 1.5e18.
However, in the second iteration, targetIndex is 2 so it tries to set targetArray[2] but is out of bounds, so it reverts
Proof of Concept
-
A new pool gets created with a rule that requires the previous averages and initialisePoolRuleIntermediateValues gets called
The _setInitialMovingAverages saves the array of int256 that contain the double of moving averages of int128.
-
When the updateWeightRunner will try to perform an update, the CalculateNewWeights function will be called
-
The update will fail because the _quantAMMUnpack128Array function will fail due to an out of bounds error as explained above
Coded Proof of Concept
This bug would have been caught simply by calling the CalculateNewWeights function in any of the tests, however in all of them it uses a custom function called CalculateUnguardedWeights that does not revert but it is only implemented in the mock implementation.
As an example this test uses the same values as testCorrectUpdateWithLambdaPointFiveAndTwoWeights but it simply calls the CalculateNewWeights function:
function testPOCDoSWhenCalculateWeights() 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] = PRBMathSD59x18.fromInt(3);
data[1] = PRBMathSD59x18.fromInt(4);
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;
uint64[] memory lambda = new uint64[]();
lambda[0] = 0.7e18;
uint64 epsilonMax = 0.5e18;
uint64 absoluteWeightGuardRail = 0.4e18;
rule.initialisePoolRuleIntermediateValues(
address(mockPool),
prevMovingAverages,
variances,
mockPool.numAssets()
);
vm.expectRevert();
rule.CalculateNewWeights(prevWeights, data, address(mockPool), parameters, lambda, epsilonMax, absoluteWeightGuardRail);
}
The result of this test is a failure with an out of bound error.
[Revert] panic: array out-of-bounds access (0x32)
However, if the previous moving averages would not have the double of elements just the same as the tokens in the pool it does not revert:
function testPOCWorksWhenNoDuplicatedArrays() 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] = PRBMathSD59x18.fromInt(3);
data[1] = PRBMathSD59x18.fromInt(4);
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);
uint64[] memory lambda = new uint64[]();
lambda[0] = 0.7e18;
uint64 epsilonMax = 0.5e18;
uint64 absoluteWeightGuardRail = 0.4e18;
rule.initialisePoolRuleIntermediateValues(
address(mockPool),
prevMovingAverages,
variances,
mockPool.numAssets()
);
rule.CalculateNewWeights(prevWeights, data, address(mockPool), parameters, lambda, epsilonMax, absoluteWeightGuardRail);
}
This test passes successfully
Impact
High, the pool will not be able to get any update
Tools Used
Manual review
Recommendations
Adjust the _quantAMMUnpack128Array such that when the targetArray gets filled, just return it instead of continuing the loop.
