From 2bc9c1e9ce4dbb2b977331e3fca0fd45a2549b01 Mon Sep 17 00:00:00 2001 From: "Sebastian.Bor" Date: Mon, 24 Jan 2022 12:24:21 +0000 Subject: [PATCH 1/5] feat: Ensure realm authority can be set to realm's governances only --- governance/program/src/instruction.rs | 16 ++-- governance/program/src/processor/mod.rs | 6 +- .../processor/process_set_realm_authority.rs | 17 +++- governance/program/src/state/governance.rs | 10 +++ .../tests/process_set_realm_authority.rs | 78 ++++++++++++++++++- 5 files changed, 116 insertions(+), 11 deletions(-) diff --git a/governance/program/src/instruction.rs b/governance/program/src/instruction.rs index 22e3dee7314..662bf570baa 100644 --- a/governance/program/src/instruction.rs +++ b/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, + /// 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, // 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, diff --git a/governance/program/src/processor/mod.rs b/governance/program/src/processor/mod.rs index 4c864ddf483..b3dff2dc241 100644 --- a/governance/program/src/processor/mod.rs +++ b/governance/program/src/processor/mod.rs @@ -190,9 +190,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) } diff --git a/governance/program/src/processor/process_set_realm_authority.rs b/governance/program/src/processor/process_set_realm_authority.rs index 570445e5b56..0d4b038314a 100644 --- a/governance/program/src/processor/process_set_realm_authority.rs +++ b/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, + remove_authority: bool, ) -> ProgramResult { let account_info_iter = &mut accounts.iter(); @@ -27,6 +30,16 @@ 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 + 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())?; diff --git a/governance/program/src/state/governance.rs b/governance/program/src/state/governance.rs index 5db3adf52cd..9a9fa711d67 100644 --- a/governance/program/src/state/governance.rs +++ b/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, diff --git a/governance/program/tests/process_set_realm_authority.rs b/governance/program/tests/process_set_realm_authority.rs index 65a545a45f0..6c18304d8e6 100644 --- a/governance/program/tests/process_set_realm_authority.rs +++ b/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()); +} From 94c6f3ecfb2bbddfa70d4ffe1a8553915700799c Mon Sep 17 00:00:00 2001 From: "Sebastian.Bor" Date: Mon, 24 Jan 2022 15:01:56 +0000 Subject: [PATCH 2/5] chore: update comments for governance account check --- governance/program/src/processor/process_set_realm_authority.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/governance/program/src/processor/process_set_realm_authority.rs b/governance/program/src/processor/process_set_realm_authority.rs index 0d4b038314a..d52977ef72b 100644 --- a/governance/program/src/processor/process_set_realm_authority.rs +++ b/governance/program/src/processor/process_set_realm_authority.rs @@ -34,6 +34,8 @@ pub fn process_set_realm_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)?; From b7f5e78b6230609aba4f08b5ac9cfe3198f0217c Mon Sep 17 00:00:00 2001 From: "Sebastian.Bor" Date: Mon, 24 Jan 2022 15:20:29 +0000 Subject: [PATCH 3/5] chore: update realm authority comments --- governance/program/src/state/legacy.rs | 2 +- governance/program/src/state/realm.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/governance/program/src/state/legacy.rs b/governance/program/src/state/legacy.rs index eac9d219949..8258ea01388 100644 --- a/governance/program/src/state/legacy.rs +++ b/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 transferer to Realm Governance to make the Realm self governed through proposals pub authority: Option, /// Governance Realm name diff --git a/governance/program/src/state/realm.rs b/governance/program/src/state/realm.rs index e4fc77ac22c..9be7d18e7b2 100644 --- a/governance/program/src/state/realm.rs +++ b/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 transferer to Realm Governance to make the Realm self governed through proposals pub authority: Option, /// Governance Realm name From fc994c56026b213abe06a27aa39acddbad2c234a Mon Sep 17 00:00:00 2001 From: Sebastian Bor Date: Mon, 24 Jan 2022 19:14:07 +0000 Subject: [PATCH 4/5] chore: update comments Co-authored-by: Jon Cinque --- governance/program/src/state/legacy.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/governance/program/src/state/legacy.rs b/governance/program/src/state/legacy.rs index 8258ea01388..fa308cdddc0 100644 --- a/governance/program/src/state/legacy.rs +++ b/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 should be transferer to Realm Governance to 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, /// Governance Realm name From 6cc0e262cb2ccc4fb1261a59c600e85fe108e6a2 Mon Sep 17 00:00:00 2001 From: Sebastian Bor Date: Mon, 24 Jan 2022 19:14:14 +0000 Subject: [PATCH 5/5] chore: update comments Co-authored-by: Jon Cinque --- governance/program/src/state/realm.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/governance/program/src/state/realm.rs b/governance/program/src/state/realm.rs index 9be7d18e7b2..2e846bc2f53 100644 --- a/governance/program/src/state/realm.rs +++ b/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 should be transferer to Realm Governance to 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, /// Governance Realm name