code/cl8y-dex-terraclassic

Fork 0

Blacklisted maker's resting limit orders keep filling — maker side of a fill is never blacklist-checked (freeze bypass) #468

New issue

Closed

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

Brouie commented

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

(Migrated from gitlab.com)

Came out of the QA security sweep on the pair contract. BlacklistWallet is supposed to freeze a wallet completely, but it doesn't touch a wallet's resting limit orders. If a blacklisted maker M already has orders sitting on the book, any taker that crosses those orders still fills them, delivers M's escrowed leg to the taker, and pays M's offer token straight into M's own balance. M keeps trading and keeps receiving freely-transferable CW20 while frozen.

Where

The blacklist gate on the swap/receive path only looks at the taker side. In execute_receive, contract.rs:735-747:

let mut wallets = vec![token_sender.clone()];
if let Cw20HookMsg::Swap { trader: Some(t), .. } = &hook_msg {
    wallets.push(deps.api.addr_validate(t)?);
}
gate_trading_blacklist(deps.as_ref(), &env.contract.address, &wallets, ...)?;

wallets is only [token_sender, optional trader] — i.e. the taker (and the address they're trading on behalf of). gate_trading_blacklist (contract.rs:239) just forwards those to the blacklist guard. Nothing in that set is the resting maker.

The maker side is built entirely from order.owner with zero blacklist reference:

match aggregation keys payouts purely by owner — orderbook.rs:1470 (bid book) and orderbook.rs:1638 (ask book): let entry = maker_payouts.entry(order.owner.clone()).or_default();
maker_payout_transfer_messages (orderbook.rs:143-162) turns that map into one CW20 Transfer { recipient: owner, amount } per distinct owner — no filtering.
those transfers are emitted first in the swap settlement batch at contract.rs:1213.

So the fill-time code path never consults the blacklist for the order owner at all.

Why it happens

BlacklistWallet(M) blocks every path where M is the acting address — cancel, claim, place, and swap-as-taker are all gated (see the gate_trading_blacklist calls at contract.rs:604/641/651/661/671/686/742). But a fill against M's resting order is driven by the taker's transaction, not M's. M isn't the sender, isn't the trader, and the matching engine only cares about order.owner for payout bookkeeping. The guard is checked at entry against the taker; the maker is discovered later during the book walk and never re-checked.

How to hit it

M places one or more resting limit orders (escrows the leg).
Governance blacklists M with BlacklistWallet(M).
M can no longer cancel or claim those orders — but they're still live on the book.
Any taker T sends a swap that crosses M's price. T can be M's own alt wallet (nothing ties T to M).
Fill executes: M's escrowed asset goes to T, and T's offer token is transferred to recipient: M (contract.rs:1213 via the map from orderbook.rs:1470/1638).

Net result: M completes a trade and receives spendable CW20 into its balance while blacklisted. Since T can be M-controlled, M can drain its own frozen escrow out through matched self-fills and land the proceeds in a fresh wallet. The freeze is effectively cosmetic against a maker who already has book depth.

Impact

Defeats the containment BlacklistWallet is meant to provide — a frozen wallet with pre-existing resting orders can still transact and pull value out. This is the maker-side gap in blacklist enforcement, so it belongs with #456, under the #381 hardening umbrella.

Fix direction

Enforce the blacklist at fill time on the maker owner, not just at entry on the taker. Options, roughly in order of cleanliness:

During the book walk (orderbook.rs bid path ~:1470, ask path ~:1638), check order.owner against the blacklist and skip that order — ideally auto-cancel/refund it so frozen makers' liquidity drops off the book rather than lingering. Refund would need to respect that the owner is frozen (park it as claimable-post-unfreeze, or route per policy) since paying a blacklisted owner is the thing we're preventing.
Or, minimally, filter maker_payouts before maker_payout_transfer_messages (orderbook.rs:143) so blacklisted owners don't receive the offer-token transfer — but that leaves their escrow half-consumed and their order state weird, so skipping/cancelling at match time is the sounder route.

Whichever way, the maker owner set needs to reach the same blacklist check the taker already goes through. The guard already exists (blacklist_guard::assert_trade_not_blacklisted_deps); it just isn't wired into the maker side of a match.

Came out of the QA security sweep on the pair contract. `BlacklistWallet` is supposed to freeze a wallet completely, but it doesn't touch a wallet's *resting* limit orders. If a blacklisted maker M already has orders sitting on the book, any taker that crosses those orders still fills them, delivers M's escrowed leg to the taker, and pays M's offer token straight into M's own balance. M keeps trading and keeps receiving freely-transferable CW20 while frozen. ### Where The blacklist gate on the swap/receive path only looks at the taker side. In `execute_receive`, `contract.rs:735-747`: ``` let mut wallets = vec![token_sender.clone()]; if let Cw20HookMsg::Swap { trader: Some(t), .. } = &hook_msg { wallets.push(deps.api.addr_validate(t)?); } gate_trading_blacklist(deps.as_ref(), &env.contract.address, &wallets, ...)?; ``` `wallets` is only `[token_sender, optional trader]` — i.e. the taker (and the address they're trading on behalf of). `gate_trading_blacklist` (`contract.rs:239`) just forwards those to the blacklist guard. Nothing in that set is the resting maker. The maker side is built entirely from `order.owner` with zero blacklist reference: - match aggregation keys payouts purely by owner — `orderbook.rs:1470` (bid book) and `orderbook.rs:1638` (ask book): `let entry = maker_payouts.entry(order.owner.clone()).or_default();` - `maker_payout_transfer_messages` (`orderbook.rs:143-162`) turns that map into one CW20 `Transfer { recipient: owner, amount }` per distinct owner — no filtering. - those transfers are emitted first in the swap settlement batch at `contract.rs:1213`. So the fill-time code path never consults the blacklist for the order owner at all. ### Why it happens `BlacklistWallet(M)` blocks every path where M is the *acting* address — cancel, claim, place, and swap-as-taker are all gated (see the `gate_trading_blacklist` calls at `contract.rs:604/641/651/661/671/686/742`). But a fill against M's resting order is driven by the *taker's* transaction, not M's. M isn't the sender, isn't the `trader`, and the matching engine only cares about `order.owner` for payout bookkeeping. The guard is checked at entry against the taker; the maker is discovered later during the book walk and never re-checked. ### How to hit it 1. M places one or more resting limit orders (escrows the leg). 2. Governance blacklists M with `BlacklistWallet(M)`. 3. M can no longer cancel or claim those orders — but they're still live on the book. 4. Any taker T sends a swap that crosses M's price. T can be M's own alt wallet (nothing ties T to M). 5. Fill executes: M's escrowed asset goes to T, and T's offer token is transferred to `recipient: M` (`contract.rs:1213` via the map from `orderbook.rs:1470/1638`). Net result: M completes a trade and receives spendable CW20 into its balance while blacklisted. Since T can be M-controlled, M can drain its own frozen escrow out through matched self-fills and land the proceeds in a fresh wallet. The freeze is effectively cosmetic against a maker who already has book depth. ### Impact Defeats the containment `BlacklistWallet` is meant to provide — a frozen wallet with pre-existing resting orders can still transact and pull value out. This is the maker-side gap in blacklist enforcement, so it belongs with #456, under the #381 hardening umbrella. ### Fix direction Enforce the blacklist at fill time on the maker owner, not just at entry on the taker. Options, roughly in order of cleanliness: - During the book walk (`orderbook.rs` bid path ~:1470, ask path ~:1638), check `order.owner` against the blacklist and skip that order — ideally auto-cancel/refund it so frozen makers' liquidity drops off the book rather than lingering. Refund would need to respect that the owner is frozen (park it as claimable-post-unfreeze, or route per policy) since paying a blacklisted owner is the thing we're preventing. - Or, minimally, filter `maker_payouts` before `maker_payout_transfer_messages` (`orderbook.rs:143`) so blacklisted owners don't receive the offer-token transfer — but that leaves their escrow half-consumed and their order state weird, so skipping/cancelling at match time is the sounder route. Whichever way, the maker owner set needs to reach the same blacklist check the taker already goes through. The guard already exists (`blacklist_guard::assert_trade_not_blacklisted_deps`); it just isn't wired into the maker side of a match.

PlasticDigits commented

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

(Migrated from gitlab.com)

mentioned in commit 48bee6ae44

mentioned in commit 48bee6ae44a88c2a9e9b19946fe4f9cf4bd0b72e

PlasticDigits commented

2026-07-07 02:19:21 +00:00

(Migrated from gitlab.com)

mentioned in merge request !1009

PlasticDigits commented

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

(Migrated from gitlab.com)

mentioned in commit 529b34e1a7

mentioned in commit 529b34e1a744b2ab6cb304dc076e1050e403aad9

PlasticDigits commented

2026-07-07 02:39:18 +00:00

(Migrated from gitlab.com)

mentioned in commit 23a36d0f4f

mentioned in commit 23a36d0f4f04aa5fe4f6268c5857e25900bbc7e6

PlasticDigits commented

2026-07-07 02:39:22 +00:00

(Migrated from gitlab.com)

mentioned in merge request !1010

PlasticDigits commented

2026-07-07 02:52:49 +00:00

(Migrated from gitlab.com)

Verification — #468 (blacklisted maker resting limit fill bypass)

Result: PASS — fix is on main; no repo changes required.

Acceptance criteria (issue body + docs)

Criterion	Result	How verified
Blacklisted maker's resting limit is not filled on taker hybrid swap	PASS	`cargo test -p cl8y-dex-tests blacklisted_maker_resting_limit_not_filled_taker_can_still_swap` — maker offer-token (token A) balance unchanged after taker crosses bid
No offer-token CW20 payout to blacklisted maker	PASS	Same test: `assert_eq!(maker_token_a_after, maker_token_a_before)`
Resting order parks off-book for post-unblacklist claim	PASS	Same test: `ExpiredLimitRefund { order_id: 1 }` returns parked row with `owner == maker`
Maker cannot cancel/claim while blacklisted (freeze semantics)	PASS	`cargo test -p cl8y-dex-tests wallet_blacklist_blocks_swap_lp_limits_and_unban_restores` (limit cancel + claim rejected; unblacklist restores)
Book walk checks `order.owner` via factory `BlacklistCheck` (L19)	PASS	Code: `orderbook.rs` `skip_blacklisted_maker_order` / `maker_owner_is_trade_blacklisted`; `blacklist_guard.rs` `TradeBlacklistGate`; wired from `contract.rs` hybrid execute + simulate
`simulate_match_*` skips blacklisted makers read-only (no park)	PASS	`orderbook.rs` simulate paths use `maker_owner_is_trade_blacklisted` with `park_off_book=false`
Docs / invariants cross-linked	PASS	`docs/contracts-security-audit.md` L19 + B1; `docs/limit-orders.md` § Blacklisted maker; `docs/security-model.md`; `docs/user-incident-faq.md`; `skills/AGENTS_BLACKLIST_DECISION.md`
Blacklist decision doc drift guard	PASS	`make check-blacklist-decision-docs`
Full blacklist regression suite	PASS	`cargo test -p cl8y-dex-tests blacklist_tests -- --test-threads=1` (10/10)

Notes

Issue scenario (place → blacklist → taker fill attempt) is fully exercised by the dedicated integration test; no LocalTerra/Keplr manual step required for this contract-only security gate.
Ask-side symmetry is implemented in match_asks / simulate_match_asks (same skip_blacklisted_maker_order helper); only bid path has a dedicated integration test today.

Follow-up (optional)

Add make verify-issue-468 script (grep doc anchors + run regression test) for parity with verify-issue-467 and other security issues.

## Verification — #468 (blacklisted maker resting limit fill bypass) **Result: PASS** — fix is on `main`; no repo changes required. ### Acceptance criteria (issue body + docs) | Criterion | Result | How verified | |-----------|--------|--------------| | Blacklisted maker's resting limit is **not filled** on taker hybrid swap | **PASS** | `cargo test -p cl8y-dex-tests blacklisted_maker_resting_limit_not_filled_taker_can_still_swap` — maker offer-token (token A) balance unchanged after taker crosses bid | | No offer-token CW20 payout to blacklisted maker | **PASS** | Same test: `assert_eq!(maker_token_a_after, maker_token_a_before)` | | Resting order **parks** off-book for post-unblacklist claim | **PASS** | Same test: `ExpiredLimitRefund { order_id: 1 }` returns parked row with `owner == maker` | | Maker cannot cancel/claim while blacklisted (freeze semantics) | **PASS** | `cargo test -p cl8y-dex-tests wallet_blacklist_blocks_swap_lp_limits_and_unban_restores` (limit cancel + claim rejected; unblacklist restores) | | Book walk checks `order.owner` via factory `BlacklistCheck` (L19) | **PASS** | Code: `orderbook.rs` `skip_blacklisted_maker_order` / `maker_owner_is_trade_blacklisted`; `blacklist_guard.rs` `TradeBlacklistGate`; wired from `contract.rs` hybrid execute + simulate | | `simulate_match_*` skips blacklisted makers read-only (no park) | **PASS** | `orderbook.rs` simulate paths use `maker_owner_is_trade_blacklisted` with `park_off_book=false` | | Docs / invariants cross-linked | **PASS** | `docs/contracts-security-audit.md` L19 + B1; `docs/limit-orders.md` § Blacklisted maker; `docs/security-model.md`; `docs/user-incident-faq.md`; `skills/AGENTS_BLACKLIST_DECISION.md` | | Blacklist decision doc drift guard | **PASS** | `make check-blacklist-decision-docs` | | Full blacklist regression suite | **PASS** | `cargo test -p cl8y-dex-tests blacklist_tests -- --test-threads=1` (10/10) | ### Notes - Issue scenario (place → blacklist → taker fill attempt) is fully exercised by the dedicated integration test; no LocalTerra/Keplr manual step required for this contract-only security gate. - Ask-side symmetry is implemented in `match_asks` / `simulate_match_asks` (same `skip_blacklisted_maker_order` helper); only bid path has a dedicated integration test today. ### Follow-up (optional) - Add `make verify-issue-468` script (grep doc anchors + run regression test) for parity with `verify-issue-467` and other security issues.

PlasticDigits (Migrated from gitlab.com) closed this issue

2026-07-07 02:52:50 +00:00

leonardocolucci commented

2026-07-31 17:34:20 +00:00

(Migrated from gitlab.com)

mentioned in issue #504