Nomination pools: Fix payout destination in permissionless unbond (#3110)

This fixes a bug in nomination pools that msitakenly claimed the
rewards, upon pool destruction, into the caller of `unbond` and not the
actual member.

More description and a PA coming soon. Opening this for now to expediate
backport.

---------

Co-authored-by: Gonçalo Pestana <g6pestana@gmail.com>

prdoc/pr_3110.prdoc

@@ -0,0 +1,12 @@
+title: Nomination pools Fix payout destination in permissionless unbond
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      This PR fixes an issue whereby when a nomination pool allowed for permissionless unbonding of
+      funds, the implicit claimed rewards were mistakenly sent to the caller of the `unbond`, and
+      not the actual member. A nomination pool only allows permissionless unbonding when its state
+      was set into `Destroying` by the operator
+
+crates:
+  - name: pallet-nomination-pools

substrate/frame/nomination-pools/Cargo.toml

@@ -16,8 +16,12 @@ targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
 # parity
-codec = { package = "parity-scale-codec", version = "3.6.1", default-features = false, features = ["derive"] }
-scale-info = { version = "2.10.0", default-features = false, features = ["derive"] }
+codec = { package = "parity-scale-codec", version = "3.6.1", default-features = false, features = [
+	"derive",
+] }
+scale-info = { version = "2.10.0", default-features = false, features = [
+	"derive",
+] }
 
 # FRAME
 frame-support = { path = "../support", default-features = false }

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

@@ -478,8 +478,16 @@ impl Default for ClaimPermission {
 }
 
 /// A member in a pool.
-#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound, CloneNoBound)]
-#[cfg_attr(feature = "std", derive(frame_support::PartialEqNoBound, DefaultNoBound))]
+#[derive(
+	Encode,
+	Decode,
+	MaxEncodedLen,
+	TypeInfo,
+	RuntimeDebugNoBound,
+	CloneNoBound,
+	frame_support::PartialEqNoBound,
+)]
+#[cfg_attr(feature = "std", derive(DefaultNoBound))]
 #[codec(mel_bound(T: Config))]
 #[scale_info(skip_type_params(T))]
 pub struct PoolMember<T: Config> {
@@ -2140,7 +2148,12 @@ pub mod pallet {
 				bonded_pool.points,
 				bonded_pool.commission.current(),
 			)?;
-			let _ = Self::do_reward_payout(&who, &mut member, &mut bonded_pool, &mut reward_pool)?;
+			let _ = Self::do_reward_payout(
+				&member_account,
+				&mut member,
+				&mut bonded_pool,
+				&mut reward_pool,
+			)?;
 
 			let current_era = T::Staking::current_era();
 			let unbond_era = T::Staking::bonding_duration().saturating_add(current_era);
@@ -2929,6 +2942,11 @@ impl<T: Config> Pallet<T> {
 		bonded_pool: BondedPool<T>,
 		reward_pool: RewardPool<T>,
 	) {
+		// The pool id of a member cannot change in any case, so we use it to make sure
+		// `member_account` is the right one.
+		debug_assert_eq!(PoolMembers::<T>::get(member_account).unwrap().pool_id, member.pool_id);
+		debug_assert_eq!(member.pool_id, bonded_pool.id);
+
 		bonded_pool.put();
 		RewardPools::insert(member.pool_id, reward_pool);
 		PoolMembers::<T>::insert(member_account, member);
@@ -2995,6 +3013,7 @@ impl<T: Config> Pallet<T> {
 		reward_pool: &mut RewardPool<T>,
 	) -> Result<BalanceOf<T>, DispatchError> {
 		debug_assert_eq!(member.pool_id, bonded_pool.id);
+		debug_assert_eq!(&mut PoolMembers::<T>::get(member_account).unwrap(), member);
 
 		// a member who has no skin in the game anymore cannot claim any rewards.
 		ensure!(!member.active_points().is_zero(), Error::<T>::FullyUnbonding);
@@ -3111,18 +3130,19 @@ impl<T: Config> Pallet<T> {
 
 	fn do_bond_extra(
 		signer: T::AccountId,
-		who: T::AccountId,
+		member_account: T::AccountId,
 		extra: BondExtra<BalanceOf<T>>,
 	) -> DispatchResult {
-		if signer != who {
+		if signer != member_account {
 			ensure!(
-				ClaimPermissions::<T>::get(&who).can_bond_extra(),
+				ClaimPermissions::<T>::get(&member_account).can_bond_extra(),
 				Error::<T>::DoesNotHavePermission
 			);
 			ensure!(extra == BondExtra::Rewards, Error::<T>::BondExtraRestricted);
 		}
 
-		let (mut member, mut bonded_pool, mut reward_pool) = Self::get_member_with_pools(&who)?;
+		let (mut member, mut bonded_pool, mut reward_pool) =
+			Self::get_member_with_pools(&member_account)?;
 
 		// payout related stuff: we must claim the payouts, and updated recorded payout data
 		// before updating the bonded pool points, similar to that of `join` transaction.
@@ -3131,14 +3151,18 @@ impl<T: Config> Pallet<T> {
 			bonded_pool.points,
 			bonded_pool.commission.current(),
 		)?;
-		let claimed =
-			Self::do_reward_payout(&who, &mut member, &mut bonded_pool, &mut reward_pool)?;
+		let claimed = Self::do_reward_payout(
+			&member_account,
+			&mut member,
+			&mut bonded_pool,
+			&mut reward_pool,
+		)?;
 
 		let (points_issued, bonded) = match extra {
 			BondExtra::FreeBalance(amount) =>
-				(bonded_pool.try_bond_funds(&who, amount, BondType::Later)?, amount),
+				(bonded_pool.try_bond_funds(&member_account, amount, BondType::Later)?, amount),
 			BondExtra::Rewards =>
-				(bonded_pool.try_bond_funds(&who, claimed, BondType::Later)?, claimed),
+				(bonded_pool.try_bond_funds(&member_account, claimed, BondType::Later)?, claimed),
 		};
 
 		bonded_pool.ok_to_be_open()?;
@@ -3146,12 +3170,12 @@ impl<T: Config> Pallet<T> {
 			member.points.checked_add(&points_issued).ok_or(Error::<T>::OverflowRisk)?;
 
 		Self::deposit_event(Event::<T>::Bonded {
-			member: who.clone(),
+			member: member_account.clone(),
 			pool_id: member.pool_id,
 			bonded,
 			joined: false,
 		});
-		Self::put_member_with_pools(&who, member, bonded_pool, reward_pool);
+		Self::put_member_with_pools(&member_account, member, bonded_pool, reward_pool);
 
 		Ok(())
 	}
@@ -3200,18 +3224,27 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
-	fn do_claim_payout(signer: T::AccountId, who: T::AccountId) -> DispatchResult {
-		if signer != who {
+	pub(crate) fn do_claim_payout(
+		signer: T::AccountId,
+		member_account: T::AccountId,
+	) -> DispatchResult {
+		if signer != member_account {
 			ensure!(
-				ClaimPermissions::<T>::get(&who).can_claim_payout(),
+				ClaimPermissions::<T>::get(&member_account).can_claim_payout(),
 				Error::<T>::DoesNotHavePermission
 			);
 		}
-		let (mut member, mut bonded_pool, mut reward_pool) = Self::get_member_with_pools(&who)?;
+		let (mut member, mut bonded_pool, mut reward_pool) =
+			Self::get_member_with_pools(&member_account)?;
 
-		let _ = Self::do_reward_payout(&who, &mut member, &mut bonded_pool, &mut reward_pool)?;
+		let _ = Self::do_reward_payout(
+			&member_account,
+			&mut member,
+			&mut bonded_pool,
+			&mut reward_pool,
+		)?;
 
-		Self::put_member_with_pools(&who, member, bonded_pool, reward_pool);
+		Self::put_member_with_pools(&member_account, member, bonded_pool, reward_pool);
 		Ok(())
 	}
 

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

@@ -1182,6 +1182,12 @@ mod claim_payout {
 			assert_eq!(payout, 0);
 			assert_eq!(member, del(0.0));
 			assert_eq!(reward_pool, rew(0, 0, 0));
+			Pools::put_member_with_pools(
+				&10,
+				member.clone(),
+				bonded_pool.clone(),
+				reward_pool.clone(),
+			);
 
 			// Given the pool has earned some rewards for the first time
 			deposit_rewards(5);
@@ -1203,6 +1209,12 @@ mod claim_payout {
 			assert_eq!(payout, 5);
 			assert_eq!(reward_pool, rew(0, 0, 5));
 			assert_eq!(member, del(0.5));
+			Pools::put_member_with_pools(
+				&10,
+				member.clone(),
+				bonded_pool.clone(),
+				reward_pool.clone(),
+			);
 
 			// Given the pool has earned rewards again
 			deposit_rewards(10);
@@ -1220,6 +1232,12 @@ mod claim_payout {
 			assert_eq!(payout, 10);
 			assert_eq!(reward_pool, rew(0, 0, 15));
 			assert_eq!(member, del(1.5));
+			Pools::put_member_with_pools(
+				&10,
+				member.clone(),
+				bonded_pool.clone(),
+				reward_pool.clone(),
+			);
 
 			// Given the pool has earned no new rewards
 			Currency::set_balance(&default_reward_account(), ed);
@@ -1234,6 +1252,12 @@ mod claim_payout {
 			assert_eq!(payout, 0);
 			assert_eq!(reward_pool, rew(0, 0, 15));
 			assert_eq!(member, del(1.5));
+			Pools::put_member_with_pools(
+				&10,
+				member.clone(),
+				bonded_pool.clone(),
+				reward_pool.clone(),
+			);
 		});
 	}
 
@@ -1279,6 +1303,12 @@ mod claim_payout {
 				assert_eq!(payout, 10);
 				assert_eq!(del_10, del(10, 1));
 				assert_eq!(reward_pool, rew(0, 0, 10));
+				Pools::put_member_with_pools(
+					&10,
+					del_10.clone(),
+					bonded_pool.clone(),
+					reward_pool.clone(),
+				);
 
 				// When
 				let payout =
@@ -1293,6 +1323,12 @@ mod claim_payout {
 				assert_eq!(payout, 40);
 				assert_eq!(del_40, del(40, 1));
 				assert_eq!(reward_pool, rew(0, 0, 50));
+				Pools::put_member_with_pools(
+					&40,
+					del_40.clone(),
+					bonded_pool.clone(),
+					reward_pool.clone(),
+				);
 
 				// When
 				let payout =
@@ -1307,6 +1343,12 @@ mod claim_payout {
 				assert_eq!(payout, 50);
 				assert_eq!(del_50, del(50, 1));
 				assert_eq!(reward_pool, rew(0, 0, 100));
+				Pools::put_member_with_pools(
+					&50,
+					del_50.clone(),
+					bonded_pool.clone(),
+					reward_pool.clone(),
+				);
 
 				// Given the reward pool has some new rewards
 				deposit_rewards(50);
@@ -1324,6 +1366,12 @@ mod claim_payout {
 				assert_eq!(payout, 5);
 				assert_eq!(del_10, del_float(10, 1.5));
 				assert_eq!(reward_pool, rew(0, 0, 105));
+				Pools::put_member_with_pools(
+					&10,
+					del_10.clone(),
+					bonded_pool.clone(),
+					reward_pool.clone(),
+				);
 
 				// When
 				let payout =
@@ -1338,6 +1386,12 @@ mod claim_payout {
 				assert_eq!(payout, 20);
 				assert_eq!(del_40, del_float(40, 1.5));
 				assert_eq!(reward_pool, rew(0, 0, 125));
+				Pools::put_member_with_pools(
+					&40,
+					del_40.clone(),
+					bonded_pool.clone(),
+					reward_pool.clone(),
+				);
 
 				// Given del_50 hasn't claimed and the reward pools has just earned 50
 				deposit_rewards(50);
@@ -1355,6 +1409,12 @@ mod claim_payout {
 				assert_eq!(payout, 50);
 				assert_eq!(del_50, del_float(50, 2.0));
 				assert_eq!(reward_pool, rew(0, 0, 175));
+				Pools::put_member_with_pools(
+					&50,
+					del_50.clone(),
+					bonded_pool.clone(),
+					reward_pool.clone(),
+				);
 
 				// When
 				let payout =
@@ -1369,6 +1429,12 @@ mod claim_payout {
 				assert_eq!(payout, 5);
 				assert_eq!(del_10, del_float(10, 2.0));
 				assert_eq!(reward_pool, rew(0, 0, 180));
+				Pools::put_member_with_pools(
+					&10,
+					del_10.clone(),
+					bonded_pool.clone(),
+					reward_pool.clone(),
+				);
 
 				// Given del_40 hasn't claimed and the reward pool has just earned 400
 				deposit_rewards(400);
@@ -1386,6 +1452,12 @@ mod claim_payout {
 				assert_eq!(payout, 40);
 				assert_eq!(del_10, del_float(10, 6.0));
 				assert_eq!(reward_pool, rew(0, 0, 220));
+				Pools::put_member_with_pools(
+					&10,
+					del_10.clone(),
+					bonded_pool.clone(),
+					reward_pool.clone(),
+				);
 
 				// Given del_40 + del_50 haven't claimed and the reward pool has earned 20
 				deposit_rewards(20);
@@ -1399,6 +1471,12 @@ mod claim_payout {
 				assert_eq!(payout, 2);
 				assert_eq!(del_10, del_float(10, 6.2));
 				assert_eq!(reward_pool, rew(0, 0, 222));
+				Pools::put_member_with_pools(
+					&10,
+					del_10.clone(),
+					bonded_pool.clone(),
+					reward_pool.clone(),
+				);
 
 				// When
 				let payout =
@@ -1409,6 +1487,12 @@ mod claim_payout {
 				assert_eq!(payout, 188); // 20 (from the 50) + 160 (from the 400) + 8 (from the 20)
 				assert_eq!(del_40, del_float(40, 6.2));
 				assert_eq!(reward_pool, rew(0, 0, 410));
+				Pools::put_member_with_pools(
+					&40,
+					del_40.clone(),
+					bonded_pool.clone(),
+					reward_pool.clone(),
+				);
 
 				// When
 				let payout =
@@ -1419,6 +1503,12 @@ mod claim_payout {
 				assert_eq!(payout, 210); // 200 (from the 400) + 10 (from the 20)
 				assert_eq!(del_50, del_float(50, 6.2));
 				assert_eq!(reward_pool, rew(0, 0, 620));
+				Pools::put_member_with_pools(
+					&50,
+					del_50.clone(),
+					bonded_pool.clone(),
+					reward_pool.clone(),
+				);
 			});
 	}
 
@@ -2490,6 +2580,38 @@ mod unbond {
 			})
 	}
 
+	#[test]
+	fn member_unbond_destroying_with_pending_rewards() {
+		ExtBuilder::default()
+			.min_join_bond(10)
+			.add_members(vec![(20, 20)])
+			.build_and_execute(|| {
+				unsafe_set_state(1, PoolState::Destroying);
+				let random = 123;
+
+				// given the pool some pending rewards.
+				assert_eq!(pending_rewards_for_delegator(20), 0);
+				deposit_rewards(10);
+				assert_eq!(pending_rewards_for_delegator(20), 6);
+
+				// any random user can unbond 20 now.
+				assert_ok!(Pools::unbond(RuntimeOrigin::signed(random), 20, 20));
+				assert_eq!(PoolMembers::<Runtime>::get(20).unwrap().active_points(), 0);
+				assert_eq!(PoolMembers::<Runtime>::get(20).unwrap().unbonding_points(), 20);
+
+				assert_eq!(
+					pool_events_since_last_call(),
+					vec![
+						Event::Created { depositor: 10, pool_id: 1 },
+						Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true },
+						Event::Bonded { member: 20, pool_id: 1, bonded: 20, joined: true },
+						Event::PaidOut { member: 20, pool_id: 1, payout: 6 },
+						Event::Unbonded { member: 20, pool_id: 1, balance: 20, points: 20, era: 3 }
+					]
+				);
+			})
+	}
+
 	#[test]
 	fn depositor_unbond_open() {
 		// depositor in pool, pool state open