From bb71e41e573cd2b0b94526c9e2091dec86cd48a4 Mon Sep 17 00:00:00 2001 From: Loris Moulin Date: Tue, 23 Jun 2026 12:04:03 -0300 Subject: [PATCH 1/3] Fix crowdloan reentrancy bug --- pallets/crowdloan/src/lib.rs | 6 +- pallets/crowdloan/src/tests.rs | 128 +++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 3 deletions(-) diff --git a/pallets/crowdloan/src/lib.rs b/pallets/crowdloan/src/lib.rs index 8be80c0cc7..0144c43046 100644 --- a/pallets/crowdloan/src/lib.rs +++ b/pallets/crowdloan/src/lib.rs @@ -620,6 +620,9 @@ pub mod pallet { ensure!(crowdloan.raised == crowdloan.cap, Error::::CapNotRaised); ensure!(!crowdloan.finalized, Error::::AlreadyFinalized); + crowdloan.finalized = true; + Crowdloans::::insert(crowdloan_id, &crowdloan); + match (&crowdloan.call, &crowdloan.target_address) { (Some(call), None) => { // Set the current crowdloan id so the dispatched call @@ -659,9 +662,6 @@ pub mod pallet { } } - crowdloan.finalized = true; - Crowdloans::::insert(crowdloan_id, &crowdloan); - Self::deposit_event(Event::::Finalized { crowdloan_id }); Ok(()) diff --git a/pallets/crowdloan/src/tests.rs b/pallets/crowdloan/src/tests.rs index a23de52198..9cffa10ef5 100644 --- a/pallets/crowdloan/src/tests.rs +++ b/pallets/crowdloan/src/tests.rs @@ -1875,6 +1875,134 @@ fn test_finalize_fails_if_call_fails() { }); } +// The finalize `call` cannot re-enter `withdraw` on the same crowdloan: it is rejected and +// the extrinsic reverts, so no funds move and `raised` stays consistent with the real balance. +#[test] +fn test_finalize_blocks_reentrant_withdraw() { + TestState::default() + .with_balance(U256::from(1), 200.into()) // creator + .with_balance(U256::from(2), 200.into()) // contributor + .build_and_execute(|| { + let creator: AccountOf = U256::from(1); + let contributor: AccountOf = U256::from(2); + let deposit: BalanceOf = 50.into(); + let min_contribution: BalanceOf = 10.into(); + let cap: BalanceOf = 100.into(); + let end: BlockNumberFor = 50; + let crowdloan_id: CrowdloanId = 0; + + // The finalize call re-enters `withdraw` on the same crowdloan. + let reentrant_call = Box::new(RuntimeCall::Crowdloan( + pallet_crowdloan::Call::::withdraw { crowdloan_id }, + )); + + assert_ok!(Crowdloan::create( + RuntimeOrigin::signed(creator), + deposit, + min_contribution, + cap, + end, + Some(reentrant_call), + None, + )); + run_to_block(10); + + // Creator contributes 30 over the deposit (total 80); contributor fills the cap. + assert_ok!(Crowdloan::contribute( + RuntimeOrigin::signed(creator), + crowdloan_id, + 30.into() + )); + assert_ok!(Crowdloan::contribute( + RuntimeOrigin::signed(contributor), + crowdloan_id, + 20.into() + )); + + let funds_account = pallet_crowdloan::Pallet::::funds_account(crowdloan_id); + assert_eq!(Balances::free_balance(funds_account), cap); + let creator_balance_before = Balances::free_balance(creator); + + run_to_block(60); + + // Finalize dispatches the re-entrant withdraw, which is rejected with + // `AlreadyFinalized`. Wrap in a storage layer to model the per-extrinsic + // transaction the runtime applies in production, so the revert is observable. + let outcome = frame_support::storage::with_storage_layer(|| { + Crowdloan::finalize(RuntimeOrigin::signed(creator), crowdloan_id) + }); + assert_err!(outcome, pallet_crowdloan::Error::::AlreadyFinalized); + + // No funds were extracted and accounting is intact. + assert_eq!(Balances::free_balance(creator), creator_balance_before); + assert_eq!(Balances::free_balance(funds_account), cap); + assert_eq!(pallet_crowdloan::CurrentCrowdloanId::::get(), None); + let crowdloan = pallet_crowdloan::Crowdloans::::get(crowdloan_id).unwrap(); + assert!(!crowdloan.finalized); + assert_eq!(crowdloan.raised, cap); + + // Contributor funds are not frozen: the contributor can still withdraw. + assert_ok!(Crowdloan::withdraw( + RuntimeOrigin::signed(contributor), + crowdloan_id + )); + assert_eq!(Balances::free_balance(contributor), 200.into()); + }); +} + +// A re-entrant `refund` embedded as the finalize call is likewise rejected before moving funds. +#[test] +fn test_finalize_blocks_reentrant_refund() { + TestState::default() + .with_balance(U256::from(1), 200.into()) // creator + .with_balance(U256::from(2), 200.into()) // contributor + .build_and_execute(|| { + let creator: AccountOf = U256::from(1); + let contributor: AccountOf = U256::from(2); + let deposit: BalanceOf = 50.into(); + let min_contribution: BalanceOf = 10.into(); + let cap: BalanceOf = 100.into(); + let end: BlockNumberFor = 50; + let crowdloan_id: CrowdloanId = 0; + + let reentrant_call = Box::new(RuntimeCall::Crowdloan( + pallet_crowdloan::Call::::refund { crowdloan_id }, + )); + + assert_ok!(Crowdloan::create( + RuntimeOrigin::signed(creator), + deposit, + min_contribution, + cap, + end, + Some(reentrant_call), + None, + )); + run_to_block(10); + + assert_ok!(Crowdloan::contribute( + RuntimeOrigin::signed(creator), + crowdloan_id, + 30.into() + )); + assert_ok!(Crowdloan::contribute( + RuntimeOrigin::signed(contributor), + crowdloan_id, + 20.into() + )); + + let funds_account = pallet_crowdloan::Pallet::::funds_account(crowdloan_id); + run_to_block(60); + + // The re-entrant refund hits the `finalized` guard before transferring anything. + assert_err!( + Crowdloan::finalize(RuntimeOrigin::signed(creator), crowdloan_id), + pallet_crowdloan::Error::::AlreadyFinalized + ); + assert_eq!(Balances::free_balance(funds_account), cap); + }); +} + #[test] fn test_refund_succeeds() { TestState::default() From 7c45c9760404da2e4691ca0818165e0fc189d231 Mon Sep 17 00:00:00 2001 From: Loris Moulin Date: Wed, 24 Jun 2026 09:49:57 -0300 Subject: [PATCH 2/3] Trigger CI From d141ec22203bcbb1ee5984874039d1ae14a48dc0 Mon Sep 17 00:00:00 2001 From: Loris Moulin Date: Wed, 24 Jun 2026 11:22:56 -0300 Subject: [PATCH 3/3] Check CrowdloanCurrentId in finalize --- pallets/crowdloan/src/lib.rs | 6 +++ pallets/crowdloan/src/tests.rs | 72 ++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/pallets/crowdloan/src/lib.rs b/pallets/crowdloan/src/lib.rs index 0144c43046..f7d771f4a7 100644 --- a/pallets/crowdloan/src/lib.rs +++ b/pallets/crowdloan/src/lib.rs @@ -265,6 +265,8 @@ pub mod pallet { InvalidOrigin, /// The crowdloan has already been finalized. AlreadyFinalized, + /// A crowdloan finalization is already in progress. + AlreadyFinalizing, /// The crowdloan contribution period has not ended yet. ContributionPeriodNotEnded, /// The contributor has no contribution for this crowdloan. @@ -619,6 +621,10 @@ pub mod pallet { ensure!(who == crowdloan.creator, Error::::InvalidOrigin); ensure!(crowdloan.raised == crowdloan.cap, Error::::CapNotRaised); ensure!(!crowdloan.finalized, Error::::AlreadyFinalized); + ensure!( + CurrentCrowdloanId::::get().is_none(), + Error::::AlreadyFinalizing + ); crowdloan.finalized = true; Crowdloans::::insert(crowdloan_id, &crowdloan); diff --git a/pallets/crowdloan/src/tests.rs b/pallets/crowdloan/src/tests.rs index 9cffa10ef5..863ca9ae60 100644 --- a/pallets/crowdloan/src/tests.rs +++ b/pallets/crowdloan/src/tests.rs @@ -1875,6 +1875,78 @@ fn test_finalize_fails_if_call_fails() { }); } +#[test] +fn test_finalize_fails_if_another_finalize_is_in_progress() { + TestState::default() + .with_balance(U256::from(1), 300.into()) + .with_balance(U256::from(2), 300.into()) + .build_and_execute(|| { + let creator: AccountOf = U256::from(1); + let contributor: AccountOf = U256::from(2); + let deposit: BalanceOf = 50.into(); + let min_contribution: BalanceOf = 10.into(); + let cap: BalanceOf = 100.into(); + let end: BlockNumberFor = 50; + let first_crowdloan_id: CrowdloanId = 0; + let second_crowdloan_id: CrowdloanId = 1; + + let nested_finalize_call = Box::new(RuntimeCall::Crowdloan(pallet_crowdloan::Call::< + Test, + >::finalize { + crowdloan_id: second_crowdloan_id, + })); + + assert_ok!(Crowdloan::create( + RuntimeOrigin::signed(creator), + deposit, + min_contribution, + cap, + end, + Some(nested_finalize_call), + None, + )); + assert_ok!(Crowdloan::create( + RuntimeOrigin::signed(creator), + deposit, + min_contribution, + cap, + end, + Some(noop_call()), + None, + )); + + run_to_block(10); + + assert_ok!(Crowdloan::contribute( + RuntimeOrigin::signed(contributor), + first_crowdloan_id, + 50.into() + )); + assert_ok!(Crowdloan::contribute( + RuntimeOrigin::signed(contributor), + second_crowdloan_id, + 50.into() + )); + + run_to_block(60); + + assert_err!( + Crowdloan::finalize(RuntimeOrigin::signed(creator), first_crowdloan_id), + pallet_crowdloan::Error::::AlreadyFinalizing + ); + + assert_eq!(pallet_crowdloan::CurrentCrowdloanId::::get(), None); + assert!( + pallet_crowdloan::Crowdloans::::get(first_crowdloan_id) + .is_some_and(|c| !c.finalized) + ); + assert!( + pallet_crowdloan::Crowdloans::::get(second_crowdloan_id) + .is_some_and(|c| !c.finalized) + ); + }); +} + // The finalize `call` cannot re-enter `withdraw` on the same crowdloan: it is rejected and // the extrinsic reverts, so no funds move and `raised` stays consistent with the real balance. #[test]