Limit the authority to adjust nomination pool deposits (#10399)

Up until this point, when EDs of chains using nomination pools were
reduced, the subsequent reward account funds in exces could be claimed
by anyone, despite the fact that they had been typically provided at the
beginning by the pool owner. We therefore limit access to these funds
only to the pool owner and the (optional) root account when EDs get
reduced. The restriction does not apply to the increase in EDs, as these
imply that funds are transferred into the pool rather than out of it.

Fixes paritytech-secops/srlabs_findings#413

---------

Signed-off-by: Andrei Trandafir <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: andreitrand

prdoc/pr_10399.prdoc

@@ -0,0 +1,8 @@
+title: Limit the authority to adjust nomination pool deposits
+doc:
+- audience: Runtime Dev
+  description: |-
+    Up until this point, when EDs of chains using nomination pools were reduced, the subsequent reward account funds in exces could be claimed by anyone, despite the fact that they had been typically provided at the beginning by the pool owner. We therefore limit access to these funds only to the pool owner and the (optional) root account when EDs get reduced. The restriction does not apply to the increase in EDs, as these imply that funds are transferred into the pool rather than out of it.
+crates:
+- name: pallet-nomination-pools
+  bump: major

substrate/frame/nomination-pools/src/lib.rs

@@ -3719,8 +3719,16 @@ impl<T: Config> Pallet<T> {
 		Self::freeze_pool_deposit(reward_acc)?;
 
 		if pre_frozen_balance > min_balance {
+			// Ensure the caller is the depositor or the root.
+			ensure!(
+				who == bonded_pool.roles.depositor ||
+					bonded_pool.roles.root.as_ref().map_or(false, |root| &who == root),
+				Error::<T>::DoesNotHavePermission
+			);
+
 			// Transfer excess back to depositor.
 			let excess = pre_frozen_balance.saturating_sub(min_balance);
+
 			T::Currency::transfer(reward_acc, &who, excess, Preservation::Preserve)?;
 			Self::deposit_event(Event::<T>::MinBalanceExcessAdjusted {
 				pool_id: pool,

substrate/frame/nomination-pools/src/tests.rs

@@ -17,7 +17,7 @@
 
 use super::*;
 use crate::{mock::*, Event};
-use frame_support::{assert_err, assert_noop, assert_ok};
+use frame_support::{assert_err, assert_noop, assert_ok, hypothetically};
 use pallet_balances::Event as BEvent;
 use sp_runtime::{
 	bounded_btree_map,
@@ -420,7 +420,7 @@ mod reward_pool {
 			// clear events
 			pool_events_since_last_call();
 
-			// Then: Anyone can permissionlessly can adjust ED deposit.
+			// Then: Anyone can permissionlessly adjust ED deposit upwards.
 
 			// make sure caller has enough funds..
 			assert_ok!(Currency::mint_into(&99, 100));
@@ -445,12 +445,17 @@ mod reward_pool {
 			// When: ED is decreased and reward account has excess ED frozen
 			ExistentialDeposit::set(5);
 
+			let bonded_pool = BondedPool::<Runtime>::get(1).unwrap();
+			let owner = bonded_pool.roles.depositor;
+
+			assert_eq!(owner, 10);
+
 			// And:: adjust ED deposit is called
-			let pre_balance = Currency::free_balance(&100);
-			assert_ok!(Pools::adjust_pool_deposit(RuntimeOrigin::signed(100), 1));
+			let pre_balance = Currency::free_balance(&owner);
+			assert_ok!(Pools::adjust_pool_deposit(RuntimeOrigin::signed(owner), 1));
 
-			// Then: excess ED is claimed by the caller
-			assert_eq!(Currency::free_balance(&100), pre_balance + 45);
+			// Then: excess ED is claimed by the pool depositor
+			assert_eq!(Currency::free_balance(&owner), pre_balance + 45);
 
 			assert_eq!(
 				pool_events_since_last_call(),
@@ -459,6 +464,104 @@ mod reward_pool {
 		});
 	}
 
+	#[test]
+	fn pool_owner_can_adjust_deposit_downards() {
+		ExtBuilder::default().max_members_per_pool(Some(5)).build_and_execute(|| {
+			// Given: a nomination pool with no reward deficit
+
+			// Set an initial ED and adjust to there as a baseline.
+			let ed_baseline = 50;
+			let ed_delta = 20;
+
+			ExistentialDeposit::set(ed_baseline);
+
+			// Pool some rewards and check the imbalance.
+			deposit_rewards(50);
+			assert_eq!(reward_imbalance(1), Surplus(0));
+
+			let bonded_pool = BondedPool::<Runtime>::get(1).unwrap();
+			let owner = bonded_pool.roles.depositor;
+			let root = bonded_pool.roles.root.unwrap();
+
+			assert_eq!(owner, 10);
+			assert_eq!(root, 900);
+
+			Currency::set_balance(&owner, 100);
+			assert_ok!(Pools::adjust_pool_deposit(RuntimeOrigin::signed(owner), 1));
+
+			hypothetically!({
+				// When the ED is adjusted downards (decreased)
+				Currency::set_balance(&owner, 100);
+				ExistentialDeposit::set(ed_baseline - ed_delta);
+
+				// Then a standard account cannot adjust the pool deposit downards
+				Currency::set_balance(&70, 100);
+				assert_err!(
+					Pools::adjust_pool_deposit(RuntimeOrigin::signed(70), 1),
+					Error::<T>::DoesNotHavePermission
+				);
+
+				// And the pool owner can adjust deposit downards
+				let pre_balance = Currency::free_balance(&owner);
+				assert_ok!(Pools::adjust_pool_deposit(RuntimeOrigin::signed(owner), 1));
+				assert_eq!(reward_imbalance(1), Surplus(0));
+
+				// And the pool owner's balance increases by the ED difference.
+				assert_eq!(Currency::free_balance(&owner), pre_balance + ed_delta);
+			});
+
+			// When the ED is adjusted downards (decreased)
+			Currency::set_balance(&root, 100);
+			ExistentialDeposit::set(ed_baseline - ed_delta * 2);
+
+			// Then a standard account cannot adjust the pool deposit downards
+			Currency::set_balance(&7, 100);
+			assert_err!(
+				Pools::adjust_pool_deposit(RuntimeOrigin::signed(7), 1),
+				Error::<T>::DoesNotHavePermission
+			);
+
+			// And the root can also adjust the deposit downwards
+			let pre_balance = Currency::free_balance(&root);
+			assert_ok!(Pools::adjust_pool_deposit(RuntimeOrigin::signed(root), 1));
+			assert_eq!(reward_imbalance(1), Surplus(0));
+
+			// And the root's balance increases by the ED difference.
+			assert_eq!(Currency::free_balance(&root), pre_balance + ed_delta * 2);
+		});
+	}
+
+	#[test]
+	fn anyone_can_adjust_deposit_upwards() {
+		ExtBuilder::default().max_members_per_pool(Some(5)).build_and_execute(|| {
+			// Given: a nomination pool with no reward deficit
+
+			// Set an initial ED and adjust to there as a baseline.
+			let ed_baseline = 20;
+			let ed_delta = 40;
+			ExistentialDeposit::set(ed_baseline);
+
+			// Pool some rewards and check the imbalance.
+			deposit_rewards(50);
+			assert_eq!(reward_imbalance(1), Surplus(0));
+
+			Currency::set_balance(&10, 100);
+			assert_ok!(Pools::adjust_pool_deposit(RuntimeOrigin::signed(10), 1));
+
+			// When the ED increases
+			ExistentialDeposit::set(ed_baseline + ed_delta);
+
+			// Then anyone can adjust the pool deposit upwards if they have enough funds.
+			Currency::set_balance(&70, 100);
+			let pre_balance = Currency::free_balance(&70);
+			assert_ok!(Pools::adjust_pool_deposit(RuntimeOrigin::signed(70), 1));
+
+			// And the caller's balance decreases by the ED difference.
+			assert_eq!(Currency::free_balance(&70), pre_balance - ed_delta);
+			assert_eq!(reward_imbalance(1), Surplus(0));
+		});
+	}
+
 	#[test]
 	fn topping_up_does_not_work_for_pools_with_no_deficit() {
 		ExtBuilder::default().max_members_per_pool(Some(5)).build_and_execute(|| {