diff --git a/docs/src/token.md b/docs/src/token.md index 3f3aa2e0..1f257709 100644 --- a/docs/src/token.md +++ b/docs/src/token.md @@ -247,11 +247,6 @@ Account's owner via the `Revoke` instruction. ### Multisignatures -Warning: SPL Token v2's multisignature validation is compromised and should not -be used. Validation will erroneously pass a single valid signer signs m times. -For more information: -https://github.com/solana-labs/solana-program-library/issues/477 - M of N multisignatures are supported and can be used in place of Mint authorities or Account owners or delegates. Multisignature authorities must be initialized with the `InitializeMultisig` instruction. Initialization specifies diff --git a/token/program/src/instruction.rs b/token/program/src/instruction.rs index 2e2d9907..5c8e1b00 100644 --- a/token/program/src/instruction.rs +++ b/token/program/src/instruction.rs @@ -56,14 +56,11 @@ pub enum TokenInstruction { InitializeAccount, /// Initializes a multisignature account with N provided signers. /// - /// Warning, this instruction is compromised: - /// https://github.com/solana-labs/solana-program-library/issues/477 - /// /// Multisignature accounts can used in place of any single owner/delegate accounts in any /// token instruction that require an owner/delegate to be present. The variant field represents the /// number of signers (M) required to validate this multisignature account. /// - /// The `DangerInitializeMultisig` instruction requires no signers and MUST be included within + /// The `InitializeMultisig` instruction requires no signers and MUST be included within /// the same Transaction as the system program's `CreateInstruction` that creates the account /// being initialized. Otherwise another party can acquire ownership of the uninitialized account. /// @@ -72,7 +69,7 @@ pub enum TokenInstruction { /// 0. `[writable]` The multisignature account to initialize. /// 1. `[]` Rent sysvar /// 2. ..2+N. `[]` The signer accounts, must equal to N where 1 <= N <= 11. - DangerInitializeMultisig { + InitializeMultisig { /// The number of signers (M) required to validate this multisignature account. m: u8, }, @@ -353,7 +350,7 @@ impl TokenInstruction { 1 => Self::InitializeAccount, 2 => { let &m = rest.get(0).ok_or(InvalidInstruction)?; - Self::DangerInitializeMultisig { m } + Self::InitializeMultisig { m } } 3 | 4 | 7 | 8 => { let amount = rest @@ -449,7 +446,7 @@ impl TokenInstruction { Self::pack_pubkey_option(freeze_authority, &mut buf); } Self::InitializeAccount => buf.push(1), - &Self::DangerInitializeMultisig { m } => { + &Self::InitializeMultisig { m } => { buf.push(2); buf.push(m); } @@ -624,8 +621,8 @@ pub fn initialize_account( }) } -/// Creates a `DangerInitializeMultisig` instruction. -pub fn danger_initialize_multisig( +/// Creates a `InitializeMultisig` instruction. +pub fn initialize_multisig( token_program_id: &Pubkey, multisig_pubkey: &Pubkey, signer_pubkeys: &[&Pubkey], @@ -637,7 +634,7 @@ pub fn danger_initialize_multisig( { return Err(ProgramError::MissingRequiredSignature); } - let data = TokenInstruction::DangerInitializeMultisig { m }.pack(); + let data = TokenInstruction::InitializeMultisig { m }.pack(); let mut accounts = Vec::with_capacity(1 + 1 + signer_pubkeys.len()); accounts.push(AccountMeta::new(*multisig_pubkey, false)); @@ -1083,7 +1080,7 @@ mod test { let unpacked = TokenInstruction::unpack(&expect).unwrap(); assert_eq!(unpacked, check); - let check = TokenInstruction::DangerInitializeMultisig { m: 1 }; + let check = TokenInstruction::InitializeMultisig { m: 1 }; let packed = check.pack(); let expect = Vec::from([2u8, 1]); assert_eq!(packed, expect); diff --git a/token/program/src/processor.rs b/token/program/src/processor.rs index 76336363..a4ce7a09 100644 --- a/token/program/src/processor.rs +++ b/token/program/src/processor.rs @@ -4,7 +4,7 @@ use crate::{ error::TokenError, - instruction::{is_valid_signer_index, AuthorityType, TokenInstruction}, + instruction::{is_valid_signer_index, AuthorityType, TokenInstruction, MAX_SIGNERS}, option::COption, pack::{IsInitialized, Pack}, state::{Account, AccountState, Mint, Multisig}, @@ -99,8 +99,8 @@ impl Processor { Ok(()) } - /// Processes a [DangerInitializeMultisig](enum.TokenInstruction.html) instruction. - pub fn process_danger_initialize_multisig(accounts: &[AccountInfo], m: u8) -> ProgramResult { + /// Processes a [InitializeMultisig](enum.TokenInstruction.html) instruction. + pub fn process_initialize_multisig(accounts: &[AccountInfo], m: u8) -> ProgramResult { let account_info_iter = &mut accounts.iter(); let multisig_info = next_account_info(account_info_iter)?; let multisig_info_data_len = multisig_info.data_len(); @@ -637,9 +637,9 @@ impl Processor { info!("Instruction: InitializeAccount"); Self::process_initialize_account(accounts) } - TokenInstruction::DangerInitializeMultisig { m } => { - info!("Instruction: DangerInitializeMultisig"); - Self::process_danger_initialize_multisig(accounts, m) + TokenInstruction::InitializeMultisig { m } => { + info!("Instruction: InitializeMultisig"); + Self::process_initialize_multisig(accounts, m) } TokenInstruction::Transfer { amount } => { info!("Instruction: Transfer"); @@ -714,14 +714,18 @@ impl Processor { { let multisig = Multisig::unpack(&owner_account_info.data.borrow())?; let mut num_signers = 0; + let mut matched = [false; MAX_SIGNERS]; for signer in signers.iter() { - if multisig.signers[0..multisig.n as usize].contains(signer.key) { + for (position, key) in multisig.signers[0..multisig.n as usize].iter().enumerate() { + if key == signer.key && !matched[position] { if !signer.is_signer { return Err(ProgramError::MissingRequiredSignature); } + matched[position] = true; num_signers += 1; } } + } if num_signers < multisig.m { return Err(ProgramError::MissingRequiredSignature); } @@ -3600,8 +3604,7 @@ mod tests { assert_eq!( Err(TokenError::NotRentExempt.into()), do_process_instruction( - danger_initialize_multisig(&program_id, &multisig_key, &[&signer_keys[0]], 1) - .unwrap(), + initialize_multisig(&program_id, &multisig_key, &[&signer_keys[0]], 1).unwrap(), vec![ &mut multisig_account, &mut rent_sysvar, @@ -3615,7 +3618,7 @@ mod tests { // single signer let account_info_iter = &mut signer_accounts.iter_mut(); do_process_instruction( - danger_initialize_multisig(&program_id, &multisig_key, &[&signer_keys[0]], 1).unwrap(), + initialize_multisig(&program_id, &multisig_key, &[&signer_keys[0]], 1).unwrap(), vec![ &mut multisig_account, &mut rent_sysvar, @@ -3627,7 +3630,7 @@ mod tests { // multiple signer let account_info_iter = &mut signer_accounts.iter_mut(); do_process_instruction( - danger_initialize_multisig( + initialize_multisig( &program_id, &multisig_delegate_key, &signer_key_refs, @@ -4087,7 +4090,7 @@ mod tests { { let mut multisig = Multisig::unpack_unchecked(&owner_account_info.data.borrow()).unwrap(); - multisig.m = 2; // TODO 11? + multisig.m = 11; multisig.n = 11; Multisig::pack(multisig, &mut owner_account_info.data.borrow_mut()).unwrap(); } @@ -4097,6 +4100,34 @@ mod tests { Processor::validate_owner(&program_id, &owner_key, &owner_account_info, &signers) ); signers[5].is_signer = true; + + // 11:11, single signer signs multiple times + { + let mut signer_lamports = 0; + let mut signer_data = vec![]; + let signers = vec![ + AccountInfo::new( + &signer_keys[5], + true, + false, + &mut signer_lamports, + &mut signer_data, + &program_id, + false, + Epoch::default(), + ); + MAX_SIGNERS + 1 + ]; + let mut multisig = + Multisig::unpack_unchecked(&owner_account_info.data.borrow()).unwrap(); + multisig.m = 11; + multisig.n = 11; + Multisig::pack(multisig, &mut owner_account_info.data.borrow_mut()).unwrap(); + assert_eq!( + Err(ProgramError::MissingRequiredSignature), + Processor::validate_owner(&program_id, &owner_key, &owner_account_info, &signers) + ); + } } #[test]