backport #479 to v2

jackcmay · jackcmay · commit f68ff75ba0ee · 2020-09-21T17:08:37.000-07:00
diff --git 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
@@ -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
@@ -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]
