fix: stake-pool require signature in SetManager (#2273)

* add check for new_manager's signature in SetManager

* cli arg edit

Author: billythedummy

stake-pool/cli/src/main.rs

@@ -1311,15 +1311,15 @@ fn command_withdraw(
 fn command_set_manager(
     config: &Config,
     stake_pool_address: &Pubkey,
-    new_manager: &Option<Pubkey>,
+    new_manager: &Option<Keypair>,
     new_fee_receiver: &Option<Pubkey>,
 ) -> CommandResult {
     let stake_pool = get_stake_pool(&config.rpc_client, stake_pool_address)?;
 
     // If new accounts are missing in the arguments use the old ones
-    let new_manager = match new_manager {
-        None => stake_pool.manager,
-        Some(value) => *value,
+    let (new_manager_pubkey, mut signers): (Pubkey, Vec<&dyn Signer>) = match new_manager {
+        None => (stake_pool.manager, vec![]),
+        Some(value) => (value.pubkey(), vec![value]),
     };
     let new_fee_receiver = match new_fee_receiver {
         None => stake_pool.manager_fee_account,
@@ -1336,15 +1336,18 @@ fn command_set_manager(
         }
     };
 
-    let mut signers = vec![config.fee_payer.as_ref(), config.manager.as_ref()];
+    signers.append(&mut vec![
+        config.fee_payer.as_ref(),
+        config.manager.as_ref(),
+    ]);
     unique_signers!(signers);
     let transaction = checked_transaction_with_signers(
         config,
         &[spl_stake_pool::instruction::set_manager(
             &spl_stake_pool::id(),
             stake_pool_address,
             &config.manager.pubkey(),
-            &new_manager,
+            &new_manager_pubkey,
             &new_fee_receiver,
         )],
         &signers,
@@ -1999,10 +2002,10 @@ fn main() {
             .arg(
                 Arg::with_name("new_manager")
                     .long("new-manager")
-                    .validator(is_pubkey)
-                    .value_name("ADDRESS")
+                    .validator(is_keypair)
+                    .value_name("KEYPAIR")
                     .takes_value(true)
-                    .help("Public key for the new stake pool manager."),
+                    .help("Keypair for the new stake pool manager."),
             )
             .arg(
                 Arg::with_name("new_fee_receiver")
@@ -2360,7 +2363,7 @@ fn main() {
         }
         ("set-manager", Some(arg_matches)) => {
             let stake_pool_address = pubkey_of(arg_matches, "pool").unwrap();
-            let new_manager: Option<Pubkey> = pubkey_of(arg_matches, "new_manager");
+            let new_manager: Option<Keypair> = keypair_of(arg_matches, "new_manager");
             let new_fee_receiver: Option<Pubkey> = pubkey_of(arg_matches, "new_fee_receiver");
             command_set_manager(
                 &config,

stake-pool/program/src/instruction.rs

@@ -1103,7 +1103,7 @@ pub fn set_manager(
     let accounts = vec![
         AccountMeta::new(*stake_pool, false),
         AccountMeta::new_readonly(*manager, true),
-        AccountMeta::new_readonly(*new_manager, false),
+        AccountMeta::new_readonly(*new_manager, true),
         AccountMeta::new_readonly(*new_fee_receiver, false),
     ];
     Instruction {

stake-pool/program/src/processor.rs

@@ -2366,6 +2366,10 @@ impl Processor {
         }
 
         stake_pool.check_manager(manager_info)?;
+        if !new_manager_info.is_signer {
+            msg!("New manager signature missing");
+            return Err(StakePoolError::SignatureMissing.into());
+        }
 
         if stake_pool.pool_mint
             != spl_token::state::Account::unpack_from_slice(&new_manager_fee_info.data.borrow())?
@@ -2412,7 +2416,7 @@ impl Processor {
         Ok(())
     }
 
-    /// Processes [SetManager](enum.Instruction.html).
+    /// Processes [SetStaker](enum.Instruction.html).
     fn process_set_staker(program_id: &Pubkey, accounts: &[AccountInfo]) -> ProgramResult {
         let account_info_iter = &mut accounts.iter();
         let stake_pool_info = next_account_info(account_info_iter)?;

stake-pool/program/tests/set_manager.rs

@@ -71,7 +71,10 @@ async fn test_set_manager() {
         )],
         Some(&payer.pubkey()),
     );
-    transaction.sign(&[&payer, &stake_pool_accounts.manager], recent_blockhash);
+    transaction.sign(
+        &[&payer, &stake_pool_accounts.manager, &new_manager],
+        recent_blockhash,
+    );
     banks_client.process_transaction(transaction).await.unwrap();
 
     let stake_pool = get_account(&mut banks_client, &stake_pool_accounts.stake_pool.pubkey()).await;
@@ -116,7 +119,7 @@ async fn test_set_manager_by_malicious() {
 }
 
 #[tokio::test]
-async fn test_set_manager_without_signature() {
+async fn test_set_manager_without_existing_signature() {
     let (mut banks_client, payer, recent_blockhash, stake_pool_accounts, new_pool_fee, new_manager) =
         setup().await;
 
@@ -126,6 +129,48 @@ async fn test_set_manager_without_signature() {
     let accounts = vec![
         AccountMeta::new(stake_pool_accounts.stake_pool.pubkey(), false),
         AccountMeta::new_readonly(stake_pool_accounts.manager.pubkey(), false),
+        AccountMeta::new_readonly(new_manager.pubkey(), true),
+        AccountMeta::new_readonly(new_pool_fee.pubkey(), false),
+    ];
+    let instruction = Instruction {
+        program_id: id(),
+        accounts,
+        data,
+    };
+
+    let mut transaction = Transaction::new_with_payer(&[instruction], Some(&payer.pubkey()));
+    transaction.sign(&[&payer, &new_manager], recent_blockhash);
+    let transaction_error = banks_client
+        .process_transaction(transaction)
+        .await
+        .err()
+        .unwrap();
+
+    match transaction_error {
+        TransportError::TransactionError(TransactionError::InstructionError(
+            _,
+            InstructionError::Custom(error_index),
+        )) => {
+            let program_error = error::StakePoolError::SignatureMissing as u32;
+            assert_eq!(error_index, program_error);
+        }
+        _ => panic!(
+            "Wrong error occurs while try to set new manager without existing manager signature"
+        ),
+    }
+}
+
+#[tokio::test]
+async fn test_set_manager_without_new_signature() {
+    let (mut banks_client, payer, recent_blockhash, stake_pool_accounts, new_pool_fee, new_manager) =
+        setup().await;
+
+    let data = instruction::StakePoolInstruction::SetManager
+        .try_to_vec()
+        .unwrap();
+    let accounts = vec![
+        AccountMeta::new(stake_pool_accounts.stake_pool.pubkey(), false),
+        AccountMeta::new_readonly(stake_pool_accounts.manager.pubkey(), true),
         AccountMeta::new_readonly(new_manager.pubkey(), false),
         AccountMeta::new_readonly(new_pool_fee.pubkey(), false),
     ];
@@ -136,7 +181,7 @@ async fn test_set_manager_without_signature() {
     };
 
     let mut transaction = Transaction::new_with_payer(&[instruction], Some(&payer.pubkey()));
-    transaction.sign(&[&payer], recent_blockhash);
+    transaction.sign(&[&payer, &stake_pool_accounts.manager], recent_blockhash);
     let transaction_error = banks_client
         .process_transaction(transaction)
         .await
@@ -151,7 +196,9 @@ async fn test_set_manager_without_signature() {
             let program_error = error::StakePoolError::SignatureMissing as u32;
             assert_eq!(error_index, program_error);
         }
-        _ => panic!("Wrong error occurs while try to set new manager without signature"),
+        _ => {
+            panic!("Wrong error occurs while try to set new manager without new manager signature")
+        }
     }
 }
 
@@ -199,7 +246,10 @@ async fn test_set_manager_with_wrong_mint_for_pool_fee_acc() {
         )],
         Some(&payer.pubkey()),
     );
-    transaction.sign(&[&payer, &stake_pool_accounts.manager], recent_blockhash);
+    transaction.sign(
+        &[&payer, &stake_pool_accounts.manager, &new_manager],
+        recent_blockhash,
+    );
     let transaction_error = banks_client
         .process_transaction(transaction)
         .await