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. The diff is limited to crowdloan pallet logic, regression tests, and the required runtime spec_version bump.

Static review found the reentrancy guards are placed before fund-moving paths: finalize now marks the crowdloan finalized before dispatching the configured call, so re-entrant withdraw / refund hit AlreadyFinalized, and nested finalize is rejected while CurrentCrowdloanId is set. Failed dispatch state is covered by the FRAME dispatch storage-layer rollback assumption already reconciled in the prior run.

Findings

No findings.

Prior-comment reconciliation

• aebbb50f: no longer applies — The prior concern assumed the early finalized write could persist on failed finalization; the current review agrees with the prior rerun that FRAME dispatch storage-layer rollback makes that failure state non-persistent.

Conclusion

No malicious behavior or Skeptic-level security vulnerability found in the current diff. The patch closes the reentrancy window without adding a new bypass or persistent failed-finalize lock.

---

🔍 AI Review — Auditor (domain review)

VERDICT: 👎

Gittensor: UNKNOWN; author is an established repo contributor with write permission. Duplicate-work check found no better crowdloan candidate among overlapping PRs.

PR body is substantive and matches the intended fix. runtime/src/lib.rs bumps spec_version from 423 to 424.

Validation: git diff --check passed. I attempted the targeted cargo test -p pallet-crowdloan test_finalize_fails_if_another_finalize_is_in_progress -- --exact; it could not complete in this sandbox because the normal Cargo home is read-only, and a writable /tmp Cargo home then needed network access to fetch git dependencies.

Findings

Sev	File	Finding
HIGH	pallets/crowdloan/src/lib.rs:610	Finalize still lacks transactional rollback	(post-failed)

Prior-comment reconciliation

• 0ca1be61: not addressed — The same rollback concern still applies: the PR moved the finalized write earlier, but finalize itself is not explicitly transactional.

Conclusion

Blocking until finalize is made explicitly transactional. The reentrancy guard is the right direction, but the early finalized write must roll back on all fallible finalization paths.

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

[sam0x17]

needs spec bump and a few things

[github-actions]

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

[github-actions]

🔄 AI review updated — Skeptic: SAFE Auditor: 👎