Summary
The UpdateRule contract contains a critical issue in the CalculateNewWeights function. When the requiresPrevAverage is true for a rule, the function attempts to handle both new and old moving averages. However, the unpacking method _quantAMMUnpack128Array is not correctly implemented for this case, leading to index out-of-bound errors when processing.
Vulnerability Details
The vulnerability arises due to an inconsistency in the unpacking logic for moving averages when requiresPrevAverage is true.
The calculationMovingAverage array for a pool with 3 assets is structured as follows:
calculationMovingAverage=[Avg0, Avg1,Avg2, OldAvg0,OldAvg1, OldAvg2]
for (; locals.i < locals.nMinusOne; ) {
if (locals.requiresPrevAverage) {
locals.calculationMovingAverage[locals.i + locals.numberOfAssets] = locals.currMovingAverage[locals.i];
}
locals.calculationMovingAverage[locals.i] = locals.updatedMovingAverage[locals.i];
unchecked {
locals.secondIndex = locals.i + 1;
}
if (locals.requiresPrevAverage) {
locals.calculationMovingAverage[locals.secondIndex + locals.numberOfAssets] = locals.currMovingAverage[
locals.secondIndex
];
}
locals.calculationMovingAverage[locals.secondIndex] = locals.updatedMovingAverage[locals.secondIndex];
if (!locals.requiresPrevAverage) {
movingAverages[_pool][locals.storageIndex] = _quantAMMPackTwo128(
locals.updatedMovingAverage[locals.i],
locals.updatedMovingAverage[locals.secondIndex]
);
}
unchecked {
++locals.storageIndex;
locals.i += 2;
}
}
if (locals.numberOfAssets % 2 != 0) {
locals.lastAssetIndex = locals.numberOfAssets - 1;
unchecked {
locals.nMinusOne = ((locals.lastAssetIndex) / 2);
}
if (locals.requiresPrevAverage) {
locals.calculationMovingAverage[locals.lastAssetIndex + locals.numberOfAssets] = locals
.currMovingAverage[locals.lastAssetIndex];
}
locals.calculationMovingAverage[locals.lastAssetIndex] = locals.updatedMovingAverage[locals.lastAssetIndex];
if (!locals.requiresPrevAverage) {
movingAverages[_pool][locals.nMinusOne] = locals.updatedMovingAverage[locals.lastAssetIndex];
}
}
if (locals.requiresPrevAverage) {
movingAverages[_pool] = _quantAMMPack128Array(locals.calculationMovingAverage);
}
https://github.com/Cyfrin/2024-12-quantamm/blob/a775db4273eb36e7b4536c5b60207c9f17541b92/pkg/pool-quantamm/contracts/rules/UpdateRule.sol#L116
The packed movingAverages from calculationMovingAverage for a pool with 3 assets is structured as follows:
[(Avg0, Avg1), (Avg2, OldAvg0), (OldAvg1, OldAvg2)]
For a pool with 3 assets, movingAverages[_pool].length is 3, and numberOfAssets is also 3. When CalculateNewWeights is called next time, this leads to targetArray being initialized with a length of 3. Inside the _quantAMMUnpack128Array loop, the function tries to assign values to indices 0, 1, 2, 3, 4, and 5 in targetArray, resulting in an out-of-bound error.
locals.currMovingAverage = _quantAMMUnpack128Array(movingAverages[_pool], locals.numberOfAssets);
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;
}
}
if (!divisibleByTwo) {
targetArray[_targetArrayLength - 1] = int256(int128(_sourceArray[sourceArrayLengthMinusOne]));
}
}
}
https://github.com/Cyfrin/2024-12-quantamm/blob/a775db4273eb36e7b4536c5b60207c9f17541b92/pkg/pool-quantamm/contracts/QuantAMMStorage.sol#L291
Test:
Inside UpdateRuleTest create a new mock contract with _requiresPrevMovingAverage is 1
contract MockRule2 is MockUpdateRule {
constructor(address _updateWeightRunner) MockUpdateRule(_updateWeightRunner) {}
function _requiresPrevMovingAverage() internal pure virtual override returns (uint16) {
return 1;
}
function movingAveragesLength(address _pool) public view returns (uint) {
return movingAverages[_pool].length;
}
}
function testNewMovingAveragesFail() public {
MockRule2 rule = new MockRule2(owner);
vm.startPrank(owner);
int256[][] memory parameters = new int256[][]();
parameters[0] = new int256[]();
parameters[0][0] = PRBMathSD59x18.fromInt(1);
parameters[1] = new int256[]();
parameters[1][0] = PRBMathSD59x18.fromInt(1);
int256[] memory previousAlphas = new int256[]();
previousAlphas[0] = PRBMathSD59x18.fromInt(1);
previousAlphas[1] = PRBMathSD59x18.fromInt(2);
int256[] memory prevMovingAverages = new int256[]();
prevMovingAverages[0] = PRBMathSD59x18.fromInt(0);
prevMovingAverages[1] = PRBMathSD59x18.fromInt(0);
int256[] memory movingAverages = new int256[]();
movingAverages[0] = 0.9e18;
movingAverages[1] = PRBMathSD59x18.fromInt(1) + 0.2e18;
uint64[] memory lambdas = new uint64[]();
lambdas[0] = uint64(0.7e18);
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 epsilonMax = 0.1e18;
int256[] memory expectedResults = new int256[]();
expectedResults[0] = 0.49775e18;
expectedResults[1] = 0.50225e18;
rule.setWeights(expectedResults);
rule.initialisePoolRuleIntermediateValues(address(mockPool), prevMovingAverages, previousAlphas, 2);
uint256 updatedMovingAverages = rule.movingAveragesLength(address(mockPool));
assertEq(updatedMovingAverages, 1, "movingAverages length should be 2");
rule.CalculateNewWeights(
prevWeights,
data,
address(mockPool),
parameters,
lambdas,
uint64(uint256(epsilonMax)),
uint64(0.2e18)
);
uint256 updatedMovingAverages2 = rule.movingAveragesLength(address(mockPool));
assertEq(updatedMovingAverages2, 2, "movingAverages length should be 2");
rule.CalculateNewWeights(
prevWeights,
data,
address(mockPool),
parameters,
lambdas,
uint64(uint256(epsilonMax)),
uint64(0.2e18)
);
vm.stopPrank();
}
Impact
The vulnerability causes the CalculateNewWeights function to fail when processing pools with rule requiresPrevAverage = true
Tools Used
Manual
Recommendations
Update the _quantAMMUnpack128Array function to correctly handle cases where requiresPrevAverage = true
