code/cl8y-dex-terraclassic

Fork 0

oracle_update panics on extreme reserve ratio (Decimal::from_ratio) and permanently bricks a pair, locking LP funds #465

New issue

Closed

opened 2026-07-01 12:57:01 +00:00 by Brouie · 6 comments

Brouie commented

2026-07-01 12:57:01 +00:00

(Migrated from gitlab.com)

Came out of the #381 security-hardening sweep, poking at the TWAP oracle path. There's a panic in oracle_update that a swap can arm but not trip, so the pair commits an unsafe state and then every subsequent op dies. Net result: pair bricked, LP funds locked.

Where

smartcontracts/contracts/pair/src/contract.rs

308  let dt = block_time - last_ts;
309  let price_a = Decimal::from_ratio(reserve_b, reserve_a);
310  let price_b = Decimal::from_ratio(reserve_a, reserve_b);

oracle_update runs at the top of every reserve-mutating path:

swap: :970
provide/deposit: :1561
withdraw_liquidity: :1747

Why it panics

Decimal::from_ratio is the unwrapping variant — internally it's checked_from_ratio(...).unwrap(), so it panics when the resulting value exceeds Decimal::MAX (u128::MAX / 1e18, ~3.40e20). A pair with reserve_a = 1 and reserve_b > ~3.4e20 raw makes price_a = reserve_b / reserve_a blow past Decimal::MAX and the tx aborts with an unhandled panic instead of a clean error.

The nasty part is the timing. The oracle is deliberately sampled on the reserves before the current op mutates them — that's the manipulation-resistance design, and it's even spelled out in the comment at :259-262 ("an attacker's trade in this block does NOT influence the observation recorded for this block"). Every call passes the pre-op reserve_a/reserve_b loaded just above the call (:967, :1558, :1743).

That design is exactly what turns a revert into a permanent brick:

The swap that DRIVES the reserves to the extreme ratio evaluates from_ratio on the prior (still-safe) reserves, so it does not panic. It commits the extreme reserve state.
From the next block onward (block_time > last_ts, so it gets past the :304 early-return), every op — swap, provide, withdraw — loads the committed extreme reserves and calls from_ratio on them. Panic. Tx aborts.
Because withdraw_liquidity hits the same call at :1747, LPs can't even pull out. The funds are stuck.

Note the is_zero guard at :273 only covers the divide-by-zero edge, not the overflow edge, so it doesn't help here.

Repro

On a shallow 18-decimal pool:

Seed a thin pool (small reserves, both 18-dec assets).
Swap enough token_b in to push reserve_a down toward ~1 raw and reserve_b above ~3.4e20 raw — that's roughly ~340 whole tokens of an 18-dec asset, so not an exotic amount on a shallow pool. This swap SUCCEEDS (it sampled the old ratio).
Wait for the next block. Now try anything — swap, deposit, or withdraw. oracle_update runs from_ratio(reserve_b, reserve_a) on the committed extreme reserves and panics. Every op aborts.

Pair is bricked and LP liquidity is unrecoverable. It doesn't even need to be adversarial — a big enough legit swap on a thin pool trips it.

Fix direction

The oracle should never be able to hard-panic a state transition. Concretely:

Swap Decimal::from_ratio for Decimal::checked_from_ratio at :309-310 and degrade gracefully on overflow — skip the observation for this update (return Ok(()) without recording), or clamp the price to Decimal::MAX, rather than letting the whole tx panic. Skipping is cleanest: a missed TWAP sample is far better than a bricked pair.
Consider widening the intermediate price math so realistic ratios don't hit the ceiling in the first place.

This is the same u128/Decimal-no-widening family as the constant-product overflow finding — same root cause (unchecked arithmetic that can exceed the 128-bit/Decimal range on lopsided reserves), just surfacing in the oracle instead of the AMM invariant. Worth fixing them together and cross-linking under #381.

@PlasticDigits flagging this one as launch-blocker — it's a permanent LP fund lock reachable by a single oversized swap on any shallow pool, and the withdraw path is stuck behind the same panic so there's no self-rescue.

Came out of the #381 security-hardening sweep, poking at the TWAP oracle path. There's a panic in `oracle_update` that a swap can arm but not trip, so the pair commits an unsafe state and then every subsequent op dies. Net result: pair bricked, LP funds locked. ## Where `smartcontracts/contracts/pair/src/contract.rs` ```rust 308 let dt = block_time - last_ts; 309 let price_a = Decimal::from_ratio(reserve_b, reserve_a); 310 let price_b = Decimal::from_ratio(reserve_a, reserve_b); ``` `oracle_update` runs at the top of every reserve-mutating path: - swap: `:970` - provide/deposit: `:1561` - withdraw_liquidity: `:1747` ## Why it panics `Decimal::from_ratio` is the unwrapping variant — internally it's `checked_from_ratio(...).unwrap()`, so it panics when the resulting value exceeds `Decimal::MAX` (`u128::MAX / 1e18`, ~`3.40e20`). A pair with `reserve_a = 1` and `reserve_b > ~3.4e20` raw makes `price_a = reserve_b / reserve_a` blow past `Decimal::MAX` and the tx aborts with an unhandled panic instead of a clean error. The nasty part is the timing. The oracle is deliberately sampled on the reserves *before* the current op mutates them — that's the manipulation-resistance design, and it's even spelled out in the comment at `:259-262` ("an attacker's trade in this block does NOT influence the observation recorded for this block"). Every call passes the pre-op `reserve_a`/`reserve_b` loaded just above the call (`:967`, `:1558`, `:1743`). That design is exactly what turns a revert into a permanent brick: - The swap that DRIVES the reserves to the extreme ratio evaluates `from_ratio` on the *prior* (still-safe) reserves, so it does not panic. It commits the extreme reserve state. - From the next block onward (`block_time > last_ts`, so it gets past the `:304` early-return), every op — swap, provide, withdraw — loads the committed extreme reserves and calls `from_ratio` on them. Panic. Tx aborts. - Because withdraw_liquidity hits the same call at `:1747`, LPs can't even pull out. The funds are stuck. Note the `is_zero` guard at `:273` only covers the divide-by-zero edge, not the overflow edge, so it doesn't help here. ## Repro On a shallow 18-decimal pool: 1. Seed a thin pool (small reserves, both 18-dec assets). 2. Swap enough `token_b` in to push `reserve_a` down toward ~1 raw and `reserve_b` above ~`3.4e20` raw — that's roughly ~340 whole tokens of an 18-dec asset, so not an exotic amount on a shallow pool. This swap SUCCEEDS (it sampled the old ratio). 3. Wait for the next block. Now try anything — swap, deposit, or withdraw. `oracle_update` runs `from_ratio(reserve_b, reserve_a)` on the committed extreme reserves and panics. Every op aborts. Pair is bricked and LP liquidity is unrecoverable. It doesn't even need to be adversarial — a big enough legit swap on a thin pool trips it. ## Fix direction The oracle should never be able to hard-panic a state transition. Concretely: - Swap `Decimal::from_ratio` for `Decimal::checked_from_ratio` at `:309-310` and degrade gracefully on overflow — skip the observation for this update (return `Ok(())` without recording), or clamp the price to `Decimal::MAX`, rather than letting the whole tx panic. Skipping is cleanest: a missed TWAP sample is far better than a bricked pair. - Consider widening the intermediate price math so realistic ratios don't hit the ceiling in the first place. This is the same `u128`/`Decimal`-no-widening family as the constant-product overflow finding — same root cause (unchecked arithmetic that can exceed the 128-bit/`Decimal` range on lopsided reserves), just surfacing in the oracle instead of the AMM invariant. Worth fixing them together and cross-linking under #381. @PlasticDigits flagging this one as launch-blocker — it's a permanent LP fund lock reachable by a single oversized swap on any shallow pool, and the withdraw path is stuck behind the same panic so there's no self-rescue.

Brouie commented

2026-07-01 12:58:30 +00:00

(Migrated from gitlab.com)

mentioned in issue #464

Brouie commented

2026-07-01 12:58:31 +00:00

(Migrated from gitlab.com)

Companion finding, same root cause (unwidened native-width reserve math): #464 (constant-product k = reserve_a*reserve_b overflows u128 for 18-dec pools). Fix them together.

Brouie commented

2026-07-01 13:19:21 +00:00

(Migrated from gitlab.com)

mentioned in merge request !1002

Brouie commented

2026-07-01 13:19:36 +00:00

(Migrated from gitlab.com)

Fixed alongside #464 in MR !1002 (commit 4811caf9). oracle_update now uses Decimal::checked_from_ratio and skips the observation on an extreme ratio instead of panicking, so neither the swap nor the withdraw path can brick the pair. Added oracle_overflow_tests (extreme-ratio -> Ok, normal ratio still records) that panic against the pre-fix code. Needs review @PlasticDigits.

Fixed alongside #464 in MR !1002 (commit 4811caf9). oracle_update now uses Decimal::checked_from_ratio and skips the observation on an extreme ratio instead of panicking, so neither the swap nor the withdraw path can brick the pair. Added oracle_overflow_tests (extreme-ratio -> Ok, normal ratio still records) that panic against the pre-fix code. Needs review @PlasticDigits.

PlasticDigits commented

2026-07-01 13:55:41 +00:00

(Migrated from gitlab.com)

mentioned in commit fcdcbd87f2

mentioned in commit fcdcbd87f20d7a74d7bd56bf50d77d0a97671ccc

PlasticDigits commented

2026-07-07 02:14:11 +00:00

(Migrated from gitlab.com)

Verification — #465 (oracle_update extreme-ratio panic / LP brick)

Verified on main @ fcdcbd87 (fix landed in 4811caf9 via !1002).

Acceptance criteria

Item	Result	How verified
`oracle_update` must not use panicking `Decimal::from_ratio` on reserve prices	PASS	`smartcontracts/contracts/pair/src/contract.rs` lines 332–337 use `Decimal::checked_from_ratio`; overflow branch returns `Ok(())` (skips observation)
Extreme ratio (`reserve_b/reserve_a > Decimal::MAX`) must not panic	PASS	`cargo test -p cl8y-dex-pair oracle_overflow_tests` — `extreme_ratio_degrades_gracefully_instead_of_panicking` passes (`reserve_a=1`, `reserve_b=Uint128::MAX`)
Normal ratios still record TWAP observations	PASS	`normal_ratio_still_records_observation` passes
Swap / provide / withdraw paths call fixed `oracle_update` and must not brick pair	PASS	`make test-contracts` — 393 integration + 44 pair unit tests, 0 failures; includes `test_oracle_observations_recorded_on_withdraw`, `test_withdraw_liquidity`, swap boundary tests
Oracle integration suite (manipulation resistance, TWAP, cardinality)	PASS	`cargo test -p cl8y-dex-tests oracle` — 13/13 pass
Fix merged and deployed in tree (not pending MR)	PASS	`glab mr view 284` → merged; commit message documents #465 fix

Issue repro (manual on-chain)

The issue's 3-step shallow-pool repro (swap to extreme ratio → next-block ops panic) is covered at the unit level by oracle_overflow_tests, which directly exercises the pre-op-reserve oracle_update path that previously panicked. Full LocalTerra on-chain replay was not run (not required — unit + integration suites encode the brick scenario and withdraw path).

Follow-up (non-blocking)

oracle_observe_single (query-only TWAP extrapolation at contract.rs:404–405) still uses Decimal::from_ratio. That cannot brick LP funds (read-only query), but a future hardening pass could mirror checked_from_ratio there for consistent query error handling instead of panic.

## Verification — #465 (oracle_update extreme-ratio panic / LP brick) Verified on `main` @ `fcdcbd87` (fix landed in `4811caf9` via !1002). ### Acceptance criteria | Item | Result | How verified | |------|--------|--------------| | `oracle_update` must not use panicking `Decimal::from_ratio` on reserve prices | **PASS** | `smartcontracts/contracts/pair/src/contract.rs` lines 332–337 use `Decimal::checked_from_ratio`; overflow branch returns `Ok(())` (skips observation) | | Extreme ratio (`reserve_b/reserve_a > Decimal::MAX`) must not panic | **PASS** | `cargo test -p cl8y-dex-pair oracle_overflow_tests` — `extreme_ratio_degrades_gracefully_instead_of_panicking` passes (`reserve_a=1`, `reserve_b=Uint128::MAX`) | | Normal ratios still record TWAP observations | **PASS** | `normal_ratio_still_records_observation` passes | | Swap / provide / withdraw paths call fixed `oracle_update` and must not brick pair | **PASS** | `make test-contracts` — 393 integration + 44 pair unit tests, 0 failures; includes `test_oracle_observations_recorded_on_withdraw`, `test_withdraw_liquidity`, swap boundary tests | | Oracle integration suite (manipulation resistance, TWAP, cardinality) | **PASS** | `cargo test -p cl8y-dex-tests oracle` — 13/13 pass | | Fix merged and deployed in tree (not pending MR) | **PASS** | `glab mr view 284` → **merged**; commit message documents #465 fix | ### Issue repro (manual on-chain) The issue's 3-step shallow-pool repro (swap to extreme ratio → next-block ops panic) is covered at the unit level by `oracle_overflow_tests`, which directly exercises the pre-op-reserve `oracle_update` path that previously panicked. Full LocalTerra on-chain replay was **not** run (not required — unit + integration suites encode the brick scenario and withdraw path). ### Follow-up (non-blocking) `oracle_observe_single` (query-only TWAP extrapolation at `contract.rs:404–405`) still uses `Decimal::from_ratio`. That cannot brick LP funds (read-only query), but a future hardening pass could mirror `checked_from_ratio` there for consistent query error handling instead of panic.

PlasticDigits (Migrated from gitlab.com) closed this issue

2026-07-07 02:14:12 +00:00