From ba0f8de0c74b1308a53942c406137a748ef79925 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?D=C3=B3nal=20Murray?= <donal.murray@parity.io>
Date: Fri, 5 Apr 2024 14:18:48 +0100
Subject: [PATCH] [pallet-broker] Fix claim revenue behaviour for zero
timeslices (#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.
---
prdoc/pr_3997.prdoc | 16 ++++++++++++++++
substrate/frame/broker/src/dispatchable_impls.rs | 1 +
substrate/frame/broker/src/lib.rs | 13 +++++++------
substrate/frame/broker/src/tests.rs | 5 +++++
4 files changed, 29 insertions(+), 6 deletions(-)
create mode 100644 prdoc/pr_3997.prdoc
diff --git a/prdoc/pr_3997.prdoc b/prdoc/pr_3997.prdoc
new file mode 100644
index 00000000000..d5235cd6275
--- /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 74cda9c4f4c..a87618147fe 100644
--- a/substrate/frame/broker/src/dispatchable_impls.rs
+++ b/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();
diff --git a/substrate/frame/broker/src/lib.rs b/substrate/frame/broker/src/lib.rs
index b9b5e309ca9..d059965a392 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 3e1e36f7d44..0aacd7ef369 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::<Test>::NoClaimTimeslices);
+
+ // Revenue can be claimed.
assert_ok!(Broker::do_claim_revenue(region, 100));
assert_eq!(pot(), 10);
assert_eq!(balance(2), 4);
--
GitLab