#48 Fix Audit Issues (#49)

* #48 Fix Audit Issues

* #48 Fix Audit Issues

* #48 Fix Audit Issues

.gitignore

@@ -1,6 +1,7 @@
 # Generated by Cargo
 # will have compiled files and executables
 target
+deploy
 
 # Remove Cargo.lock from gitignore if creating an executable, leave it for libraries
 # More information here https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html

init-governance.sh

@@ -12,10 +12,10 @@ solana -v --keypair artifacts/voters/voter5.keypair airdrop 100
 
 spl-token create-token --decimals 6 --fee-payer artifacts/payer.keypair artifacts/community-mint.keypair
 
-solana program deploy --program-id artifacts/spl-governance.keypair -v deploy/spl_governance.so
+solana program deploy --program-id artifacts/spl-governance.keypair --keypair artifacts/creator.keypair -v deploy/spl_governance.so
 
-solana program deploy --program-id artifacts/addin-fixed-weights.keypair -v deploy/spl_governance_addin_fixed_weights.so
+solana program deploy --program-id artifacts/addin-fixed-weights.keypair --keypair artifacts/creator.keypair -v deploy/spl_governance_addin_fixed_weights.so
 
-solana program deploy --program-id artifacts/addin-vesting.keypair -v deploy/spl_governance_addin_vesting.so
+solana program deploy --program-id artifacts/addin-vesting.keypair --keypair artifacts/creator.keypair -v deploy/spl_governance_addin_vesting.so
 
-solana program deploy --program-id artifacts/maintenance.keypair -v deploy/maintenance.so
+solana program deploy --program-id artifacts/maintenance.keypair --keypair artifacts/creator.keypair -v deploy/maintenance.so

maintenance-test-scripts/src/main.rs

@@ -37,11 +37,11 @@ use maintenance::{
 };
 
 const MAINTENANCE_KEY_FILE_PATH: &str = "../artifacts/maintenance.keypair";
-const MAINTAIN_AUTHORITY_KEY_FILE_PATH: &str = "../artifacts/voter1.keypair";
-const NEW_MAINTAIN_AUTHORITY_KEY_FILE_PATH: &str = "../artifacts/voter2.keypair";
-const ADDIN_INITIAL_AUTHORITY_KEY_FILE_PATH: &str = "../artifacts/voter3.keypair";
+const MAINTAIN_AUTHORITY_KEY_FILE_PATH: &str = "../artifacts/voters/voter1.keypair";
+const NEW_MAINTAIN_AUTHORITY_KEY_FILE_PATH: &str = "../artifacts/voters/voter2.keypair";
+const ADDIN_INITIAL_AUTHORITY_KEY_FILE_PATH: &str = "../artifacts/creator.keypair";
 const ADDIN_KEY_FILE_PATH: &str = "../artifacts/addin-fixed-weights.keypair";
-const BUFFER_ADDRESS: &str = "447xS9Kc6m1Dg8UEnyMFrnao5ZL82HTaq7UEjfLZDWEb";
+const BUFFER_ADDRESS: &str = "8U2jtFxrqij5iqna99DFFpndSj5M388TB2eYbwfzmvYg";
 
 fn main() {
 

maintenance/program/src/error.rs

@@ -17,18 +17,33 @@ pub enum MaintenanceError {
     /// Wrong Authority
     #[error("Wrong Authority")]
     WrongAuthority,
+    /// Incorrect Bpf-Loader-Upgradeable Program Id
+    #[error("Incorrect Bpf-Loader-Upgradeable Program Id")]
+    IncorrectBpfLoaderProgramId,
     /// Buffer Data Offset Error
     #[error("Buffer Data Offset Error")]
     BufferDataOffsetError,
     /// Authority Deserialization Error
     #[error("Authority Deserialization Error")]
     AuthorityDeserializationError,
+    /// Number of Delegates Exceeds Limit
+    #[error("Number of Delegates Exceeds Limit")]
+    NumberOfDelegatesExceedsLimit,
+    /// Number of Code Hashes Exceeds Limit
+    #[error("Number of Code Hashes Exceeds Limit")]
+    NumberOfCodeHashesExceedsLimit,
     /// Wrong Authority Delegate
     #[error("Wrong Authority Delegate")]
     WrongDelegate,
     /// Wrong Upgrade Code Hash
     #[error("Wrong Upgrade Code Hash")]
     WrongCodeHash,
+    /// Maintenance Record Account Matches Spill Account And Can't Be Closed
+    #[error("Maintenance Record Account Matches Spill Account")]
+    MaintenanceRecordAccountMatchesSpillAccount,
+    /// Wrong Program Data For Maintenance Record
+    #[error("Wrong Program Data For Maintenance Record")]
+    WrongProgramDataForMaintenanceRecord,
     /// Maintenance Record Must be Inactive
     #[error("Maintenance record must be inactive")]
     RecordMustBeInactive,

maintenance/program/src/instruction.rs

@@ -71,10 +71,12 @@ pub enum MaintenanceInstruction {
 
     /// Closes MaintenanceRecord owned by the program
     ///
-    /// 0. `[writable]` MaintenanceRecord
-    /// 1. `[]` Maintained program data account
-    /// 2. `[signer]` Authority
-    /// 3. `[writable]` Spill destination
+    /// 0. `[]` Bpf Loader Upgradeable Program Id
+    /// 1. `[writable]` MaintenanceRecord
+    /// 2. `[]` Maintained program account
+    /// 3. `[]` Maintained program data account
+    /// 4. `[signer]` Authority
+    /// 5. `[writable]` Spill destination
     CloseMaintenance { },
 }
 
@@ -240,7 +242,9 @@ pub fn close_maintenance(
     let (maintenance_record, _): (Pubkey, u8) = get_maintenance_record_address(program_id, program_address);
 
     let accounts = vec![
+        AccountMeta::new_readonly(bpf_loader_upgradeable::id(), false),
         AccountMeta::new(maintenance_record, false),
+        AccountMeta::new_readonly(*program_address, false),
         AccountMeta::new_readonly(programdata_address, false),
         AccountMeta::new_readonly(*authority, true),
         AccountMeta::new(*spill, false),

maintenance/program/src/processor.rs

@@ -5,6 +5,7 @@ use borsh::{ BorshDeserialize, BorshSerialize };
 use solana_program::{
     account_info::{next_account_info, AccountInfo},
     bpf_loader_upgradeable::{
+        self,
         set_upgrade_authority,
         upgrade,
         UpgradeableLoaderState,
@@ -92,6 +93,7 @@ pub fn process_create_maintenance(
 
     let maintenance_record_data = MaintenanceRecord {
         account_type: MaintenanceAccountType::MaintenanceRecord,
+        maintained_address: *address_info.key,
         authority: *authority_info.key,
         delegate: Vec::new(),
         hashes: Vec::new(),
@@ -128,6 +130,10 @@ pub fn process_set_delegates(
         return Err(MaintenanceError::MissingRequiredSigner.into());
     }
 
+    if delegate.len() > 10 {
+        return Err(MaintenanceError::NumberOfDelegatesExceedsLimit.into());
+    }
+
     let mut maintenance_record = get_account_data::<MaintenanceRecord>(program_id, maintenance_record_info)?;
 
     if *authority_info.key != maintenance_record.authority {
@@ -156,6 +162,10 @@ pub fn process_set_code_hashes(
         return Err(MaintenanceError::MissingRequiredSigner.into());
     }
 
+    if hashes.len() > 10 {
+        return Err(MaintenanceError::NumberOfCodeHashesExceedsLimit.into());
+    }
+
     let mut maintenance_record = get_account_data::<MaintenanceRecord>(program_id, maintenance_record_info)?;
 
     if *authority_info.key != maintenance_record.authority {
@@ -193,7 +203,7 @@ pub fn process_upgrade(
     let maintenance_record = get_account_data::<MaintenanceRecord>(program_id, maintenance_record_info)?;
 
     if maintenance_record.authority != *authority_info.key &&
-        maintenance_record.delegate.iter().any(|&item| item == *authority_info.key )
+        !maintenance_record.delegate.iter().any(|&item| item == *authority_info.key )
     {
         return Err(MaintenanceError::WrongDelegate.into());
     }
@@ -206,7 +216,7 @@ pub fn process_upgrade(
     };
     // msg!("MAINTENANCE-INSTRUCTION: UPGRADE Buffer Hash {:?}", buffer_hash);
 
-    if maintenance_record.hashes.iter().any(|&item| item == buffer_hash ) {
+    if !maintenance_record.hashes.iter().any(|&item| item == buffer_hash ) {
         return Err(MaintenanceError::WrongCodeHash.into());
     }
 
@@ -301,27 +311,39 @@ pub fn process_close_maintenance(
 ) -> ProgramResult {
     let account_info_iter = &mut accounts.iter();
 
-    let maintenance_record_info = next_account_info(account_info_iter)?; // 0
-    let program_data_info = next_account_info(account_info_iter)?; // 1
-    let authority_info = next_account_info(account_info_iter)?; // 2
-    let spill_info = next_account_info(account_info_iter)?; // 3
+    let bpf_loader_program_info = next_account_info(account_info_iter)?; // 0
+    let maintenance_record_info = next_account_info(account_info_iter)?; // 1
+    let maintained_program_info = next_account_info(account_info_iter)?; // 2
+    let maintained_program_data_info = next_account_info(account_info_iter)?; // 3
+    let authority_info = next_account_info(account_info_iter)?; // 4
+    let spill_info = next_account_info(account_info_iter)?; // 5
 
     if !authority_info.is_signer {
         return Err(MaintenanceError::MissingRequiredSigner.into());
     }
 
+    if !bpf_loader_upgradeable::check_id(bpf_loader_program_info.key) {
+        return Err(MaintenanceError::IncorrectBpfLoaderProgramId.into());
+    }
+
     if maintenance_record_info.key == spill_info.key {
-        return Err(ProgramError::InvalidAccountData);
+        return Err(MaintenanceError::MaintenanceRecordAccountMatchesSpillAccount.into());
     }
 
+    let (maintained_program_data, _) = Pubkey::find_program_address(&[maintained_program_info.key.as_ref()], bpf_loader_program_info.key);
     let maintenance_record = get_account_data::<MaintenanceRecord>(program_id, maintenance_record_info)?;
 
+    if *maintained_program_data_info.key != maintained_program_data ||
+        *maintained_program_info.key != maintenance_record.maintained_address {
+        return Err(MaintenanceError::WrongProgramDataForMaintenanceRecord.into());
+    }
+
     if *authority_info.key != maintenance_record.authority {
         return Err(MaintenanceError::WrongAuthority.into());
     }
 
     let upgradeable_loader_state: UpgradeableLoaderState =
-        bincode::deserialize(&program_data_info.data.borrow()).map_err(|_| ProgramError::from(MaintenanceError::AuthorityDeserializationError) )?;
+        bincode::deserialize(&maintained_program_data_info.data.borrow()).map_err(|_| ProgramError::from(MaintenanceError::AuthorityDeserializationError) )?;
 
     let program_authority: Pubkey = 
         match upgradeable_loader_state {

maintenance/program/src/state.rs

@@ -19,6 +19,7 @@ pub enum MaintenanceAccountType {
 #[derive(Clone, Debug, PartialEq, BorshDeserialize, BorshSerialize, BorshSchema)]
 pub struct MaintenanceRecord {
     pub account_type: MaintenanceAccountType,
+    pub maintained_address: Pubkey,
     pub authority: Pubkey,
     pub delegate: Vec<Pubkey>,
     pub hashes: Vec<Hash>,
@@ -48,6 +49,7 @@ mod tests {
     fn test_maintenance_record_packing() {
         let maintenance_record_source = MaintenanceRecord {
             account_type: MaintenanceAccountType::MaintenanceRecord,
+            maintained_address: Pubkey::new_unique(),
             authority: Pubkey::new_unique(),
             delegate: Vec::new(),
             hashes: Vec::new(),