From 48bd036db24151fc1b7d8c76c309de3853a62a94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=B3nal=20Murray?= 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 0000000000000..d5235cd627518 --- /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 74cda9c4f4cb4..a87618147fea2 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 b9b5e309ca91e..d059965a392ac 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 3e1e36f7d4489..0aacd7ef3696c 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);