diff --git a/prdoc/pr_3997.prdoc b/prdoc/pr_3997.prdoc new file mode 100644 index 000000000000..d5235cd62751 --- /dev/null +++ b/prdoc/pr_3997.prdoc @@ -0,0 +1,16 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: "[pallet-broker] Fix claim revenue behaviour for zero timeslices" + +doc: + - audience: Runtime Dev + description: | + Add a check in the `claim_revenue` broker call to ensure that max_timeslices > 0 and errors if + not. This mitigates an issue where an attacker could call claim_revenue for an existing region + that is owed a revenue, with max_timeslices set to 0 for free indefinitely, which would + represent an unbounded spam attack. + +crates: +- name: pallet-broker + bump: patch diff --git a/substrate/frame/broker/src/dispatchable_impls.rs b/substrate/frame/broker/src/dispatchable_impls.rs index 74cda9c4f4cb..a87618147fea 100644 --- a/substrate/frame/broker/src/dispatchable_impls.rs +++ b/substrate/frame/broker/src/dispatchable_impls.rs @@ -329,6 +329,7 @@ impl Pallet { mut region: RegionId, max_timeslices: Timeslice, ) -> DispatchResult { + ensure!(max_timeslices > 0, Error::::NoClaimTimeslices); let mut contribution = InstaPoolContribution::::take(region).ok_or(Error::::UnknownContribution)?; let contributed_parts = region.mask.count_ones(); diff --git a/substrate/frame/broker/src/lib.rs b/substrate/frame/broker/src/lib.rs index b9b5e309ca91..d059965a392a 100644 --- a/substrate/frame/broker/src/lib.rs +++ b/substrate/frame/broker/src/lib.rs @@ -474,6 +474,8 @@ pub mod pallet { AlreadyExpired, /// The configuration could not be applied because it is invalid. InvalidConfig, + /// The revenue must be claimed for 1 or more timeslices. + NoClaimTimeslices, } #[pallet::hooks] @@ -680,13 +682,12 @@ pub mod pallet { /// Claim the revenue owed from inclusion in the Instantaneous Coretime Pool. /// - /// - `origin`: Must be a Signed origin of the account which owns the Region `region_id`. + /// - `origin`: Must be a Signed origin. /// - `region_id`: The Region which was assigned to the Pool. - /// - `max_timeslices`: The maximum number of timeslices which should be processed. This may - /// effect the weight of the call but should be ideally made equivalent to the length of - /// the Region `region_id`. If it is less than this, then further dispatches will be - /// required with the `region_id` which makes up any remainders of the region to be - /// collected. + /// - `max_timeslices`: The maximum number of timeslices which should be processed. This + /// must be greater than 0. This may affect the weight of the call but should be ideally + /// made equivalent to the length of the Region `region_id`. If less, further dispatches + /// will be required with the same `region_id` to claim revenue for the remainder. #[pallet::call_index(12)] #[pallet::weight(T::WeightInfo::claim_revenue(*max_timeslices))] pub fn claim_revenue( diff --git a/substrate/frame/broker/src/tests.rs b/substrate/frame/broker/src/tests.rs index 3e1e36f7d448..0aacd7ef3696 100644 --- a/substrate/frame/broker/src/tests.rs +++ b/substrate/frame/broker/src/tests.rs @@ -372,6 +372,11 @@ fn instapool_payouts_work() { advance_to(11); assert_eq!(pot(), 14); assert_eq!(revenue(), 106); + + // Cannot claim for 0 timeslices. + assert_noop!(Broker::do_claim_revenue(region, 0), Error::::NoClaimTimeslices); + + // Revenue can be claimed. assert_ok!(Broker::do_claim_revenue(region, 100)); assert_eq!(pot(), 10); assert_eq!(balance(2), 4);