harden(factory): refund/reject attached uluna even when pair fee is disabled (#276) #763

Merged

Brouie merged 1 commit from qa/276-fee-fund-hardening into main

2026-06-05 04:13:03 +00:00

Brouie commented

2026-06-05 04:02:31 +00:00

(Migrated from gitlab.com)

Follow-up hardening on #276 — the original MR (!754) merged before this landed, so it's a fresh MR against the now-merged code.

Self-review of the BankMsg fund handling (the one part with no in-repo precedent) caught a stuck-funds edge that's now on main: the whole fee block was gated on if fee != 0, so if governance disables the fee (sets it 0) and a caller mistakenly attaches uluna, it was neither forwarded nor refunded — stuck in the factory. A stray denom while the fee's off had the same hole.

Fix: the denom check, the paid >= fee guard, and the excess refund now run regardless of the fee value; only the treasury send stays conditional on fee > 0. So with the fee disabled, any attached uluna is fully refunded, and nothing can get stuck in any path.

Test create_pair_refunds_uluna_when_fee_disabled (fee=0 + attach uluna -> treasury 0, factory 0, full refund). Contract suite green. No closing keyword.

Follow-up hardening on #276 — the original MR (!754) merged before this landed, so it's a fresh MR against the now-merged code. Self-review of the `BankMsg` fund handling (the one part with no in-repo precedent) caught a **stuck-funds edge that's now on main**: the whole fee block was gated on `if fee != 0`, so if governance **disables** the fee (sets it 0) and a caller mistakenly attaches uluna, it was neither forwarded nor refunded — stuck in the factory. A stray denom while the fee's off had the same hole. Fix: the denom check, the `paid >= fee` guard, and the excess refund now run **regardless of the fee value**; only the treasury send stays conditional on `fee > 0`. So with the fee disabled, any attached uluna is fully refunded, and nothing can get stuck in any path. Test `create_pair_refunds_uluna_when_fee_disabled` (fee=0 + attach uluna -> treasury 0, factory 0, full refund). Contract suite green. No closing keyword.

PlasticDigits commented

2026-06-05 04:05:39 +00:00

(Migrated from gitlab.com)

Security review

Commit reviewed: c117428b7a48179eb348c7524fab830217c91357
Scope: execute_create_pair fund handling in smartcontracts/contracts/factory/src/contract.rs (+ regression test in smartcontracts/tests/src/lib.rs). MR hardens BankMsg paths when pair_creation_fee_uluna == 0.

Method: Traced attacker-controlled info.funds / info.sender through denom checks, paid >= fee, treasury send, and refund BankMsgs; compared behavior for fee > 0 vs fee == 0; checked rollback on failed pair instantiate (SubMsg::reply_on_success); reviewed existing fee tests and prior MR discussion threads (none).

Outcome: FINDINGS: 0 medium+

No inline threads — nothing to anchor on the diff.

Notes (informational, not findings)

The diff fixes a real stuck-funds bug on main: when the fee was zero, attached uluna (or stray denoms) skipped the old if !fee.is_zero() block and could remain in the factory.
With this change, denom rejection, paid >= fee, and refund = paid - fee run for all fee values; only the treasury Send is gated on fee > 0. Refunds go only to info.sender and cannot exceed coins attached in the same CreatePair call (paid is derived from info.funds, not the factory’s historical balance).
Fee bypass when fee > 0 remains blocked by InsufficientPairCreationFee; atomic rollback on failed instantiation is unchanged.

Security review: no medium+ findings on this diff.

## Security review **Commit reviewed:** `c117428b7a48179eb348c7524fab830217c91357` **Scope:** `execute_create_pair` fund handling in `smartcontracts/contracts/factory/src/contract.rs` (+ regression test in `smartcontracts/tests/src/lib.rs`). MR hardens `BankMsg` paths when `pair_creation_fee_uluna == 0`. **Method:** Traced attacker-controlled `info.funds` / `info.sender` through denom checks, `paid >= fee`, treasury send, and refund `BankMsg`s; compared behavior for `fee > 0` vs `fee == 0`; checked rollback on failed pair instantiate (`SubMsg::reply_on_success`); reviewed existing fee tests and prior MR discussion threads (none). **Outcome:** `FINDINGS: 0` medium+ No inline threads — nothing to anchor on the diff. ### Notes (informational, not findings) - The diff **fixes** a real stuck-funds bug on `main`: when the fee was zero, attached `uluna` (or stray denoms) skipped the old `if !fee.is_zero()` block and could remain in the factory. - With this change, denom rejection, `paid >= fee`, and `refund = paid - fee` run for all fee values; only the treasury `Send` is gated on `fee > 0`. Refunds go only to `info.sender` and cannot exceed coins attached in the same `CreatePair` call (`paid` is derived from `info.funds`, not the factory’s historical balance). - Fee bypass when `fee > 0` remains blocked by `InsufficientPairCreationFee`; atomic rollback on failed instantiation is unchanged. **Security review: no medium+ findings on this diff.**

PlasticDigits commented

2026-06-05 04:13:04 +00:00

(Migrated from gitlab.com)

mentioned in commit 4ab91ed936

mentioned in commit 4ab91ed9366656445edbf4a07162f67a9c9aa83b