diff --git a/prdoc/pr_3639.prdoc b/prdoc/pr_3639.prdoc new file mode 100644 index 000000000000..46e3f6f4d763 --- /dev/null +++ b/prdoc/pr_3639.prdoc @@ -0,0 +1,19 @@ +title: Prevents staking controllers from becoming stashes of different ledgers; Ensures that no ledger in bad state is mutated. + +doc: + - audience: Runtime User + description: | + This PR introduces a fix to the staking logic which prevents an existing controller from bonding as a stash of another ledger, which + lead to staking ledger inconsistencies down the line. In addition, it adds a few (temporary) gates to prevent ledgers that are already + in a bad state from mutating its state. + + In summary: + * Checks if stash is already a controller when calling `Call::bond` and fails if that's the case; + * Ensures that all fetching ledgers from storage are done through the `StakingLedger` API; + * Ensures that a `Error::BadState` is returned if the ledger bonding is in a bad state. This prevents bad ledgers from mutating (e.g. + `bond_extra`, `set_controller`, etc) its state and avoid further data inconsistencies. + * Prevents stashes which are controllers or another ledger from calling `set_controller`, since that may lead to a bad state. + * Adds further try-state runtime checks that check if there are ledgers in a bad state based on their bonded metadata. + +crates: +- name: pallet-staking diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 84bb4d167dcb..5f74d36b0407 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -32,8 +32,8 @@ //! state consistency. use frame_support::{ - defensive, - traits::{LockableCurrency, WithdrawReasons}, + defensive, ensure, + traits::{Defensive, LockableCurrency, WithdrawReasons}, }; use sp_staking::StakingAccount; use sp_std::prelude::*; @@ -106,18 +106,39 @@ impl StakingLedger { /// This getter can be called with either a controller or stash account, provided that the /// account is properly wrapped in the respective [`StakingAccount`] variant. This is meant to /// abstract the concept of controller/stash accounts from the caller. + /// + /// Returns [`Error::BadState`] when a bond is in "bad state". A bond is in a bad state when a + /// stash has a controller which is bonding a ledger associated with another stash. pub(crate) fn get(account: StakingAccount) -> Result, Error> { - let controller = match account { - StakingAccount::Stash(stash) => >::get(stash).ok_or(Error::::NotStash), - StakingAccount::Controller(controller) => Ok(controller), - }?; + let (stash, controller) = match account.clone() { + StakingAccount::Stash(stash) => + (stash.clone(), >::get(&stash).ok_or(Error::::NotStash)?), + StakingAccount::Controller(controller) => ( + Ledger::::get(&controller) + .map(|l| l.stash) + .ok_or(Error::::NotController)?, + controller, + ), + }; - >::get(&controller) + let ledger = >::get(&controller) .map(|mut ledger| { ledger.controller = Some(controller.clone()); ledger }) - .ok_or(Error::::NotController) + .ok_or(Error::::NotController)?; + + // if ledger bond is in a bad state, return error to prevent applying operations that may + // further spoil the ledger's state. A bond is in bad state when the bonded controller is + // associted with a different ledger (i.e. a ledger with a different stash). + // + // See for more details. + ensure!( + Bonded::::get(&stash) == Some(controller) && ledger.stash == stash, + Error::::BadState + ); + + Ok(ledger) } /// Returns the reward destination of a staking ledger, stored in [`Payee`]. @@ -201,6 +222,30 @@ impl StakingLedger { } } + /// Sets the ledger controller to its stash. + pub(crate) fn set_controller_to_stash(self) -> Result<(), Error> { + let controller = self.controller.as_ref() + .defensive_proof("Ledger's controller field didn't exist. The controller should have been fetched using StakingLedger.") + .ok_or(Error::::NotController)?; + + ensure!(self.stash != *controller, Error::::AlreadyPaired); + + // check if the ledger's stash is a controller of another ledger. + if let Some(bonded_ledger) = Ledger::::get(&self.stash) { + // there is a ledger bonded by the stash. In this case, the stash of the bonded ledger + // should be the same as the ledger's stash. Otherwise fail to prevent data + // inconsistencies. See for more + // details. + ensure!(bonded_ledger.stash == self.stash, Error::::BadState); + } + + >::remove(&controller); + >::insert(&self.stash, &self); + >::insert(&self.stash, &self.stash); + + Ok(()) + } + /// Clears all data related to a staking ledger and its bond in both [`Ledger`] and [`Bonded`] /// storage items and updates the stash staking lock. pub(crate) fn kill(stash: &T::AccountId) -> Result<(), Error> { diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index d2afd8f26e24..5833d59c4599 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -342,6 +342,11 @@ where pub(crate) type StakingCall = crate::Call; pub(crate) type TestCall = ::RuntimeCall; +parameter_types! { + // if true, skips the try-state for the test running. + pub static SkipTryStateCheck: bool = false; +} + pub struct ExtBuilder { nominate: bool, validator_count: u32, @@ -451,6 +456,10 @@ impl ExtBuilder { self.balance_factor = factor; self } + pub fn try_state(self, enable: bool) -> Self { + SkipTryStateCheck::set(!enable); + self + } fn build(self) -> sp_io::TestExternalities { sp_tracing::try_init_simple(); let mut storage = frame_system::GenesisConfig::::default().build_storage().unwrap(); @@ -579,7 +588,9 @@ impl ExtBuilder { let mut ext = self.build(); ext.execute_with(test); ext.execute_with(|| { - Staking::do_try_state(System::block_number()).unwrap(); + if !SkipTryStateCheck::get() { + Staking::do_try_state(System::block_number()).unwrap(); + } }); } } @@ -800,6 +811,57 @@ pub(crate) fn bond_controller_stash(controller: AccountId, stash: AccountId) -> Ok(()) } +pub(crate) fn setup_double_bonded_ledgers() { + assert_ok!(Staking::bond(RuntimeOrigin::signed(1), 10, RewardDestination::Staked)); + assert_ok!(Staking::bond(RuntimeOrigin::signed(2), 20, RewardDestination::Staked)); + assert_ok!(Staking::bond(RuntimeOrigin::signed(3), 20, RewardDestination::Staked)); + // not relevant to the test case, but ensures try-runtime checks pass. + [1, 2, 3] + .iter() + .for_each(|s| Payee::::insert(s, RewardDestination::Staked)); + + // we want to test the case where a controller can also be a stash of another ledger. + // for that, we change the controller/stash bonding so that: + // * 2 becomes controller of 1. + // * 3 becomes controller of 2. + // * 4 becomes controller of 3. + let ledger_1 = Ledger::::get(1).unwrap(); + let ledger_2 = Ledger::::get(2).unwrap(); + let ledger_3 = Ledger::::get(3).unwrap(); + + // 4 becomes controller of 3. + Bonded::::mutate(3, |controller| *controller = Some(4)); + Ledger::::insert(4, ledger_3); + + // 3 becomes controller of 2. + Bonded::::mutate(2, |controller| *controller = Some(3)); + Ledger::::insert(3, ledger_2); + + // 2 becomes controller of 1 + Bonded::::mutate(1, |controller| *controller = Some(2)); + Ledger::::insert(2, ledger_1); + // 1 is not controller anymore. + Ledger::::remove(1); + + // checks. now we have: + // * 3 ledgers + assert_eq!(Ledger::::iter().count(), 3); + // * stash 1 has controller 2. + assert_eq!(Bonded::::get(1), Some(2)); + assert_eq!(StakingLedger::::paired_account(StakingAccount::Stash(1)), Some(2)); + assert_eq!(Ledger::::get(2).unwrap().stash, 1); + + // * stash 2 has controller 3. + assert_eq!(Bonded::::get(2), Some(3)); + assert_eq!(StakingLedger::::paired_account(StakingAccount::Stash(2)), Some(3)); + assert_eq!(Ledger::::get(3).unwrap().stash, 2); + + // * stash 3 has controller 4. + assert_eq!(Bonded::::get(3), Some(4)); + assert_eq!(StakingLedger::::paired_account(StakingAccount::Stash(3)), Some(4)); + assert_eq!(Ledger::::get(4).unwrap().stash, 3); +} + #[macro_export] macro_rules! assert_session_era { ($session:expr, $era:expr) => { diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 40f30735258f..f8d2ed8767d2 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -162,7 +162,8 @@ impl Pallet { let controller = Self::bonded(&validator_stash).ok_or_else(|| { Error::::NotStash.with_weight(T::WeightInfo::payout_stakers_alive_staked(0)) })?; - let ledger = >::get(&controller).ok_or(Error::::NotController)?; + + let ledger = Self::ledger(StakingAccount::Controller(controller))?; let page = EraInfo::::get_next_claimable_page(era, &validator_stash, &ledger) .ok_or_else(|| { Error::::AlreadyClaimed @@ -1709,7 +1710,7 @@ impl StakingInterface for Pallet { ) -> Result { let ctrl = Self::bonded(&who).ok_or(Error::::NotStash)?; Self::withdraw_unbonded(RawOrigin::Signed(ctrl.clone()).into(), num_slashing_spans) - .map(|_| !Ledger::::contains_key(&ctrl)) + .map(|_| !StakingLedger::::is_bonded(StakingAccount::Controller(ctrl))) .map_err(|with_post| with_post.error) } @@ -1817,6 +1818,8 @@ impl Pallet { "VoterList contains non-staker" ); + Self::check_bonded_consistency()?; + Self::check_payees()?; Self::check_nominators()?; Self::check_exposures()?; Self::check_paged_exposures()?; @@ -1824,6 +1827,82 @@ impl Pallet { Self::check_count() } + /// Invariants: + /// * A controller should not be associated with more than one ledger. + /// * A bonded (stash, controller) pair should have only one associated ledger. I.e. if the + /// ledger is bonded by stash, the controller account must not bond a different ledger. + /// * A bonded (stash, controller) pair must have an associated ledger. + /// NOTE: these checks result in warnings only. Once + /// is resolved, turn warns into check + /// failures. + fn check_bonded_consistency() -> Result<(), TryRuntimeError> { + use sp_std::collections::btree_set::BTreeSet; + + let mut count_controller_double = 0; + let mut count_double = 0; + let mut count_none = 0; + // sanity check to ensure that each controller in Bonded storage is associated with only one + // ledger. + let mut controllers = BTreeSet::new(); + + for (stash, controller) in >::iter() { + if !controllers.insert(controller.clone()) { + count_controller_double += 1; + } + + match (>::get(&stash), >::get(&controller)) { + (Some(_), Some(_)) => + // if stash == controller, it means that the ledger has migrated to + // post-controller. If no migration happened, we expect that the (stash, + // controller) pair has only one associated ledger. + if stash != controller { + count_double += 1; + }, + (None, None) => { + count_none += 1; + }, + _ => {}, + }; + } + + if count_controller_double != 0 { + log!( + warn, + "a controller is associated with more than one ledger ({} occurrences)", + count_controller_double + ); + }; + + if count_double != 0 { + log!(warn, "single tuple of (stash, controller) pair bonds more than one ledger ({} occurrences)", count_double); + } + + if count_none != 0 { + log!(warn, "inconsistent bonded state: (stash, controller) pair missing associated ledger ({} occurrences)", count_none); + } + + Ok(()) + } + + /// Invariants: + /// * A bonded ledger should always have an assigned `Payee`. + /// * The number of entries in `Payee` and of bonded staking ledgers *must* match. + /// * The stash account in the ledger must match that of the bonded acount. + fn check_payees() -> Result<(), TryRuntimeError> { + ensure!( + (Ledger::::iter().count() == Payee::::iter().count()) && + (Ledger::::iter().count() == Bonded::::iter().count()), + "number of entries in payee storage items does not match the number of bonded ledgers", + ); + + Ok(()) + } + + /// Invariants: + /// * Number of voters in `VoterList` match that of the number of Nominators and Validators in + /// the system (validator is both voter and target). + /// * Number of targets in `TargetList` matches the number of validators in the system. + /// * Current validator count is bounded by the election provider's max winners. fn check_count() -> Result<(), TryRuntimeError> { ensure!( ::VoterList::count() == @@ -1842,6 +1921,11 @@ impl Pallet { Ok(()) } + /// Invariants: + /// * `ledger.controller` is not stored in the storage (but populated at retrieval). + /// * Stake consistency: ledger.total == ledger.active + sum(ledger.unlocking). + /// * The controller keyeing the ledger and the ledger stash matches the state of the `Bonded` + /// storage. fn check_ledgers() -> Result<(), TryRuntimeError> { Bonded::::iter() .map(|(_, ctrl)| Self::ensure_ledger_consistent(ctrl)) @@ -1849,8 +1933,10 @@ impl Pallet { Ok(()) } + /// Invariants: + /// * For each era exposed validator, check if the exposure total is sane (exposure.total = + /// exposure.own + exposure.own). fn check_exposures() -> Result<(), TryRuntimeError> { - // a check per validator to ensure the exposure struct is always sane. let era = Self::active_era().unwrap().index; ErasStakers::::iter_prefix_values(era) .map(|expo| { @@ -1868,6 +1954,10 @@ impl Pallet { .collect::>() } + /// Invariants: + /// * For each paged era exposed validator, check if the exposure total is sane (exposure.total + /// = exposure.own + exposure.own). + /// * Paged exposures metadata (`ErasStakersOverview`) matches the paged exposures state. fn check_paged_exposures() -> Result<(), TryRuntimeError> { use sp_staking::PagedExposureMetadata; use sp_std::collections::btree_map::BTreeMap; @@ -1932,6 +2022,8 @@ impl Pallet { .collect::>() } + /// Invariants: + /// * Checks that each nominator has its entire stake correctly distributed. fn check_nominators() -> Result<(), TryRuntimeError> { // a check per nominator to ensure their entire stake is correctly distributed. Will only // kick-in if the nomination was submitted before the current era. diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 18ad3e4a6cf1..4a04ea5741e9 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -782,6 +782,8 @@ pub mod pallet { SnapshotTargetsSizeExceeded { size: u32 }, /// A new force era mode was set. ForceEra { mode: Forcing }, + /// Report of a controller batch deprecation. + ControllerBatchDeprecated { failures: u32 }, } #[pallet::error] @@ -924,6 +926,11 @@ pub mod pallet { return Err(Error::::AlreadyBonded.into()) } + // An existing controller cannot become a stash. + if StakingLedger::::is_bonded(StakingAccount::Controller(stash.clone())) { + return Err(Error::::AlreadyPaired.into()) + } + // Reject a bond which is considered to be _dust_. if value < T::Currency::minimum_balance() { return Err(Error::::InsufficientBond.into()) @@ -964,7 +971,6 @@ pub mod pallet { #[pallet::compact] max_additional: BalanceOf, ) -> DispatchResult { let stash = ensure_signed(origin)?; - let mut ledger = Self::ledger(StakingAccount::Stash(stash.clone()))?; let stash_balance = T::Currency::free_balance(&stash); @@ -1310,8 +1316,6 @@ pub mod pallet { pub fn set_controller(origin: OriginFor) -> DispatchResult { let stash = ensure_signed(origin)?; - // the bonded map and ledger are mutated directly as this extrinsic is related to a - // (temporary) passive migration. Self::ledger(StakingAccount::Stash(stash.clone())).map(|ledger| { let controller = ledger.controller() .defensive_proof("ledger was fetched used the StakingInterface, so controller field must exist; qed.") @@ -1321,10 +1325,7 @@ pub mod pallet { // stash is already its own controller. return Err(Error::::AlreadyPaired.into()) } - // update bond and ledger. - >::remove(controller); - >::insert(&stash, &stash); - >::insert(&stash, ledger); + let _ = ledger.set_controller_to_stash()?; Ok(()) })? } diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index bac2530b19bb..082bbc128182 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -1241,6 +1241,23 @@ fn bond_extra_works() { }); } +#[test] +fn bond_extra_controller_bad_state_works() { + ExtBuilder::default().try_state(false).build_and_execute(|| { + assert_eq!(StakingLedger::::get(StakingAccount::Stash(31)).unwrap().stash, 31); + + // simulate ledger in bad state: the controller 41 is associated to the stash 31 and 41. + Bonded::::insert(31, 41); + + // we confirm that the ledger is in bad state: 31 has 41 as controller and when fetching + // the ledger associated with the controler 41, its stash is 41 (and not 31). + assert_eq!(Ledger::::get(41).unwrap().stash, 41); + + // if the ledger is in this bad state, the `bond_extra` should fail. + assert_noop!(Staking::bond_extra(RuntimeOrigin::signed(31), 10), Error::::BadState); + }) +} + #[test] fn bond_extra_and_withdraw_unbonded_works() { // @@ -1740,6 +1757,7 @@ fn rebond_emits_right_value_in_event() { #[test] fn reward_to_stake_works() { ExtBuilder::default() + .try_state(false) .nominate(false) .set_status(31, StakerStatus::Idle) .set_status(41, StakerStatus::Idle) @@ -6783,7 +6801,7 @@ mod ledger { #[test] fn paired_account_works() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().try_state(false).build_and_execute(|| { assert_ok!(Staking::bond( RuntimeOrigin::signed(10), 100, @@ -6818,7 +6836,7 @@ mod ledger { #[test] fn get_ledger_works() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().try_state(false).build_and_execute(|| { // stash does not exist assert!(StakingLedger::::get(StakingAccount::Stash(42)).is_err()); @@ -6856,6 +6874,49 @@ mod ledger { }) } + #[test] + fn get_ledger_bad_state_fails() { + ExtBuilder::default().has_stakers(false).try_state(false).build_and_execute(|| { + setup_double_bonded_ledgers(); + + // Case 1: double bonded but not corrupted: + // stash 2 has controller 3: + assert_eq!(Bonded::::get(2), Some(3)); + assert_eq!(Ledger::::get(3).unwrap().stash, 2); + + // stash 2 is also a controller of 1: + assert_eq!(Bonded::::get(1), Some(2)); + assert_eq!(StakingLedger::::paired_account(StakingAccount::Stash(1)), Some(2)); + assert_eq!(Ledger::::get(2).unwrap().stash, 1); + + // although 2 is double bonded (it is a controller and a stash of different ledgers), + // we can safely retrieve the ledger and mutate it since the correct ledger is + // returned. + let ledger_result = StakingLedger::::get(StakingAccount::Stash(2)); + assert_eq!(ledger_result.unwrap().stash, 2); // correct ledger. + + let ledger_result = StakingLedger::::get(StakingAccount::Controller(2)); + assert_eq!(ledger_result.unwrap().stash, 1); // correct ledger. + + // fetching ledger 1 by its stash works. + let ledger_result = StakingLedger::::get(StakingAccount::Stash(1)); + assert_eq!(ledger_result.unwrap().stash, 1); + + // Case 2: corrupted ledger bonding. + // in this case, we simulate what happens when fetching a ledger by stash returns a + // ledger with a different stash. when this happens, we return an error instead of the + // ledger to prevent ledger mutations. + let mut ledger = Ledger::::get(2).unwrap(); + assert_eq!(ledger.stash, 1); + ledger.stash = 2; + Ledger::::insert(2, ledger); + + // now, we are prevented from fetching the ledger by stash from 1. It's associated + // controller (2) is now bonding a ledger with a different stash (2, not 1). + assert!(StakingLedger::::get(StakingAccount::Stash(1)).is_err()); + }) + } + #[test] fn bond_works() { ExtBuilder::default().build_and_execute(|| { @@ -6879,6 +6940,28 @@ mod ledger { }) } + #[test] + fn bond_controller_cannot_be_stash_works() { + ExtBuilder::default().build_and_execute(|| { + let (stash, controller) = testing_utils::create_unique_stash_controller::( + 0, + 10, + RewardDestination::Staked, + false, + ) + .unwrap(); + + assert_eq!(Bonded::::get(stash), Some(controller)); + assert_eq!(Ledger::::get(controller).map(|l| l.stash), Some(stash)); + + // existing controller should not be able become a stash. + assert_noop!( + Staking::bond(RuntimeOrigin::signed(controller), 10, RewardDestination::Staked), + Error::::AlreadyPaired, + ); + }) + } + #[test] fn is_bonded_works() { ExtBuilder::default().build_and_execute(|| { @@ -6896,4 +6979,35 @@ mod ledger { >::remove(42); // ensures try-state checks pass. }) } + + #[test] + fn set_controller_with_bad_state_ok() { + ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| { + setup_double_bonded_ledgers(); + + // in this case, setting controller works due to the ordering of the calls. + assert_ok!(Staking::set_controller(RuntimeOrigin::signed(1))); + assert_ok!(Staking::set_controller(RuntimeOrigin::signed(2))); + assert_ok!(Staking::set_controller(RuntimeOrigin::signed(3))); + }) + } + + #[test] + fn set_controller_with_bad_state_fails() { + ExtBuilder::default().has_stakers(false).try_state(false).build_and_execute(|| { + setup_double_bonded_ledgers(); + + // setting the controller of ledger associated with stash 3 fails since its stash is a + // controller of another ledger. + assert_noop!( + Staking::set_controller(RuntimeOrigin::signed(3)), + Error::::BadState + ); + assert_noop!( + Staking::set_controller(RuntimeOrigin::signed(2)), + Error::::BadState + ); + assert_ok!(Staking::set_controller(RuntimeOrigin::signed(1))); + }) + } }