code/cl8y-dex-terraclassic

Fork 0

Security: missing assert_pair_in_registry on two factory batch limit config functions [SEC-I03] (F01) #455

New issue

Closed

opened 2026-06-30 17:31:17 +00:00 by totdking · 9 comments

totdking commented

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

(Migrated from gitlab.com)

Summary

Two factory admin functions -- execute_set_pair_limit_batch_max and execute_set_pair_limit_clean_config -- validate the pair address with deps.api.addr_validate but do not call assert_pair_in_registry. Every other single-pair admin function in the factory calls assert_pair_in_registry after address validation. Without the registry check, governance can issue UpdateLimitOrderConfig or UpdateLimitCleanConfig to any arbitrary valid address, not just factory-registered pairs. Practical impact is low because only governance can invoke these messages, but the inconsistency breaks the uniform registry guard pattern and introduces a discoverability gap.

What Was Checked

smartcontracts/contracts/factory/src/contract.rs lines 718-739 (execute_set_pair_limit_batch_max): calls deps.api.addr_validate then proceeds without assert_pair_in_registry.
smartcontracts/contracts/factory/src/contract.rs lines 741-764 (execute_set_pair_limit_clean_config): same pattern, no registry check.
Compared against execute_set_pair_fee (line 352), execute_set_pair_hooks (line 396), execute_set_discount_registry (line 422), execute_set_pair_paused (line 661), execute_sweep_pair (line 687): all call assert_pair_in_registry after addr_validate.

Expected (per checklist)

All factory admin functions that target a pair address call assert_pair_in_registry immediately after address validation, consistently enforcing that the target is a factory-registered pair.

Actual

execute_set_pair_limit_batch_max and execute_set_pair_limit_clean_config omit the registry check. The pattern is inconsistent with all other single-pair admin functions.

Suggested Fix

Add assert_pair_in_registry(&deps, &pair_addr)? to both functions immediately after deps.api.addr_validate(&pair)?, following the pattern in execute_set_pair_fee. No behavior change for valid governance operations targeting registered pairs.

Verification Checklist

assert_pair_in_registry added to execute_set_pair_limit_batch_max after addr_validate
assert_pair_in_registry added to execute_set_pair_limit_clean_config after addr_validate
Test added: governance calling SetPairLimitBatchMax or SetPairLimitCleanConfig against a non-registered address returns an error
Existing tests for these functions still pass

Cc: @PlasticDigits

### Summary Two factory admin functions -- `execute_set_pair_limit_batch_max` and `execute_set_pair_limit_clean_config` -- validate the pair address with `deps.api.addr_validate` but do not call `assert_pair_in_registry`. Every other single-pair admin function in the factory calls `assert_pair_in_registry` after address validation. Without the registry check, governance can issue `UpdateLimitOrderConfig` or `UpdateLimitCleanConfig` to any arbitrary valid address, not just factory-registered pairs. Practical impact is low because only governance can invoke these messages, but the inconsistency breaks the uniform registry guard pattern and introduces a discoverability gap. --- ### What Was Checked - `smartcontracts/contracts/factory/src/contract.rs` lines 718-739 (`execute_set_pair_limit_batch_max`): calls `deps.api.addr_validate` then proceeds without `assert_pair_in_registry`. - `smartcontracts/contracts/factory/src/contract.rs` lines 741-764 (`execute_set_pair_limit_clean_config`): same pattern, no registry check. - Compared against `execute_set_pair_fee` (line 352), `execute_set_pair_hooks` (line 396), `execute_set_discount_registry` (line 422), `execute_set_pair_paused` (line 661), `execute_sweep_pair` (line 687): all call `assert_pair_in_registry` after `addr_validate`. --- ### Expected (per checklist) All factory admin functions that target a pair address call `assert_pair_in_registry` immediately after address validation, consistently enforcing that the target is a factory-registered pair. --- ### Actual `execute_set_pair_limit_batch_max` and `execute_set_pair_limit_clean_config` omit the registry check. The pattern is inconsistent with all other single-pair admin functions. --- ### Suggested Fix Add `assert_pair_in_registry(&deps, &pair_addr)?` to both functions immediately after `deps.api.addr_validate(&pair)?`, following the pattern in `execute_set_pair_fee`. No behavior change for valid governance operations targeting registered pairs. --- ### Verification Checklist - [ ] `assert_pair_in_registry` added to `execute_set_pair_limit_batch_max` after `addr_validate` - [ ] `assert_pair_in_registry` added to `execute_set_pair_limit_clean_config` after `addr_validate` - [ ] Test added: governance calling `SetPairLimitBatchMax` or `SetPairLimitCleanConfig` against a non-registered address returns an error - [ ] Existing tests for these functions still pass Cc: @PlasticDigits

totdking commented

2026-06-30 17:36:32 +00:00

(Migrated from gitlab.com)

changed title from Security: missing assert_pair_in_registry on two factory batch limit config functions [SEC-I03] to Security: missing assert_pair_in_registry on two factory batch limit config functions [SEC-I03] (F01)

<p>changed title from <code class="idiff">Security: missing assert_pair_in_registry on two factory batch limit config functions [SEC-I03]</code> to <code class="idiff">Security: missing assert_pair_in_registry on two factory batch limit config functions [SEC-I03]<span class="idiff left right addition"> (F01)</span></code></p>

totdking commented

2026-06-30 17:36:32 +00:00

(Migrated from gitlab.com)

changed the description

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:03:31 +00:00

(Migrated from gitlab.com)

mentioned in merge request !981

Brouie commented

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

(Migrated from gitlab.com)

Took this one. Added assert_pair_in_registry to both execute_set_pair_limit_batch_max and execute_set_pair_limit_clean_config, right after addr_validate, matching the pattern in execute_set_pair_fee. Also fixed a stale doc comment on the batch-max fn (it had the update_config blurb).

So governance can no longer dispatch UpdateLimitOrderConfig / UpdateLimitCleanConfig at an arbitrary valid-but-unregistered address — same uniform guard as every other single-pair admin fn now.

Tests (new in pair_addr_registry_tests):

set_pair_limit_batch_max_rejects_unregistered_pair -> PairNotInRegistry
set_pair_limit_clean_config_rejects_unregistered_pair -> PairNotInRegistry
set_pair_limit_config_accepts_registered_pair -> both succeed against a registered pair

factory lib 5/0, full smartcontracts workspace 460/0.

MR !981, branch qa/455-factory-registry-guard, commit f481b2b4. Needs your review/merge @PlasticDigits — leaving it open for verification.

Took this one. Added `assert_pair_in_registry` to both `execute_set_pair_limit_batch_max` and `execute_set_pair_limit_clean_config`, right after `addr_validate`, matching the pattern in `execute_set_pair_fee`. Also fixed a stale doc comment on the batch-max fn (it had the `update_config` blurb). So governance can no longer dispatch `UpdateLimitOrderConfig` / `UpdateLimitCleanConfig` at an arbitrary valid-but-unregistered address — same uniform guard as every other single-pair admin fn now. Tests (new in `pair_addr_registry_tests`): - `set_pair_limit_batch_max_rejects_unregistered_pair` -> PairNotInRegistry - `set_pair_limit_clean_config_rejects_unregistered_pair` -> PairNotInRegistry - `set_pair_limit_config_accepts_registered_pair` -> both succeed against a registered pair factory lib 5/0, full smartcontracts workspace 460/0. MR !981, branch `qa/455-factory-registry-guard`, commit `f481b2b4`. Needs your review/merge @PlasticDigits — leaving it open for verification.

PlasticDigits commented

2026-06-30 21:56:08 +00:00

(Migrated from gitlab.com)

mentioned in commit e242a3abd1

mentioned in commit e242a3abd1a25cc9c5dd27448f7d0d5fbbb0b307

PlasticDigits commented

2026-06-30 22:15:52 +00:00

(Migrated from gitlab.com)

Verification complete — all criteria PASS

Verified on main at merge commit e242a3ab (MR !981, f481b2b4).

Checklist

Criterion	Result	How verified
`assert_pair_in_registry` in `execute_set_pair_limit_batch_max` after `addr_validate`	PASS	`contract.rs:725-726` — `addr_validate` then `assert_pair_in_registry`
`assert_pair_in_registry` in `execute_set_pair_limit_clean_config` after `addr_validate`	PASS	`contract.rs:752-753` — same pattern as `execute_set_pair_fee`
Test: governance `SetPairLimitBatchMax` / `SetPairLimitCleanConfig` on non-registered address returns error	PASS	`set_pair_limit_batch_max_rejects_unregistered_pair` and `set_pair_limit_clean_config_rejects_unregistered_pair` both assert `ContractError::PairNotInRegistry`
Registered-pair success path	PASS	`set_pair_limit_config_accepts_registered_pair` — both fns succeed when `PAIR_ADDR_REGISTERED` is set
Existing tests still pass	PASS	`make test-contracts` — factory lib 5/0, full smartcontracts workspace 460/0

Commands

cargo test -p cl8y-dex-factory pair_addr_registry_tests   # 5/0
make test-contracts                                        # 460/0

Notes

Fix restores uniform registry guard across all single-pair factory admin functions. Practical impact remains low (governance-only), but the inconsistency and discoverability gap from SEC-I03 F01 are resolved.

Closing as verified.

## Verification complete — all criteria PASS Verified on `main` at merge commit `e242a3ab` (MR !981, `f481b2b4`). ### Checklist | Criterion | Result | How verified | |-----------|--------|--------------| | `assert_pair_in_registry` in `execute_set_pair_limit_batch_max` after `addr_validate` | **PASS** | `contract.rs:725-726` — `addr_validate` then `assert_pair_in_registry` | | `assert_pair_in_registry` in `execute_set_pair_limit_clean_config` after `addr_validate` | **PASS** | `contract.rs:752-753` — same pattern as `execute_set_pair_fee` | | Test: governance `SetPairLimitBatchMax` / `SetPairLimitCleanConfig` on non-registered address returns error | **PASS** | `set_pair_limit_batch_max_rejects_unregistered_pair` and `set_pair_limit_clean_config_rejects_unregistered_pair` both assert `ContractError::PairNotInRegistry` | | Registered-pair success path | **PASS** | `set_pair_limit_config_accepts_registered_pair` — both fns succeed when `PAIR_ADDR_REGISTERED` is set | | Existing tests still pass | **PASS** | `make test-contracts` — factory lib 5/0, full smartcontracts workspace **460/0** | ### Commands ```bash cargo test -p cl8y-dex-factory pair_addr_registry_tests # 5/0 make test-contracts # 460/0 ``` ### Notes Fix restores uniform registry guard across all single-pair factory admin functions. Practical impact remains low (governance-only), but the inconsistency and discoverability gap from SEC-I03 F01 are resolved. Closing as verified.

PlasticDigits (Migrated from gitlab.com) closed this issue

2026-06-30 22:15:53 +00:00

Brouie commented

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

(Migrated from gitlab.com)

mentioned in issue #337