Summary
Negative weight values are certainly not intended and will not interact well with the balancer pools, however, it is possible due to insufficient kappa validation in the DifferenceMomentumUpdateRule::validParameters function.
Vulnerability Details
Unlike the validation on lambda values, there is insufficient validation on kappa values which can lead to negative weight values being returned, breaking the logic of the protocol as negative values for weights don't make sense.
Also note that insufficient input validation (despite the function being permissioned) was the factor contributing to the first medium on the cyfrin report.
Impact
The contract will not be able to interact with the balancer pools if negative weight values are returned.
POC
Test setup
DifferenceMomentumUpdateRule public rule;
MockPool public mockPool;
address internal owner;
address internal addr1;
address internal addr2;
int256 private constant MAX128 = int256(type(int128).max);
int256 private constant MIN128 = int256(type(int128).min);
function setUp() public {
(address ownerLocal, address addr1Local, address addr2Local) = (vm.addr(1), vm.addr(2), vm.addr(3));
owner = ownerLocal;
addr1 = addr1Local;
addr2 = addr2Local;
rule = new DifferenceMomentumUpdateRule(owner);
mockPool = new MockPool(3600, 1 ether, address(rule));
}
I also added these functions to the DifferenceMomentumUpdateRule contract to make testing slightly easier,
function setShortMovingAveragesForTesting(address _pool, int256[] memory _shortMovingAverages) external {
shortMovingAverages[_pool] = _shortMovingAverages;
}
function getWeightsExternalThatIJustMadeForTesting(
int256[] calldata _prevWeights,
int256[] memory _data,
int256[][] calldata _parameters,
QuantAMMPoolParameters memory _poolParameters
) external returns (int256[] memory) {
return _getWeights(_prevWeights,_data,_parameters,_poolParameters);
}
Validation that negative kappa values are possible:
function testNegativeKappaValuesCanBeAccepted() public view {
int256[][] memory parameters = new int256[][]();
parameters[0] = new int256[]();
parameters[1] = new int256[]();
parameters[0][0] = -0.1e18;
parameters[1][0] = 0.1e18;
bool result = rule.validParameters(parameters);
assertTrue(result);
}
[PASS] testNegativeKappaValuesCanBeAccepted() (gas: 11479)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.52ms (268.80µs CPU time)
Now here is evidence that these negative kappa values can return negative weight values
function testNegativeKappaValuesCanCauseNegativeWeightValuesToBeReturned() public {
int256[] memory initialMovingAverages = new int256[]();
initialMovingAverages[0] = 1950e18;
initialMovingAverages[1] = 29500e18;
initialMovingAverages[2] = 1e18;
rule.setShortMovingAveragesForTesting(
address(mockPool),
_quantAMMPack128Array(initialMovingAverages)
);
int256[] memory prevWeights = new int256[]();
prevWeights[0] = 0.333333333333333333e18;
prevWeights[1] = 0.333333333333333333e18;
prevWeights[2] = 0.333333333333333333e18;
int256[] memory priceData = new int256[]();
priceData[0] = 2000e18;
priceData[1] = 30000e18;
priceData[2] = 1e18;
int256[][] memory parameters = new int256[][]();
parameters[0] = new int256[]();
parameters[1] = new int256[]();
parameters[0][0] = -4e20;
parameters[1][0] = 0.9e18;
parameters[1][1] = 0.9e18;
parameters[1][2] = 0.9e18;
int128[] memory lambda = new int128[]();
lambda[0] = int128(0.9e18);
lambda[1] = int128(0.9e18);
lambda[2] = int128(0.9e18);
int256[] memory movingAverage = new int256[]();
movingAverage[0] = 1950e18;
movingAverage[1] = 29500e18;
movingAverage[2] = 1e18;
int256[] memory newWeights = rule.getWeightsExternalThatIJustMadeForTesting(
prevWeights,
priceData,
parameters,
QuantAMMPoolParameters(
address(mockPool),
3,
lambda,
movingAverage
)
);
console.logInt(newWeights[0]);
console.logInt(newWeights[1]);
console.logInt(newWeights[2]);
}
├─ [0] console::log(-124438649862382667 [-1.244e17]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log(223236274083741333 [2.232e17]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log(901202375778641333 [9.012e17]) [staticcall]
Now in the CalculateNewWeights function where _getWeights is called from, there is a follow up call to QuantAMMMathGuard::_guardQuantAMMWeights, however, even after this call negative values can still be returned, shown through this test:
function testCallGuardWeightsReturnsNegativeValues() public {
int256[] memory weights = new int256[]();
weights[0] = -124438649862382667;
weights[1] = 223236274083741333;
weights[2] = 901202375778641333;
int256[] memory prevWeights = new int256[]();
prevWeights[0] = 0.333333333333333333e18;
prevWeights[1] = 0.333333333333333333e18;
prevWeights[2] = 0.333333333333333333e18;
int256 epsilonMax = 1e18;
int256 absoluteWeightGuardRail = 0.1e18;
int256[] memory guardedWeightsResult = _guardQuantAMMWeights(weights, prevWeights, epsilonMax, absoluteWeightGuardRail);
console.logInt(guardedWeightsResult[0]);
console.logInt(guardedWeightsResult[1]);
console.logInt(guardedWeightsResult[2]);
}
Traces:
[9850] DifferenceMomentumRuleTest::testCallGuardWeightsReturnsNegativeValues()
├─ [0] console::log(-151140808344209000 [-1.511e17]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log(251140808344209000 [2.511e17]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log(900000000000000000 [9e17]) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
(to make this function work I had to change the prevWeights in math guard to be memory not calldata, but the principle of the vulnerability remains the same.
Once these negative values of weights are returned, the protocol will certainly not work properly.
Tools Used
Manual review
Recommendations
Limit kappa values to being > 0, or check that the result is > 0 for when kappa values are scalar (like the check which is done for when kappa values are vectors: https://github.com/Cyfrin/2024-12-quantamm/blob/main/pkg/pool-quantamm/contracts/rules/DifferenceMomentumUpdateRule.sol#L148)
