Security: blacklist guard fails open when factory query returns an error [SEC-I03] (F-02)

totdking commented

2026-06-30 17:37:37 +00:00

(Migrated from gitlab.com)

No description provided.

totdking commented

2026-06-30 17:38:17 +00:00

(Migrated from gitlab.com)

Summary

When the factory BlacklistCheck query returns any error, both pair and router blacklist guards silently allow the trade. In pair/src/blacklist_guard.rs and router/src/blacklist_guard.rs the error arm returns Ok(None) or equivalent, permitting the operation to proceed. The current behavior is intentionally documented as a pre-1.5.0 factory compatibility shim. However, if the factory address stored in a pair or router became stale after an emergency factory migration without updating stored addresses, all blacklist protections on every swap, limit placement, and LP operation would silently become no-ops.

What Was Checked

smartcontracts/contracts/pair/src/blacklist_guard.rs: error arm on factory BlacklistCheck query returns Ok(None) with a note referencing pre-1.5.0 factory compatibility.
smartcontracts/contracts/router/src/blacklist_guard.rs lines 58-69 (assert_router_swap_not_blacklisted): same fail-open pattern on factory query error.
Neither test file has a test that simulates factory query failure to confirm and audit the fail-open behavior.

Expected (per checklist)

Factory query errors during blacklist checks are either treated as blocking (default deny) or gated on a factory version check that confirms the silent pass only applies to the documented pre-1.5.0 compatibility case, not to any factory error indiscriminately.

Actual

All factory query errors silently pass the blacklist check. A factory address mismatch or factory unavailability would disable all blacklist enforcement with no observable signal to the operator or the transaction sender.

Suggested Fix

Replace the blanket fail-open error arm with a factory version check: query factory version first; if version >= 1.5.0 and the BlacklistCheck query fails, treat the error as blocking. Alternatively, treat all factory query errors as blocking and document the pre-1.5.0 compatibility case as a known-acceptable regression for deployments running old factories.

Verification Checklist

Error arm on factory query reviewed and updated to block or version-gate the silent pass
Test added: factory query returning an error causes the guard to block the operation (or is explicitly gated on a version check)
Docs updated if the pre-1.5.0 compatibility behavior is removed or constrained

Cc: @PlasticDigits

### Summary When the factory `BlacklistCheck` query returns any error, both pair and router blacklist guards silently allow the trade. In `pair/src/blacklist_guard.rs` and `router/src/blacklist_guard.rs` the error arm returns `Ok(None)` or equivalent, permitting the operation to proceed. The current behavior is intentionally documented as a pre-1.5.0 factory compatibility shim. However, if the factory address stored in a pair or router became stale after an emergency factory migration without updating stored addresses, all blacklist protections on every swap, limit placement, and LP operation would silently become no-ops. --- ### What Was Checked - `smartcontracts/contracts/pair/src/blacklist_guard.rs`: error arm on factory `BlacklistCheck` query returns `Ok(None)` with a note referencing pre-1.5.0 factory compatibility. - `smartcontracts/contracts/router/src/blacklist_guard.rs` lines 58-69 (`assert_router_swap_not_blacklisted`): same fail-open pattern on factory query error. - Neither test file has a test that simulates factory query failure to confirm and audit the fail-open behavior. --- ### Expected (per checklist) Factory query errors during blacklist checks are either treated as blocking (default deny) or gated on a factory version check that confirms the silent pass only applies to the documented pre-1.5.0 compatibility case, not to any factory error indiscriminately. --- ### Actual All factory query errors silently pass the blacklist check. A factory address mismatch or factory unavailability would disable all blacklist enforcement with no observable signal to the operator or the transaction sender. --- ### Suggested Fix Replace the blanket fail-open error arm with a factory version check: query factory version first; if version \>= 1.5.0 and the `BlacklistCheck` query fails, treat the error as blocking. Alternatively, treat all factory query errors as blocking and document the pre-1.5.0 compatibility case as a known-acceptable regression for deployments running old factories. --- ### Verification Checklist - [ ] Error arm on factory query reviewed and updated to block or version-gate the silent pass - [ ] Test added: factory query returning an error causes the guard to block the operation (or is explicitly gated on a version check) - [ ] Docs updated if the pre-1.5.0 compatibility behavior is removed or constrained Cc: @PlasticDigits

totdking commented

2026-06-30 17:52:59 +00:00

(Migrated from gitlab.com)

mentioned in issue #452

totdking commented

2026-06-30 18:37:55 +00:00

(Migrated from gitlab.com)

mentioned in issue #381

Brouie commented

2026-06-30 19:04:26 +00:00

(Migrated from gitlab.com)

mentioned in merge request !982

Brouie commented

2026-06-30 19:08:18 +00:00

(Migrated from gitlab.com)

Picked this up. Replaced the blanket Err(_) => Ok(None) / Ok(()) fail-open in both guards (pair + router) with a version-gated decision in dex_common::blacklist::blacklist_query_error_blocks, which reads the factory's cw2 version from raw storage (contract_info):

factory reachable AND version >= 1.5.0 -> BlacklistCheck must work, so an error is anomalous -> BLOCK (new ContractError::BlacklistGuardUnavailable, an observable signal instead of a silent pass)
factory reachable AND version < 1.5.0 -> documented pre-1.5.0 compat -> fail open
version unreadable (no contract / not cw2 / unit-test double) -> preserve the legacy fail-open

One honest residual I documented in the helper: an orphaned/typo'd factory address that resolves to no contract still fails open under that last branch. I went fail-open there on purpose so genuine pre-1.5.0 factories and the unit-test doubles keep working — and on Terra Classic a stored factory pointer can't really resolve to "no contract" (contracts aren't deletable, so a bad address is only a deploy-time typo). If you want the stricter posture, it's a one-line flip of the None arm to block + a test update; your call.

The version probe only runs on the error path, so the normal flow pays no extra gas.

Tests: 5 new unit tests on the decision fn (current/newer factory block; pre-1.5 + both unreadable cases fail open). Blacklist integration suite 10/0 unchanged (real factory exercises the happy path, never hits the error arm). Full workspace 460/0.

MR !982, branch qa/456-blacklist-failopen-gate, commit 7cc0afb5. Needs review/merge @PlasticDigits — leaving open for verification.

Picked this up. Replaced the blanket `Err(_) => Ok(None)` / `Ok(())` fail-open in both guards (pair + router) with a version-gated decision in `dex_common::blacklist::blacklist_query_error_blocks`, which reads the factory's cw2 version from raw storage (`contract_info`): - factory reachable AND version >= 1.5.0 -> BlacklistCheck must work, so an error is anomalous -> BLOCK (new `ContractError::BlacklistGuardUnavailable`, an observable signal instead of a silent pass) - factory reachable AND version < 1.5.0 -> documented pre-1.5.0 compat -> fail open - version unreadable (no contract / not cw2 / unit-test double) -> preserve the legacy fail-open One honest residual I documented in the helper: an orphaned/typo'd factory address that resolves to *no contract* still fails open under that last branch. I went fail-open there on purpose so genuine pre-1.5.0 factories and the unit-test doubles keep working — and on Terra Classic a stored factory pointer can't really resolve to "no contract" (contracts aren't deletable, so a bad address is only a deploy-time typo). If you want the stricter posture, it's a one-line flip of the `None` arm to block + a test update; your call. The version probe only runs on the error path, so the normal flow pays no extra gas. Tests: 5 new unit tests on the decision fn (current/newer factory block; pre-1.5 + both unreadable cases fail open). Blacklist integration suite 10/0 unchanged (real factory exercises the happy path, never hits the error arm). Full workspace 460/0. MR !982, branch `qa/456-blacklist-failopen-gate`, commit `7cc0afb5`. Needs review/merge @PlasticDigits — leaving open for verification.

PlasticDigits commented

2026-06-30 21:57:09 +00:00

(Migrated from gitlab.com)

mentioned in commit 4940a5fcaa

mentioned in commit 4940a5fcaad95471bbcd001b2fb9ea2b84531dd3

PlasticDigits commented

2026-06-30 22:06:09 +00:00

(Migrated from gitlab.com)

We are not yet deployed on mainnet so amy references to old 1.5 factories should be deleted, we are not deployed and do not need backwards compatibility.

PlasticDigits commented

2026-06-30 22:19:14 +00:00

(Migrated from gitlab.com)

mentioned in commit f5085087ab

mentioned in commit f5085087abc5c3ad374b11184723a6ad5577cca2

PlasticDigits commented

2026-06-30 22:19:29 +00:00

(Migrated from gitlab.com)

mentioned in merge request !993

PlasticDigits commented

2026-07-01 00:06:46 +00:00

(Migrated from gitlab.com)

mentioned in commit 11c4fb4788

mentioned in commit 11c4fb4788a79e9dd86f95485a21072c262f6442

PlasticDigits commented

2026-07-01 00:33:58 +00:00

(Migrated from gitlab.com)

mentioned in commit 1ce2e1800b

mentioned in commit 1ce2e1800b78a8cff2daa44dc2d34281ff0e8e09

PlasticDigits commented

2026-07-01 00:34:02 +00:00

(Migrated from gitlab.com)

mentioned in merge request !994

PlasticDigits commented

2026-07-01 00:44:36 +00:00

(Migrated from gitlab.com)

mentioned in commit e23df4a270

mentioned in commit e23df4a270ff1cac6f0bfc07573a3a5a1ac40304

PlasticDigits commented

2026-07-01 00:44:43 +00:00

(Migrated from gitlab.com)

mentioned in merge request !999

PlasticDigits commented

2026-07-01 01:11:33 +00:00

(Migrated from gitlab.com)

mentioned in commit 45d7dacb1e

mentioned in commit 45d7dacb1eeb928c8a992fd2126f2ae393d3a72d

PlasticDigits commented

2026-07-01 01:14:03 +00:00

(Migrated from gitlab.com)

Verification — #456 (SEC-I03 F02)

Verified on main @ 4f4f4de1 (post-merge of !982, !999, and default-deny follow-up f5085087).

Checklist

Item	Result	How verified
Error arm on factory `BlacklistCheck` query updated to block (no silent fail-open)	PASS	Code review: `pair/src/blacklist_guard.rs` and `router/src/blacklist_guard.rs` both map `Err(_)` → `ContractError::BlacklistGuardUnavailable` (default deny). Pre-1.5.0 fail-open shim removed per maintainer comment.
Test: factory query error blocks the operation	PASS	`cargo test factory_blacklist_query_error_blocks_swap` — swap rejected with `"Blacklist guard unavailable"` when pair factory pointer points at a CW20 without `BlacklistCheck`.
Docs updated for fail-closed posture	PASS	`docs/security-model.md` § Trading blacklist, `docs/contracts-security-audit.md` B1 row, `skills/AGENTS_BLACKLIST_DECISION.md` all document fail-closed guard and link #456.
Blacklist integration suite unchanged (happy path)	PASS	`cargo test blacklist_tests` — 9/9 pass (wallet/token/pair dimensions, router multihop, unban restore).
Full contract workspace regression	PASS	`make test-contracts` — 456 tests, 0 failures.

Code pointers (merged)

Pair guard: probe_factory_blacklist returns Err(BlacklistGuardUnavailable) on query failure.
Router guard: assert_router_swap_not_blacklisted same on BlacklistCheck error.
Observable error string: "Blacklist guard unavailable: factory blacklist check failed".

Follow-ups (optional, non-blocking)

Add a router-specific integration test mirroring factory_blacklist_query_error_blocks_swap (router currently covered by code review + wallet-blacklist multihop tests, but not the factory-query-error path).

Verdict: all acceptance criteria met — closing.

## Verification — #456 (SEC-I03 F02) Verified on `main` @ `4f4f4de1` (post-merge of !982, !999, and default-deny follow-up `f5085087`). ### Checklist | Item | Result | How verified | |------|--------|--------------| | Error arm on factory `BlacklistCheck` query updated to block (no silent fail-open) | **PASS** | Code review: `pair/src/blacklist_guard.rs` and `router/src/blacklist_guard.rs` both map `Err(_)` → `ContractError::BlacklistGuardUnavailable` (default deny). Pre-1.5.0 fail-open shim removed per maintainer comment. | | Test: factory query error blocks the operation | **PASS** | `cargo test factory_blacklist_query_error_blocks_swap` — swap rejected with `"Blacklist guard unavailable"` when pair factory pointer points at a CW20 without `BlacklistCheck`. | | Docs updated for fail-closed posture | **PASS** | `docs/security-model.md` § Trading blacklist, `docs/contracts-security-audit.md` B1 row, `skills/AGENTS_BLACKLIST_DECISION.md` all document fail-closed guard and link #456. | | Blacklist integration suite unchanged (happy path) | **PASS** | `cargo test blacklist_tests` — 9/9 pass (wallet/token/pair dimensions, router multihop, unban restore). | | Full contract workspace regression | **PASS** | `make test-contracts` — 456 tests, 0 failures. | ### Code pointers (merged) - Pair guard: `probe_factory_blacklist` returns `Err(BlacklistGuardUnavailable)` on query failure. - Router guard: `assert_router_swap_not_blacklisted` same on `BlacklistCheck` error. - Observable error string: `"Blacklist guard unavailable: factory blacklist check failed"`. ### Follow-ups (optional, non-blocking) - Add a router-specific integration test mirroring `factory_blacklist_query_error_blocks_swap` (router currently covered by code review + wallet-blacklist multihop tests, but not the factory-query-error path). **Verdict: all acceptance criteria met — closing.**

PlasticDigits (Migrated from gitlab.com) closed this issue

2026-07-01 01:14:04 +00:00

Brouie commented

2026-07-01 11:30:47 +00:00

(Migrated from gitlab.com)

mentioned in issue #337

Brouie commented

2026-07-01 11:44:41 +00:00

(Migrated from gitlab.com)

mentioned in merge request !1001

Brouie commented

2026-07-01 11:45:02 +00:00

(Migrated from gitlab.com)

Closed issue so not reopening — just linking the follow-up. The router-side fail-closed arm this fix added (assert_router_swap_not_blacklisted, Err(_) => BlacklistGuardUnavailable) had no test; only the pair side did (factory_blacklist_query_error_blocks_swap).

Wrote one: MR !1001 (branch qa/456-router-blacklist-guard-test, commit 30ab853c) — a mock-querier test where the factory answers the Pair query but errors on BlacklistCheck, asserting the swap is rejected with BlacklistGuardUnavailable, plus healthy and blacklisted control cases. Test-only, no behavior change; router suite 3/3, fmt + clippy clean, and I confirmed the fail-closed test fails against a fail-open guard first (pre-fix proof).

Review/merge when you get to it @PlasticDigits.

Closed issue so not reopening — just linking the follow-up. The router-side fail-closed arm this fix added (assert_router_swap_not_blacklisted, Err(_) => BlacklistGuardUnavailable) had no test; only the pair side did (factory_blacklist_query_error_blocks_swap). Wrote one: MR !1001 (branch qa/456-router-blacklist-guard-test, commit 30ab853c) — a mock-querier test where the factory answers the Pair query but errors on BlacklistCheck, asserting the swap is rejected with BlacklistGuardUnavailable, plus healthy and blacklisted control cases. Test-only, no behavior change; router suite 3/3, fmt + clippy clean, and I confirmed the fail-closed test fails against a fail-open guard first (pre-fix proof). Review/merge when you get to it @PlasticDigits.

PlasticDigits commented

2026-07-01 12:04:51 +00:00

(Migrated from gitlab.com)

mentioned in commit 14e53a7915

mentioned in commit 14e53a7915d86585b322417388f580d493c324b1

Brouie commented

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

(Migrated from gitlab.com)

mentioned in issue #468

PlasticDigits commented

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

(Migrated from gitlab.com)

mentioned in merge request !1009

Rows
Columns

Security: blacklist guard fails open when factory query returns an error [SEC-I03] (F-02) #456

Summary

What Was Checked

Expected (per checklist)

Actual

Suggested Fix

Verification Checklist

Verification — #456 (SEC-I03 F02)

Checklist

Code pointers (merged)

Follow-ups (optional, non-blocking)