From a190e0e9253562fdca9c1b6e9541a7ea0a50c018 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Tue, 30 Jan 2024 13:11:32 +0000 Subject: [PATCH] Nomination pools: Fix payout destination in permissionless unbond (#3110) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- prdoc/pr_3110.prdoc | 12 ++ substrate/frame/nomination-pools/Cargo.toml | 8 +- substrate/frame/nomination-pools/src/lib.rs | 73 ++++++++--- substrate/frame/nomination-pools/src/tests.rs | 122 ++++++++++++++++++ 4 files changed, 193 insertions(+), 22 deletions(-) create mode 100644 prdoc/pr_3110.prdoc diff --git a/prdoc/pr_3110.prdoc b/prdoc/pr_3110.prdoc new file mode 100644 index 000000000000..06c0fafea04f --- /dev/null +++ b/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 diff --git a/substrate/frame/nomination-pools/Cargo.toml b/substrate/frame/nomination-pools/Cargo.toml index a1429d220175..cac092c98dc4 100644 --- a/substrate/frame/nomination-pools/Cargo.toml +++ b/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 } diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index c4e5ad1f700a..c2653d8bdabc 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/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 { @@ -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 Pallet { bonded_pool: BondedPool, reward_pool: RewardPool, ) { + // 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::::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::::insert(member_account, member); @@ -2995,6 +3013,7 @@ impl Pallet { reward_pool: &mut RewardPool, ) -> Result, DispatchError> { debug_assert_eq!(member.pool_id, bonded_pool.id); + debug_assert_eq!(&mut PoolMembers::::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::::FullyUnbonding); @@ -3111,18 +3130,19 @@ impl Pallet { fn do_bond_extra( signer: T::AccountId, - who: T::AccountId, + member_account: T::AccountId, extra: BondExtra>, ) -> DispatchResult { - if signer != who { + if signer != member_account { ensure!( - ClaimPermissions::::get(&who).can_bond_extra(), + ClaimPermissions::::get(&member_account).can_bond_extra(), Error::::DoesNotHavePermission ); ensure!(extra == BondExtra::Rewards, Error::::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 Pallet { 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 Pallet { member.points.checked_add(&points_issued).ok_or(Error::::OverflowRisk)?; Self::deposit_event(Event::::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 Pallet { 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::::get(&who).can_claim_payout(), + ClaimPermissions::::get(&member_account).can_claim_payout(), Error::::DoesNotHavePermission ); } - let (mut member, mut bonded_pool, mut reward_pool) = Self::get_member_with_pools(&who)?; - - let _ = Self::do_reward_payout(&who, &mut member, &mut bonded_pool, &mut reward_pool)?; + let (mut member, mut bonded_pool, mut reward_pool) = + Self::get_member_with_pools(&member_account)?; + + 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(()) } diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 657b50e8594a..47f48ba98b7f 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/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::::get(20).unwrap().active_points(), 0); + assert_eq!(PoolMembers::::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