docs: SEC-I03 admin controls, blacklist/pause coverage, and value-flow bug review #452

New issue

Closed

opened 2026-06-30 17:16:56 +00:00 by totdking · 6 comments

totdking commented

2026-06-30 17:16:56 +00:00

(Migrated from gitlab.com)

No description provided.

totdking commented

2026-06-30 17:19:56 +00:00

(Migrated from gitlab.com)

Summary

Agent-assisted review of admin controls, blacklist/pause coverage, and obvious value-flow bugs completed. Overall verdict: PASS with two low-severity housekeeping findings and one informational design note. No critical or high-severity issues found.

What Was Done

Three areas reviewed:

Admin controls: all 18 factory admin execute variants reach ensure_governance. All pair admin operations gate on info.sender != pair_info.lp_admin. Router SetWrapMapper queries factory governance correctly. No bypass path found.
Blacklist/pause coverage: guard exercised across all user-facing paths (swap, hybrid swap, provide, withdraw, all six limit order operations). Token blacklist includes both pair tokens in every guard call. Router guard includes sender, all route tokens, and all route pairs in a single BlacklistCheck call.
Value-flow: constant-product rounding is conservative. k-invariant asserted post-swap. Hook fee total guard prevents deduction exceeding output. min_return and max_spread enforced before transfers. LP share math uses floor division with bounds checks on both legs. MINIMUM_LIQUIDITY burned permanently on first deposit.

Findings Triaged

I03-F01 (Low): missing assert_pair_in_registry on execute_set_pair_limit_batch_max and execute_set_pair_limit_clean_config -- filed as Issue 54 (#455)
I03-F02 (Low): blacklist guard silently passes when factory query errors -- filed as Issue 55 (#456)
I03-F03 (Info): pause freezes escrow retrieval with no governance force-refund path -- filed as Issue 56 (#457)

Finding	Severity	GitLab #
I03-F01: missing assert_pair_in_registry on two factory functions	Low	https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/work_items/455
I03-F02: blacklist guard fails open on factory query error	Low	#456
I03-F03: pause freezes escrow with no force-refund path	Info	https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/work_items/457

Verification Checklist

Admin auth reviewed across all 18 factory execute variants
Blacklist/pause guard coverage confirmed across all user-facing execute paths
Swap and LP value-flow accounting reviewed and confirmed conservative
All findings triaged and filed or documented

### Summary Agent-assisted review of admin controls, blacklist/pause coverage, and obvious value-flow bugs completed. Overall verdict: PASS with two low-severity housekeeping findings and one informational design note. No critical or high-severity issues found. ### What Was Done Three areas reviewed: 1. Admin controls: all 18 factory admin execute variants reach `ensure_governance`. All pair admin operations gate on `info.sender != pair_info.lp_admin`. Router `SetWrapMapper` queries factory governance correctly. No bypass path found. 2. Blacklist/pause coverage: guard exercised across all user-facing paths (swap, hybrid swap, provide, withdraw, all six limit order operations). Token blacklist includes both pair tokens in every guard call. Router guard includes sender, all route tokens, and all route pairs in a single `BlacklistCheck` call. 3. Value-flow: constant-product rounding is conservative. k-invariant asserted post-swap. Hook fee total guard prevents deduction exceeding output. min_return and max_spread enforced before transfers. LP share math uses floor division with bounds checks on both legs. MINIMUM_LIQUIDITY burned permanently on first deposit. ### Findings Triaged - I03-F01 (Low): missing `assert_pair_in_registry` on `execute_set_pair_limit_batch_max` and `execute_set_pair_limit_clean_config` -- filed as Issue 54 (#455) - I03-F02 (Low): blacklist guard silently passes when factory query errors -- filed as Issue 55 (#456) - I03-F03 (Info): pause freezes escrow retrieval with no governance force-refund path -- filed as Issue 56 (#457) ### Related Issues | Finding | Severity | GitLab # | |---------|----------|----------| | I03-F01: missing assert_pair_in_registry on two factory functions | Low | https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/work_items/455 | | I03-F02: blacklist guard fails open on factory query error | Low | #456 | | I03-F03: pause freezes escrow with no force-refund path | Info | https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/work_items/457 | ### Verification Checklist - [x] Admin auth reviewed across all 18 factory execute variants - [x] Blacklist/pause guard coverage confirmed across all user-facing execute paths - [x] Swap and LP value-flow accounting reviewed and confirmed conservative - [x] All findings triaged and filed or documented ###

totdking commented

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

(Migrated from gitlab.com)

mentioned in issue #381

Brouie commented

2026-07-01 11:19:20 +00:00

(Migrated from gitlab.com)

Went back over the two code findings on a fresh full-stack redeploy (chain + indexer rebuilt to current main, 21381a0e), so this isn't just "merged" — it's live on-chain.

I03-F01 / #455 — assert_pair_in_registry on the limit-config admin fns:

On-chain: SetPairLimitBatchMax to a valid-but-unregistered address fails in execution (code 5, "Pair not found in factory registry") — tried both a real contract addr (the router) and a plain account addr, both rejected. The same call on the registered pair returns code 0 and delegates through to update_limit_order_config. So the guard is real, not just compiled.
Also walked every single-pair admin fn on the factory (set_pair_fee/hooks/discount_registry/paused/sweep + the two limit-config): all seven carry the registry guard now. 3 new unit tests pass.

I03-F02 / #456 — blacklist guard default-deny:

The deployed guard is unconditional now (the version-gate variant got dropped upstream) — any factory BlacklistCheck query error blocks the trade in both pair and router.
Live happy-path holds: real EMBER->CORAL swaps through the pair and through the router both return code 0 while the factory answers healthy, so default-deny doesn't false-block normal trades. The error->block path is unit-covered on the pair side (factory_blacklist_query_error_blocks_swap).
One follow-up worth a ticket, not a blocker: the router-side fail-closed arm has no unit test yet (only the pair side does) — a router-side factory-query-error unit test would match them up.

Checklist is complete and the children are merged + live-verified — good to close from my side. @PlasticDigits

Went back over the two code findings on a fresh full-stack redeploy (chain + indexer rebuilt to current main, 21381a0e), so this isn't just "merged" — it's live on-chain. I03-F01 / #455 — assert_pair_in_registry on the limit-config admin fns: - On-chain: SetPairLimitBatchMax to a valid-but-unregistered address fails in execution (code 5, "Pair not found in factory registry") — tried both a real contract addr (the router) and a plain account addr, both rejected. The same call on the registered pair returns code 0 and delegates through to update_limit_order_config. So the guard is real, not just compiled. - Also walked every single-pair admin fn on the factory (set_pair_fee/hooks/discount_registry/paused/sweep + the two limit-config): all seven carry the registry guard now. 3 new unit tests pass. I03-F02 / #456 — blacklist guard default-deny: - The deployed guard is unconditional now (the version-gate variant got dropped upstream) — any factory BlacklistCheck query error blocks the trade in both pair and router. - Live happy-path holds: real EMBER->CORAL swaps through the pair and through the router both return code 0 while the factory answers healthy, so default-deny doesn't false-block normal trades. The error->block path is unit-covered on the pair side (factory_blacklist_query_error_blocks_swap). - One follow-up worth a ticket, not a blocker: the router-side fail-closed arm has no unit test yet (only the pair side does) — a router-side factory-query-error unit test would match them up. Checklist is complete and the children are merged + live-verified — good to close from my side. @PlasticDigits

totdking commented

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

(Migrated from gitlab.com)

mentioned in issue #472

totdking commented

2026-07-01 18:45:59 +00:00

(Migrated from gitlab.com)

mentioned in merge request !1006

totdking commented

2026-07-02 16:20:57 +00:00

(Migrated from gitlab.com)

Good to close on this end. As said by @Brouie blacklist guard functions as needed and assert_pair registry admin fn all carry the registry guard. Ready to close on this end @PlasticDigits

totdking (Migrated from gitlab.com) closed this issue

2026-07-03 08:45:04 +00:00