code/cl8y-dex-terraclassic

Fork 0

Investigate: Factory PENDING_PAIR race — wrong assets may be registered #121

New issue

Closed

opened 2026-05-03 11:59:33 +00:00 by PlasticDigits · 6 comments

PlasticDigits commented

2026-05-03 11:59:33 +00:00

(Migrated from gitlab.com)

Summary

Investigate reported behavior in the factory contract around PENDING_PAIR, execute_create_pair, and reply_instantiate_pair: a single global pending slot combined with unused MessageInfo may cause incorrect registry entries and unintended permissionless pair creation.

Reported severity

High

Location

File: smartcontracts/contracts/factory/src/contract.rs
execute_create_pair: lines ~119–198
reply_instantiate_pair: lines ~596–618

Code behavior (as reported)

execute_create_pair takes _info: MessageInfo unused (permissionless callers). It saves pending assets:

PENDING_PAIR.save(deps.storage, &asset_infos)?; // single global slot
let sub_msg = SubMsg::reply_on_success(
    WasmMsg::Instantiate { ... },
    REPLY_INSTANTIATE_PAIR,
);

reply_instantiate_pair loads from that slot and registers the pair:

let contract_addr = parse_reply_contract_address(msg)?;
let asset_infos = PENDING_PAIR.load(deps.storage)?; // reads whatever is there now
PENDING_PAIR.remove(deps.storage);
// ...
PAIRS.save(deps.storage, &key, &pair_info_resp)?;

Issue A — Permissionless pair creation

MessageInfo is explicitly ignored; any wallet could call CreatePair for whitelisted-code-ID tokens, bypassing governance intent if governance-gated creation was assumed. Risks cited: spam/unwanted pairs, squatting on key pairs, subtle token-ordering traps. Report notes permissionless AMM creation is sometimes intentional; paired with Issue B this becomes more serious.

Issue B — Single-slot `PENDING_PAIR` race

CosmWasm executes one tx at a time per contract, but two CreatePair txs in the same block from different senders could run sequentially: the second overwrites PENDING_PAIR before the first reply_instantiate_pair runs, so the first reply may read the second caller’s asset_infos and register wrong assets for the first pair’s address. Impact: factory registry disagrees with the pair contract’s real token setup → incorrect routing → potential fund loss.

Impact (as reported)

Wrong asset_infos for a new pair → incorrect routing → fund loss
Spam pair creation → operational pollution, storage growth
Pair squatting before governance-intended pairs

Recommended fix (from report)

Race: Replace Item<[AssetInfo; 2]> with something keyed uniquely, e.g. Map<u64, [AssetInfo; 2]> by reply ID, or Map<Addr, [AssetInfo; 2]> keyed by instantiating contract address from the reply; use a unique reply ID per pending pair where appropriate.
Permissionless: If governance-gated creation is desired, add ensure_governance to execute_create_pair. If permissionless is intentional, document it and still fix Issue B.

Suggested test (from report)

fn test_concurrent_create_pair_registers_correct_assets() {
    // create pair A-B and pair C-D in the same block (sequential in multi-test)
    // assert PAIRS[A-B].asset_infos == [A, B]
    // assert PAIRS[C-D].asset_infos == [C, D]
    // (current behavior may associate A-B address with C-D assets)
}

Next steps

Confirm whether CreatePair is intended to be permissionless for this DEX.
Reproduce cross-tx / same-block ordering with cw-multi-test or integration tests.
Design storage keying for pending pairs and migration if needed.
Add regression test as above.

## Summary Investigate reported behavior in the factory contract around `PENDING_PAIR`, `execute_create_pair`, and `reply_instantiate_pair`: a single global pending slot combined with unused `MessageInfo` may cause incorrect registry entries and unintended permissionless pair creation. ## Reported severity High ## Location - File: `smartcontracts/contracts/factory/src/contract.rs` - `execute_create_pair`: lines ~119–198 - `reply_instantiate_pair`: lines ~596–618 ## Code behavior (as reported) `execute_create_pair` takes `_info: MessageInfo` **unused** (permissionless callers). It saves pending assets: ```rust PENDING_PAIR.save(deps.storage, &asset_infos)?; // single global slot let sub_msg = SubMsg::reply_on_success( WasmMsg::Instantiate { ... }, REPLY_INSTANTIATE_PAIR, ); ``` `reply_instantiate_pair` loads from that slot and registers the pair: ```rust let contract_addr = parse_reply_contract_address(msg)?; let asset_infos = PENDING_PAIR.load(deps.storage)?; // reads whatever is there now PENDING_PAIR.remove(deps.storage); // ... PAIRS.save(deps.storage, &key, &pair_info_resp)?; ``` ## Issue A — Permissionless pair creation `MessageInfo` is explicitly ignored; any wallet could call `CreatePair` for whitelisted-code-ID tokens, bypassing governance intent if governance-gated creation was assumed. Risks cited: spam/unwanted pairs, squatting on key pairs, subtle token-ordering traps. Report notes permissionless AMM creation is sometimes intentional; paired with Issue B this becomes more serious. ## Issue B — Single-slot `PENDING_PAIR` race CosmWasm executes one tx at a time per contract, but two `CreatePair` txs in the same block from different senders could run sequentially: the second overwrites `PENDING_PAIR` before the first `reply_instantiate_pair` runs, so the first reply may read the second caller’s `asset_infos` and register **wrong assets** for the first pair’s address. Impact: factory registry disagrees with the pair contract’s real token setup → incorrect routing → potential fund loss. ## Impact (as reported) - Wrong `asset_infos` for a new pair → incorrect routing → fund loss - Spam pair creation → operational pollution, storage growth - Pair squatting before governance-intended pairs ## Recommended fix (from report) 1. **Race:** Replace `Item<[AssetInfo; 2]>` with something keyed uniquely, e.g. `Map<u64, [AssetInfo; 2]>` by reply ID, or `Map<Addr, [AssetInfo; 2]>` keyed by instantiating contract address from the reply; use a unique reply ID per pending pair where appropriate. 2. **Permissionless:** If governance-gated creation is desired, add `ensure_governance` to `execute_create_pair`. If permissionless is intentional, document it and still fix Issue B. ## Suggested test (from report) ```rust fn test_concurrent_create_pair_registers_correct_assets() { // create pair A-B and pair C-D in the same block (sequential in multi-test) // assert PAIRS[A-B].asset_infos == [A, B] // assert PAIRS[C-D].asset_infos == [C, D] // (current behavior may associate A-B address with C-D assets) } ``` ## Next steps - [ ] Confirm whether `CreatePair` is intended to be permissionless for this DEX. - [ ] Reproduce cross-tx / same-block ordering with `cw-multi-test` or integration tests. - [ ] Design storage keying for pending pairs and migration if needed. - [ ] Add regression test as above.

PlasticDigits commented

2026-05-03 12:26:45 +00:00

(Migrated from gitlab.com)

mentioned in commit 256763ba88

mentioned in commit 256763ba8897b8087009f2675e386bf47e1119df

PlasticDigits commented

2026-05-03 12:26:55 +00:00

(Migrated from gitlab.com)

Fix merged to `main` (please verify) — @brouie

Summary

Permissionless CreatePair preserved (no governance gate on pair creation).
Invariant: at most one factory CreatePair that enters the pending/instantiate path per block height (PAIR_CREATION_BLOCK + ContractError::OnePairCreationPerBlock).
Issue #121 / cross-tx race: documented that on standard Cosmos SDK execution, a full transaction (factory execute + WasmMsg::Instantiate submessages + reply) finishes before the next tx runs, so the second transaction overwriting PENDING_PAIR before the first tx’s reply scenario does not apply on-chain. The per-block gate is still defense-in-depth + explicit rate limit and matches the desired product constraint.

Code / docs

Factory: smartcontracts/contracts/factory/src/{contract,state,error,lib}.rs
Regression test: factory_tests::test_create_pair_one_per_block_then_next_block_ok (+ test helpers advance block between multiple creates).
Human docs: docs/security-model.md (new subsection), docs/contracts-terraclassic.md, docs/contracts-security-audit.md (F1).
Agent skill: skills/AGENTS_LOCALNET_TRADING_SWARM.md (rule 6: one create_pair per block in automation).

Verification checklist for @brouie

On testnet/localnet, submit two create_pair txs in the same block (or from the same client without waiting for height): second should fail with Only one CreatePair may run per block; retry next block.
Retry the second after the next block: should succeed (valid tokens / whitelist / no duplicate pair).
Confirm permissionless behavior: non-governance wallet can still create_pair when whitelist rules pass.
Read docs/security-model.md#createpair-rate-limit-and-pending-state and confirm wording matches your understanding of atomicity vs the per-block gate.
If you maintain scripts that create many pairs in a loop, ensure one tx per block (or a delay until height increments); see swarm skill rule 6.

Issue left open for your sign-off.

## Fix merged to `main` (please verify) — @brouie ### Summary - **Permissionless `CreatePair` preserved** (no governance gate on pair creation). - **Invariant:** at most **one** factory `CreatePair` that enters the pending/instantiate path **per block height** (`PAIR_CREATION_BLOCK` + `ContractError::OnePairCreationPerBlock`). - **Issue #121 / cross-tx race:** documented that on **standard Cosmos SDK** execution, a **full transaction** (factory execute + `WasmMsg::Instantiate` submessages + `reply`) finishes **before** the next tx runs, so the *second transaction overwriting `PENDING_PAIR` before the first tx’s reply* scenario **does not apply on-chain**. The per-block gate is still **defense-in-depth + explicit rate limit** and matches the desired product constraint. ### Code / docs - Factory: `smartcontracts/contracts/factory/src/{contract,state,error,lib}.rs` - Regression test: `factory_tests::test_create_pair_one_per_block_then_next_block_ok` (+ test helpers advance block between multiple creates). - Human docs: `docs/security-model.md` (new subsection), `docs/contracts-terraclassic.md`, `docs/contracts-security-audit.md` (**F1**). - Agent skill: `skills/AGENTS_LOCALNET_TRADING_SWARM.md` (rule 6: one `create_pair` per block in automation). ### Verification checklist for @brouie 1. [ ] On testnet/localnet, submit **two** `create_pair` **txs in the same block** (or from the same client without waiting for height): second should fail with *Only one CreatePair may run per block; retry next block*. 2. [ ] Retry the second **after** the next block: should succeed (valid tokens / whitelist / no duplicate pair). 3. [ ] Confirm **permissionless** behavior: non-governance wallet can still `create_pair` when whitelist rules pass. 4. [ ] Read `docs/security-model.md#createpair-rate-limit-and-pending-state` and confirm wording matches your understanding of **atomicity vs** the per-block gate. 5. [ ] If you maintain scripts that create **many** pairs in a loop, ensure **one tx per block** (or a delay until height increments); see swarm skill rule 6. Issue left **open** for your sign-off.

PlasticDigits commented

2026-05-03 12:28:47 +00:00

(Migrated from gitlab.com)

@brouie Update: at merges **** on top of the #121 per-block gate, so imports both and . Factory CONTRACT_VERSION is 1.1.0 (122 migrate path). The verification checklist in the previous note still applies.

@brouie **Update:** at merges **** on top of the #121 per-block gate, so imports both and . Factory **CONTRACT_VERSION** is **1.1.0** (122 migrate path). The verification checklist in the previous note still applies.

PlasticDigits commented

2026-05-03 12:28:55 +00:00

(Migrated from gitlab.com)

Update for brouie: main commit c8f713c merges the pair address registry work (GitLab 122) on top of the 121 per-block CreatePair gate. Factory imports both PAIR_ADDR_REGISTERED and PAIR_CREATION_BLOCK; contract version is 1.1.0. Prior verification checklist still applies.

Update for brouie: main commit c8f713c merges the pair address registry work (GitLab 122) on top of the 121 per-block CreatePair gate. Factory imports both PAIR_ADDR_REGISTERED and PAIR_CREATION_BLOCK; contract version is 1.1.0. Prior verification checklist still applies.

Brouie commented

2026-05-05 23:55:34 +00:00

(Migrated from gitlab.com)

mentioned in issue #133

Brouie commented

2026-05-06 04:20:22 +00:00

(Migrated from gitlab.com)

ran your named test:

test factory_tests::test_create_pair_one_per_block_then_next_block_ok ... ok

full workspace cargo test 321/321 PASS, 0 failures (up from 308/308 baseline at 5c744ee — 13 new tests across #121/#122/#123).

source review of the gate:

PAIR_CREATION_BLOCK: Item<u64> declared in state.rs:44
OnePairCreationPerBlock error variant in error.rs:25
gate fires at contract.rs:190-196 inside execute_create_pair — loads stored block, compares to env.block.height, errors if same height, then saves current height after passing. positioned before any PENDING_PAIR.save so the flow is clean.

verified the docs section docs/security-model.md#createpair-rate-limit-and-pending-state reads correctly — the wording on cosmos-sdk tx atomicity (full execute + submessages + reply finishing before next tx) tracks with how I understand the runtime, and the per-block gate as explicit rate-limit + defense-in-depth makes sense even if the cross-tx race scenario does not apply in practice.

agent skill rule 6 in AGENTS_LOCALNET_TRADING_SWARM.md covers swarm scripts that loop create_pair — useful guardrail.

still pending: live ops verification — submitting two create_pair txs in the same block on LocalTerra/Station and observing the OnePairCreationPerBlock revert on the second, then retry on next block. that is laptop-session work, queued behind #115 and #118 item 4 in the live-walk debt.

ready for close on your side once you are happy with source PASS, or hold open if you want me to schedule the chain-ops walk first.

@PlasticDigits — source-side verification PASS on the per-block gate. happy with the design rationale on the cross-tx atomicity argument too — gate is good defense-in-depth and matches the desired product constraint regardless. ran your named test: ``` test factory_tests::test_create_pair_one_per_block_then_next_block_ok ... ok ``` full workspace cargo test 321/321 PASS, 0 failures (up from 308/308 baseline at `5c744ee` — 13 new tests across #121/#122/#123). source review of the gate: - `PAIR_CREATION_BLOCK: Item<u64>` declared in `state.rs:44` - `OnePairCreationPerBlock` error variant in `error.rs:25` - gate fires at `contract.rs:190-196` inside `execute_create_pair` — loads stored block, compares to `env.block.height`, errors if same height, then saves current height after passing. positioned before any `PENDING_PAIR.save` so the flow is clean. verified the docs section `docs/security-model.md#createpair-rate-limit-and-pending-state` reads correctly — the wording on cosmos-sdk tx atomicity (full execute + submessages + reply finishing before next tx) tracks with how I understand the runtime, and the per-block gate as explicit rate-limit + defense-in-depth makes sense even if the cross-tx race scenario does not apply in practice. agent skill rule 6 in `AGENTS_LOCALNET_TRADING_SWARM.md` covers swarm scripts that loop `create_pair` — useful guardrail. still pending: live ops verification — submitting two `create_pair` txs in the same block on LocalTerra/Station and observing the `OnePairCreationPerBlock` revert on the second, then retry on next block. that is laptop-session work, queued behind #115 and #118 item 4 in the live-walk debt. ready for close on your side once you are happy with source PASS, or hold open if you want me to schedule the chain-ops walk first.

PlasticDigits (Migrated from gitlab.com) closed this issue

2026-05-06 08:15:47 +00:00