From dc7041ba07810483234a22b4cccc2b4ff0f73656 Mon Sep 17 00:00:00 2001 From: Michael Vines Date: Fri, 5 Feb 2021 22:40:07 -0800 Subject: [PATCH] Require lockup authority to change withdraw authority on locked stake (#14861) --- programs/stake/src/stake_instruction.rs | 72 +++++- programs/stake/src/stake_state.rs | 313 ++++++++++++++++++++++-- sdk/src/feature_set.rs | 5 + 3 files changed, 363 insertions(+), 27 deletions(-) diff --git a/programs/stake/src/stake_instruction.rs b/programs/stake/src/stake_instruction.rs index c04a979eae..5179fa5ba3 100644 --- a/programs/stake/src/stake_instruction.rs +++ b/programs/stake/src/stake_instruction.rs @@ -8,6 +8,7 @@ use serde_derive::{Deserialize, Serialize}; use solana_sdk::{ clock::{Epoch, UnixTimestamp}, decode_error::DecodeError, + feature_set, instruction::{AccountMeta, Instruction, InstructionError}, keyed_account::{from_keyed_account, get_signers, next_keyed_account, KeyedAccount}, process_instruction::InvokeContext, @@ -41,6 +42,12 @@ pub enum StakeError { #[error("stake account merge failed due to different authority, lockups or state")] MergeMismatch, + + #[error("custodian address not present")] + CustodianMissing, + + #[error("custodian signature not present")] + CustodianSignatureMissing, } impl DecodeError for StakeError { @@ -484,17 +491,66 @@ pub fn process_instruction( &from_keyed_account::(next_keyed_account(keyed_accounts)?)?, ), StakeInstruction::Authorize(authorized_pubkey, stake_authorize) => { - me.authorize(&signers, &authorized_pubkey, stake_authorize) + let require_custodian_for_locked_stake_authorize = invoke_context.is_feature_active( + &feature_set::require_custodian_for_locked_stake_authorize::id(), + ); + + if require_custodian_for_locked_stake_authorize { + let clock = from_keyed_account::(next_keyed_account(keyed_accounts)?)?; + let _current_authority = next_keyed_account(keyed_accounts)?; + let custodian = keyed_accounts.next().map(|ka| ka.unsigned_key()); + + me.authorize( + &signers, + &authorized_pubkey, + stake_authorize, + require_custodian_for_locked_stake_authorize, + &clock, + custodian, + ) + } else { + me.authorize( + &signers, + &authorized_pubkey, + stake_authorize, + require_custodian_for_locked_stake_authorize, + &Clock::default(), + None, + ) + } } StakeInstruction::AuthorizeWithSeed(args) => { let authority_base = next_keyed_account(keyed_accounts)?; - me.authorize_with_seed( - &authority_base, - &args.authority_seed, - &args.authority_owner, - &args.new_authorized_pubkey, - args.stake_authorize, - ) + let require_custodian_for_locked_stake_authorize = invoke_context.is_feature_active( + &feature_set::require_custodian_for_locked_stake_authorize::id(), + ); + + if require_custodian_for_locked_stake_authorize { + let clock = from_keyed_account::(next_keyed_account(keyed_accounts)?)?; + let custodian = keyed_accounts.next().map(|ka| ka.unsigned_key()); + + me.authorize_with_seed( + &authority_base, + &args.authority_seed, + &args.authority_owner, + &args.new_authorized_pubkey, + args.stake_authorize, + require_custodian_for_locked_stake_authorize, + &clock, + custodian, + ) + } else { + me.authorize_with_seed( + &authority_base, + &args.authority_seed, + &args.authority_owner, + &args.new_authorized_pubkey, + args.stake_authorize, + require_custodian_for_locked_stake_authorize, + &Clock::default(), + None, + ) + } } StakeInstruction::DelegateStake => { let vote = next_keyed_account(keyed_accounts)?; diff --git a/programs/stake/src/stake_state.rs b/programs/stake/src/stake_state.rs index fe8f5aa11c..44a00eb43d 100644 --- a/programs/stake/src/stake_state.rs +++ b/programs/stake/src/stake_state.rs @@ -478,6 +478,7 @@ impl Authorized { signers: &HashSet, new_authorized: &Pubkey, stake_authorize: StakeAuthorize, + lockup_custodian_args: Option<(&Lockup, &Clock, Option<&Pubkey>)>, ) -> Result<(), InstructionError> { match stake_authorize { StakeAuthorize::Staker => { @@ -488,6 +489,24 @@ impl Authorized { self.staker = *new_authorized } StakeAuthorize::Withdrawer => { + if let Some((lockup, clock, custodian)) = lockup_custodian_args { + if lockup.is_in_force(&clock, None) { + match custodian { + None => { + return Err(StakeError::CustodianMissing.into()); + } + Some(custodian) => { + if !signers.contains(custodian) { + return Err(StakeError::CustodianSignatureMissing.into()); + } + + if lockup.is_in_force(&clock, Some(custodian)) { + return Err(StakeError::LockupInForce.into()); + } + } + } + } + } self.check(signers, stake_authorize)?; self.withdrawer = *new_authorized } @@ -771,6 +790,9 @@ pub trait StakeAccount { signers: &HashSet, new_authority: &Pubkey, stake_authorize: StakeAuthorize, + require_custodian_for_locked_stake_authorize: bool, + clock: &Clock, + custodian: Option<&Pubkey>, ) -> Result<(), InstructionError>; fn authorize_with_seed( &self, @@ -779,6 +801,9 @@ pub trait StakeAccount { authority_owner: &Pubkey, new_authority: &Pubkey, stake_authorize: StakeAuthorize, + require_custodian_for_locked_stake_authorize: bool, + clock: &Clock, + custodian: Option<&Pubkey>, ) -> Result<(), InstructionError>; fn delegate( &self, @@ -854,16 +879,35 @@ impl<'a> StakeAccount for KeyedAccount<'a> { signers: &HashSet, new_authority: &Pubkey, stake_authorize: StakeAuthorize, + require_custodian_for_locked_stake_authorize: bool, + clock: &Clock, + custodian: Option<&Pubkey>, ) -> Result<(), InstructionError> { match self.state()? { StakeState::Stake(mut meta, stake) => { - meta.authorized - .authorize(signers, new_authority, stake_authorize)?; + meta.authorized.authorize( + signers, + new_authority, + stake_authorize, + if require_custodian_for_locked_stake_authorize { + Some((&meta.lockup, clock, custodian)) + } else { + None + }, + )?; self.set_state(&StakeState::Stake(meta, stake)) } StakeState::Initialized(mut meta) => { - meta.authorized - .authorize(signers, new_authority, stake_authorize)?; + meta.authorized.authorize( + signers, + new_authority, + stake_authorize, + if require_custodian_for_locked_stake_authorize { + Some((&meta.lockup, clock, custodian)) + } else { + None + }, + )?; self.set_state(&StakeState::Initialized(meta)) } _ => Err(InstructionError::InvalidAccountData), @@ -876,6 +920,9 @@ impl<'a> StakeAccount for KeyedAccount<'a> { authority_owner: &Pubkey, new_authority: &Pubkey, stake_authorize: StakeAuthorize, + require_custodian_for_locked_stake_authorize: bool, + clock: &Clock, + custodian: Option<&Pubkey>, ) -> Result<(), InstructionError> { let mut signers = HashSet::default(); if let Some(base_pubkey) = authority_base.signer_key() { @@ -885,7 +932,14 @@ impl<'a> StakeAccount for KeyedAccount<'a> { authority_owner, )?); } - self.authorize(&signers, &new_authority, stake_authorize) + self.authorize( + &signers, + &new_authority, + stake_authorize, + require_custodian_for_locked_stake_authorize, + clock, + custodian, + ) } fn delegate( &self, @@ -1637,12 +1691,134 @@ mod tests { let mut authorized = Authorized::auto(&staker); let mut signers = HashSet::new(); assert_eq!( - authorized.authorize(&signers, &staker, StakeAuthorize::Staker), + authorized.authorize(&signers, &staker, StakeAuthorize::Staker, None), Err(InstructionError::MissingRequiredSignature) ); signers.insert(staker); assert_eq!( - authorized.authorize(&signers, &staker, StakeAuthorize::Staker), + authorized.authorize(&signers, &staker, StakeAuthorize::Staker, None), + Ok(()) + ); + } + + #[test] + fn test_authorized_authorize_with_custodian() { + let staker = solana_sdk::pubkey::new_rand(); + let custodian = solana_sdk::pubkey::new_rand(); + let invalid_custodian = solana_sdk::pubkey::new_rand(); + let mut authorized = Authorized::auto(&staker); + let mut signers = HashSet::new(); + signers.insert(staker); + + let lockup = Lockup { + epoch: 1, + unix_timestamp: 1, + custodian, + }; + let clock = Clock { + epoch: 0, + unix_timestamp: 0, + ..Clock::default() + }; + + // Legacy behaviour when the `require_custodian_for_locked_stake_authorize` feature is + // inactive + assert_eq!( + authorized.authorize(&signers, &staker, StakeAuthorize::Withdrawer, None), + Ok(()) + ); + + // No lockup, no custodian + assert_eq!( + authorized.authorize( + &signers, + &staker, + StakeAuthorize::Withdrawer, + Some((&Lockup::default(), &clock, None)) + ), + Ok(()) + ); + + // No lockup, invalid custodian not a signer + assert_eq!( + authorized.authorize( + &signers, + &staker, + StakeAuthorize::Withdrawer, + Some((&Lockup::default(), &clock, Some(&invalid_custodian))) + ), + Ok(()) // <== invalid custodian doesn't matter, there's no lockup + ); + + // Lockup active, invalid custodian not a signer + assert_eq!( + authorized.authorize( + &signers, + &staker, + StakeAuthorize::Withdrawer, + Some((&lockup, &clock, Some(&invalid_custodian))) + ), + Err(StakeError::CustodianSignatureMissing.into()), + ); + + signers.insert(invalid_custodian); + + // No lockup, invalid custodian is a signer + assert_eq!( + authorized.authorize( + &signers, + &staker, + StakeAuthorize::Withdrawer, + Some((&Lockup::default(), &clock, Some(&invalid_custodian))) + ), + Ok(()) // <== invalid custodian doesn't matter, there's no lockup + ); + + // Lockup active, invalid custodian is a signer + signers.insert(invalid_custodian); + assert_eq!( + authorized.authorize( + &signers, + &staker, + StakeAuthorize::Withdrawer, + Some((&lockup, &clock, Some(&invalid_custodian))) + ), + Err(StakeError::LockupInForce.into()), // <== invalid custodian rejected + ); + + signers.remove(&invalid_custodian); + + // Lockup active, no custodian + assert_eq!( + authorized.authorize( + &signers, + &staker, + StakeAuthorize::Withdrawer, + Some((&lockup, &clock, None)) + ), + Err(StakeError::CustodianMissing.into()), + ); + + // Lockup active, custodian not a signer + assert_eq!( + authorized.authorize( + &signers, + &staker, + StakeAuthorize::Withdrawer, + Some((&lockup, &clock, Some(&custodian))) + ), + Err(StakeError::CustodianSignatureMissing.into()), + ); + + // Lockup active, custodian is a signer + signers.insert(custodian); + assert_eq!( + authorized.authorize( + &signers, + &staker, + StakeAuthorize::Withdrawer, + Some((&lockup, &clock, Some(&custodian))) + ), Ok(()) ); } @@ -3561,7 +3737,14 @@ mod tests { let stake_keyed_account = KeyedAccount::new(&new_authority, true, &stake_account); let signers = vec![new_authority].into_iter().collect(); assert_eq!( - stake_keyed_account.authorize(&signers, &new_authority, StakeAuthorize::Staker), + stake_keyed_account.authorize( + &signers, + &new_authority, + StakeAuthorize::Staker, + false, + &Clock::default(), + None + ), Err(InstructionError::InvalidAccountData) ); } @@ -3588,11 +3771,25 @@ mod tests { let stake_pubkey0 = solana_sdk::pubkey::new_rand(); let signers = vec![stake_authority].into_iter().collect(); assert_eq!( - stake_keyed_account.authorize(&signers, &stake_pubkey0, StakeAuthorize::Staker), + stake_keyed_account.authorize( + &signers, + &stake_pubkey0, + StakeAuthorize::Staker, + false, + &Clock::default(), + None + ), Ok(()) ); assert_eq!( - stake_keyed_account.authorize(&signers, &stake_pubkey0, StakeAuthorize::Withdrawer), + stake_keyed_account.authorize( + &signers, + &stake_pubkey0, + StakeAuthorize::Withdrawer, + false, + &Clock::default(), + None + ), Ok(()) ); if let StakeState::Initialized(Meta { authorized, .. }) = @@ -3607,7 +3804,14 @@ mod tests { // A second authorization signed by the stake_keyed_account should fail let stake_pubkey1 = solana_sdk::pubkey::new_rand(); assert_eq!( - stake_keyed_account.authorize(&signers, &stake_pubkey1, StakeAuthorize::Staker), + stake_keyed_account.authorize( + &signers, + &stake_pubkey1, + StakeAuthorize::Staker, + false, + &Clock::default(), + None + ), Err(InstructionError::MissingRequiredSignature) ); @@ -3616,7 +3820,14 @@ mod tests { // Test a second authorization by the newly authorized pubkey let stake_pubkey2 = solana_sdk::pubkey::new_rand(); assert_eq!( - stake_keyed_account.authorize(&signers0, &stake_pubkey2, StakeAuthorize::Staker), + stake_keyed_account.authorize( + &signers0, + &stake_pubkey2, + StakeAuthorize::Staker, + false, + &Clock::default(), + None + ), Ok(()) ); if let StakeState::Initialized(Meta { authorized, .. }) = @@ -3626,7 +3837,14 @@ mod tests { } assert_eq!( - stake_keyed_account.authorize(&signers0, &stake_pubkey2, StakeAuthorize::Withdrawer), + stake_keyed_account.authorize( + &signers0, + &stake_pubkey2, + StakeAuthorize::Withdrawer, + false, + &Clock::default(), + None + ), Ok(()) ); if let StakeState::Initialized(Meta { authorized, .. }) = @@ -3695,6 +3913,9 @@ mod tests { &id(), &new_authority, StakeAuthorize::Staker, + false, + &Clock::default(), + None, ), Err(InstructionError::MissingRequiredSignature) ); @@ -3707,6 +3928,9 @@ mod tests { &id(), &new_authority, StakeAuthorize::Staker, + false, + &Clock::default(), + None, ), Err(InstructionError::MissingRequiredSignature) ); @@ -3719,6 +3943,9 @@ mod tests { &id(), &new_authority, StakeAuthorize::Staker, + false, + &Clock::default(), + None, ), Ok(()) ); @@ -3731,6 +3958,9 @@ mod tests { &id(), &new_authority, StakeAuthorize::Withdrawer, + false, + &Clock::default(), + None, ), Ok(()) ); @@ -3743,6 +3973,9 @@ mod tests { &id(), &new_authority, StakeAuthorize::Withdrawer, + false, + &Clock::default(), + None, ), Err(InstructionError::MissingRequiredSignature) ); @@ -3766,7 +3999,14 @@ mod tests { let new_authority = solana_sdk::pubkey::new_rand(); let signers = vec![withdrawer_pubkey].into_iter().collect(); assert_eq!( - stake_keyed_account.authorize(&signers, &new_authority, StakeAuthorize::Staker), + stake_keyed_account.authorize( + &signers, + &new_authority, + StakeAuthorize::Staker, + false, + &Clock::default(), + None + ), Ok(()) ); @@ -3774,28 +4014,56 @@ mod tests { let mallory_pubkey = solana_sdk::pubkey::new_rand(); let signers = vec![new_authority].into_iter().collect(); assert_eq!( - stake_keyed_account.authorize(&signers, &mallory_pubkey, StakeAuthorize::Staker), + stake_keyed_account.authorize( + &signers, + &mallory_pubkey, + StakeAuthorize::Staker, + false, + &Clock::default(), + None + ), Ok(()) ); // Verify the original staker no longer has access. let new_stake_pubkey = solana_sdk::pubkey::new_rand(); assert_eq!( - stake_keyed_account.authorize(&signers, &new_stake_pubkey, StakeAuthorize::Staker), + stake_keyed_account.authorize( + &signers, + &new_stake_pubkey, + StakeAuthorize::Staker, + false, + &Clock::default(), + None + ), Err(InstructionError::MissingRequiredSignature) ); // Verify the withdrawer (pulled from cold storage) can save the day. let signers = vec![withdrawer_pubkey].into_iter().collect(); assert_eq!( - stake_keyed_account.authorize(&signers, &new_stake_pubkey, StakeAuthorize::Withdrawer), + stake_keyed_account.authorize( + &signers, + &new_stake_pubkey, + StakeAuthorize::Withdrawer, + false, + &Clock::default(), + None + ), Ok(()) ); // Attack! Verify the staker cannot be used to authorize a withdraw. let signers = vec![new_stake_pubkey].into_iter().collect(); assert_eq!( - stake_keyed_account.authorize(&signers, &mallory_pubkey, StakeAuthorize::Withdrawer), + stake_keyed_account.authorize( + &signers, + &mallory_pubkey, + StakeAuthorize::Withdrawer, + false, + &Clock::default(), + None + ), Ok(()) ); } @@ -5509,7 +5777,14 @@ mod tests { let new_staker_pubkey = solana_sdk::pubkey::new_rand(); assert_eq!( - stake_keyed_account.authorize(&signers, &new_staker_pubkey, StakeAuthorize::Staker), + stake_keyed_account.authorize( + &signers, + &new_staker_pubkey, + StakeAuthorize::Staker, + false, + &Clock::default(), + None + ), Ok(()) ); let authorized = diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index bed0652c94..99a8697689 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -278,6 +278,10 @@ pub mod track_writable_deescalation { solana_sdk::declare_id!("HVPSxqskEtRLRT2ZeEMmkmt9FWqoFX4vrN6f5VaadLED"); } +pub mod require_custodian_for_locked_stake_authorize { + solana_sdk::declare_id!("D4jsDcXaqdW8tDAWn8H4R25Cdns2YwLneujSL1zvjW6R"); +} + pub mod spl_token_v2_self_transfer_fix { solana_sdk::declare_id!("BL99GYhdjjcv6ys22C9wPgn2aTVERDbPHHo4NbS3hgp7"); } @@ -325,6 +329,7 @@ lazy_static! { (turbine_retransmit_peers_patch::id(), "turbine retransmit peers patch #14631"), (prevent_upgrade_and_invoke::id(), "prevent upgrade and invoke in same tx batch"), (track_writable_deescalation::id(), "track account writable deescalation"), + (require_custodian_for_locked_stake_authorize::id(), "require custodian to authorize withdrawer change for locked stake"), (spl_token_v2_self_transfer_fix::id(), "spl-token self-transfer fix"), (matching_buffer_upgrade_authorities::id(), "Upgradeable buffer and program authorities must match"), (full_inflation::candidate_example::vote::id(), "Community vote allowing candidate_example to enable full inflation"),