[pallet-broker] Fix claim revenue behaviour for zero timeslices (paritytech#3997)

This PR adds a check that `max_timeslices > 0` and errors if not. It
also adds a test for this behaviour and cleans up some misleading docs.

Author: seadanda

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

substrate/frame/broker/src/dispatchable_impls.rs

@@ -329,6 +329,7 @@ impl<T: Config> Pallet<T> {
 		mut region: RegionId,
 		max_timeslices: Timeslice,
 	) -> DispatchResult {
+		ensure!(max_timeslices > 0, Error::<T>::NoClaimTimeslices);
 		let mut contribution =
 			InstaPoolContribution::<T>::take(region).ok_or(Error::<T>::UnknownContribution)?;
 		let contributed_parts = region.mask.count_ones();

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(

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::<Test>::NoClaimTimeslices);
+
+		// Revenue can be claimed.
 		assert_ok!(Broker::do_claim_revenue(region, 100));
 		assert_eq!(pot(), 10);
 		assert_eq!(balance(2), 4);