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

@@ -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

core/src/non_circulating_supply.rs

@@ -23,14 +23,14 @@ pub fn calculate_non_circulating_supply(bank: &Arc<Bank>) -> 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);

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(

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<Pubkey>) -> bool {
+    pub fn is_in_force(&self, clock: &Clock, custodian: Option<&Pubkey>) -> bool {
-        (self.unix_timestamp > clock.unix_timestamp || self.epoch > clock.epoch)
+        if custodian == Some(&self.custodian) {
-            && !signers.contains(&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<Pubkey>,
+        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<Pubkey>,
+        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();
+            let custodian_account = Account::new_ref(1, 0, &system_program::id());
-            signers_with_custodian.insert(custodian);
+            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::<StakeState>(),
+            &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::<HashSet<_>>();
         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,
         );

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,
         );