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: SAFE

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

No .github/ai-review/*, Copilot-instruction, dependency, lockfile, or build-script changes are present in this PR. The diff is limited to pallets/crowdloan logic and regression tests.

Findings

No findings.

Prior-comment reconciliation

• aebbb50f: no longer applies — The prior finding assumed the early finalized write could persist on a failed finalize. Local FRAME macro sources and existing runtime comments/tests indicate pallet dispatchables run inside storage layers; failed dispatch rolls back the early write and CurrentCrowdloanId state.

Conclusion

No malicious behavior or security vulnerability found in the current diff. The prior rollback concern does not apply here because pallet dispatchables execute inside FRAME storage layers, and the added reentrancy guards fail before funds move while failed finalization rolls back storage changes.

---

📜 Previous run (superseded)

Sev	File	Finding	Status
HIGH	pallets/crowdloan/src/lib.rs:630	Make finalize transactional before setting finalized early	⏭️ No longer applies The prior finding assumed the early finalized write could persist on a failed finalize. Local FRAME macro sources and existing runtime comments/tests indicate pallet dispatchables run inside storage layers; failed dispatch rolls back the early write and CurrentCrowdloanId state.

---

🔍 AI Review — Auditor (domain review)

VERDICT: 👎

Gittensor: UNKNOWN; author is an established repo contributor with write permission. No overlapping PRs found.

PR body is substantive and matches the intended reentrancy fix. I did not auto-fill the PR description.

Duplicate-work check: no overlapping PRs in the pre-fetched context.

Validation note: targeted cargo test -p pallet-crowdloan ... could not run because rustup attempted to create temp files under read-only /home/runner/.rustup/tmp. Spec-version auto-fix was not attempted because the devnet runtime-version endpoint could not be resolved from this sandbox.

Findings

Sev	File	Finding
HIGH	pallets/crowdloan/src/lib.rs:630	Finalize commits state before fallible dispatch	inline

Conclusion

Blocking because the new write-before-dispatch ordering makes failed finalization paths non-atomic and can leave crowdloans finalized or CurrentCrowdloanId stuck without completing the fund movement. The reentrancy direction is right, but the failure paths need transactional rollback or equivalent cleanup before merge.

[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

[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: SAFE Auditor: 👎