Improve signature checks in config_api (#5001)

automerge

programs/config_api/src/config_instruction.rs

@@ -50,8 +50,10 @@ pub fn store<T: ConfigState>(
 ) -> Instruction {
     let mut account_metas = vec![AccountMeta::new(*config_account_pubkey, is_config_signer)];
     for (signer_pubkey, _) in keys.iter().filter(|(_, is_signer)| *is_signer) {
+        if signer_pubkey != config_account_pubkey {
             account_metas.push(AccountMeta::new(*signer_pubkey, true));
         }
+    }
     let account_data = (ConfigKeys { keys }, data);
     Instruction::new(id(), &account_data, account_metas)
 }

programs/config_api/src/config_processor.rs

@@ -16,6 +16,27 @@ pub fn process_instruction(
         error!("Invalid ConfigKeys data: {:?} {:?}", data, err);
         InstructionError::InvalidInstructionData
     })?;
+
+    let current_data: ConfigKeys = deserialize(&keyed_accounts[0].account.data).map_err(|err| {
+        error!("Invalid data in account[0]: {:?} {:?}", data, err);
+        InstructionError::InvalidAccountData
+    })?;
+    let current_signer_keys: Vec<Pubkey> = current_data
+        .keys
+        .iter()
+        .filter(|(_, is_signer)| *is_signer)
+        .map(|(pubkey, _)| *pubkey)
+        .collect();
+
+    if current_signer_keys.is_empty() {
+        // Config account keypair must be a signer on account initilization,
+        // or when no signers specified in Config data
+        if keyed_accounts[0].signer_key().is_none() {
+            error!("account[0].signer_key().is_none()");
+            Err(InstructionError::MissingRequiredSignature)?;
+        }
+    }
+
     let mut counter = 0;
     for (i, (signer, _)) in key_list
         .keys
@@ -24,6 +45,7 @@ pub fn process_instruction(
         .enumerate()
     {
         counter += 1;
+        if signer != keyed_accounts[0].unsigned_key() {
             let account_index = i + 1;
             let signer_account = keyed_accounts.get(account_index);
             if signer_account.is_none() {
@@ -42,15 +64,32 @@ pub fn process_instruction(
                 );
                 Err(InstructionError::MissingRequiredSignature)?;
             }
+            // If Config account is already initialized, update signatures must match Config data
+            if !current_data.keys.is_empty()
+                && current_signer_keys
+                    .iter()
+                    .find(|&pubkey| pubkey == signer)
+                    .is_none()
+            {
+                error!("account {:?} is not in stored signer list", signer);
+                Err(InstructionError::MissingRequiredSignature)?;
             }
-    if counter == 0 {
+        } else if keyed_accounts[0].signer_key().is_none() {
-        // If Config data does not specify any signers, Config account keypair must be a signer
-        if keyed_accounts[0].signer_key().is_none() {
             error!("account[0].signer_key().is_none()");
             Err(InstructionError::MissingRequiredSignature)?;
         }
     }
 
+    // Check for Config data signers not present in incoming account update
+    if current_signer_keys.len() > counter {
+        error!(
+            "too few signers: {:?}; expected: {:?}",
+            counter,
+            current_signer_keys.len()
+        );
+        Err(InstructionError::MissingRequiredSignature)?;
+    }
+
     if keyed_accounts[0].account.data.len() < data.len() {
         error!("instruction data too large");
         Err(InstructionError::InvalidInstructionData)?;
@@ -271,20 +310,7 @@ mod tests {
 
         bank_client
             .send_message(&[&mint_keypair, &signer0], message)
-            .unwrap();
+            .unwrap_err();
-
-        let config_account_data = bank_client
-            .get_account_data(&config_pubkey)
-            .unwrap()
-            .unwrap();
-        let meta_length = ConfigKeys::serialized_size(keys.clone());
-        let meta_data: ConfigKeys = deserialize(&config_account_data[0..meta_length]).unwrap();
-        assert_eq!(meta_data.keys, keys);
-        let config_account_data = &config_account_data[meta_length..config_account_data.len()];
-        assert_eq!(
-            my_config,
-            MyConfig::deserialize(&config_account_data).unwrap()
-        );
     }
 
     #[test]
@@ -316,4 +342,138 @@ mod tests {
             .send_message(&[&mint_keypair, &config_keypair], message)
             .unwrap_err();
     }
+
+    #[test]
+    fn test_config_updates() {
+        solana_logger::setup();
+        let (bank, mint_keypair) = create_bank(10_000);
+        let pubkey = Pubkey::new_rand();
+        let signer0 = Keypair::new();
+        let signer1 = Keypair::new();
+        let keys = vec![
+            (pubkey, false),
+            (signer0.pubkey(), true),
+            (signer1.pubkey(), true),
+        ];
+        let (bank_client, config_keypair) =
+            create_config_account(bank, &mint_keypair, keys.clone());
+        let config_pubkey = config_keypair.pubkey();
+
+        let my_config = MyConfig::new(42);
+
+        let instruction = config_instruction::store(&config_pubkey, true, keys.clone(), &my_config);
+        let message = Message::new_with_payer(vec![instruction], Some(&mint_keypair.pubkey()));
+
+        bank_client
+            .send_message(
+                &[&mint_keypair, &config_keypair, &signer0, &signer1],
+                message,
+            )
+            .unwrap();
+
+        // Update with expected signatures
+        let new_config = MyConfig::new(84);
+        let instruction =
+            config_instruction::store(&config_pubkey, false, keys.clone(), &new_config);
+        let message = Message::new_with_payer(vec![instruction], Some(&mint_keypair.pubkey()));
+        bank_client
+            .send_message(&[&mint_keypair, &signer0, &signer1], message)
+            .unwrap();
+
+        let config_account_data = bank_client
+            .get_account_data(&config_pubkey)
+            .unwrap()
+            .unwrap();
+        let meta_length = ConfigKeys::serialized_size(keys.clone());
+        let meta_data: ConfigKeys = deserialize(&config_account_data[0..meta_length]).unwrap();
+        assert_eq!(meta_data.keys, keys);
+        let config_account_data = &config_account_data[meta_length..config_account_data.len()];
+        assert_eq!(
+            new_config,
+            MyConfig::deserialize(&config_account_data).unwrap()
+        );
+
+        // Attempt update with incomplete signatures
+        let keys = vec![(pubkey, false), (signer0.pubkey(), true)];
+        let instruction =
+            config_instruction::store(&config_pubkey, false, keys.clone(), &my_config);
+        let message = Message::new_with_payer(vec![instruction], Some(&mint_keypair.pubkey()));
+        bank_client
+            .send_message(&[&mint_keypair, &signer0], message)
+            .unwrap_err();
+
+        // Attempt update with incorrect signatures
+        let signer2 = Keypair::new();
+        let keys = vec![
+            (pubkey, false),
+            (signer0.pubkey(), true),
+            (signer2.pubkey(), true),
+        ];
+        let instruction =
+            config_instruction::store(&config_pubkey, false, keys.clone(), &my_config);
+        let message = Message::new_with_payer(vec![instruction], Some(&mint_keypair.pubkey()));
+        bank_client
+            .send_message(&[&mint_keypair, &signer0, &signer2], message)
+            .unwrap_err();
+    }
+
+    #[test]
+    fn test_config_updates_requiring_config() {
+        solana_logger::setup();
+        let (bank, mint_keypair) = create_bank(10_000);
+        let pubkey = Pubkey::new_rand();
+        let signer0 = Keypair::new();
+        let keys = vec![
+            (pubkey, false),
+            (signer0.pubkey(), true),
+            (signer0.pubkey(), true),
+        ]; // Dummy keys for account sizing
+        let (bank_client, config_keypair) =
+            create_config_account(bank, &mint_keypair, keys.clone());
+        let config_pubkey = config_keypair.pubkey();
+        let keys = vec![
+            (pubkey, false),
+            (signer0.pubkey(), true),
+            (config_keypair.pubkey(), true),
+        ];
+
+        let my_config = MyConfig::new(42);
+
+        let instruction = config_instruction::store(&config_pubkey, true, keys.clone(), &my_config);
+        let message = Message::new_with_payer(vec![instruction], Some(&mint_keypair.pubkey()));
+
+        bank_client
+            .send_message(&[&mint_keypair, &config_keypair, &signer0], message)
+            .unwrap();
+
+        // Update with expected signatures
+        let new_config = MyConfig::new(84);
+        let instruction =
+            config_instruction::store(&config_pubkey, true, keys.clone(), &new_config);
+        let message = Message::new_with_payer(vec![instruction], Some(&mint_keypair.pubkey()));
+        bank_client
+            .send_message(&[&mint_keypair, &config_keypair, &signer0], message)
+            .unwrap();
+
+        let config_account_data = bank_client
+            .get_account_data(&config_pubkey)
+            .unwrap()
+            .unwrap();
+        let meta_length = ConfigKeys::serialized_size(keys.clone());
+        let meta_data: ConfigKeys = deserialize(&config_account_data[0..meta_length]).unwrap();
+        assert_eq!(meta_data.keys, keys);
+        let config_account_data = &config_account_data[meta_length..config_account_data.len()];
+        assert_eq!(
+            new_config,
+            MyConfig::deserialize(&config_account_data).unwrap()
+        );
+
+        // Attempt update with incomplete signatures
+        let keys = vec![(pubkey, false), (config_keypair.pubkey(), true)];
+        let instruction = config_instruction::store(&config_pubkey, true, keys.clone(), &my_config);
+        let message = Message::new_with_payer(vec![instruction], Some(&mint_keypair.pubkey()));
+        bank_client
+            .send_message(&[&mint_keypair, &config_keypair], message)
+            .unwrap_err();
+    }
 }