code/cl8y-dex-terraclassic
4
0
Fork 0
Code Issues 35 Pull requests 6 Projects Releases Packages Wiki Activity

security(oracle): price_times_dt overflow still bricks pair after #465 skip #1224

New issue
Closed
opened 2026-09-11 07:24:50 +00:00 by PlasticDigits · 3 comments
PlasticDigits commented 2026-09-11 07:24:50 +00:00
Owner
Copy link

Summary

Follow-up to closed #465. The pair TWAP path can still permanently brick a pool and lock LP capital after a single unprivileged reserve-skewing swap.

#465 replaced panicking Decimal::from_ratio with Decimal::checked_from_ratio and skips the observation only when the reserve ratio cannot be represented as a Decimal (ratio above Decimal::MAX). That guard does not cover the next multiply: price_times_dt in dex-common does Uint128(price.atomics()) * dt. A ratio just below Decimal::MAX constructs a Decimal, passes the skip, then overflows u128. oracle_update(...)? sits at the top of every reserve-mutating execute path, so the overflow becomes a hard revert.

This is a live funds-at-risk / liveness defect on any pair whose oracle still uses that multiply, not a hypothetical design note. It is not a steal-from-pool drain: the attacker does not receive other LPs’ tokens. Impact is permanent lock of the pair’s reserves until a governance wasm migration. Do not treat this as a duplicate of #465 (that ticket’s AC was “no from_ratio panic when ratio exceeds Decimal::MAX,” and those tests still pass while this hole remains).

Impact (Immunefi-style)

Question Answer
Funds at risk today? Yes, locked. After the pair commits a large-but-representable reserve ratio, later swaps, provides, and withdrawals revert. LPs cannot exit.
Auth / admin required? No. Any address that can swap (or otherwise move reserves) on a thin pool can arm the state.
Theft vs lock? Lock / DoS of the pair, not a value-extraction drain. Recovery is a 2-of-3 / governance pair wasm migrate, not a user self-rescue.
Sticky once armed? Yes. Failed oracle_update prevents any later RESERVES write. The ratio cannot rebalance and dt only grows, so the overflow does not heal.
Scope Pair contract + dex-common TWAP helper. Factory / router inherit the brick because they call the pair.

Affected surface

Path Role
smartcontracts/packages/dex-common/src/oracle.rs (price_times_dt) price.atomics() * dt in u128
smartcontracts/contracts/pair/src/contract.rs (oracle_update) #465 skip only on checked_from_ratio Err
execute_swap / execute_provide_liquidity / execute_withdraw_liquidity oracle_update(...)? before reserve writes; no other writer of RESERVES

Invariant that is broken: oracle observation must never be able to freeze reserve mutation. A missed TWAP sample is acceptable; a bricked pair is not.

Arithmetic bound (for implementers, not a public attack recipe): Decimal stores 18 fractional digits in a u128. Decimal::MAX.atomics() is u128::MAX (~3.40e38). A constructible price whose atomics are ~1e38 overflows atomics * dt for any dt of a few seconds. Terra Classic block times easily exceed that. Ratios in that band are below the #465 skip threshold (checked_from_ratio succeeds).

Constraints / guardrails

  • Do not reopen or retarget #465 ACs. Keep checked_from_ratio skip for ratios that cannot be a Decimal. Add a second fail-open path for price × dt overflow (skip observation or widen), never a ? that aborts execute.
  • Do not remove TWAP manipulation resistance: still sample pre-op reserves. The bug is overflow handling, not when the price is read.
  • Prefer skip observation (Ok without recording) over clamping to Decimal::MAX unless a spec owner documents why a clamp cannot bias TWAP. Same policy as #465.
  • Widening the accumulator (Uint256) is acceptable if it preserves existing TWAP query encoding. Do not silently change observation cardinality, window, or query JSON.
  • Do not gate this on lowering MAX_PAIR_ASSET_DECIMALS or on AMM k widening (#464 is a different product). 6-dec thin pools can still hit a representable-but-overflowing ratio.
  • Query-only helpers (e.g. oracle_observe_single still using from_ratio, noted on #465) are out of scope unless they can abort a state transition. Do not expand this ticket into a full oracle rewrite.
  • No public on-chain attack transaction. Verification is unit/integration tests in-tree.
  • Founder-required: CosmWasm pair code. No community autoland. Do not add ready.

Relevant files

Path Why
smartcontracts/packages/dex-common/src/oracle.rs price_times_dt; add checked/widened multiply + tests
smartcontracts/contracts/pair/src/contract.rs oracle_update skip vs ?; call sites on swap / provide / withdraw
Pair oracle_overflow_tests (existing #465 suite) Must stay green; extend with sub-Decimal::MAX × dt case
smartcontracts/tests pair integration (oracle / withdraw / swap) Prove execute paths stay live after a lopsided-but-representable ratio is committed

Recommended direction

  1. Make price_times_dt (and any sibling price * dt in the same helper) checked or 256-bit. On overflow: skip this observation the same way #465 skips an unrepresentable ratio. Never return Err that execute_* propagates.
  2. Optionally skip earlier if price.atomics() cannot be multiplied by the current dt (including large dt after idle). dt growth must not be a second brick.
  3. Add focused dex-common unit tests: (a) small price × large dt still records when the product fits; (b) near-max representable Decimal × modest dt does not error.
  4. Add a pair unit/integration test that commits a reserve ratio inside Decimal range but outside atomics * dt for dt ≥ a few seconds, then asserts swap, provide, and withdraw succeed (oracle may skip). Existing extreme_ratio_degrades_gracefully_instead_of_panicking remains the #465 case.

Acceptance criteria

  • AC1. price_times_dt (or its replacement) cannot overflow-abort. Overflow → skip observation or a widened product that still Oks.
  • AC2. After a committed reserve ratio that is representable as Decimal but whose atomics * dt overflows u128, execute_swap, execute_provide_liquidity, and execute_withdraw_liquidity do not revert on oracle arithmetic. LPs can withdraw.
  • AC3. #465 behavior preserved: ratio that fails checked_from_ratio still skips; no Decimal::from_ratio panic on the execute path.
  • AC4. Normal (balanced) pools still record TWAP observations (normal_ratio_still_records_observation or equivalent stays green).
  • AC5. cargo test for dex-common oracle helpers + cl8y-dex-pair oracle overflow / observation tests + make test-contracts (or the repo’s documented contract suite) pass.

Verification (non-exploitative)

Do not publish a step-by-step mainnet or CW20-hook attack. Verify in unit tests:

  1. Direct helper: construct a Decimal with atomics large enough that atomics * dt exceeds u128::MAX for a small dt (seconds). Assert the helper/oracle update returns Ok and does not record a wrapping value.
  2. Pair: set pre-op reserves to a lopsided ratio below the checked_from_ratio failure line, advance block time so dt is non-zero, invoke oracle_update then each execute entry. Expect success, not price x dt overflow (or equivalent ContractError).
  3. Regression: existing oracle_overflow_tests (Uint128::MAX / 1-unit extreme ratio) still skip without panic.
  4. Optional: cargo test -p cl8y-dex-tests oracle remains green.

Already-bricked live pairs are an ops/governance migrate, not this ticket’s code path. This issue only restores forward liveness in wasm.

First-pass model recommendation

Recommendation: grok-high

Rationale: Security / funds-lock on CosmWasm pair + shared dex-common TWAP math. Founder-required surface (contracts, wasm, migrate-class recovery). Cross-cutting: helper multiply, oracle_update control flow, three execute entrypoints, and observation semantics. Composer is disallowed for security and for contract/wasm work even if the edit were a few lines. Verify with the focused oracle overflow tests above plus the existing #465 suite — not a live-chain attack.

## Summary Follow-up to closed [#465](https://git.cl8y.com/code/cl8y-dex-terraclassic/issues/465). The pair TWAP path can still **permanently brick a pool and lock LP capital** after a single unprivileged reserve-skewing swap. [#465](https://git.cl8y.com/code/cl8y-dex-terraclassic/issues/465) replaced panicking `Decimal::from_ratio` with `Decimal::checked_from_ratio` and **skips the observation** only when the reserve ratio cannot be represented as a `Decimal` (ratio **above** `Decimal::MAX`). That guard does not cover the next multiply: `price_times_dt` in `dex-common` does `Uint128(price.atomics()) * dt`. A ratio **just below** `Decimal::MAX` constructs a `Decimal`, passes the skip, then overflows `u128`. `oracle_update(...)?` sits at the top of every reserve-mutating execute path, so the overflow becomes a hard revert. This is a **live funds-at-risk / liveness** defect on any pair whose oracle still uses that multiply, not a hypothetical design note. It is **not** a steal-from-pool drain: the attacker does not receive other LPs’ tokens. Impact is **permanent lock** of the pair’s reserves until a **governance wasm migration**. Do not treat this as a duplicate of [#465](https://git.cl8y.com/code/cl8y-dex-terraclassic/issues/465) (that ticket’s AC was “no `from_ratio` panic when ratio exceeds `Decimal::MAX`,” and those tests still pass while this hole remains). ## Impact (Immunefi-style) | Question | Answer | | --- | --- | | Funds at risk today? | **Yes, locked.** After the pair commits a large-but-representable reserve ratio, later swaps, provides, and withdrawals revert. LPs cannot exit. | | Auth / admin required? | **No.** Any address that can swap (or otherwise move reserves) on a thin pool can arm the state. | | Theft vs lock? | **Lock / DoS of the pair**, not a value-extraction drain. Recovery is a 2-of-3 / governance **pair wasm migrate**, not a user self-rescue. | | Sticky once armed? | **Yes.** Failed `oracle_update` prevents any later `RESERVES` write. The ratio cannot rebalance and `dt` only grows, so the overflow does not heal. | | Scope | Pair contract + `dex-common` TWAP helper. Factory / router inherit the brick because they call the pair. | ## Affected surface | Path | Role | | --- | --- | | `smartcontracts/packages/dex-common/src/oracle.rs` (`price_times_dt`) | `price.atomics() * dt` in `u128` | | `smartcontracts/contracts/pair/src/contract.rs` (`oracle_update`) | `#465` skip only on `checked_from_ratio` `Err` | | `execute_swap` / `execute_provide_liquidity` / `execute_withdraw_liquidity` | `oracle_update(...)?` **before** reserve writes; no other writer of `RESERVES` | Invariant that is broken: **oracle observation must never be able to freeze reserve mutation.** A missed TWAP sample is acceptable; a bricked pair is not. Arithmetic bound (for implementers, not a public attack recipe): `Decimal` stores 18 fractional digits in a `u128`. `Decimal::MAX.atomics()` is `u128::MAX` (~3.40e38). A constructible price whose atomics are ~1e38 overflows `atomics * dt` for any `dt` of a few seconds. Terra Classic block times easily exceed that. Ratios in that band are **below** the `#465` skip threshold (`checked_from_ratio` succeeds). ## Constraints / guardrails - Do **not** reopen or retarget [#465](https://git.cl8y.com/code/cl8y-dex-terraclassic/issues/465) ACs. Keep `checked_from_ratio` skip for ratios that cannot be a `Decimal`. Add a **second** fail-open path for `price × dt` overflow (skip observation or widen), never a `?` that aborts execute. - Do **not** remove TWAP manipulation resistance: still sample **pre-op** reserves. The bug is overflow handling, not when the price is read. - Prefer **skip observation** (`Ok` without recording) over clamping to `Decimal::MAX` unless a spec owner documents why a clamp cannot bias TWAP. Same policy as [#465](https://git.cl8y.com/code/cl8y-dex-terraclassic/issues/465). - Widening the accumulator (`Uint256`) is acceptable if it preserves existing TWAP query encoding. Do not silently change observation cardinality, window, or query JSON. - Do **not** gate this on lowering `MAX_PAIR_ASSET_DECIMALS` or on AMM `k` widening ([#464](https://git.cl8y.com/code/cl8y-dex-terraclassic/issues/464) is a different product). 6-dec thin pools can still hit a representable-but-overflowing ratio. - Query-only helpers (e.g. `oracle_observe_single` still using `from_ratio`, noted on [#465](https://git.cl8y.com/code/cl8y-dex-terraclassic/issues/465)) are out of scope unless they can abort a state transition. Do not expand this ticket into a full oracle rewrite. - No public on-chain attack transaction. Verification is unit/integration tests in-tree. - Founder-required: CosmWasm pair code. No community autoland. Do not add `ready`. ## Relevant files | Path | Why | | --- | --- | | `smartcontracts/packages/dex-common/src/oracle.rs` | `price_times_dt`; add checked/widened multiply + tests | | `smartcontracts/contracts/pair/src/contract.rs` | `oracle_update` skip vs `?`; call sites on swap / provide / withdraw | | Pair `oracle_overflow_tests` (existing `#465` suite) | Must stay green; extend with **sub-`Decimal::MAX` × `dt`** case | | `smartcontracts/tests` pair integration (oracle / withdraw / swap) | Prove execute paths stay live after a lopsided-but-representable ratio is committed | ## Recommended direction 1. Make `price_times_dt` (and any sibling `price * dt` in the same helper) **checked** or **256-bit**. On overflow: skip this observation the same way `#465` skips an unrepresentable ratio. Never return `Err` that `execute_*` propagates. 2. Optionally skip earlier if `price.atomics()` cannot be multiplied by the current `dt` (including large `dt` after idle). `dt` growth must not be a second brick. 3. Add focused `dex-common` unit tests: (a) small price × large `dt` still records when the product fits; (b) near-max representable `Decimal` × modest `dt` does not error. 4. Add a pair unit/integration test that commits a reserve ratio **inside** `Decimal` range but **outside** `atomics * dt` for `dt ≥` a few seconds, then asserts swap, provide, and withdraw **succeed** (oracle may skip). Existing `extreme_ratio_degrades_gracefully_instead_of_panicking` remains the `#465` case. ## Acceptance criteria - AC1. `price_times_dt` (or its replacement) cannot overflow-abort. Overflow → skip observation or a widened product that still `Ok`s. - AC2. After a committed reserve ratio that is representable as `Decimal` but whose `atomics * dt` overflows `u128`, `execute_swap`, `execute_provide_liquidity`, and `execute_withdraw_liquidity` **do not revert** on oracle arithmetic. LPs can withdraw. - AC3. `#465` behavior preserved: ratio that fails `checked_from_ratio` still skips; no `Decimal::from_ratio` panic on the execute path. - AC4. Normal (balanced) pools still record TWAP observations (`normal_ratio_still_records_observation` or equivalent stays green). - AC5. `cargo test` for `dex-common` oracle helpers + `cl8y-dex-pair` oracle overflow / observation tests + `make test-contracts` (or the repo’s documented contract suite) pass. ## Verification (non-exploitative) Do **not** publish a step-by-step mainnet or CW20-hook attack. Verify in unit tests: 1. Direct helper: construct a `Decimal` with atomics large enough that `atomics * dt` exceeds `u128::MAX` for a small `dt` (seconds). Assert the helper/oracle update returns `Ok` and does not record a wrapping value. 2. Pair: set pre-op reserves to a lopsided ratio **below** the `checked_from_ratio` failure line, advance block time so `dt` is non-zero, invoke `oracle_update` then each execute entry. Expect success, not `price x dt overflow` (or equivalent `ContractError`). 3. Regression: existing `oracle_overflow_tests` (Uint128::MAX / 1-unit extreme ratio) still skip without panic. 4. Optional: `cargo test -p cl8y-dex-tests oracle` remains green. Already-bricked live pairs are an **ops/governance migrate**, not this ticket’s code path. This issue only restores **forward** liveness in wasm. ## First-pass model recommendation Recommendation: grok-high Rationale: Security / funds-lock on CosmWasm pair + shared `dex-common` TWAP math. Founder-required surface (contracts, wasm, migrate-class recovery). Cross-cutting: helper multiply, `oracle_update` control flow, three execute entrypoints, and observation semantics. Composer is disallowed for security and for contract/wasm work even if the edit were a few lines. Verify with the focused oracle overflow tests above plus the existing `#465` suite — not a live-chain attack.
PlasticDigits commented 2026-09-22 15:24:23 +00:00
Author
Owner
Copy link

256 bit preferred

256 bit preferred
PlasticDigits commented 2026-09-22 17:09:43 +00:00
Author
Owner
Copy link

Wasm from #1323 is on main (c17e71d3). make verify-issue-1322 passed before merge, and Woodpecker ci/woodpecker/pr/woodpecker succeeded on the updated head. The live pair is still the old code. Columbus-5 migrate to cw2 1.18.0, keeping OBSERVATIONS, is #1324.

Wasm from #1323 is on main (`c17e71d3`). `make verify-issue-1322` passed before merge, and Woodpecker `ci/woodpecker/pr/woodpecker` succeeded on the updated head. The live pair is still the old code. Columbus-5 migrate to cw2 1.18.0, keeping `OBSERVATIONS`, is #1324.
PlasticDigits commented 2026-09-24 09:15:36 +00:00
Author
Owner
Copy link

Verified on main at f3dce2d4 with make verify-issue-1224: all 17 checks passed, covering Uint256 price×dt and accumulation, legacy u128 JSON zero-extension, pair and Charts windows crossing 2^128, and preservation of the #465/#1231 ratio-skip behavior.

O1322-1–O1322-8 are documented and cross-linked in twap-oracle, the security audit, testing, integrator docs, and the third-party agent skill. The merged implementation and shared verification are tracked in #1322. Columbus-5 migration and live-pair smoke verification remain in #1324.

Verified on main at f3dce2d4 with `make verify-issue-1224`: all 17 checks passed, covering Uint256 price×dt and accumulation, legacy u128 JSON zero-extension, pair and Charts windows crossing 2^128, and preservation of the #465/#1231 ratio-skip behavior. O1322-1–O1322-8 are documented and cross-linked in [twap-oracle](https://git.cl8y.com/code/cl8y-dex-terraclassic/src/branch/main/docs/twap-oracle.md), the [security audit](https://git.cl8y.com/code/cl8y-dex-terraclassic/src/branch/main/docs/contracts-security-audit.md), [testing](https://git.cl8y.com/code/cl8y-dex-terraclassic/src/branch/main/docs/testing.md), [integrator docs](https://git.cl8y.com/code/cl8y-dex-terraclassic/src/branch/main/docs/integrators.md), and the [third-party agent skill](https://git.cl8y.com/code/cl8y-dex-terraclassic/src/branch/main/skills/AGENTS_TWAP_CUMULATIVE_U256.md). The merged implementation and shared verification are tracked in #1322. Columbus-5 migration and live-pair smoke verification remain in #1324.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
renovate/tokio-util-0.x-lockfile
renovate/thiserror-2.x-lockfile
renovate/serde_json-1.x-lockfile
renovate/serde-monorepo
renovate/libc-0.x-lockfile
renovate/chrono-0.x-lockfile
cac-design-issue-1206
cac-design-issue-1317
cac-design-issue-1316
cac-design-issue-1311
cac-design-issue-1300
cac-design-issue-1306
cac-design-issue-1305
cac-design-issue-1302
cac-design-issue-1277
No results found.
Labels
Clear labels   
agent:fix_bugfix

   
agent:fix_conflicts

   
agent:fix_security

   
agent:gap_analysis

   
agent:implement

   
agent:implement
Queue implement gate

  
agent:implement
Queue implement gate

  
agent:open_issues

   
agent:ready

   
agent:research

   
agent:security_audit

   
agent:verify

   
architecture
Architecture

  
backend
API and services

  
blocker:hybrid
Blocks hybrid launch

  
blocker:launch
Blocks launch

  
blocker:limit-orders
Blocks limits launch

  
blocker:v2
Blocks v2

  
block:log_only

   
block:security

   
bug

   
ci

   
contracts
Smart contracts

  
correctness

   
deploy

   
dev

   
devops

   
docs

   
documentation

   
duplicate

   
e2e

   
enhancement

   
epic
Epic

  
feature

   
frontend

   
functional-completion
Functional

  
gas

   
good first issue

   
governance

   
help wanted

   
high-risk

   
hooks

   
hybrid
Hybrid swaps

  
indexer
Indexer

  
infra
CI/CD

  
infrastructure

   
integrators

   
invalid

   
launch-blocker

   
limit-orders
Limit orders

  
localnet

   
localterra

   
low priority

   
missing-implementation
Implementation

  
needs-design
Design decision required

  
ops

   
performance
  
priority
high

  
priority
medium

  
product

   
qa

   
QA

   
question

   
ready
Definition of Ready

  
ready
Definition of Ready

  
research

   
scripts

   
security
Security

  
security-hardening
Security hardening

  
smartcontracts

   
tech-debt

   
testing
Testing

  
ux

   
UX

   
v2
Pool-only swaps

  
verification

   
wontfix
No labels
agent:fix_bugfix
agent:fix_conflicts
agent:fix_security
agent:gap_analysis
agent:implement
agent:implement
agent:implement
agent:open_issues
agent:ready
agent:research
agent:security_audit
agent:verify
architecture
backend
blocker:hybrid
blocker:launch
blocker:limit-orders
blocker:v2
block:log_only
block:security
bug
ci
contracts
correctness
deploy
dev
devops
docs
documentation
duplicate
e2e
enhancement
epic
feature
frontend
functional-completion
gas
good first issue
governance
help wanted
high-risk
hooks
hybrid
indexer
infra
infrastructure
integrators
invalid
launch-blocker
limit-orders
localnet
localterra
low priority
missing-implementation
needs-design
ops
performance
priority
high
priority
medium
product
qa
QA
question
ready
ready
research
scripts
security
security-hardening
smartcontracts
tech-debt
testing
ux
UX
v2
verification
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
code/cl8y-dex-terraclassic#1224
No description provided.