Skip to content

fix: U256::mul overflow at src/amm/invariant.rs:26 (cross1+cross2) #706

Description

@Syringe7

Bug summary

U256::mul in src/amm/invariant.rs (lines 18–46) uses a plain + for two near-u128::MAX values that can overflow:

fn mul(a: u128, b: u128) -> Self {
    let a_lo = a as u64;
    let a_hi = (a >> 64) as u64;
    let b_lo = b as u64;
    let b_hi = (b >> 64) as u64;
    let lo = (a_lo as u128) * (b_lo as u128);
    let cross1 = (a_hi as u128) * (b_lo as u128);   // can approach u128::MAX
    let cross2 = (a_lo as u128) * (b_hi as u128);   // can approach u128::MAX
    let hi   = (a_hi as u128) * (b_hi as u128);
    let mid  = cross1 + cross2;                     // OVERFLOW at line 26
    ...
}

cross1 and cross2 are each u64 × u64 products bounded above by (2^64 − 1)² ≈ 2¹²⁸ − 2⁶⁵ + 1. Their sum can therefore exceed u128::MAX whenever both a and b are larger than 2⁶⁴.

A second overflow point at the bottom of mul (line 32) — let hi = hi + mid_hi + (carry1 as u128); — is structurally sound because hi and mid_hi carry at most 1 bit each, but the line-26 overflow must be fixed first before correctness analysis there is meaningful.

How it was found

PR #705 added a proptest-based fuzz harness. cargo test -p stellarflow-contracts-fuzz --release produced 30 passed / 7 failed / 37 total. All 7 failures trace to this single bug. The fuzz harness uses #[path = "..."] to include src/amm/invariant.rs verbatim, so reproducing does not require the PR — running the harness on main would surface the same failures once the fuzz crate is added (or you can apply just tests/fuzz/ from PR #705 and run it on main).

Affected tests (7 of 37)

AMM module unit test (1)

  • src/amm/invariant.rs::tests::test_u256_mul_max_bounds — fails with a = b = u128::MAX.

proptest properties (6, all transitively call U256::mul)

  • prop_no_panic_compute_swap_out
  • prop_no_panic_compute_lp_shares
  • prop_no_panic_compute_remove_liquidity
  • prop_no_panic_assert_invariant_stable
  • prop_k_monotonicity
  • prop_mint_burn_roundtrip

(The 2 passing proptest properties are prop_swap_out_floor_rounding — bound to inputs ≤ 10⁶ to avoid overflow — and prop_slippage_enforcement — which doesn't touch U256::mul at all. The other 28 passing tests are in src/amm/invariant.rs::tests (21 of 22) and src/amm/slippage.rs::tests (all 7).)

Minimal failing inputs (proptest-shrunk)

From tests/fuzz/proptest-regressions/lib.txt (committed in PR #705):

Arg shape Saved minimal input Failing properties covered
LP-shape (5 args) amount_a = 340282366920938463463374607431768211455, amount_b = 1, reserve_a = 1, reserve_b = 1, total_shares = 340282366920938463463374607431768211455 prop_no_panic_compute_lp_shares, prop_mint_burn_roundtrip (mint path)
Swap-shape (3 args, near-max) reserve_in = 1, reserve_out = 340282366920938463463374607431768211455, amount_in = 340282366920938463463374607431768211454 prop_no_panic_compute_swap_out, prop_k_monotonicity
Assert-shape (4 args, both reserves max) reserve_in_before = 340282366920938463463374607431768211455, reserve_out_before = 340282366920938463463374607431768211455, amount_in = 0, amount_out = 0 prop_no_panic_assert_invariant_stable
Remove-shape (4 args, shares = total_shares) shares = 340282366920938463463374607431768211455, total_shares = 340282366920938463463374607431768211455, reserve_a = 0, reserve_b = 340282366920938463463374607431768211455 prop_no_panic_compute_remove_liquidity, prop_mint_burn_roundtrip (burn path)
Swap-shape (3 args, mid-range) amount_in = 170141183460469231731687303715884105727, reserve_in = 1, reserve_out = 243622705781881063091400931132831378339 prop_no_panic_compute_swap_out, prop_k_monotonicity

The 7th failure (test_u256_mul_max_bounds) is the AMM module's own #[test], not a proptest property — cargo test runs it alongside the proptest properties in the same binary, but proptest's regression-saving machinery only fires for proptest-managed properties.

Suggested fix

Replace the bare + at line 26 with carry-propagating arithmetic:

// Instead of:
//   let mid  = cross1 + cross2;
//   let mid_lo = mid << 64;
//   let mid_hi = mid >> 64;
//   let (lo, carry1) = lo.overflowing_add(mid_lo);
//   let hi = hi + mid_hi + (carry1 as u128);

// Compute the carry from cross1 + cross2 explicitly:
let (mid, carry1) = cross1.overflowing_add(cross2);
let mid_lo = mid << 64;
let mid_hi = (mid >> 64) + (carry1 as u128);

// Existing carry chain (carry2 is seg2-after-mid_hi, but `+` below is
// now bounded because mid_hi is at most 1 << 64):
let (lo, carry2) = lo.overflowing_add(mid_lo);
let hi = hi + mid_hi + (carry2 as u128);

U256(lo, hi)

A cleaner refactor is to fold cross1 and cross2 into lo and hi directly without an intermediate mid, avoiding + entirely. That variant needs an external review for negative-test coverage but eliminates the second cross-term overflow surface.

How to verify the fix

  1. Apply the fix to src/amm/invariant.rs::U256::mul.

  2. From the repo root, run:

    cargo test -p stellarflow-contracts-fuzz --release

    This requires tests/fuzz/ from PR #705 to be merged (or applied locally). Without it, just run cargo test --workspace and notice test_u256_mul_max_bounds is also failing — fixing the AMM math resolves that too.

  3. Expected after the fix: all 37 tests pass (30 currently passing + 7 currently failing).

  4. Optional stress run: PROPTEST_CASES=1_000_000 cargo test -p stellarflow-contracts-fuzz --release.

Severity and priority

  • Severity: HighU256::mul is the bedrock of every AMM math operation. The overflow is reachable from public compute_swap_out / compute_lp_shares / compute_remove_liquidity / assert_invariant_stable entry points.
  • Priority: should be tracked separately from PR feat(fuzz): AMM math invariant fuzz harness (closes #625) #705 so the fuzz harness can land without depending on the production-code fix.

Related

Metadata

Metadata

Assignees

Labels

Stellar WaveIssues in the Stellar wave program

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions