code/ust1-window
Watch
4
0
Fork
You've already forked ust1-window
 0
Code Issues 2 Pull requests 2 Projects Releases Packages Wiki Activity

feat(security): H-1 — deposit min_ust1_out + fee_bps cap #26

New issue
Closed
opened 2026-08-08 06:01:00 +00:00 by PlasticDigits · 4 comments
PlasticDigits commented 2026-08-08 06:01:00 +00:00 (Migrated from gitlab.com)

Summary

Close H-1 from audits/INTERNAL_KIMIK3_1786162831.md: give deposits the same slippage seatbelt withdraw already has, and stop allowing governance to set a 100% fee that can zero out in-flight depositors.

Audit ref: H-1 (F-05 in GROKCODE)
Deferred from: #25 / MR !41 (intentionally out of scope; zero-output dust guards M-2/M-3 landed, but not min-out / fee cap)
Related: #22 (collapse path can compound fee/slippage grief), withdraw min_vfdusd_out (existing)

Current codebase

Component Path Behavior today
Deposit hook contracts/ust1-window/src/msg.rs Cw20HookMsg::Deposit {} No fields — no min_ust1_out
Withdraw hook same file Withdraw { min_vfdusd_out } Slippage protected
Deposit exec contracts/ust1-window/src/contract.rs Computes ust1_out via deposit_vfdusd_to_ust1; dust ZeroOutput guard exists (M-3 / #25); no min-out check
Fee update ExecuteMsg::SetFeeBps / exec_set_fee_bps Rejects only fee_bps > 10_000 (BPS_DENOM) — 100% is legal
Instantiate InstantiateMsg.fee_bps Same <= 10_000 validation
Defaults ust1-common DEFAULT_FEE_BPS = 100 Production intent is 1%, but chain allows 100%

Problem: A deposit racing a SetFeeBps to 10000 (or a compromised/malicious governance) can yield ust1_out = 0 or near-zero while the full vFDUSD principal is still forwarded to treasury. Withdraw already has min_vfdusd_out; deposit does not. Dust zero-output revert (M-3) only covers the exact-zero case — not “user expected N UST1 and got dust because fee jumped.”

Why this is needed

  1. Asymmetric UX/security: redeemers can bound slippage; depositors cannot.
  2. fee_bps = 10000 is a governance footgun and a mempool grief vector against honest depositors.
  3. Audit H-1 remains open after the #25 hardening bundle by design; dust guards alone do not close it.
  4. Integrators / frontends have no on-chain min-out to encode quotes safely for deposits.

Constraints / guardrails

  1. Breaking hook change: Deposit { min_ust1_out: Uint128 } (mirror withdraw). Document migration for any off-chain callers/frontends; empty {} must stop decoding once the field is required (or use #[serde(default)] only if product explicitly wants backward-compat with min=0 — prefer required field so callers cannot silently omit protection).
  2. Cap fee_bps at a sane maximum (recommend ≤ 500 bps / 5%, or document chosen constant in ust1-common) in both instantiate and SetFeeBps.
  3. Keep existing M-2/M-3 ZeroOutput guards; min-out is additional (ust1_out < min_ust1_out → dedicated error, e.g. Slippage / MinOutNotMet).
  4. Do not weaken pause, oracle freshness/pause checks, rolling limits, or withdraw min_vfdusd_out.
  5. No change to fee-split math (chain_tax_and_cmm_protocol) beyond the new ceiling.
  6. Out of scope: H-2 multisig / drop governance mint; H-7 rolling-limit redesign.

Relevant files

  • contracts/ust1-window/src/msg.rs — Cw20HookMsg::Deposit
  • contracts/ust1-window/src/contract.rs — deposit path, exec_set_fee_bps, instantiate validation
  • contracts/ust1-window/src/error.rs — min-out / fee-cap errors
  • contracts/ust1-window/src/multitest.rs — deposit slip / fee-cap cases (many Deposit {} call sites)
  • smartcontracts-terraclassic/packages/ust1-common/src/lib.rs — optional MAX_FEE_BPS constant
  • smartcontracts-terraclassic/tests/src/*.rs — update deposit hooks in integration fixtures
  • docs/DEPLOYMENT.md / README.md — fee cap + deposit min-out docs
  • skills/audit-hardening-bundle/SKILL.md (or new skill) — cross-link H-1

Recommended direction

  1. Change Cw20HookMsg::Deposit to { min_ust1_out: Uint128 }.
  2. After math + zero-output guard: if ust1_out < min_ust1_out { return Err(...) } before mint / treasury forward.
  3. Introduce MAX_FEE_BPS (e.g. 500) in ust1-common; enforce in instantiate + SetFeeBps.
  4. Update all multitest/integration Deposit {} call sites with explicit min_ust1_out (0 only in tests that intentionally probe dust/min-out).
  5. Document: default fee 100 bps; hard cap; depositors must set min-out from quoted effective rate.

Acceptance criteria

  • Deposit requires min_ust1_out; deposit reverts when ust1_out < min_ust1_out with no mint / no treasury credit.
  • SetFeeBps and instantiate reject fee_bps > MAX_FEE_BPS (chosen cap documented; 100% no longer legal).
  • fee_bps within cap and ust1_out >= min still succeed as today (modulo intentional test updates).
  • Withdraw path unchanged; M-2/M-3 dust guards still green.
  • Docs/skills updated; all window + integration tests updated for new hook shape.

Test plan (all paths)

  1. Happy deposit: min_ust1_out ≤ computed out → mint + treasury forward.
  2. Slippage revert: set min_ust1_out above computed out → err; balances unchanged.
  3. Dust still reverts: tiny amount → ZeroOutput (M-3) before/without needing min-out.
  4. Fee cap instantiate: fee_bps = MAX+1 → reject; fee_bps = MAX → ok.
  5. Fee cap SetFeeBps: governance sets MAX+1 → reject; sets MAX → ok; unauthorized → Unauthorized.
  6. Withdraw regression: min_vfdusd_out still enforced; fee at cap still withdraws correctly.
  7. Decode break: old Deposit {} JSON without field fails decode (if field required) — assert intentionally.

Test plan (attack / hack / abuse vectors)

  1. Mempool fee grief: governance (or attacker who is gov) broadcasts SetFeeBps(10000) against pending deposits — with cap, cannot reach 100%; with min-out, depositor still reverts rather than lose principal.
  2. min_out = 0 footgun: allowed for power users/tests but document; combine with ZeroOutput so exact-zero still reverts.
  3. Quote sandwich: stale frontend quote vs oracle update — min-out protects depositor; ensure error is distinct from OracleStale / OraclePaused.
  4. Governance grief via max fee: at cap (e.g. 5%), economic damage bounded; rolling limits still apply.
  5. Hook confusion: sending Withdraw on vFDUSD or Deposit on UST1 still rejected by token/routing checks (existing).

Verification criteria

  • Multitest + integration green with new deposit shape and fee cap.
  • Audit H-1 marked addressed when merged to deploy branch.
  • DEPLOYMENT states MAX_FEE_BPS and that depositors must pass min_ust1_out.
  • Linked from #25 follow-ups as the deferred H-1 track.
## Summary Close **H-1** from [`audits/INTERNAL_KIMIK3_1786162831.md`](../audits/INTERNAL_KIMIK3_1786162831.md): give deposits the same slippage seatbelt withdraw already has, and stop allowing governance to set a 100% fee that can zero out in-flight depositors. **Audit ref:** H-1 (F-05 in GROKCODE) **Deferred from:** [#25](https://gitlab.com/PlasticDigits/ust1-window/-/issues/25) / MR !41 (intentionally out of scope; zero-output dust guards M-2/M-3 landed, but not min-out / fee cap) **Related:** [#22](https://gitlab.com/PlasticDigits/ust1-window/-/issues/22) (collapse path can compound fee/slippage grief), withdraw `min_vfdusd_out` (existing) --- ## Current codebase | Component | Path | Behavior today | |-----------|------|----------------| | Deposit hook | `contracts/ust1-window/src/msg.rs` `Cw20HookMsg::Deposit {}` | **No fields** — no `min_ust1_out` | | Withdraw hook | same file `Withdraw { min_vfdusd_out }` | Slippage protected | | Deposit exec | `contracts/ust1-window/src/contract.rs` | Computes `ust1_out` via `deposit_vfdusd_to_ust1`; dust `ZeroOutput` guard exists (M-3 / #25); **no min-out check** | | Fee update | `ExecuteMsg::SetFeeBps` / `exec_set_fee_bps` | Rejects only `fee_bps > 10_000` (`BPS_DENOM`) — **100% is legal** | | Instantiate | `InstantiateMsg.fee_bps` | Same `<= 10_000` validation | | Defaults | `ust1-common` `DEFAULT_FEE_BPS = 100` | Production intent is 1%, but chain allows 100% | **Problem:** A deposit racing a `SetFeeBps` to 10000 (or a compromised/malicious governance) can yield `ust1_out = 0` or near-zero while the full vFDUSD principal is still forwarded to treasury. Withdraw already has `min_vfdusd_out`; deposit does not. Dust zero-output revert (M-3) only covers the exact-zero case — not “user expected N UST1 and got dust because fee jumped.” --- ## Why this is needed 1. Asymmetric UX/security: redeemers can bound slippage; depositors cannot. 2. `fee_bps = 10000` is a governance footgun and a mempool grief vector against honest depositors. 3. Audit H-1 remains open after the #25 hardening bundle by design; dust guards alone do not close it. 4. Integrators / frontends have no on-chain min-out to encode quotes safely for deposits. --- ## Constraints / guardrails 1. **Breaking hook change:** `Deposit { min_ust1_out: Uint128 }` (mirror withdraw). Document migration for any off-chain callers/frontends; empty `{}` must stop decoding once the field is required (or use `#[serde(default)]` only if product explicitly wants backward-compat with `min=0` — prefer **required** field so callers cannot silently omit protection). 2. Cap `fee_bps` at a sane maximum (recommend **≤ 500 bps / 5%**, or document chosen constant in `ust1-common`) in **both** instantiate and `SetFeeBps`. 3. Keep existing M-2/M-3 `ZeroOutput` guards; min-out is additional (`ust1_out < min_ust1_out` → dedicated error, e.g. `Slippage` / `MinOutNotMet`). 4. Do not weaken pause, oracle freshness/pause checks, rolling limits, or withdraw `min_vfdusd_out`. 5. No change to fee-split math (`chain_tax_and_cmm_protocol`) beyond the new ceiling. 6. Out of scope: H-2 multisig / drop governance mint; H-7 rolling-limit redesign. --- ## Relevant files - `contracts/ust1-window/src/msg.rs` — `Cw20HookMsg::Deposit` - `contracts/ust1-window/src/contract.rs` — deposit path, `exec_set_fee_bps`, instantiate validation - `contracts/ust1-window/src/error.rs` — min-out / fee-cap errors - `contracts/ust1-window/src/multitest.rs` — deposit slip / fee-cap cases (many `Deposit {}` call sites) - `smartcontracts-terraclassic/packages/ust1-common/src/lib.rs` — optional `MAX_FEE_BPS` constant - `smartcontracts-terraclassic/tests/src/*.rs` — update deposit hooks in integration fixtures - `docs/DEPLOYMENT.md` / `README.md` — fee cap + deposit min-out docs - `skills/audit-hardening-bundle/SKILL.md` (or new skill) — cross-link H-1 --- ## Recommended direction 1. Change `Cw20HookMsg::Deposit` to `{ min_ust1_out: Uint128 }`. 2. After math + zero-output guard: `if ust1_out < min_ust1_out { return Err(...) }` before mint / treasury forward. 3. Introduce `MAX_FEE_BPS` (e.g. 500) in `ust1-common`; enforce in instantiate + `SetFeeBps`. 4. Update all multitest/integration `Deposit {}` call sites with explicit `min_ust1_out` (0 only in tests that intentionally probe dust/min-out). 5. Document: default fee 100 bps; hard cap; depositors must set min-out from quoted effective rate. --- ## Acceptance criteria - [ ] `Deposit` requires `min_ust1_out`; deposit reverts when `ust1_out < min_ust1_out` with no mint / no treasury credit. - [ ] `SetFeeBps` and instantiate reject `fee_bps > MAX_FEE_BPS` (chosen cap documented; **100% no longer legal**). - [ ] `fee_bps` within cap and `ust1_out >= min` still succeed as today (modulo intentional test updates). - [ ] Withdraw path unchanged; M-2/M-3 dust guards still green. - [ ] Docs/skills updated; all window + integration tests updated for new hook shape. --- ## Test plan (all paths) 1. **Happy deposit:** `min_ust1_out` ≤ computed out → mint + treasury forward. 2. **Slippage revert:** set `min_ust1_out` above computed out → err; balances unchanged. 3. **Dust still reverts:** tiny amount → `ZeroOutput` (M-3) before/without needing min-out. 4. **Fee cap instantiate:** `fee_bps = MAX+1` → reject; `fee_bps = MAX` → ok. 5. **Fee cap SetFeeBps:** governance sets `MAX+1` → reject; sets `MAX` → ok; unauthorized → Unauthorized. 6. **Withdraw regression:** `min_vfdusd_out` still enforced; fee at cap still withdraws correctly. 7. **Decode break:** old `Deposit {}` JSON without field fails decode (if field required) — assert intentionally. --- ## Test plan (attack / hack / abuse vectors) 1. **Mempool fee grief:** governance (or attacker who is gov) broadcasts `SetFeeBps(10000)` against pending deposits — with cap, cannot reach 100%; with min-out, depositor still reverts rather than lose principal. 2. **min_out = 0 footgun:** allowed for power users/tests but document; combine with ZeroOutput so exact-zero still reverts. 3. **Quote sandwich:** stale frontend quote vs oracle update — min-out protects depositor; ensure error is distinct from `OracleStale` / `OraclePaused`. 4. **Governance grief via max fee:** at cap (e.g. 5%), economic damage bounded; rolling limits still apply. 5. **Hook confusion:** sending `Withdraw` on vFDUSD or `Deposit` on UST1 still rejected by token/routing checks (existing). --- ## Verification criteria - Multitest + integration green with new deposit shape and fee cap. - Audit H-1 marked addressed when merged to deploy branch. - DEPLOYMENT states `MAX_FEE_BPS` and that depositors must pass `min_ust1_out`. - Linked from #25 follow-ups as the deferred H-1 track.
PlasticDigits commented 2026-08-08 06:01:02 +00:00 (Migrated from gitlab.com)

marked as related to #25

marked as related to #25
PlasticDigits commented 2026-08-08 06:01:53 +00:00 (Migrated from gitlab.com)

mentioned in issue #28

mentioned in issue #28
PlasticDigits (Migrated from gitlab.com) closed this issue 2026-08-08 09:46:48 +00:00
PlasticDigits commented 2026-08-08 09:46:49 +00:00 (Migrated from gitlab.com)

Rejected - such an incident can be mitigated by administrative intervention to mint tokens to the affected accounts.

Rejected - such an incident can be mitigated by administrative intervention to mint tokens to the affected accounts.
PlasticDigits commented 2026-08-08 09:58:36 +00:00 (Migrated from gitlab.com)

mentioned in merge request !45

mentioned in merge request !45
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
issue/50
renovate/configure
No results found.
Labels
Clear labels   
enhancement

   
governance

   
QA

   
testing
No labels
enhancement
governance
QA
testing
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/ust1-window#26
No description provided.