From 11a55514464a12e7736ea98eafad4fc26a0eb86c Mon Sep 17 00:00:00 2001 From: Han Yang Date: Fri, 13 Aug 2021 21:16:43 -0700 Subject: [PATCH] fix: stake-pool require signature in SetManager (#2273) * add check for new_manager's signature in SetManager * cli arg edit --- stake-pool/cli/src/main.rs | 23 +++++---- stake-pool/program/src/instruction.rs | 2 +- stake-pool/program/src/processor.rs | 6 ++- stake-pool/program/tests/set_manager.rs | 62 ++++++++++++++++++++++--- 4 files changed, 75 insertions(+), 18 deletions(-) diff --git a/stake-pool/cli/src/main.rs b/stake-pool/cli/src/main.rs index 5d4c6e7c..061d5f20 100644 --- a/stake-pool/cli/src/main.rs +++ b/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, + new_manager: &Option, new_fee_receiver: &Option, ) -> 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,7 +1336,10 @@ 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, @@ -1344,7 +1347,7 @@ fn command_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_of(arg_matches, "new_manager"); + let new_manager: Option = keypair_of(arg_matches, "new_manager"); let new_fee_receiver: Option = pubkey_of(arg_matches, "new_fee_receiver"); command_set_manager( &config, diff --git a/stake-pool/program/src/instruction.rs b/stake-pool/program/src/instruction.rs index ecb89525..a7633e9f 100644 --- a/stake-pool/program/src/instruction.rs +++ b/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 { diff --git a/stake-pool/program/src/processor.rs b/stake-pool/program/src/processor.rs index 1d87c739..23fbd62d 100644 --- a/stake-pool/program/src/processor.rs +++ b/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)?; diff --git a/stake-pool/program/tests/set_manager.rs b/stake-pool/program/tests/set_manager.rs index d50d2f5f..9909486a 100644 --- a/stake-pool/program/tests/set_manager.rs +++ b/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,7 +129,7 @@ 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(), false), + AccountMeta::new_readonly(new_manager.pubkey(), true), AccountMeta::new_readonly(new_pool_fee.pubkey(), false), ]; let instruction = Instruction { @@ -136,7 +139,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, &new_manager], recent_blockhash); let transaction_error = banks_client .process_transaction(transaction) .await @@ -151,7 +154,51 @@ 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 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), + ]; + let instruction = Instruction { + program_id: id(), + accounts, + data, + }; + + let mut transaction = Transaction::new_with_payer(&[instruction], Some(&payer.pubkey())); + transaction.sign(&[&payer, &stake_pool_accounts.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 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