Summary
When the rule requires the previous moving averages in UpdateRule::CalculateNewWeights, the movingAverages[_pool] array length is twice the number of assets. However, when unpacking it using the QuantAMMStorage::_quantAMMUnpack128Array function, the amount of data requested is the same as the number of assets, i.e, half of the original length, causing the process to revert with an array out-of-bounds access error.
This issue causes rules like MinimumVarianceUpdateRule to permanently revert.
Vulnerability Details
QuantAMMStorage::_quantAMMUnpack128Array function is used to unpack n/2 256 bit integers into n 128 bit integers (as stated in the natspec). By design, this function requires the target array to be exactly the same length as the source array before being packed, otherwise it will revert with an array out-of-bounds access error, as can be seen in the function code.
> QuantAMMStorage.sol
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]));
}
}
On the other hand, the UpdateRule::CalculateNewWeights function is used to calculate the new weights of the pool. When previous data is required, the movingAverages[_pool] array length is twice the number of assets. This array is packed taking its full length (2 * numberOfAssets), however it is unpacked as if its length were the same as the number of assets in the pool. In this case, the discrepancy between the packing and unpacking length causes the process to revert because the QuantAMMStorage::_quantAMMUnpack128Array function does not allow to unpack fewer elements than originally packed.
> UpdateRule.sol
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) {
require(msg.sender == updateWeightRunner, "UNAUTH_CALC");
QuantAMMUpdateRuleLocals memory locals;
locals.numberOfAssets = _prevWeights.length;
locals.nMinusOne = locals.numberOfAssets - 1;
locals.lambda = new int128[](_lambdaStore.length);
for (locals.i; locals.i < locals.lambda.length; ) {
locals.lambda[locals.i] = int128(uint128(_lambdaStore[locals.i]));
unchecked {
++locals.i;
}
}
locals.requiresPrevAverage = _requiresPrevMovingAverage() == REQ_PREV_MAVG_VAL;
locals.intermediateMovingAverageStateLength = locals.numberOfAssets;
if (locals.requiresPrevAverage) {
unchecked {
@>[0] locals.intermediateMovingAverageStateLength *= 2;
}
}
@>[1] locals.currMovingAverage = new int256[]();
locals.updatedMovingAverage = new int256[](locals.numberOfAssets);
@>[2] locals.calculationMovingAverage = new int256[]();
@>[3] locals.currMovingAverage = _quantAMMUnpack128Array(movingAverages[_pool], locals.numberOfAssets);
... snip
if (locals.requiresPrevAverage) {
@>[4] movingAverages[_pool] = _quantAMMPack128Array(locals.calculationMovingAverage);
}
QuantAMMPoolParameters memory poolParameters;
poolParameters.lambda = locals.lambda;
poolParameters.movingAverage = locals.calculationMovingAverage;
poolParameters.pool = _pool;
locals.unGuardedUpdatedWeights = _getWeights(_prevWeights, _data, _parameters, poolParameters);
updatedWeights = _guardQuantAMMWeights(
locals.unGuardedUpdatedWeights,
_prevWeights,
int128(uint128(_epsilonMax)),
int128(uint128(_absoluteWeightGuardRail))
);
}
Proof of concept
To reproduce the issue, it is possible to use the existent tests with slightly modifications. The steps to follow are:
Set the UpdateRule::_requiresPrevMovingAverage function to return 1, to indicate that previous moving averages are required
Take a test and make sure to call the UpdateRule::CalculateNewWeights function two times. This is important as in the first call all the initialized variables are 'adjusted' to guarantee everything works fine, but in the second time all of them are modified and become more 'realistic'.
Step 1. Set the UpdateRule::_requiresPrevMovingAverage function to return 1
It is necessary to modify the MockUpdateRule::_requiresPrevMovingAverage function.
> MockUpdateRule.sol
function _requiresPrevMovingAverage() internal pure virtual override returns (uint16) {
- return 0;
+ return 1;
}
*** Note. After this modification, the issue can be seen when executing the UpdateRule.t.sol::testUpdateRuleMovingAverageStorage test. It will fail because of the same root cause explained in this report.
Step 2. Take a test and make sure to call the UpdateRule::CalculateNewWeights function two times
Lets take the testUpdateRuleAuthCalc test located in the test/foundry/rules/UpdateRule.t.sol file. After the call to the CalculateNewWeights function, add a second call to the same function and execute the test.
> UpdateRule.t.sol
function testUpdateRuleAuthCalc() public {
vm.startPrank(owner);
// Define local variables for the parameters
...snip
//does not revert
updateRule.CalculateNewWeights(
prevWeights,
data,
address(mockPool),
parameters,
lambdas,
uint64(uint256(epsilonMax)),
uint64(0.2e18));
// @audit - the same parameters are ok
+ //does revert
+ updateRule.CalculateNewWeights(
+ prevWeights,
+ data,
+ address(mockPool),
+ parameters,
+ lambdas,
+ uint64(uint256(epsilonMax)),
+ uint64(0.2e18));
vm.stopPrank();
}
When executing the test, process will revert with the array out-of-bounds access error, causing a permanent denial of service.
Impact
Impact: High
Likelihood: Medium
Tools Used
Manual Review
Recommendations
It is required to modify the CalculateNewWeights function to pack the correct array. As movingAverages[_pool] does not require to contain the previous values, a valid solution is the following:
> UpdateRule.sol
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) {
require(msg.sender == updateWeightRunner, "UNAUTH_CALC");
QuantAMMUpdateRuleLocals memory locals;
locals.numberOfAssets = _prevWeights.length;
locals.nMinusOne = locals.numberOfAssets - 1;
locals.lambda = new int128[](_lambdaStore.length);
... snip
//because of mixing of prev and current if the numassets is odd it is makes normal code unreadable to do inline
//this means for rules requiring prev moving average there is an addition SSTORE and local packed array
if (locals.requiresPrevAverage) {
- movingAverages[_pool] = _quantAMMPack128Array(locals.calculationMovingAverage);
+ movingAverages[_pool] = _quantAMMPack128Array(locals.updatedMovingAverage);
}
QuantAMMPoolParameters memory poolParameters;
poolParameters.lambda = locals.lambda;
poolParameters.movingAverage = locals.calculationMovingAverage;
poolParameters.pool = _pool;
//calling the function in the derived contract specific to the specific rule
locals.unGuardedUpdatedWeights = _getWeights(_prevWeights, _data, _parameters, poolParameters);
//Guard weights is done in the base contract so regardless of the rule the logic will always be executed
updatedWeights = _guardQuantAMMWeights(
locals.unGuardedUpdatedWeights,
_prevWeights,
int128(uint128(_epsilonMax)),
int128(uint128(_absoluteWeightGuardRail))
);
}
