Skip to content

Commit

Permalink
Nomination pools: Fix payout destination in permissionless unbond (pa…
Browse files Browse the repository at this point in the history 
…ritytech#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>
  • Loading branch information
@kianenigma @gpestana
kianenigma and gpestana authored Jan 30, 2024
1 parent a63c01d commit b55b42e
Show file tree
Hide file tree
Showing 3 changed files with 181 additions and 22 deletions.
8 changes: 6 additions & 2 deletions substrate/frame/nomination-pools/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
73 changes: 53 additions & 20 deletions substrate/frame/nomination-pools/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand All @@ -3131,27 +3151,31 @@ 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()?;
member.points =
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(())
}
Expand Down Expand Up @@ -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 _ = 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(())
}

Expand Down
122 changes: 122 additions & 0 deletions substrate/frame/nomination-pools/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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(),
);
});
}

Expand Down Expand Up @@ -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 =
Expand All @@ -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 =
Expand All @@ -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);
Expand All @@ -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 =
Expand All @@ -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);
Expand All @@ -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 =
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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 =
Expand All @@ -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 =
Expand All @@ -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(),
);
});
}

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit b55b42e

Please sign in to comment.