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

MomentumUpdateRule Asymmetric Handling of Negative Prices

iepathos

Summary

The MomentumUpdateRule contract handles negative prices asymmetrically compared to positive prices, leading to inconsistent weight calculations. While ChainlinkOracle enforces positive prices (require(data > 0)), negative prices can still occur through the project's MultiHopOracle which performs mathematical operations that may result in negative values. This asymmetry could cause unexpected pool behavior and potential economic vulnerabilities in pools using MultiHopOracle or similar oracles that support negative prices.

Vulnerability Details

Location: pkg/pool-quantamm/contracts/rules/MomentumUpdateRule.sol

The issue occurs in the weight calculation where negative prices produce asymmetric results compared to equivalent positive prices:

//1/p(t) · ∂p(t)/∂t used in both the main part of the equation and normalisation
locals.newWeights[locals.i] = ONE.div(locals.denominator).mul(int256(locals.newWeights[locals.i]));

When handling negative prices (which can occur through MultiHopOracle's mathematical operations), the sign propagates through multiple calculations affecting both the normalization factor and final weights. The asymmetry is even more pronounced than in AntiMomentumUpdateRule and works in the opposite direction.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.26;
​
import "forge-std/Test.sol";
import "../../../contracts/mock/MockRuleInvoker.sol";
import "../../../contracts/mock/mockRules/MockMomentumRule.sol";
import "../../../contracts/mock/MockPool.sol";
import "../utils.t.sol";
​
contract QuantammMomentumNegativePriceTest is Test, QuantAMMTestUtils {
MockMomentumRule internal rule;
MockPool internal mockPool;
function setUp() public {
rule = new MockMomentumRule(address(this));
mockPool = new MockPool(3600, PRBMathSD59x18.fromInt(1), address(rule));
mockPool.setNumberOfAssets(2);
}
​
function testMomentumNegativePriceSymmetry() public {
int256[] memory prevMovingAverages = new int256[]();
prevMovingAverages[0] = 1e18;
prevMovingAverages[1] = 1e18;
​
rule.initialisePoolRuleIntermediateValues(
address(mockPool),
prevMovingAverages,
new int256[](2),
mockPool.numAssets()
);
​
int256[] memory prevWeights = new int256[]();
prevWeights[0] = 0.5e18;
prevWeights[1] = 0.5e18;
​
int256[] memory positivePrices = new int256[]();
positivePrices[0] = 2e18;
positivePrices[1] = 1e18;
​
int256[] memory negativePrices = new int256[]();
negativePrices[0] = -2e18;
negativePrices[1] = 1e18;
​
int256[][] memory parameters = new int256[][]();
parameters[0] = new int256[]();
parameters[0][0] = 0.1e18; // kappa
parameters[1] = new int256[]();
parameters[1][0] = 1e18; // use raw price
​
int128[] memory lambdas = new int128[]();
lambdas[0] = int128(0.5e18);
​
rule.CalculateUnguardedWeights(
prevWeights,
positivePrices,
address(mockPool),
parameters,
lambdas,
prevMovingAverages
);
int256[] memory positiveWeights = rule.GetResultWeights();
​
rule.CalculateUnguardedWeights(
prevWeights,
negativePrices,
address(mockPool),
parameters,
lambdas,
prevMovingAverages
);
int256[] memory negativeWeights = rule.GetResultWeights();
​
emit log_named_decimal_int("Positive price weight 0", positiveWeights[0], 18);
emit log_named_decimal_int("Positive price weight 1", positiveWeights[1], 18);
emit log_named_decimal_int("Negative price weight 0", negativeWeights[0], 18);
emit log_named_decimal_int("Negative price weight 1", negativeWeights[1], 18);
​
require(positiveWeights[0] >= 0 && positiveWeights[0] <= 1e18, "Invalid positive weight range");
require(negativeWeights[0] >= 0 && negativeWeights[0] <= 1e18, "Invalid negative weight range");
assertApproxEqAbs(positiveWeights[0], negativeWeights[0], 1e16, "Weight asymmetry detected");
}
}

Test Results:

Positive price weight 0: 0.512500000000000000
Positive price weight 1: 0.487500000000000000
Negative price weight 0: 0.531250000000000000
Negative price weight 1: 0.468750000000000000

Note that the asymmetry (0.01875) is larger than in AntiMomentumUpdateRule (0.017) and works in the opposite direction, increasing weight 0 instead of decreasing it.

Impact

  • Asymmetric weight calculations for positive vs negative prices

  • Inconsistent pool behavior depending on price sign

  • Could lead to unexpected weight distributions

  • May create arbitrage opportunities due to predictable asymmetry

  • Breaks mathematical symmetry expected in price calculations

  • Could compound with other weight calculation issues

  • Particularly concerning as asymmetry works in opposite direction to AntiMomentumUpdateRule, potentially creating complex arbitrage opportunities

Recommendations

  1. Modify price handling to maintain symmetry:

// Handle sign separately from magnitude
int256 sign = locals.denominator >= 0 ? ONE : -ONE;
int256 magnitude = locals.denominator >= 0 ? locals.denominator : -locals.denominator;
locals.newWeights[locals.i] = sign.mul(ONE.div(magnitude)).mul(int256(locals.newWeights[locals.i]));

  1. Consider architectural improvements:

    • Add explicit sign handling throughout calculations

    • Implement symmetry validation in tests

    • Add invariant checks for price sign handling

    • Consider using absolute values for intermediate calculations

    • Add documentation about price sign handling expectations

    • Consider extracting common price handling logic to a shared library

  2. Add comprehensive tests:

    • Test symmetry with various price magnitudes

    • Test edge cases with extreme price values

    • Add property-based tests for price sign handling

    • Test interactions with other weight calculation components

    • Test compounding effects over multiple updates

    • Test interactions between Momentum and AntiMomentum rules

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

invalid_getData_negative_or_zero_price

Multihop will call ChainlinkOracle and the check is in it: `require(data > 0, "INVLDDATA");` MultiHop is just here to combine Chainlinks feed when there is no direct USD price feed for a token.

Support

FAQs

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

Give us feedback!