Governance: Realm authority validation (#2787)

* feat: Ensure realm authority can be set to realm's governances only

* chore: update comments for governance account check

* chore: update realm authority comments

* chore: update comments

* chore: update comments

Co-authored-by: Jon Cinque <[email protected]>

Author: SebastianBor

governance/program/src/instruction.rs

@@ -410,10 +410,11 @@ pub enum GovernanceInstruction {
     ///
     ///   0. `[writable]` Realm account
     ///   1. `[signer]` Current Realm authority    
+    ///   2. `[]` New realm authority. Must be one of the realm governances when set
     SetRealmAuthority {
         #[allow(dead_code)]
-        /// New realm authority or None to remove authority
-        new_realm_authority: Option<Pubkey>,
+        /// Indicates whether the realm authority should be removed and set to None
+        remove_authority: bool,
     },
 
     /// Sets realm config
@@ -1271,15 +1272,20 @@ pub fn set_realm_authority(
     new_realm_authority: &Option<Pubkey>,
     // Args
 ) -> Instruction {
-    let accounts = vec![
+    let mut accounts = vec![
         AccountMeta::new(*realm, false),
         AccountMeta::new_readonly(*realm_authority, true),
     ];
 
-    let instruction = GovernanceInstruction::SetRealmAuthority {
-        new_realm_authority: *new_realm_authority,
+    let remove_authority = if let Some(new_realm_authority) = new_realm_authority {
+        accounts.push(AccountMeta::new_readonly(*new_realm_authority, false));
+        false
+    } else {
+        true
     };
 
+    let instruction = GovernanceInstruction::SetRealmAuthority { remove_authority };
+
     Instruction {
         program_id: *program_id,
         accounts,

governance/program/src/processor/mod.rs

@@ -191,9 +191,9 @@ pub fn process_instruction(
         GovernanceInstruction::FlagInstructionError {} => {
             process_flag_instruction_error(program_id, accounts)
         }
-        GovernanceInstruction::SetRealmAuthority {
-            new_realm_authority,
-        } => process_set_realm_authority(program_id, accounts, new_realm_authority),
+        GovernanceInstruction::SetRealmAuthority { remove_authority } => {
+            process_set_realm_authority(program_id, accounts, remove_authority)
+        }
         GovernanceInstruction::SetRealmConfig { config_args } => {
             process_set_realm_config(program_id, accounts, config_args)
         }

governance/program/src/processor/process_set_realm_authority.rs

@@ -7,13 +7,16 @@ use solana_program::{
     pubkey::Pubkey,
 };
 
-use crate::{error::GovernanceError, state::realm::get_realm_data_for_authority};
+use crate::{
+    error::GovernanceError,
+    state::{governance::assert_governance_for_realm, realm::get_realm_data_for_authority},
+};
 
 /// Processes SetRealmAuthority instruction
 pub fn process_set_realm_authority(
     program_id: &Pubkey,
     accounts: &[AccountInfo],
-    new_realm_authority: Option<Pubkey>,
+    remove_authority: bool,
 ) -> ProgramResult {
     let account_info_iter = &mut accounts.iter();
 
@@ -27,6 +30,18 @@ pub fn process_set_realm_authority(
         return Err(GovernanceError::RealmAuthorityMustSign.into());
     }
 
+    let new_realm_authority = if remove_authority {
+        None
+    } else {
+        // Ensure the new realm authority is one of the governances from the realm
+        // Note: This is not a security feature because governance creation is only gated with min_community_tokens_to_create_governance
+        //       The check is done to prevent scenarios where the authority could be accidentally set to a wrong or none existing account
+        let new_realm_authority_info = next_account_info(account_info_iter)?; // 2
+        assert_governance_for_realm(program_id, new_realm_authority_info, realm_info.key)?;
+
+        Some(*new_realm_authority_info.key)
+    };
+
     realm_data.authority = new_realm_authority;
 
     realm_data.serialize(&mut *realm_info.data.borrow_mut())?;

governance/program/src/state/governance.rs

@@ -127,6 +127,16 @@ pub fn get_governance_data_for_realm(
     Ok(governance_data)
 }
 
+/// Checks the given account is a governance account and belongs to the given realm
+pub fn assert_governance_for_realm(
+    program_id: &Pubkey,
+    governance_info: &AccountInfo,
+    realm: &Pubkey,
+) -> Result<(), ProgramError> {
+    get_governance_data_for_realm(program_id, governance_info, realm)?;
+    Ok(())
+}
+
 /// Returns ProgramGovernance PDA seeds
 pub fn get_program_governance_address_seeds<'a>(
     realm: &'a Pubkey,

governance/program/src/state/legacy.rs

@@ -83,7 +83,7 @@ pub struct RealmV1 {
     pub reserved: [u8; 8],
 
     /// Realm authority. The authority must sign transactions which update the realm config
-    /// The authority can be transferer to Realm Governance and hence make the Realm self governed through proposals
+    /// The authority should be transferred to Realm Governance to make the Realm self governed through proposals
     pub authority: Option<Pubkey>,
 
     /// Governance Realm name

governance/program/src/state/realm.rs

@@ -70,7 +70,7 @@ pub struct Realm {
     pub reserved: [u8; 8],
 
     /// Realm authority. The authority must sign transactions which update the realm config
-    /// The authority can be transferer to Realm Governance and hence make the Realm self governed through proposals
+    /// The authority should be transferred to Realm Governance to make the Realm self governed through proposals
     pub authority: Option<Pubkey>,
 
     /// Governance Realm name

governance/program/tests/process_set_realm_authority.rs

@@ -7,6 +7,7 @@ mod program_test;
 
 use program_test::*;
 use spl_governance::error::GovernanceError;
+use spl_governance_tools::error::GovernanceToolsError;
 
 #[tokio::test]
 async fn test_set_realm_authority() {
@@ -15,7 +16,23 @@ async fn test_set_realm_authority() {
 
     let realm_cookie = governance_test.with_realm().await;
 
-    let new_realm_authority = Pubkey::new_unique();
+    let governed_account_cookie = governance_test.with_governed_account().await;
+
+    let token_owner_record_cookie = governance_test
+        .with_community_token_deposit(&realm_cookie)
+        .await
+        .unwrap();
+
+    let account_governance_cookie = governance_test
+        .with_account_governance(
+            &realm_cookie,
+            &governed_account_cookie,
+            &token_owner_record_cookie,
+        )
+        .await
+        .unwrap();
+
+    let new_realm_authority = account_governance_cookie.address;
 
     // Act
     governance_test
@@ -31,6 +48,26 @@ async fn test_set_realm_authority() {
     assert_eq!(realm_account.authority, Some(new_realm_authority));
 }
 
+#[tokio::test]
+async fn test_set_realm_authority_with_non_existing_new_authority_error() {
+    // Arrange
+    let mut governance_test = GovernanceProgramTest::start_new().await;
+
+    let realm_cookie = governance_test.with_realm().await;
+
+    let new_realm_authority = Pubkey::new_unique();
+
+    // Act
+    let err = governance_test
+        .set_realm_authority(&realm_cookie, &Some(new_realm_authority))
+        .await
+        .err()
+        .unwrap();
+
+    // Assert
+    assert_eq!(err, GovernanceToolsError::AccountDoesNotExist.into());
+}
+
 #[tokio::test]
 async fn test_set_realm_authority_to_none() {
     // Arrange
@@ -125,3 +162,42 @@ async fn test_set_realm_authority_with_authority_must_sign_error() {
     // Assert
     assert_eq!(err, GovernanceError::RealmAuthorityMustSign.into());
 }
+
+#[tokio::test]
+async fn test_set_realm_authority_with_governance_from_other_realm_error() {
+    // Arrange
+    let mut governance_test = GovernanceProgramTest::start_new().await;
+
+    let realm_cookie = governance_test.with_realm().await;
+
+    // Setup other realm
+    let realm_cookie2 = governance_test.with_realm().await;
+
+    let governed_account_cookie2 = governance_test.with_governed_account().await;
+
+    let token_owner_record_cookie2 = governance_test
+        .with_community_token_deposit(&realm_cookie2)
+        .await
+        .unwrap();
+
+    let account_governance_cookie2 = governance_test
+        .with_account_governance(
+            &realm_cookie2,
+            &governed_account_cookie2,
+            &token_owner_record_cookie2,
+        )
+        .await
+        .unwrap();
+
+    let new_realm_authority = account_governance_cookie2.address;
+
+    // Act
+    let err = governance_test
+        .set_realm_authority(&realm_cookie, &Some(new_realm_authority))
+        .await
+        .err()
+        .unwrap();
+
+    // Assert
+    assert_eq!(err, GovernanceError::InvalidRealmForGovernance.into());
+}