diff --git a/pallets/crowdloan/src/lib.rs b/pallets/crowdloan/src/lib.rs index f7d771f4a7..245ff55256 100644 --- a/pallets/crowdloan/src/lib.rs +++ b/pallets/crowdloan/src/lib.rs @@ -457,11 +457,16 @@ pub mod pallet { Error::::ContributionTooLow ); - // Ensure the crowdloan has not reached the maximum number of contributors - ensure!( - crowdloan.contributors_count < T::MaxContributors::get(), - Error::::MaxContributorsReached - ); + // Ensure the crowdloan has not reached the maximum number of + // contributors. Existing contributors topping up are not counted + // against the limit, preventing a griefing attack where + // MaxContributors sockpuppets freeze the crowloan below its goal. + if Contributions::::get(crowdloan_id, &contributor).is_none() { + ensure!( + crowdloan.contributors_count < T::MaxContributors::get(), + Error::::MaxContributorsReached + ); + } // Compute how much room is left before the crowdloan reaches its cap. let left_to_raise = crowdloan @@ -909,8 +914,11 @@ pub mod pallet { // Only the creator can update the cap. ensure!(who == crowdloan.creator, Error::::InvalidOrigin); - // The new cap should be greater than the actual raised amount. - ensure!(new_cap >= crowdloan.raised, Error::::CapTooLow); + // The new cap must be at least the current cap (cap can only be + // raised, never lowered). Allowing a creator to lower the cap to + // the already-raised amount would let them finalize early and + // redirect all contributions. + ensure!(new_cap >= crowdloan.cap, Error::::CapTooLow); crowdloan.cap = new_cap; Crowdloans::::insert(crowdloan_id, &crowdloan); diff --git a/pallets/crowdloan/src/tests.rs b/pallets/crowdloan/src/tests.rs index 863ca9ae60..2488dd6714 100644 --- a/pallets/crowdloan/src/tests.rs +++ b/pallets/crowdloan/src/tests.rs @@ -1015,8 +1015,12 @@ fn test_contribute_fails_if_max_contributors_has_been_reached() { )); } - // try to contribute - let contributor: AccountOf = U256::from(10); + // Account 11 is a genuinely new contributor; once MaxContributors is + // reached (creator counts as contributor #1 plus accounts 2..=10) a + // new contributor is rejected. Reusing account 10 would no longer + // fail here, since under the fix an existing contributor may top up + // past the limit. + let contributor: AccountOf = U256::from(11); assert_err!( Crowdloan::contribute(RuntimeOrigin::signed(contributor), crowdloan_id, amount), pallet_crowdloan::Error::::MaxContributorsReached @@ -1024,6 +1028,66 @@ fn test_contribute_fails_if_max_contributors_has_been_reached() { }); } +#[test] +fn audit_probe_existing_contributor_can_top_up_at_limit() { + // After the fix, an existing contributor is not counted against + // MaxContributors, so they can still raise their stake even once the limit + // is reached. Under the old code this top-up would fail with + // MaxContributorsReached. + TestState::default() + .with_balance(U256::from(1), 100.into()) + .with_balance(U256::from(2), 100.into()) + .with_balance(U256::from(3), 100.into()) + .with_balance(U256::from(4), 100.into()) + .with_balance(U256::from(5), 100.into()) + .with_balance(U256::from(6), 100.into()) + .with_balance(U256::from(7), 100.into()) + .with_balance(U256::from(8), 100.into()) + .with_balance(U256::from(9), 100.into()) + .with_balance(U256::from(10), 100.into()) + .build_and_execute(|| { + // create a crowdloan + let creator: AccountOf = U256::from(1); + let initial_deposit: BalanceOf = 50.into(); + let min_contribution: BalanceOf = 10.into(); + let cap: BalanceOf = 1000.into(); + let end: BlockNumberFor = 50; + + assert_ok!(Crowdloan::create( + RuntimeOrigin::signed(creator), + initial_deposit, + min_contribution, + cap, + end, + Some(noop_call()), + None + )); + + // run some blocks + run_to_block(10); + + // The creator counts as contributor #1, so accounts 2..=10 fill the + // crowdloan to MaxContributors (10). + let crowdloan_id: CrowdloanId = 0; + let amount: BalanceOf = 20.into(); + for i in 2..=10 { + assert_ok!(Crowdloan::contribute( + RuntimeOrigin::signed(U256::from(i)), + crowdloan_id, + amount + )); + } + + // Account 2 already contributed; topping up must succeed even though + // the contributor limit has been reached. + assert_ok!(Crowdloan::contribute( + RuntimeOrigin::signed(U256::from(2)), + crowdloan_id, + amount + )); + }); +} + #[test] fn test_contribute_fails_if_contributor_has_insufficient_balance() { TestState::default() @@ -3200,3 +3264,42 @@ fn test_update_cap_fails_if_new_cap_is_too_low() { ); }); } + +#[test] +fn audit_probe_cap_cannot_be_lowered() { + TestState::default() + .with_balance(U256::from(1), 200.into()) + .with_balance(U256::from(2), 200.into()) + .build_and_execute(|| { + let creator: AccountOf = U256::from(1); + let contributor: AccountOf = U256::from(2); + let target: AccountOf = U256::from(42); + let crowdloan_id: CrowdloanId = 0; + + assert_ok!(Crowdloan::create( + RuntimeOrigin::signed(creator), + 50.into(), + 10.into(), + 200.into(), + 100, + None, + Some(target), + )); + assert_ok!(Crowdloan::contribute( + RuntimeOrigin::signed(contributor), + crowdloan_id, + 50.into(), + )); + + // Lowering the cap from 200 to 100 is now rejected: the cap can + // only be raised, preventing a creator from finalizing early. + assert_noop!( + Crowdloan::update_cap( + RuntimeOrigin::signed(creator), + crowdloan_id, + 100.into(), + ), + Error::::CapTooLow + ); + }); +}