security(terra): withdraw approve ignores min_signatures #175

New issue

Open

opened 2026-09-12 11:40:10 +00:00 by PlasticDigits · 0 comments

PlasticDigits commented

2026-09-12 11:40:10 +00:00

Owner

Summary

Config.min_signatures is stored, validated at instantiate, and updated by UpdateMinSignatures, but the live V2 approve path never counts distinct operator (or admin) votes. One authorized caller sets pending.approved = true and starts the cancel window.

This is not a reopen of #135 (wasm / config.admin handoff to the DEX 2-of-3) or closed #111 (post-upgrade matrix that only lists instantiate min_signatures as a checklist item). Those share the config field name. The bug is WithdrawApprove ignoring the threshold.

Do not retarget #170 (operator does not execute an already dest-approved Terra→EVM row) or #164 (LCD rate-limit stalling the writer). Those are off-chain operator bugs.

Bundle (same ticket, do not split): persist distinct approvers on each pending withdraw; flip approved only when approver_count >= config.min_signatures; cover min_signatures >= 2 so a single operator cannot complete approval; update the watchtower docs that currently advertise a configurable threshold.

Current codebase

Approve is 1-of-N regardless of config

execute_withdraw_approve loads CONFIG, checks OPERATORS[sender] or info.sender == config.admin, rejects already-executed / already-approved / reused (src_chain, nonce), then:

pending.approved = true
pending.approved_at = env.block.time.seconds()
saves via save_pending_and_sync_index
marks WITHDRAW_NONCE_USED
forwards operator_funds to that caller

config.min_signatures is never read on this path. There is no approver set, no increment, no threshold compare.

PendingWithdraw stores a boolean approved and approved_at. Comment: “Whether operator has approved.” No approvers list / map.

Threshold is real on config paths only

Instantiate in contract.rs rejects min_signatures == 0 or min_signatures > operators.len(). execute_update_min_signatures re-checks 0 < min_signatures <= OPERATOR_COUNT. execute_remove_operator refuses to drop the set below the stored threshold.

Queries (query.rs) echo min_signatures. Admin message UpdateMinSignatures exists in msg.rs. Scripts document raising it (e.g. update_min_signatures to 3 in packages/contracts-terraclassic/scripts/README.md).

Tests never exercise N≥2

packages/contracts-terraclassic/bridge/tests/ instantiate helpers (including test_withdraw_flow.rs, integration.rs) set min_signatures: 1 and typically one operator. Unauthorized-caller coverage exists (test_withdraw_approve_requires_operator); threshold coverage does not.

docs/contracts-terraclassic.md Watchtower Security Model still claims “Configurable min_signatures threshold” / “Operators submit approval transactions.” Deploy docs list min_signatures as “Required operator signatures” while examples instantiate 1.

Impact (Immunefi-style)

Question	Answer
Funds at risk today?	No permissionless drain. A non-operator, non-admin cannot approve. If live instantiate is still `min_signatures: 1` (deploy docs / tests), on-chain behavior matches that config. The hole is control-gap / fake N-of-M: any `min_signatures > 1` (now or after `UpdateMinSignatures`) is not enforced. One operator or the contract admin can complete approval, start the cancel window, take `operator_funds`, and (after delay, unless a canceler fires) allow execute/mint-unlock.
Auth / admin required?	Registered operator or `config.admin`. Admin is a full bypass of the operator map and of any intended threshold.
Theft vs lock?	Compromised single operator/admin key can approve pending user withdraws as if the set had signed. Canceler + delay still apply after that one approval. Not a lock bug.
Sticky once armed?	Until CosmWasm is patched (and migrated if `PendingWithdraw` shape changes). Raising `min_signatures` on an unpatched wasm does not reduce this risk.
Scope	Terra Classic CosmWasm bridge V2 `WithdrawApprove` + `PendingWithdraw` storage + withdraw tests + watchtower docs. Not EVM/Solana programs, not operator RPC, not frontend.

Invariant that is broken: operator approval is N-of-M, where N is config.min_signatures and M is distinct registered operators (admin at most one vote unless a written exception). Today N is silently 1 whenever the caller is authorized.

Constraints / guardrails

Do not drop instantiate / UpdateMinSignatures / RemoveOperator checks. Keep min_signatures == 0 illegal. Keep unauthorized callers UnauthorizedOperator.
Same operator (or admin) voting twice must not count twice. Duplicate approve after already recorded should error (existing WithdrawAlreadyApproved is fine after the threshold is met; before threshold, reject a second vote from the same addr).
Do not start the cancel window, set approved = true, or forward operator_funds until the threshold is met. Decide explicitly when WITHDRAW_NONCE_USED is set: first distinct vote vs threshold. Prefer not paying the tip or starting approved_at on a partial vote. Document the nonce choice so competing hashes cannot be griefed without a written reason.
Admin: default is one distinct vote, not a solo override of N. If product wants admin override, that must be a documented exception with tests — do not leave an implicit bypass.
PendingWithdraw field add: use #[serde(default)] (or equivalent) so existing canonical rows still load. Do not break INV-TC-AW1 (ACTIVE_WITHDRAW_HASHES membership). Approve that is still below threshold remains active (not executed/cancelled).
Do not weaken canceler, delay, pause, rate limits, or hash/nonce replay. Execute still requires approved after the window.
Operator writer / frontend: they may assume a single WithdrawApprove succeeds into approved=true. After this fix, N>1 deployments need N distinct txs. Out of scope to redesign the operator, but tests/docs must not claim one tx meets N≥2.
Founder-required CosmWasm / wasm / operator keys. No community autoland. Do not add ready. No public mainnet approve recipe.

Relevant files

Path	Why
`packages/contracts-terraclassic/bridge/src/execute/withdraw.rs`	`execute_withdraw_approve` sets `approved` without counting votes
`packages/contracts-terraclassic/bridge/src/state.rs`	`Config.min_signatures`; `PendingWithdraw` has no approver set
`packages/contracts-terraclassic/bridge/src/execute/config.rs`	`UpdateMinSignatures` / `RemoveOperator` already enforce the number vs operator count
`packages/contracts-terraclassic/bridge/src/contract.rs`	Instantiate validates `min_signatures` then never uses it on approve
`packages/contracts-terraclassic/bridge/src/query.rs`	Exposes `min_signatures` as if it were live policy
`packages/contracts-terraclassic/bridge/tests/test_withdraw_flow.rs`	Withdraw happy path; add N≥2 cases
`packages/contracts-terraclassic/bridge/tests/integration.rs`	`test_withdraw_approve_requires_operator` — extend, do not replace
`docs/contracts-terraclassic.md`	Watchtower section claims configurable threshold

Recommended direction

Add a distinct-approver collection on the pending row (addr list or Map<(hash, addr), bool> plus count). On each authorized WithdrawApprove, insert sender if new; if count < min_signatures, persist and return attributes (approvals, required) without approved=true.
When count >= min_signatures, keep the existing terminal-approve behavior: approved=true, approved_at, index sync, nonce used (if that is the chosen moment), tip payout policy documented (last voter vs fee_collector vs split — pick one; last-voter matching today is acceptable if documented).
Instantiate a fixture with ≥2 operators and min_signatures: 2. Assert one operator leaves approved == false; the second distinct operator flips it; a third call is WithdrawAlreadyApproved; a non-operator still fails.
Align docs/contracts-terraclassic.md (and any skill that repeats “one operator approve”) with the vote-count behavior. Do not rewrite the whole legacy ApproveWithdraw message section in this ticket.

Acceptance criteria

AC1. With min_signatures == 2 and two registered operators, a single operator (or admin acting as one voter) cannot set approved or approved_at > 0. Query shows the pending row still unapproved and still in the active index.
AC2. Two distinct authorized voters meet the threshold: approved == true, cancel window starts once, execute remains blocked until delay.
AC3. Same addr voting twice does not meet N=2. Unauthorized caller still cannot vote.
AC4. min_signatures == 1 (today’s tests/deploy default) still succeeds on the first authorized approve so existing 1-of-1 deployments do not stall.
AC5. Instantiate / UpdateMinSignatures / RemoveOperator checks unchanged. Raising N above 1 on patched wasm actually requires N votes.
AC6. Docs no longer claim the threshold is enforced unless the approve path counts votes. Focused CosmWasm tests pass (cargo test -p bridge or the repo’s documented Terra contract suite).

Verification (non-exploitative)

Do not publish a mainnet single-key approve + execute sequence. Verify in multi-test / cw-multi-test only:

Fixture: two operators, min_signatures: 2, user WithdrawSubmit. First operator WithdrawApprove → approved == false; funds still held; nonce policy matches the written choice.
Second distinct operator → approved == true; further approve errors.
Repeat with admin as the first voter only: still not approved until a second distinct voter (unless a documented admin-override exception exists — then test that exception instead of AC1).
Regression: min_signatures: 1 existing withdraw-flow tests stay green (approve, cancel, uncancel, execute after delay).
INV-TC-AW1: partial votes remain in ACTIVE_WITHDRAW_HASHES; threshold approve stays active until cancel/execute.

First-pass model recommendation

Recommendation: grok-high

Rationale: Founder-required CosmWasm bridge / wasm and operator-key / N-of-M policy. Composer is disallowed for security and for contracts, auth, keys, and wallet 2-of-3. The change is not a local three-file tweak: withdraw.rs + PendingWithdraw storage (+ possible serde default / migrate), config/query consistency, and new N≥2 tests. Wrong nonce or tip timing can stall withdrawals or pay a tip before the set has signed. Verify with the multi-test cases above, not a live columbus-5 approve.

## Summary `Config.min_signatures` is stored, validated at instantiate, and updated by `UpdateMinSignatures`, but the live V2 approve path never counts distinct operator (or admin) votes. One authorized caller sets `pending.approved = true` and starts the cancel window. This is not a reopen of [#135](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/135) (wasm / `config.admin` handoff to the DEX 2-of-3) or closed [#111](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/111) (post-upgrade matrix that only lists instantiate `min_signatures` as a checklist item). Those share the config field name. The bug is `WithdrawApprove` ignoring the threshold. Do not retarget [#170](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/170) (operator does not execute an already dest-approved Terra→EVM row) or [#164](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/164) (LCD rate-limit stalling the writer). Those are off-chain operator bugs. Bundle (same ticket, do not split): persist distinct approvers on each pending withdraw; flip `approved` only when `approver_count >= config.min_signatures`; cover `min_signatures >= 2` so a single operator cannot complete approval; update the watchtower docs that currently advertise a configurable threshold. ## Current codebase ### Approve is 1-of-N regardless of config [`execute_withdraw_approve`](packages/contracts-terraclassic/bridge/src/execute/withdraw.rs) loads `CONFIG`, checks `OPERATORS[sender]` **or** `info.sender == config.admin`, rejects already-executed / already-approved / reused `(src_chain, nonce)`, then: - `pending.approved = true` - `pending.approved_at = env.block.time.seconds()` - saves via `save_pending_and_sync_index` - marks `WITHDRAW_NONCE_USED` - forwards `operator_funds` to **that** caller `config.min_signatures` is never read on this path. There is no approver set, no increment, no threshold compare. [`PendingWithdraw`](packages/contracts-terraclassic/bridge/src/state.rs) stores a boolean `approved` and `approved_at`. Comment: “Whether operator has approved.” No `approvers` list / map. ### Threshold is real on config paths only Instantiate in [`contract.rs`](packages/contracts-terraclassic/bridge/src/contract.rs) rejects `min_signatures == 0` or `min_signatures > operators.len()`. [`execute_update_min_signatures`](packages/contracts-terraclassic/bridge/src/execute/config.rs) re-checks `0 < min_signatures <= OPERATOR_COUNT`. [`execute_remove_operator`](packages/contracts-terraclassic/bridge/src/execute/config.rs) refuses to drop the set below the stored threshold. Queries (`query.rs`) echo `min_signatures`. Admin message `UpdateMinSignatures` exists in `msg.rs`. Scripts document raising it (e.g. `update_min_signatures` to 3 in `packages/contracts-terraclassic/scripts/README.md`). ### Tests never exercise N≥2 `packages/contracts-terraclassic/bridge/tests/` instantiate helpers (including `test_withdraw_flow.rs`, `integration.rs`) set `min_signatures: 1` and typically one operator. Unauthorized-caller coverage exists (`test_withdraw_approve_requires_operator`); threshold coverage does not. [`docs/contracts-terraclassic.md`](docs/contracts-terraclassic.md) Watchtower Security Model still claims “Configurable `min_signatures` threshold” / “Operators submit approval transactions.” Deploy docs list `min_signatures` as “Required operator signatures” while examples instantiate `1`. ## Impact (Immunefi-style) | Question | Answer | | --- | --- | | Funds at risk today? | **No permissionless drain.** A non-operator, non-admin cannot approve. If live instantiate is still `min_signatures: 1` (deploy docs / tests), on-chain behavior matches that config. The hole is **control-gap / fake N-of-M**: any `min_signatures > 1` (now or after `UpdateMinSignatures`) is not enforced. One operator **or** the contract admin can complete approval, start the cancel window, take `operator_funds`, and (after delay, unless a canceler fires) allow execute/mint-unlock. | | Auth / admin required? | Registered operator **or** `config.admin`. Admin is a full bypass of the operator map and of any intended threshold. | | Theft vs lock? | Compromised **single** operator/admin key can approve pending user withdraws as if the set had signed. Canceler + delay still apply after that one approval. Not a lock bug. | | Sticky once armed? | Until CosmWasm is patched (and migrated if `PendingWithdraw` shape changes). Raising `min_signatures` on an unpatched wasm **does not** reduce this risk. | | Scope | Terra Classic CosmWasm bridge V2 `WithdrawApprove` + `PendingWithdraw` storage + withdraw tests + watchtower docs. Not EVM/Solana programs, not operator RPC, not frontend. | Invariant that is broken: **operator approval is N-of-M**, where N is `config.min_signatures` and M is distinct registered operators (admin at most one vote unless a written exception). Today N is silently 1 whenever the caller is authorized. ## Constraints / guardrails - Do not drop instantiate / `UpdateMinSignatures` / `RemoveOperator` checks. Keep `min_signatures == 0` illegal. Keep unauthorized callers `UnauthorizedOperator`. - Same operator (or admin) voting twice must not count twice. Duplicate approve after already recorded should error (existing `WithdrawAlreadyApproved` is fine **after** the threshold is met; before threshold, reject a second vote from the same addr). - Do not start the cancel window, set `approved = true`, or forward `operator_funds` until the threshold is met. Decide explicitly when `WITHDRAW_NONCE_USED` is set: first distinct vote vs threshold. Prefer **not** paying the tip or starting `approved_at` on a partial vote. Document the nonce choice so competing hashes cannot be griefed without a written reason. - Admin: default is **one distinct vote**, not a solo override of N. If product wants admin override, that must be a documented exception with tests — do not leave an implicit bypass. - `PendingWithdraw` field add: use `#[serde(default)]` (or equivalent) so existing canonical rows still load. Do not break INV-TC-AW1 (`ACTIVE_WITHDRAW_HASHES` membership). Approve that is still below threshold remains active (not executed/cancelled). - Do not weaken canceler, delay, pause, rate limits, or hash/nonce replay. Execute still requires `approved` after the window. - Operator writer / frontend: they may assume a single `WithdrawApprove` succeeds into `approved=true`. After this fix, N>1 deployments need N distinct txs. Out of scope to redesign the operator, but tests/docs must not claim one tx meets N≥2. - Founder-required CosmWasm / wasm / operator keys. No community autoland. Do not add `ready`. No public mainnet approve recipe. ## Relevant files | Path | Why | | --- | --- | | `packages/contracts-terraclassic/bridge/src/execute/withdraw.rs` | `execute_withdraw_approve` sets `approved` without counting votes | | `packages/contracts-terraclassic/bridge/src/state.rs` | `Config.min_signatures`; `PendingWithdraw` has no approver set | | `packages/contracts-terraclassic/bridge/src/execute/config.rs` | `UpdateMinSignatures` / `RemoveOperator` already enforce the number vs operator count | | `packages/contracts-terraclassic/bridge/src/contract.rs` | Instantiate validates `min_signatures` then never uses it on approve | | `packages/contracts-terraclassic/bridge/src/query.rs` | Exposes `min_signatures` as if it were live policy | | `packages/contracts-terraclassic/bridge/tests/test_withdraw_flow.rs` | Withdraw happy path; add N≥2 cases | | `packages/contracts-terraclassic/bridge/tests/integration.rs` | `test_withdraw_approve_requires_operator` — extend, do not replace | | `docs/contracts-terraclassic.md` | Watchtower section claims configurable threshold | ## Recommended direction 1. Add a distinct-approver collection on the pending row (addr list or `Map<(hash, addr), bool>` plus count). On each authorized `WithdrawApprove`, insert sender if new; if `count < min_signatures`, persist and return attributes (`approvals`, `required`) **without** `approved=true`. 2. When `count >= min_signatures`, keep the existing terminal-approve behavior: `approved=true`, `approved_at`, index sync, nonce used (if that is the chosen moment), tip payout policy documented (last voter vs fee_collector vs split — pick one; last-voter matching today is acceptable if documented). 3. Instantiate a fixture with ≥2 operators and `min_signatures: 2`. Assert one operator leaves `approved == false`; the second distinct operator flips it; a third call is `WithdrawAlreadyApproved`; a non-operator still fails. 4. Align `docs/contracts-terraclassic.md` (and any skill that repeats “one operator approve”) with the vote-count behavior. Do not rewrite the whole legacy `ApproveWithdraw` message section in this ticket. ## Acceptance criteria - AC1. With `min_signatures == 2` and two registered operators, a single operator (or admin acting as one voter) cannot set `approved` or `approved_at > 0`. Query shows the pending row still unapproved and still in the active index. - AC2. Two **distinct** authorized voters meet the threshold: `approved == true`, cancel window starts once, execute remains blocked until delay. - AC3. Same addr voting twice does not meet N=2. Unauthorized caller still cannot vote. - AC4. `min_signatures == 1` (today’s tests/deploy default) still succeeds on the first authorized approve so existing 1-of-1 deployments do not stall. - AC5. Instantiate / `UpdateMinSignatures` / `RemoveOperator` checks unchanged. Raising N above 1 on patched wasm actually requires N votes. - AC6. Docs no longer claim the threshold is enforced unless the approve path counts votes. Focused CosmWasm tests pass (`cargo test -p bridge` or the repo’s documented Terra contract suite). ## Verification (non-exploitative) Do not publish a mainnet single-key approve + execute sequence. Verify in multi-test / cw-multi-test only: 1. Fixture: two operators, `min_signatures: 2`, user `WithdrawSubmit`. First operator `WithdrawApprove` → `approved == false`; funds still held; nonce policy matches the written choice. 2. Second distinct operator → `approved == true`; further approve errors. 3. Repeat with admin as the first voter only: still not approved until a second distinct voter (unless a documented admin-override exception exists — then test that exception instead of AC1). 4. Regression: `min_signatures: 1` existing withdraw-flow tests stay green (approve, cancel, uncancel, execute after delay). 5. INV-TC-AW1: partial votes remain in `ACTIVE_WITHDRAW_HASHES`; threshold approve stays active until cancel/execute. ## First-pass model recommendation Recommendation: grok-high Rationale: Founder-required CosmWasm bridge / wasm and operator-key / N-of-M policy. Composer is disallowed for security and for contracts, auth, keys, and wallet 2-of-3. The change is not a local three-file tweak: `withdraw.rs` + `PendingWithdraw` storage (+ possible serde default / migrate), config/query consistency, and new N≥2 tests. Wrong nonce or tip timing can stall withdrawals or pay a tip before the set has signed. Verify with the multi-test cases above, not a live columbus-5 approve.

PlasticDigits added the

high-risk

security

labels

2026-09-12 11:40:13 +00:00

PlasticDigits referenced this issue

2026-09-12 11:44:18 +00:00

security(evm-solana): add on-chain M-of-N operator withdraw approve #176

PlasticDigits referenced this issue

2026-09-12 11:49:25 +00:00

security(bridge): timelock cancel-window changes and raise 15s floor #177

PlasticDigits referenced this issue