From 61d9d219f943a8e5935d91dcc57ffad9b6ecbb0f Mon Sep 17 00:00:00 2001 From: Greg Fitzgerald Date: Fri, 31 Jul 2020 13:37:53 -0600 Subject: [PATCH] Withdraw authority no longer implies a custodian (#11302) * Withdraw authority no longer implies a custodian Before this change, if the withdraw authority and custodian had the same public key, then a withdraw authority signature would imply a custodian signature and lockup would be not be enforced. After this change, the client's withdraw instruction must explictly reference a custodian account in its optional sixth account argument. Likewise, the fee-payer no longer implies either a withdraw authority or custodian. * Fix test The test was configuring the stake account with the fee-payer as the withdraw authority, but then passing in a different key to the withdraw instruction's withdraw authority parameter. It only worked because the second transaction was signed by the fee-payer. --- cli/src/stake.rs | 6 +- core/src/non_circulating_supply.rs | 4 +- programs/stake/src/stake_instruction.rs | 3 +- programs/stake/src/stake_state.rs | 152 ++++++++++++++++++------ stake-monitor/src/lib.rs | 2 +- 5 files changed, 124 insertions(+), 43 deletions(-) diff --git a/cli/src/stake.rs b/cli/src/stake.rs index 653d3424cb..dd66f2faba 100644 --- a/cli/src/stake.rs +++ b/cli/src/stake.rs @@ -32,7 +32,7 @@ use solana_stake_program::{ stake_state::{Authorized, Lockup, Meta, StakeAuthorize, StakeState}, }; use solana_vote_program::vote_state::VoteState; -use std::{collections::HashSet, ops::Deref, sync::Arc}; +use std::{ops::Deref, sync::Arc}; pub const STAKE_AUTHORITY_ARG: ArgConstant<'static> = ArgConstant { name: "stake_authority", @@ -1485,7 +1485,7 @@ pub fn build_stake_state( let (active_stake, activating_stake, deactivating_stake) = stake .delegation .stake_activating_and_deactivating(current_epoch, Some(stake_history)); - let lockup = if lockup.is_in_force(clock, &HashSet::new()) { + let lockup = if lockup.is_in_force(clock, None) { Some(lockup.into()) } else { None @@ -1535,7 +1535,7 @@ pub fn build_stake_state( authorized, lockup, }) => { - let lockup = if lockup.is_in_force(clock, &HashSet::new()) { + let lockup = if lockup.is_in_force(clock, None) { Some(lockup.into()) } else { None diff --git a/core/src/non_circulating_supply.rs b/core/src/non_circulating_supply.rs index 4ba3089ef9..cbc854f271 100644 --- a/core/src/non_circulating_supply.rs +++ b/core/src/non_circulating_supply.rs @@ -23,14 +23,14 @@ pub fn calculate_non_circulating_supply(bank: &Arc) -> NonCirculatingSuppl let stake_account = StakeState::from(&account).unwrap_or_default(); match stake_account { StakeState::Initialized(meta) => { - if meta.lockup.is_in_force(&clock, &HashSet::default()) + if meta.lockup.is_in_force(&clock, None) || withdraw_authority_list.contains(&meta.authorized.withdrawer) { non_circulating_accounts_set.insert(*pubkey); } } StakeState::Stake(meta, _stake) => { - if meta.lockup.is_in_force(&clock, &HashSet::default()) + if meta.lockup.is_in_force(&clock, None) || withdraw_authority_list.contains(&meta.authorized.withdrawer) { non_circulating_accounts_set.insert(*pubkey); diff --git a/programs/stake/src/stake_instruction.rs b/programs/stake/src/stake_instruction.rs index f270d27b62..98143e97e0 100644 --- a/programs/stake/src/stake_instruction.rs +++ b/programs/stake/src/stake_instruction.rs @@ -454,7 +454,8 @@ pub fn process_instruction( to, &Clock::from_keyed_account(next_keyed_account(keyed_accounts)?)?, &StakeHistory::from_keyed_account(next_keyed_account(keyed_accounts)?)?, - &signers, + next_keyed_account(keyed_accounts)?, + keyed_accounts.next(), ) } StakeInstruction::Deactivate => me.deactivate( diff --git a/programs/stake/src/stake_state.rs b/programs/stake/src/stake_state.rs index 07792a08e2..8905f5f4f2 100644 --- a/programs/stake/src/stake_state.rs +++ b/programs/stake/src/stake_state.rs @@ -111,9 +111,11 @@ pub struct Lockup { } impl Lockup { - pub fn is_in_force(&self, clock: &Clock, signers: &HashSet) -> bool { - (self.unix_timestamp > clock.unix_timestamp || self.epoch > clock.epoch) - && !signers.contains(&self.custodian) + pub fn is_in_force(&self, clock: &Clock, custodian: Option<&Pubkey>) -> bool { + if custodian == Some(&self.custodian) { + return false; + } + self.unix_timestamp > clock.unix_timestamp || self.epoch > clock.epoch } } @@ -591,7 +593,8 @@ pub trait StakeAccount { to: &KeyedAccount, clock: &Clock, stake_history: &StakeHistory, - signers: &HashSet, + withdraw_authority: &KeyedAccount, + custodian: Option<&KeyedAccount>, ) -> Result<(), InstructionError>; } @@ -819,11 +822,19 @@ impl<'a> StakeAccount for KeyedAccount<'a> { to: &KeyedAccount, clock: &Clock, stake_history: &StakeHistory, - signers: &HashSet, + withdraw_authority: &KeyedAccount, + custodian: Option<&KeyedAccount>, ) -> Result<(), InstructionError> { + let mut signers = HashSet::new(); + let withdraw_authority_pubkey = withdraw_authority + .signer_key() + .ok_or(InstructionError::MissingRequiredSignature)?; + signers.insert(*withdraw_authority_pubkey); + let (lockup, reserve, is_staked) = match self.state()? { StakeState::Stake(meta, stake) => { - meta.authorized.check(signers, StakeAuthorize::Withdrawer)?; + meta.authorized + .check(&signers, StakeAuthorize::Withdrawer)?; // if we have a deactivation epoch and we're in cooldown let staked = if clock.epoch >= stake.delegation.deactivation_epoch { stake.delegation.stake(clock.epoch, Some(stake_history)) @@ -837,7 +848,8 @@ impl<'a> StakeAccount for KeyedAccount<'a> { (meta.lockup, staked + meta.rent_exempt_reserve, staked != 0) } StakeState::Initialized(meta) => { - meta.authorized.check(signers, StakeAuthorize::Withdrawer)?; + meta.authorized + .check(&signers, StakeAuthorize::Withdrawer)?; (meta.lockup, meta.rent_exempt_reserve, false) } @@ -852,7 +864,8 @@ impl<'a> StakeAccount for KeyedAccount<'a> { // verify that lockup has expired or that the withdrawal is signed by // the custodian, both epoch and unix_timestamp must have passed - if lockup.is_in_force(&clock, signers) { + let custodian_pubkey = custodian.and_then(|keyed_account| keyed_account.signer_key()); + if lockup.is_in_force(&clock, custodian_pubkey) { return Err(StakeError::LockupInForce.into()); } @@ -1935,7 +1948,8 @@ mod tests { &to_keyed_account, &clock, &StakeHistory::default(), - &HashSet::default(), + &to_keyed_account, // unsigned account as withdraw authority + None, ), Err(InstructionError::MissingRequiredSignature) ); @@ -1943,14 +1957,14 @@ mod tests { // signed keyed account and uninitialized should work let stake_keyed_account = KeyedAccount::new(&stake_pubkey, true, &stake_account); let to_keyed_account = KeyedAccount::new(&to, false, &to_account); - let signers = vec![stake_pubkey].into_iter().collect(); assert_eq!( stake_keyed_account.withdraw( stake_lamports, &to_keyed_account, &clock, &StakeHistory::default(), - &signers, + &stake_keyed_account, + None, ), Ok(()) ); @@ -1983,7 +1997,8 @@ mod tests { &to_keyed_account, &clock, &StakeHistory::default(), - &signers, + &stake_keyed_account, + None, ), Err(InstructionError::InsufficientFunds) ); @@ -2000,6 +2015,7 @@ mod tests { vote_keyed_account .set_state(&VoteStateVersions::Current(Box::new(VoteState::default()))) .unwrap(); + let signers = vec![stake_pubkey].into_iter().collect(); assert_eq!( stake_keyed_account.delegate( &vote_keyed_account, @@ -2022,7 +2038,8 @@ mod tests { &to_keyed_account, &clock, &StakeHistory::default(), - &signers, + &stake_keyed_account, + None, ), Ok(()) ); @@ -2038,7 +2055,8 @@ mod tests { &to_keyed_account, &clock, &StakeHistory::default(), - &signers + &stake_keyed_account, + None, ), Err(InstructionError::InsufficientFunds) ); @@ -2056,7 +2074,8 @@ mod tests { &to_keyed_account, &clock, &StakeHistory::default(), - &signers + &stake_keyed_account, + None, ), Err(InstructionError::InsufficientFunds) ); @@ -2069,7 +2088,8 @@ mod tests { &to_keyed_account, &clock, &StakeHistory::default(), - &signers + &stake_keyed_account, + None, ), Ok(()) ); @@ -2140,7 +2160,8 @@ mod tests { &to_keyed_account, &clock, &stake_history, - &signers, + &stake_keyed_account, + None, ), Err(InstructionError::InsufficientFunds) ); @@ -2162,14 +2183,14 @@ mod tests { let to_account = Account::new_ref(1, 0, &system_program::id()); let to_keyed_account = KeyedAccount::new(&to, false, &to_account); let stake_keyed_account = KeyedAccount::new(&stake_pubkey, true, &stake_account); - let signers = vec![stake_pubkey].into_iter().collect(); assert_eq!( stake_keyed_account.withdraw( total_lamports, &to_keyed_account, &Clock::default(), &StakeHistory::default(), - &signers, + &stake_keyed_account, + None, ), Err(InstructionError::InvalidAccountData) ); @@ -2203,8 +2224,6 @@ mod tests { let mut clock = Clock::default(); - let signers = vec![stake_pubkey].into_iter().collect(); - // lockup is still in force, can't withdraw assert_eq!( stake_keyed_account.withdraw( @@ -2212,21 +2231,23 @@ mod tests { &to_keyed_account, &clock, &StakeHistory::default(), - &signers, + &stake_keyed_account, + None, ), Err(StakeError::LockupInForce.into()) ); { - let mut signers_with_custodian = signers.clone(); - signers_with_custodian.insert(custodian); + let custodian_account = Account::new_ref(1, 0, &system_program::id()); + let custodian_keyed_account = KeyedAccount::new(&custodian, true, &custodian_account); assert_eq!( stake_keyed_account.withdraw( total_lamports, &to_keyed_account, &clock, &StakeHistory::default(), - &signers_with_custodian, + &stake_keyed_account, + Some(&custodian_keyed_account), ), Ok(()) ); @@ -2243,12 +2264,70 @@ mod tests { &to_keyed_account, &clock, &StakeHistory::default(), - &signers, + &stake_keyed_account, + None, ), Ok(()) ); } + #[test] + fn test_withdraw_identical_authorities() { + let stake_pubkey = Pubkey::new_rand(); + let custodian = stake_pubkey; + let total_lamports = 100; + let stake_account = Account::new_ref_data_with_space( + total_lamports, + &StakeState::Initialized(Meta { + lockup: Lockup { + unix_timestamp: 0, + epoch: 1, + custodian, + }, + ..Meta::auto(&stake_pubkey) + }), + std::mem::size_of::(), + &id(), + ) + .expect("stake_account"); + + let to = Pubkey::new_rand(); + let to_account = Account::new_ref(1, 0, &system_program::id()); + let to_keyed_account = KeyedAccount::new(&to, false, &to_account); + + let stake_keyed_account = KeyedAccount::new(&stake_pubkey, true, &stake_account); + + let clock = Clock::default(); + + // lockup is still in force, even though custodian is the same as the withdraw authority + assert_eq!( + stake_keyed_account.withdraw( + total_lamports, + &to_keyed_account, + &clock, + &StakeHistory::default(), + &stake_keyed_account, + None, + ), + Err(StakeError::LockupInForce.into()) + ); + + { + let custodian_keyed_account = KeyedAccount::new(&custodian, true, &stake_account); + assert_eq!( + stake_keyed_account.withdraw( + total_lamports, + &to_keyed_account, + &clock, + &StakeHistory::default(), + &stake_keyed_account, + Some(&custodian_keyed_account), + ), + Ok(()) + ); + } + } + #[test] fn test_stake_state_redeem_rewards() { let mut vote_state = VoteState::default(); @@ -2568,8 +2647,6 @@ mod tests { assert_eq!(authorized.staker, stake_pubkey2); } - let signers2 = vec![stake_pubkey2].into_iter().collect(); - // Test that withdrawal to account fails without authorized withdrawer assert_eq!( stake_keyed_account.withdraw( @@ -2577,11 +2654,14 @@ mod tests { &to_keyed_account, &clock, &StakeHistory::default(), - &signers, // old signer + &stake_keyed_account, // old signer + None, ), Err(InstructionError::MissingRequiredSignature) ); + let stake_keyed_account2 = KeyedAccount::new(&stake_pubkey2, true, &stake_account); + // Test a successful action by the currently authorized withdrawer let to_keyed_account = KeyedAccount::new(&to, false, &to_account); assert_eq!( @@ -2590,7 +2670,8 @@ mod tests { &to_keyed_account, &clock, &StakeHistory::default(), - &signers2, + &stake_keyed_account2, + None, ), Ok(()) ); @@ -3317,7 +3398,6 @@ mod tests { #[test] fn test_lockup_is_expired() { let custodian = Pubkey::new_rand(); - let signers = [custodian].iter().cloned().collect::>(); let lockup = Lockup { epoch: 1, unix_timestamp: 1, @@ -3331,7 +3411,7 @@ mod tests { unix_timestamp: 0, ..Clock::default() }, - &HashSet::new() + None ), true ); @@ -3343,7 +3423,7 @@ mod tests { unix_timestamp: 0, ..Clock::default() }, - &HashSet::new() + None ), true ); @@ -3355,7 +3435,7 @@ mod tests { unix_timestamp: 2, ..Clock::default() }, - &HashSet::new() + None ), true ); @@ -3367,7 +3447,7 @@ mod tests { unix_timestamp: 1, ..Clock::default() }, - &HashSet::new() + None ), false ); @@ -3379,7 +3459,7 @@ mod tests { unix_timestamp: 0, ..Clock::default() }, - &signers, + Some(&custodian), ), false, ); diff --git a/stake-monitor/src/lib.rs b/stake-monitor/src/lib.rs index 3911cd3419..09eebfdff4 100644 --- a/stake-monitor/src/lib.rs +++ b/stake-monitor/src/lib.rs @@ -452,7 +452,7 @@ mod test { let instructions = stake_instruction::create_account( &payer.pubkey(), &stake3_keypair.pubkey(), - &Authorized::auto(&payer.pubkey()), + &Authorized::auto(&stake3_keypair.pubkey()), &Lockup::default(), one_sol, );