QuantAMM
QuantAMM
49,600 OP
View results
Previous Next
Submission Details
Severity: low
Invalid

Unsafe downcasting for int128 when packing into single slot

draiakoo

Summary

There are instances where it stores values downcasted into int128 without checking if it will overflow/underflow

Vulnerability Details

When there are an odd number of elements to be stored in an array of packed int128, this function does the following:

function _quantAMMPack128Array(int256[] memory _sourceArray) internal pure returns (int256[] memory targetArray) {
uint sourceArrayLength = _sourceArray.length;
uint targetArrayLength = sourceArrayLength;
uint storageIndex;
​
require(_sourceArray.length != 0, "LEN0");
​
if (_sourceArray.length % 2 == 0) {
...
} else {
int256 lastArrayItem = _sourceArray[_sourceArray.length - 1];
require(
(lastArrayItem >= int256(type(int128).min)) && (lastArrayItem <= int256(type(int128).max)),
"Last array element overflow"
);
unchecked {
targetArrayLength = ((targetArrayLength - 1) / 2) + 1;
}
targetArray = new int256[](targetArrayLength);
uint sourceArrayLengthMinusTwo = sourceArrayLength - 2;
for (uint i; i < sourceArrayLengthMinusTwo; ) {
targetArray[storageIndex] = _quantAMMPackTwo128(_sourceArray[i], _sourceArray[i + 1]);
unchecked {
i += 2;
++storageIndex;
}
}
targetArray[storageIndex] = int256(int128(_sourceArray[sourceArrayLength - 1]));
}
}

It checks for overflow/underflow in the last element of the array. That is because inside _quantAMMPackTwo128 it already does this validation for both numbers. This way it ensures that all the data stored in the array will always fit in 128 bits and it will be safe to pack them into the slots.
However there are some instances that needs to store the informaton in the same matter as explained, but it does not ensure that the data to be stored inside fits in 128 bits.

Instances:

  1. UpdateRule::CalculateNewWeights: one of the actions of this function is to compute the new moving averages and store them into the movingAverages state variable.

    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) {
    ...
    ​
    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) {
    // instance: not checked for int128 boundaries
    -> movingAverages[_pool][locals.nMinusOne] = locals.updatedMovingAverage[locals.lastAssetIndex];
    }
    }
    ...
    }

As we can see, the function starts packing the data in pairs using the _quantAMMPackTwo128 which ensures that both numbers bit in a int128 but where there is an odd amount of data to be stored, the last if branch is triggered and without validating the last item of the array it gets stored in the last position. This might seem good because it is the only element from the slot, hence it will not override any data from a pair. However, on the next update, when it will try to retrieve the data from the array, it will only read the first 128 bits according to this function:

function _quantAMMUnpack128Array(
int256[] memory _sourceArray,
uint _targetArrayLength
) internal pure returns (int256[] memory targetArray) {
...
if (!divisibleByTwo) {
targetArray[_targetArrayLength - 1] = int256(int128(_sourceArray[sourceArrayLengthMinusOne]));
}
}

Hence, if the data that was stored there did overflow or underflow, it will not revert and the code will interpret it just as 128 bits.

  1. QuantammGradientBasedRule::_calculateQuantAMMGradient

    function _calculateQuantAMMGradient(
    int256[] memory _newData,
    QuantAMMPoolParameters memory _poolParameters
    ) internal returns (int256[] memory) {
    ...
    locals.intermediateGradientState = _quantAMMUnpack128Array(
    intermediateGradientStates[_poolParameters.pool],
    _poolParameters.numberOfAssets
    );
    ​
    ...
    if (_poolParameters.lambda.length == 1) {
    ...
    ​
    //now have to handle final sticky end if not divisible by two
    if (notDivisibleByTwo) {
    unchecked {
    ++numberOfAssetsMinusOne;
    }
    ​
    locals.intermediateValue =
    convertedLambda.mul(locals.intermediateGradientState[numberOfAssetsMinusOne]) +
    (_newData[numberOfAssetsMinusOne] - _poolParameters.movingAverage[numberOfAssetsMinusOne]).div(
    oneMinusLambda
    );
    ​
    // instance: not checked for int128 boundaries
    -> intermediateGradientStates[_poolParameters.pool][locals.storageArrayIndex] = locals.intermediateValue;
    ​
    locals.finalValues[numberOfAssetsMinusOne] = locals.mulFactor.mul(locals.intermediateValue);
    }
    } else {
    ...
    ​
    //take care of the potential sticky end if not divisible by two
    if (notDivisibleByTwo) {
    unchecked {
    ++numberOfAssetsMinusOne;
    convertedLambda = int256(_poolParameters.lambda[numberOfAssetsMinusOne]);
    oneMinusLambda = ONE - convertedLambda;
    locals.mulFactor = oneMinusLambda.pow(THREE).div(convertedLambda);
    }
    ​
    locals.intermediateValue =
    convertedLambda.mul(locals.intermediateGradientState[numberOfAssetsMinusOne]) +
    (_newData[numberOfAssetsMinusOne] - _poolParameters.movingAverage[numberOfAssetsMinusOne]).div(
    oneMinusLambda
    );
    ​
    locals.finalValues[numberOfAssetsMinusOne] = locals.mulFactor.mul(locals.intermediateValue);
    // instance: not checked for int128 boundaries
    -> intermediateGradientStates[_poolParameters.pool][locals.storageArrayIndex] = locals.intermediateValue;
    }
    }
    ​
    return locals.finalValues;
    }

  2. QuantammVarianceBasedRule::_calculateQuantAMMVariance

    function _calculateQuantAMMVariance(
    int256[] memory _newData,
    QuantAMMPoolParameters memory _poolParameters
    ) internal returns (int256[] memory) {
    ...
    locals.intermediateVarianceState = _quantAMMUnpack128Array(
    intermediateVarianceStates[_poolParameters.pool],
    locals.n
    );
    ​
    ...
    if (_poolParameters.lambda.length == 1) {
    ...
    ​
    if (locals.notDivisibleByTwo) {
    unchecked {
    ++locals.nMinusOne;
    }
    locals.intermediateState =
    locals.convertedLambda.mul(locals.intermediateVarianceState[locals.nMinusOne]) +
    (_newData[locals.nMinusOne] - _poolParameters.movingAverage[locals.n + locals.nMinusOne])
    .mul(_newData[locals.nMinusOne] - _poolParameters.movingAverage[locals.nMinusOne])
    .div(TENPOWEIGHTEEN); // p(t) - p̅(t - 1))_i * (p(t) - p̅(t))_i
    ​
    locals.intermediateVarianceState[locals.nMinusOne] = locals.intermediateState;
    locals.finalState[locals.nMinusOne] = locals.oneMinusLambda.mul(locals.intermediateState);
    // instance: not checked for int128 boundaries
    -> intermediateVarianceStates[_poolParameters.pool][locals.storageIndex] = locals
    .intermediateVarianceState[locals.nMinusOne];
    }
    } else {
    ...
    ​
    if (locals.notDivisibleByTwo) {
    unchecked {
    ++locals.nMinusOne;
    locals.convertedLambda = int256(_poolParameters.lambda[locals.nMinusOne]);
    locals.oneMinusLambda = ONE - locals.convertedLambda;
    }
    locals.intermediateState =
    locals.convertedLambda.mul(locals.intermediateVarianceState[locals.nMinusOne]) +
    (_newData[locals.nMinusOne] - _poolParameters.movingAverage[locals.n + locals.nMinusOne])
    .mul(_newData[locals.nMinusOne] - _poolParameters.movingAverage[locals.nMinusOne])
    .div(TENPOWEIGHTEEN); // p(t) - p̅(t - 1))_i * (p(t) - p̅(t))_i
    ​
    locals.intermediateVarianceState[locals.nMinusOne] = locals.intermediateState;
    locals.finalState[locals.nMinusOne] = locals.oneMinusLambda.mul(locals.intermediateState);
    // instance: not checked for int128 boundaries
    -> intermediateVarianceStates[_poolParameters.pool][locals.storageIndex] = locals
    .intermediateVarianceState[locals.nMinusOne];
    }
    }
    ​
    ...
    }

Impact

Impact: High, if it happens it will not revert, it will continue working but the average from the last element will be reset which can lead to broken computations
Likelihood: Low

Tools Used

Manual review

Recommendations

Ensure that the last element to be inserted into the array is in the range of int128. An example of the first instance:

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) {
...
​
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.updatedMovingAverage[locals.lastAssetIndex] < int256(type(int128).min) || locals.updatedMovingAverage[locals.lastAssetIndex] > int256(type(int128).max)){
++ revert();
++ }
if (!locals.requiresPrevAverage) {
movingAverages[_pool][locals.nMinusOne] = locals.updatedMovingAverage[locals.lastAssetIndex];
}
}
...
}
Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas / Admin is trusted / Pool creation is trusted / User mistake / Suppositions

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelyhood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.