fix(pair): reject zero and dust-floor belief_price in check_max_spread #1230
Labels
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
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
code/cl8y-dex-terraclassic#1230
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
dex_common::max_spread::check_max_spread(invariant L9, #197) does not validate a presentbelief_price. Two caller-supplied values break the guard:belief_price == 0—Decimal::one() / bppanics (VM abort) instead of a contract error.belief_pricelarge enough that expected outputfloor(offer_amount / bp)(implemented asoffer_amount * (Decimal::one() / bp)) is 0 — the belief branch then skips the ratio check because ofif expected_return > Uint128::zero(), so execute succeeds with no L9 bound.When
belief_priceisSome(_), the #307 material-pool floor is also skipped. A dust-floor belief therefore fail-opens both the belief shortfall check and the no-belief hybrid floor.Bundle (do not split): zero-panic, dust-floor silent skip, dedicated error, tests, integrator/L9 doc sentence.
Parent L9 work: #197. Related but not this ticket: #81 (hybrid semantics, closed), #273 / #307 / #334 (no-belief / pure-book floors). Do not reopen those unless a merged invariant is wrong.
Current codebase
smartcontracts/packages/dex-common/src/max_spread.rsbelief branch:Decimaldivision by zero panics. There is noCheckMaxSpreadErrorvariant for invalid belief.expected_return == 0was guarded to avoidDecimal::from_ratio(_, 0), but the fallback is pass, not reject.docs/integrators.md, ADR 0001, contracts-security-audit L9) state expected output isoffer_amount / belief_priceand do not requirebp > 0orexpected_return > 0.assert_max_spreadand Simulation / ReverseSimulation / HybridSimulation all delegate here. Router hops that setbelief_priceinherit the same math.swapMaxSpread.tsmirrors the no-belief path only (retail does not sendbelief_pricetoday). Integrators and greedy/pure-book execute (#334 G8) do send it.Why this is needed
L9 is the on-chain sandwich / slippage floor for anyone who sets
belief_price(TerraSwap-compatible). A panic is an unhelpful abort and can brick a query. A dust-floor belief is worse: the swap is treated as “within max_spread” even when actual output is arbitrarily bad, and hybrid #307 no longer applies. That is a fail-open of the only remaining spread check on that message.Happy-path L9 formula (shortfall vs
offer / belief_priceusingbook_net + pool_net + pool_commission) stays.Constraints / guardrails
belief_priceisNone(#197 / #273 / #307 / #334).bpwhoseexpected_return >= 1still uses the existing shortfall /max_spreadinequality (strict>).max_spread(1%). Do not invent a new retail dAppbelief_pricefield.CheckMaxSpreadErroror existingSpreadExceeded), neverpanic!/unwrap/ Decimal div-by-zero.bp = 0.expected_return > 0guard and then panicking infrom_ratio.min_return/ router per-hopmin_returnremain independent floors (#334). A validbelief_pricestill satisfies G8; an invalid one must not count as “belief was set.”belief_price0/ non-positive in any TS builder that already serializes it (scripts, e2e, integrator examples) — not a new Swap UI control.docs/integrators.mdone sentence: zero and dust-floor belief are invalid.Relevant files
smartcontracts/packages/dex-common/src/max_spread.rsDecimal::one() / bp,expected_return > 0skip, testssmartcontracts/packages/dex-common/src/max_spread.rs(CheckMaxSpreadError)smartcontracts/contracts/pair/src/contract.rsassert_max_spreadmapping to pairContractErrorsmartcontracts/contracts/router/src/contract.rsbelief_price; error passthroughsmartcontracts/contracts/pair/cl8y-dex-testshybrid belief testshybrid_belief_price_max_spread_*, pool-onlytest_swap_max_spreaddocs/integrators.mdbelief_price” paragraphdocs/contracts-security-audit.mdL9skills/AGENTS_MAX_SPREAD_HYBRID.mdfrontend-dapp/src/utils/swapMaxSpread.tsRecommended direction
Some(bp)branch, rejectbp.is_zero()with a dedicated error (e.g.InvalidBeliefPrice/BeliefPriceZero). Map it in pair/router so LCD shows an attribute, notRuntimeError.expected_returnwith checked Decimal math. If reciprocal underflows to 0 oroffer * (1/bp)floors toUint128::zero(), Err (same invalid-belief error, orSpreadExceededwithactual = 1). Do not take the currentexpected_return > 0skip.expected_return >= 1.dex-common(no chain): zero panic-regression; dust floor with hugebpand healthyactual_returnmustis_err; existingbelief_counts_pool_commission_in_actual_returnstill passes.Swapwithbelief_price: "0"returns a contract error; one withbelief_pricesuch thatoffer / bp == 0and a non-zero pool fill is rejected even atmax_spread = 1.Acceptance criteria
belief_price = Decimal::zero()(execute or Simulation / HybridSimulation). Whencheck_max_spread/ pair swap runs. Then result is a typed contract error. Not a CosmWasm panic /out of gasfrom abort.offer_amount > 0andbelief_pricesuch thatoffer_amount * (Decimal::one() / bp) == 0. When actual return (book_net + pool_net + pool_commission) is > 0. Then the call errors. It must notOk(())at default or 100%max_spread.belief_pricewithexpected_return >= 1inside tolerance. When the same inputs as today’s passing unit tests. Then stillOk(commission-in-actual, hybrid total output unchanged).belief_price: None. When pool-only and hybrid no-belief cases from #197 / #273 / #307 / #334. Then unchanged pass/fail.belief_price: "0"returns an error JSON /StdError, not a VM panic that 500s LCD.belief_pricemust be strictly positive and must produceexpected_return >= 1raw unit; otherwise the swap is rejected.min_returnstill independently enforces #334 when belief is absent; an invalid belief does not satisfy “belief was set” for G8.Expected vs actual
bp = 0floor(offer/bp) == 0Ok— L9 and #307 skippedbp, withinmax_spreadOkOkbelief_price: NoneTest plan (functional paths)
check_max_spread(Some(0), …)unitErr, no panicbpwith1/bp == 0(Decimal underflow)Errbpwithoffer * (1/bp) == 0but1/bp != 0Errbelief_counts_pool_commission_in_actual_returnhybrid_belief_price_max_spread_rejects_shortfall_on_total_outputbelief_price: Nonepool-onlytest_swap_max_spreadSwap { belief_price: "0" }belief_price: "0"bp, exact tolerance>only)bpandmin_returnsetVitest: only if a TS helper is taught to reject
0/ non-positive belief.swapMaxSpread.test.tsno-belief table stays green.Test plan (attack, hack, and abuse)
belief_priceto a huge Decimal soexpected_returnfloors to 0 and sandwich fills at any pricebelief_pricethat floors to 0 (bypass #307 by settingSome(bp))bp = 0to skipmin_returnmin_return)Simulationpanics, execute would have… )max_spread: Some(1)(100%) plus dust-floor beliefbpjust above 0 withexpected_return >= 1bp = 0belief_priceunset)Verification criteria
cd smartcontracts && cargo test -p dex-common max_spread— new cases + existing (includingbelief_counts_pool_commission_in_actual_return).cd smartcontracts && cargo test -p cl8y-dex-tests hybrid_belief_price_max_spreadcd smartcontracts && cargo test -p cl8y-dex-tests test_swap_max_spreadbelief_price: "0"does not panic (unit or multi-test).make test-contractsgreen if that target is the repo’s contract gate.swapMaxSpread.test.tsstill green if untouched.Out of scope
belief_priceon pool-only retail swaps (dApp staysnull)./route/solveexact-out.First-pass model recommendation
Recommendation: grok-high
Rationale: Production CosmWasm slippage math in
dex-common::max_spreadplus pair/router error mapping and L9 docs. Founder-required contracts / wasm scope (model-policy invariant 5). Fail-open of a sandwich guard is not a single-file UI tweak; wrong reject/pass changes execute vs simulation. Verify withdex-commonunit tests plus existing hybrid belief / pool-only spread tests — not Composer-eligible.cl8y-agent-control: queued
implementjob19394d61-77f9-4856-b956-d7f566c42ea8(not executed; no Hetzner VM).Merged onto
origin/mainvia #1238.make verify-issue-1230was 13/13. Zero / dust-floorbelief_pricenowInvalidBeliefPrice.Leftover: pair wasm store+migrate so listed columbus-5 pairs reject dust belief. Bundled with #1227 / #1231 in a new ops issue.
CI: Woodpecker did not post live statuses; merge used local gitleaks + Forgejo status.
Follow-up ops ticket: #1246. Woodpecker enablement: #1247.
columbus-5 wasm for this ticket is not live. Ops tracker: #1246.
Live pairs are 11639 / 1.16.0 (#712). LCD
HybridSimulationbelief_price: "0"still 200 (same output as omitted belief) — #1230 / #1227 / #1231 execute/query wasm still needs a 1.17.0 store+migrate (gitCONTRACT_VERSIONis still 1.15.0). Tax listed pin is 11630 (not 11611/11619); ALPHAterra1x6e64…is 1.0.0 and needs a tax cw2 bump + CMM migrate for #1228 / #1237.columbus-5 pair wasm is live: 11664 / cw2 1.17.0 (store
DB35943A4925059E770A63C9ADDF35622696088A4DFFC8A91110466034995961). Factorypair_code_id=11664,GetPairCount=20. LCD UST1/cUSTCHybridSimulationbelief_price:"0"→Invalid belief_price 0. Ops #1246.