Skip to content

hard: liquidity pool blindly pays any merchant address the creditline names - #96

Open
BigMick03 wants to merge 1 commit into
StepFi-app:mainfrom
BigMick03:fix/lp-fund-loan-hardening
Open

hard: liquidity pool blindly pays any merchant address the creditline names#96
BigMick03 wants to merge 1 commit into
StepFi-app:mainfrom
BigMick03:fix/lp-fund-loan-hardening

Conversation

@BigMick03

Copy link
Copy Markdown

Summary

fund_loan() honored transfer(pool → merchant, amount) for whatever merchant the registered creditline supplied, with zero independent verification. The pool's solvency rested entirely on the creditline contract being bug-free; a compromised creditline admin could repoint set_creditline() and drain the pool instantly, with absorb_loss() erasing the accounting trace.

This PR adds defense-in-depth caps to the liquidity pool, all disabled by default (backward compatible):

  • Per-ledger outflow cap — cumulative fund_loan outflows within a single ledger bounded to max_outflow_bps of the available-liquidity snapshot (default 10000 = disabled; admin-only setter, rejects > 10000). New error LedgerOutflowCapExceeded = 14.
  • Single-recipient concentration cap — cumulative funded-per-merchant totals tracked in persistent storage (TTL extended) and capped at max_per_merchant (admin-only setter, 0 = disabled). New error MerchantConcentrationCapExceeded = 15.
  • Optional vendor cross-checkset_vendor_registry(admin, Option<Address>); when set, fund_loan requires is_active(merchant) via try_invoke_contract before transferring (fails closed). Unset = legacy behavior. New errors VendorNotActive = 12, VendorNotRegistered = 13.
  • Granular LQFUND event — topics (LQFUND, creditline, merchant), data (amount, remaining_ledger_cap, remaining_merchant_cap) for indexer monitoring.

Semantics: caps default to disabled so honest flows are unchanged. Recommended production hardening: set_max_outflow_bps(2500) (25%/ledger) + set_max_per_merchant(<ceiling>) after deployment, so a malicious creditline is bounded to a fraction of available liquidity per ledger instead of draining the pool instantly.

Tests

18 new tests, 113 total in liquidity-pool-contract, all green: repeated-call outflow cap (rejection does not consume headroom), rolling-window reset on new ledger, default-disabled regression, setter validation + non-admin guards, per-merchant cap incl. isolation and rejected-attempt state, vendor check (approved/unregistered/suspended/unset/clear-roundtrip + non-admin guard), LQFUND event payload.

Verification: cargo test -p liquidity-pool-contract → 113 passed; cargo test --workspace → 356 passed, 0 failed; clippy clean; wasm32-unknown-unknown --release build succeeds.

Split out of PR #95 per review feedback (parameters-contract multisig hardening stays in that PR).

@BigMick03
BigMick03 requested a review from EmeditWeb as a code owner August 27, 2026 01:28
@EmeditWeb

Copy link
Copy Markdown
Member

⚠️ @BigMick03 this PR now has merge conflicts with the base branch (likely because another PR was merged first).

Please rebase/merge the base branch into your branch and resolve the conflicts — a fresh audit will run automatically once new commits land.

@EmeditWeb EmeditWeb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Automated Audit: does_not_solve

@BigMick03 Please look into the issue again and address the gaps below.

The authoritative verification flags an unresolved merge conflict, which by the audit rules means the PR cannot be approved regardless of content. Beyond that, the substantive defense is opt-in: all three mitigations (per-ledger outflow cap, per-merchant cap, vendor cross-check) default to DISABLED, so a freshly deployed pool remains exactly as vulnerable as before — a compromised creditline admin can still drain it until an admin manually configures caps and a vendor registry. The root cause (fund_loan blindly trusts the creditline-named merchant) is therefore not remediated in the default/initial state; this is defense-in-depth scaffolding, not a root-cause fix. The code itself is well-built and tested, but it does not actually close the described vulnerability out of the box.

Gaps identified:

  • Unresolved merge conflict with base branch (hard blocker per audit rules)
  • Caps/vendor-check default to disabled, so the vulnerability remains open in default deployment — root cause not remediated without out-of-band admin action
  • No enforcement that caps or a vendor registry must be set before/at deployment (or sensible secure defaults) to actually bound drain risk

CI checks: ✅ PASSED: Build and Test Contracts
Merge conflicts: ⚠️ YES — this PR has conflicts with the base branch and cannot be merged. Please resolve the conflicts before this PR can be approved.

Audited by stepfi-audit-bot 🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants