Review: Factory pair-creation BankMsg::Send to treasury (GitLab #276) #313

New issue

2026-06-05T04:08:28Z

PlasticDigits commented

2026-06-05 04:08:28 +00:00

(Migrated from gitlab.com)

Current codebase

GitLab #276 introduced pair-creation fee collection in smartcontracts/contracts/factory/src/contract.rs:

fee_msgs.push(CosmosMsg::Bank(BankMsg::Send {
    to_address: config.treasury.to_string(),
    amount: vec![Coin::new(fee.u128(), "uluna")],
}));
// overpay refund via second BankMsg::Send to sender

This was the first in-repo use of BankMsg::Send to treasury (previously fees were CW20-only flows). Treasury contract itself is external (ustr-cmm git dep).

Integration test: create_pair_charges_fee_to_treasury_and_gov_can_set_it in smartcontracts/tests/src/lib.rs.

Why this is needed

Bank sends differ from Wasm executes: native denom routing, blocked addresses, clawback, and failure modes. A careful review ensures:

Treasury address cannot be redirected by non-governance
Refund path cannot be reentered
Fee funds cannot be stuck in factory
Unexpected denoms rejected (UnexpectedPairCreationFunds)
Aligns with Terra Classic bank module semantics post-SDK 0.53

Constraints / guardrails

Review-only unless bugs found — minimize scope.
Governance paths for SetPairCreationFee / treasury update must remain sole mutators.
No change to fee magnitude defaults without governance process.
Cross-check docs/security-model.md and factory invariants.

Relevant files

Area	Path
Factory fee	`smartcontracts/contracts/factory/src/contract.rs` (~lines 220–248)
Errors	`smartcontracts/contracts/factory/src/error.rs`
State	`smartcontracts/contracts/factory/src/state.rs`
Tests	`smartcontracts/tests/src/lib.rs` (`create_pair_charges_fee_to_treasury…`)
Treasury (external)	`ustr-cmm` treasury crate
Docs	`docs/security-model.md`

Recommended direction

Checklist review document in PR description or docs/audits/factory-treasury-bank-send.md:
- Sender authorization on CreatePair
- Exact fee vs overpay refund math (no unsigned wrap)
- Treasury addr validation at instantiate / update
- Factory cannot accumulate stray native tokens
- Interaction with OnePairCreationPerBlock
Add adversarial tests if gaps found:
- Wrong denom in funds
- Treasury = factory / user / burn address
- Zero fee path
- Governance fee change mid-block
Confirm indexer/parser does not need bank events for pair creation indexing.

Acceptance criteria

Written review checklist completed with sign-off.
Any discovered issues filed or fixed in same milestone.
Adversarial tests added for each fixed gap.
No regression in pair creation happy path.

Test plan (all paths)

Path	Expected
Exact fee payment	Treasury +pair created
Underpay	Reject `InsufficientPairCreationFee`
Overpay	Treasury gets fee; user refund
Stray denom	`UnexpectedPairCreationFunds`
Zero fee	Free create still works
Gov updates treasury	New sends go to updated addr

Attack / abuse / hack vectors

Vector	Test
Redirect treasury via user tx	Only governance `UpdateConfig`
Reentrancy via refund	CosmWasm bank send is terminal
Fee bypass	No pair without fee when configured
Block fee griefing	`OnePairCreationPerBlock`

Verification criteria

make test-contracts green.
Review doc merged or attached to issue closure.
Optional external security pass on bank send pattern.

## Current codebase GitLab **#276** introduced pair-creation fee collection in `smartcontracts/contracts/factory/src/contract.rs`: ```rust fee_msgs.push(CosmosMsg::Bank(BankMsg::Send { to_address: config.treasury.to_string(), amount: vec![Coin::new(fee.u128(), "uluna")], })); // overpay refund via second BankMsg::Send to sender ``` This was the **first** in-repo use of `BankMsg::Send` to treasury (previously fees were CW20-only flows). Treasury contract itself is external (`ustr-cmm` git dep). Integration test: `create_pair_charges_fee_to_treasury_and_gov_can_set_it` in `smartcontracts/tests/src/lib.rs`. ## Why this is needed Bank sends differ from Wasm executes: native denom routing, blocked addresses, clawback, and failure modes. A careful review ensures: - Treasury address cannot be redirected by non-governance - Refund path cannot be reentered - Fee funds cannot be stuck in factory - Unexpected denoms rejected (`UnexpectedPairCreationFunds`) - Aligns with Terra Classic bank module semantics post-SDK 0.53 ## Constraints / guardrails - Review-only unless bugs found — minimize scope. - Governance paths for `SetPairCreationFee` / treasury update must remain sole mutators. - No change to fee magnitude defaults without governance process. - Cross-check `docs/security-model.md` and factory invariants. ## Relevant files | Area | Path | |------|------| | Factory fee | `smartcontracts/contracts/factory/src/contract.rs` (~lines 220–248) | | Errors | `smartcontracts/contracts/factory/src/error.rs` | | State | `smartcontracts/contracts/factory/src/state.rs` | | Tests | `smartcontracts/tests/src/lib.rs` (`create_pair_charges_fee_to_treasury…`) | | Treasury (external) | `ustr-cmm` treasury crate | | Docs | `docs/security-model.md` | ## Recommended direction 1. **Checklist review** document in PR description or `docs/audits/factory-treasury-bank-send.md`: - Sender authorization on `CreatePair` - Exact fee vs overpay refund math (no unsigned wrap) - Treasury addr validation at instantiate / update - Factory cannot accumulate stray native tokens - Interaction with `OnePairCreationPerBlock` 2. Add adversarial tests if gaps found: - Wrong denom in funds - Treasury = factory / user / burn address - Zero fee path - Governance fee change mid-block 3. Confirm indexer/parser does not need bank events for pair creation indexing. ## Acceptance criteria - [ ] Written review checklist completed with sign-off. - [ ] Any discovered issues filed or fixed in same milestone. - [ ] Adversarial tests added for each fixed gap. - [ ] No regression in pair creation happy path. ## Test plan (all paths) | Path | Expected | |------|----------| | Exact fee payment | Treasury +pair created | | Underpay | Reject `InsufficientPairCreationFee` | | Overpay | Treasury gets fee; user refund | | Stray denom | `UnexpectedPairCreationFunds` | | Zero fee | Free create still works | | Gov updates treasury | New sends go to updated addr | ## Attack / abuse / hack vectors | Vector | Test | |--------|------| | Redirect treasury via user tx | Only governance `UpdateConfig` | | Reentrancy via refund | CosmWasm bank send is terminal | | Fee bypass | No pair without fee when configured | | Block fee griefing | `OnePairCreationPerBlock` | ## Verification criteria - `make test-contracts` green. - Review doc merged or attached to issue closure. - Optional external security pass on bank send pattern.

PlasticDigits commented

2026-06-05 04:08:28 +00:00

(Migrated from gitlab.com)

marked as related to #276

ghost1 commented

2026-06-05 10:06:43 +00:00

(Migrated from gitlab.com)

mentioned in commit ce26e061f1

mentioned in commit ce26e061f1c169e549252e9514d8b6e45c6a3c82

PlasticDigits commented

2026-06-05 10:07:22 +00:00

(Migrated from gitlab.com)

mentioned in merge request !789

PlasticDigits commented

2026-06-05 10:07:27 +00:00

(Migrated from gitlab.com)

Implementation complete (review + tests)

MR: https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/merge_requests/71

Review outcome

Factory execute_create_pair bank-send logic (#276) is sound — no contract changes required. Checklist and sign-off: docs/audits/factory-treasury-bank-send.md.

Acceptance criteria

Item	Status	How verified
Written review checklist with sign-off	PASS	Audit doc (11 checklist rows)
Issues filed or fixed	PASS	No contract bugs; test gaps closed in MR
Adversarial tests per gap	PASS	`create_pair_fee_bank_send_adversarial_paths`
No happy-path regression	PASS	`make test-contracts` (363 integration tests)

Commands

make verify-issue-313
make test-contracts

Indexer

Pair creation indexing does not depend on bank send events (no factory fee parser in indexer).

Issue left open until MR !789 merges.

## Implementation complete (review + tests) **MR:** https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/merge_requests/71 ### Review outcome Factory `execute_create_pair` bank-send logic (#276) is **sound** — no contract changes required. Checklist and sign-off: [`docs/audits/factory-treasury-bank-send.md`](https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/blob/cursor/gitlab-issue-workflow-c233/docs/audits/factory-treasury-bank-send.md). ### Acceptance criteria | Item | Status | How verified | |------|--------|--------------| | Written review checklist with sign-off | **PASS** | Audit doc (11 checklist rows) | | Issues filed or fixed | **PASS** | No contract bugs; test gaps closed in MR | | Adversarial tests per gap | **PASS** | `create_pair_fee_bank_send_adversarial_paths` | | No happy-path regression | **PASS** | `make test-contracts` (363 integration tests) | ### Commands ```bash make verify-issue-313 make test-contracts ``` ### Indexer Pair creation indexing does **not** depend on bank send events (no factory fee parser in indexer). Issue left **open** until MR !789 merges.

PlasticDigits commented

2026-06-05 10:37:34 +00:00

(Migrated from gitlab.com)

mentioned in commit 0d3c4eeacf

mentioned in commit 0d3c4eeacfd39ef666e34ba458bb56a6247c8221

PlasticDigits commented

2026-06-05 11:04:51 +00:00

(Migrated from gitlab.com)

Verification complete — GitLab #313

Issue: https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/work_items/313
Implementation MR: !789 (merged)

Independent verify pass on branch cursor/gitlab-issue-verification-4a81 (2026-06-05).

Acceptance criteria

Criterion	Status	How verified
Written review checklist with sign-off	PASS	`docs/audits/factory-treasury-bank-send.md` — 11 checklist rows + attack-vector table
Issues filed or fixed in milestone	PASS	No contract bugs found; test gaps closed in !789
Adversarial tests per gap	PASS	`create_pair_fee_bank_send_adversarial_paths` (stray denom, mixed funds, overpay refund, treasury rotation)
No happy-path regression	PASS	`make test-contracts` — 374 integration tests green

Test plan

Path	Expected	Status
Exact fee payment	Treasury + pair created	PASS — `create_pair_charges_fee_to_treasury_and_gov_can_set_it`
Underpay	`InsufficientPairCreationFee`	PASS — same test
Overpay	Treasury gets fee; user refund	PASS — `create_pair_fee_bank_send_adversarial_paths`
Stray denom	`UnexpectedPairCreationFunds`	PASS — adversarial test
Zero fee	Free create; mistaken uluna refunded	PASS — `create_pair_refunds_uluna_when_fee_disabled`
Gov updates treasury	New sends to updated addr	PASS — adversarial test

Attack vectors

Vector	Status	Notes
Redirect treasury via user tx	PASS	Only governance `UpdateConfig` / instantiate
Reentrancy via refund	PASS	Terminal `BankMsg::Send`; no `reply` handler
Fee bypass	PASS	`paid < fee` rejected before pair creation
Block fee griefing	PASS	`OnePairCreationPerBlock` + fee when > 0

Indexer

PASS — no create_pair / pair-creation fee parser in indexer/; pair indexing does not depend on bank send events.

Commands run

make verify-issue-313   # PASS: 4/4 steps
make test-contracts     # 374 passed

Contract review (manual)

Reviewed execute_create_pair fee handling (contract.rs ~L236–267): uluna-only funds, underpay guard, fee to treasury, overpay refund, OnePairCreationPerBlock gate before Response. Cross-checked docs/security-model.md and invariant F2 in docs/contracts-security-audit.md.

Sign-off: Factory BankMsg::Send pair-creation fee path is sound; no further contract changes required.

## Verification complete — GitLab #313 **Issue:** https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/work_items/313 **Implementation MR:** !789 (merged) Independent verify pass on branch `cursor/gitlab-issue-verification-4a81` (2026-06-05). ### Acceptance criteria | Criterion | Status | How verified | |-----------|--------|--------------| | Written review checklist with sign-off | **PASS** | `docs/audits/factory-treasury-bank-send.md` — 11 checklist rows + attack-vector table | | Issues filed or fixed in milestone | **PASS** | No contract bugs found; test gaps closed in !789 | | Adversarial tests per gap | **PASS** | `create_pair_fee_bank_send_adversarial_paths` (stray denom, mixed funds, overpay refund, treasury rotation) | | No happy-path regression | **PASS** | `make test-contracts` — 374 integration tests green | ### Test plan | Path | Expected | Status | |------|----------|--------| | Exact fee payment | Treasury + pair created | **PASS** — `create_pair_charges_fee_to_treasury_and_gov_can_set_it` | | Underpay | `InsufficientPairCreationFee` | **PASS** — same test | | Overpay | Treasury gets fee; user refund | **PASS** — `create_pair_fee_bank_send_adversarial_paths` | | Stray denom | `UnexpectedPairCreationFunds` | **PASS** — adversarial test | | Zero fee | Free create; mistaken uluna refunded | **PASS** — `create_pair_refunds_uluna_when_fee_disabled` | | Gov updates treasury | New sends to updated addr | **PASS** — adversarial test | ### Attack vectors | Vector | Status | Notes | |--------|--------|-------| | Redirect treasury via user tx | **PASS** | Only governance `UpdateConfig` / instantiate | | Reentrancy via refund | **PASS** | Terminal `BankMsg::Send`; no `reply` handler | | Fee bypass | **PASS** | `paid < fee` rejected before pair creation | | Block fee griefing | **PASS** | `OnePairCreationPerBlock` + fee when > 0 | ### Indexer **PASS** — no `create_pair` / pair-creation fee parser in `indexer/`; pair indexing does not depend on bank send events. ### Commands run ```bash make verify-issue-313 # PASS: 4/4 steps make test-contracts # 374 passed ``` ### Contract review (manual) Reviewed `execute_create_pair` fee handling (`contract.rs` ~L236–267): uluna-only funds, underpay guard, fee to treasury, overpay refund, `OnePairCreationPerBlock` gate before `Response`. Cross-checked `docs/security-model.md` and invariant **F2** in `docs/contracts-security-audit.md`. **Sign-off:** Factory `BankMsg::Send` pair-creation fee path is sound; no further contract changes required.

PlasticDigits (Migrated from gitlab.com) closed this issue

2026-06-05 11:05:03 +00:00

PlasticDigits commented

2026-06-13 07:09:04 +00:00

(Migrated from gitlab.com)

mentioned in issue #376

Sign in to join this conversation.