Fix crowdloan reentrancy bug

[l0r1s]

Summary

• Mark crowdloans as finalized before dispatching the configured finalize call, so re-entrant calls into the same crowdloan hit the existing AlreadyFinalized guard before moving funds.
• Add an AlreadyFinalizing guard to reject nested finalize calls while CurrentCrowdloanId is set, preserving the invariant that only one crowdloan finalization context can be active at a time.
• Add regression coverage for:
  • finalize calls that attempt to re-enter withdraw or refund
  • finalize calls that attempt to nested-finalize another crowdloan, verifying AlreadyFinalizing is returned and both crowdloans remain unfinalized

[github-actions]

🛡️ AI Review — Skeptic (security review)

VERDICT: VULNERABLE

BASELINE scrutiny: established 2018 account, repo write permission, substantial prior subtensor PR history; branch fix-crowdloan-reentrancy -> devnet-ready.

No .github/ai-review/*, Copilot-instruction, dependency, lockfile, or build-script changes are present in this PR.

Findings

Sev	File	Finding
HIGH	pallets/crowdloan/src/lib.rs:630	Make finalize transactional before setting finalized early	inline

Prior-comment reconciliation

• 10c5460e: not addressed — The current diff still sets finalized before fallible work and does not add #[transactional] or an equivalent production storage transaction around finalize; the test-only with_storage_layer does not change runtime behavior.

Conclusion

The PR appears legitimate, but finalize still persists the early finalized write before fallible preimage, dispatch, and transfer work. Without a production transaction boundary, a failed finalize can leave the crowdloan finalized without completing finalization, locking funds and stale temporary state.

---

📜 Previous run (superseded)

Sev	File	Finding	Status
HIGH	pallets/crowdloan/src/lib.rs:623	Make finalize transactional before setting finalized early	➡️ Carried forward to current findings The current diff still sets finalized before fallible work and does not add #[transactional] or an equivalent production storage transaction around finalize; the test-only with_storage_layer does not change runtime behavior.

---

# 🔍 AI Review — Auditor (domain review) has not yet run on this PR.

[github-actions]

🔄 AI review updated — Skeptic: VULNERABLE

[github-actions]

🔄 AI review updated — Skeptic: VULNERABLE

[github-actions]

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

[github-actions]

[HIGH] Make finalize transactional before setting finalized early

This write now happens before several fallible operations: preimage lookup/drop, dispatched call execution, balance transfer, and invalid finalization config. Because finalize itself is not annotated #[transactional] or wrapped in an equivalent storage transaction, a later Err can leave crowdloan.finalized = true persisted without emitting Finalized or moving funds. That turns transient failures, including a re-entrant AlreadyFinalized, into a permanent lock that also blocks withdraw, refund, updates, and dissolve paths.

The new test_finalize_blocks_reentrant_withdraw wraps the call in frame_support::storage::with_storage_layer, but that only changes the test harness; production finalize still has no transaction boundary. Add use frame_support::transactional; and #[transactional] to finalize, or keep the early guard in a transactional helper, so any post-marking error rolls this storage write back.

[github-actions]

🔄 AI review updated — Skeptic: VULNERABLE

[open-junius]

One question: if the withdraw and refund cann't be dispatched successfully, should we check both types of all in create extrinsic

[github-actions]

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

[github-actions]

🔄 AI review updated — Skeptic: VULNERABLE

[github-actions]

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

[github-actions]

🔄 AI review updated — Skeptic: VULNERABLE