From 996328fc01abc84291ef0c23a6ec1ac2b1bf334f Mon Sep 17 00:00:00 2001 From: Gabriel Lopez Date: Thu, 7 Sep 2023 17:36:15 -0500 Subject: [PATCH 01/12] Allow admin to pause/unpause subdao Also fixed some typos and removed unused msg.rs from dao-dao-core --- contracts/dao-dao-core/src/contract.rs | 35 +++- contracts/dao-dao-core/src/msg.rs | 240 ------------------------- contracts/dao-dao-core/src/tests.rs | 50 +++++- packages/dao-interface/src/msg.rs | 2 + 4 files changed, 80 insertions(+), 247 deletions(-) delete mode 100644 contracts/dao-dao-core/src/msg.rs diff --git a/contracts/dao-dao-core/src/contract.rs b/contracts/dao-dao-core/src/contract.rs index 97f3a7d3e..225676432 100644 --- a/contracts/dao-dao-core/src/contract.rs +++ b/contracts/dao-dao-core/src/contract.rs @@ -107,9 +107,16 @@ pub fn execute( msg: ExecuteMsg, ) -> Result { // No actions can be performed while the DAO is paused. - if let Some(expiration) = PAUSED.may_load(deps.storage)? { - if !expiration.is_expired(&env.block) { - return Err(ContractError::Paused {}); + match &msg { + &ExecuteMsg::Unpause {} => { + // Allow the unpause action to pass through + } + _ => { + if let Some(expiration) = PAUSED.may_load(deps.storage)? { + if !expiration.is_expired(&env.block) { + return Err(ContractError::Paused {}); + } + } } } @@ -121,6 +128,7 @@ pub fn execute( execute_proposal_hook(deps.as_ref(), info.sender, msgs) } ExecuteMsg::Pause { duration } => execute_pause(deps, env, info.sender, duration), + ExecuteMsg::Unpause {} => execute_unpause(deps, env, info.sender), ExecuteMsg::Receive(_) => execute_receive_cw20(deps, info.sender), ExecuteMsg::ReceiveNft(_) => execute_receive_cw721(deps, info.sender), ExecuteMsg::RemoveItem { key } => execute_remove_item(deps, env, info.sender, key), @@ -159,8 +167,10 @@ pub fn execute_pause( sender: Addr, pause_duration: Duration, ) -> Result { - // Only the core contract may call this method. - if sender != env.contract.address { + let admin = ADMIN.load(deps.storage)?; + + // Only the core contract or admin may call this method. + if sender != env.contract.address && sender != admin { return Err(ContractError::Unauthorized {}); } @@ -174,6 +184,21 @@ pub fn execute_pause( .add_attribute("until", until.to_string())) } +pub fn execute_unpause(deps: DepsMut, env: Env, sender: Addr) -> Result { + let admin = ADMIN.load(deps.storage)?; + + // Only the admin may call this method excluding the core contract. + if sender != admin || sender == env.contract.address { + return Err(ContractError::Unauthorized {}); + } + + PAUSED.remove(deps.storage); + + Ok(Response::new() + .add_attribute("action", "execute_unpause") + .add_attribute("sender", sender)) +} + pub fn execute_admin_msgs( deps: Deps, sender: Addr, diff --git a/contracts/dao-dao-core/src/msg.rs b/contracts/dao-dao-core/src/msg.rs deleted file mode 100644 index 99f0d21b4..000000000 --- a/contracts/dao-dao-core/src/msg.rs +++ /dev/null @@ -1,240 +0,0 @@ -use crate::state::Config; -use crate::{migrate_msg::MigrateParams, query::SubDao}; -use cosmwasm_schema::{cw_serde, QueryResponses}; -use cosmwasm_std::{CosmosMsg, Empty}; -use cw_utils::Duration; -use dao_interface::ModuleInstantiateInfo; - -/// Information about an item to be stored in the items list. -#[cw_serde] -pub struct InitialItem { - /// The name of the item. - pub key: String, - /// The value the item will have at instantiation time. - pub value: String, -} - -#[cw_serde] -pub struct InstantiateMsg { - /// Optional Admin with the ability to execute DAO messages - /// directly. Useful for building SubDAOs controlled by a parent - /// DAO. If no admin is specified the contract is set as its own - /// admin so that the admin may be updated later by governance. - pub admin: Option, - /// The name of the core contract. - pub name: String, - /// A description of the core contract. - pub description: String, - /// An image URL to describe the core module contract. - pub image_url: Option, - - /// If true the contract will automatically add received cw20 - /// tokens to its treasury. - pub automatically_add_cw20s: bool, - /// If true the contract will automatically add received cw721 - /// tokens to its treasury. - pub automatically_add_cw721s: bool, - - /// Instantiate information for the core contract's voting - /// power module. - pub voting_module_instantiate_info: ModuleInstantiateInfo, - /// Instantiate information for the core contract's proposal modules. - /// NOTE: the pre-propose-base package depends on it being the case - /// that the core module instantiates its proposal module. - pub proposal_modules_instantiate_info: Vec, - - /// The items to instantiate this DAO with. Items are arbitrary - /// key-value pairs whose contents are controlled by governance. - /// - /// It is an error to provide two items with the same key. - pub initial_items: Option>, - /// Implements the DAO Star standard: - pub dao_uri: Option, -} - -#[cw_serde] -pub enum ExecuteMsg { - /// Callable by the Admin, if one is configured. - /// Executes messages in order. - ExecuteAdminMsgs { msgs: Vec> }, - /// Callable by proposal modules. The DAO will execute the - /// messages in the hook in order. - ExecuteProposalHook { msgs: Vec> }, - /// Pauses the DAO for a set duration. - /// When paused the DAO is unable to execute proposals - Pause { duration: Duration }, - /// Executed when the contract receives a cw20 token. Depending on - /// the contract's configuration the contract will automatically - /// add the token to its treasury. - Receive(cw20::Cw20ReceiveMsg), - /// Executed when the contract receives a cw721 token. Depending - /// on the contract's configuration the contract will - /// automatically add the token to its treasury. - ReceiveNft(cw721::Cw721ReceiveMsg), - /// Removes an item from the governance contract's item map. - RemoveItem { key: String }, - /// Adds an item to the governance contract's item map. If the - /// item already exists the existing value is overridden. If the - /// item does not exist a new item is added. - SetItem { key: String, value: String }, - /// Callable by the admin of the contract. If ADMIN is None the - /// admin is set as the contract itself so that it may be updated - /// later by vote. If ADMIN is Some a new admin is proposed and - /// that new admin may become the admin by executing the - /// `AcceptAdminNomination` message. - /// - /// If there is already a pending admin nomination the - /// `WithdrawAdminNomination` message must be executed before a - /// new admin may be nominated. - NominateAdmin { admin: Option }, - /// Callable by a nominated admin. Admins are nominated via the - /// `NominateAdmin` message. Accepting a nomination will make the - /// nominated address the new admin. - /// - /// Requiring that the new admin accepts the nomination before - /// becoming the admin protects against a typo causing the admin - /// to change to an invalid address. - AcceptAdminNomination {}, - /// Callable by the current admin. Withdraws the current admin - /// nomination. - WithdrawAdminNomination {}, - /// Callable by the core contract. Replaces the current - /// governance contract config with the provided config. - UpdateConfig { config: Config }, - /// Updates the list of cw20 tokens this contract has registered. - UpdateCw20List { - to_add: Vec, - to_remove: Vec, - }, - /// Updates the list of cw721 tokens this contract has registered. - UpdateCw721List { - to_add: Vec, - to_remove: Vec, - }, - /// Updates the governance contract's governance modules. Module - /// instantiate info in `to_add` is used to create new modules and - /// install them. - UpdateProposalModules { - /// NOTE: the pre-propose-base package depends on it being the - /// case that the core module instantiates its proposal module. - to_add: Vec, - to_disable: Vec, - }, - /// Callable by the core contract. Replaces the current - /// voting module with a new one instantiated by the governance - /// contract. - UpdateVotingModule { module: ModuleInstantiateInfo }, - /// Update the core module to add/remove SubDAOs and their charters - UpdateSubDaos { - to_add: Vec, - to_remove: Vec, - }, -} - -#[cw_serde] -#[derive(QueryResponses)] -pub enum QueryMsg { - /// Get's the DAO's admin. Returns `Addr`. - #[returns(cosmwasm_std::Addr)] - Admin {}, - /// Get's the currently nominated admin (if any). - #[returns(crate::query::AdminNominationResponse)] - AdminNomination {}, - /// Gets the contract's config. - #[returns(Config)] - Config {}, - /// Gets the token balance for each cw20 registered with the - /// contract. - #[returns(crate::query::Cw20BalanceResponse)] - Cw20Balances { - start_after: Option, - limit: Option, - }, - /// Lists the addresses of the cw20 tokens in this contract's - /// treasury. - #[returns(Vec)] - Cw20TokenList { - start_after: Option, - limit: Option, - }, - /// Lists the addresses of the cw721 tokens in this contract's - /// treasury. - #[returns(Vec)] - Cw721TokenList { - start_after: Option, - limit: Option, - }, - /// Dumps all of the core contract's state in a single - /// query. Useful for frontends as performance for queries is more - /// limited by network times than compute times. - #[returns(crate::query::DumpStateResponse)] - DumpState {}, - /// Gets the address associated with an item key. - #[returns(crate::query::GetItemResponse)] - GetItem { key: String }, - /// Lists all of the items associted with the contract. For - /// example, given the items `{ "group": "foo", "subdao": "bar"}` - /// this query would return `[("group", "foo"), ("subdao", - /// "bar")]`. - #[returns(Vec)] - ListItems { - start_after: Option, - limit: Option, - }, - /// Returns contract version info - #[returns(dao_interface::voting::InfoResponse)] - Info {}, - /// Gets all proposal modules associated with the - /// contract. - #[returns(Vec)] - ProposalModules { - start_after: Option, - limit: Option, - }, - /// Gets the active proposal modules associated with the - /// contract. - #[returns(Vec)] - ActiveProposalModules { - start_after: Option, - limit: Option, - }, - /// Gets the number of active and total proposal modules - /// registered with this module. - #[returns(crate::query::ProposalModuleCountResponse)] - ProposalModuleCount {}, - /// Returns information about if the contract is currently paused. - #[returns(crate::query::PauseInfoResponse)] - PauseInfo {}, - /// Gets the contract's voting module. - #[returns(cosmwasm_std::Addr)] - VotingModule {}, - /// Returns all SubDAOs with their charters in a vec. - /// start_after is bound exclusive and asks for a string address. - #[returns(Vec)] - ListSubDaos { - start_after: Option, - limit: Option, - }, - /// Implements the DAO Star standard: - #[returns(crate::query::DaoURIResponse)] - DaoURI {}, - /// Returns the voting power for an address at a given height. - #[returns(dao_interface::voting::VotingPowerAtHeightResponse)] - VotingPowerAtHeight { - address: String, - height: Option, - }, - /// Returns the total voting power at a given block height. - #[returns(dao_interface::voting::TotalPowerAtHeightResponse)] - TotalPowerAtHeight { height: Option }, -} - -#[allow(clippy::large_enum_variant)] -#[cw_serde] -pub enum MigrateMsg { - FromV1 { - dao_uri: Option, - params: Option, - }, - FromCompatible {}, -} diff --git a/contracts/dao-dao-core/src/tests.rs b/contracts/dao-dao-core/src/tests.rs index dac148175..e390d93f6 100644 --- a/contracts/dao-dao-core/src/tests.rs +++ b/contracts/dao-dao-core/src/tests.rs @@ -1044,7 +1044,7 @@ fn test_admin_permissions() { ); res.unwrap_err(); - // Proposal mdoule can't call ExecuteAdminMsgs + // Proposal module can't call ExecuteAdminMsgs let res = app.execute_contract( proposal_module.address.clone(), core_addr.clone(), @@ -1117,7 +1117,7 @@ fn test_admin_permissions() { ); res.unwrap_err(); - // Admin can call ExecuteAdminMsgs, here an admin pasues the DAO + // Admin can call ExecuteAdminMsgs, here an admin pauses the DAO let res = app.execute_contract( Addr::unchecked("admin"), core_with_admin_addr.clone(), @@ -1150,6 +1150,52 @@ fn test_admin_permissions() { // DAO unpauses after 10 blocks app.update_block(|block| block.height += 11); + // Admin can directly pause the DAO + let res = app.execute_contract( + Addr::unchecked("admin"), + core_with_admin_addr.clone(), + &ExecuteMsg::Pause { + duration: Duration::Height(10), + }, + &[], + ); + res.unwrap(); + + let paused: PauseInfoResponse = app + .wrap() + .query_wasm_smart(core_with_admin_addr.clone(), &QueryMsg::PauseInfo {}) + .unwrap(); + assert_eq!( + paused, + PauseInfoResponse::Paused { + expiration: Expiration::AtHeight(start_height + 21) + } + ); + + // Admin can unpause the DAO + let res = app.execute_contract( + Addr::unchecked("admin"), + core_with_admin_addr.clone(), + &ExecuteMsg::Unpause {}, + &[], + ); + res.unwrap(); + + let paused: PauseInfoResponse = app + .wrap() + .query_wasm_smart(core_with_admin_addr.clone(), &QueryMsg::PauseInfo {}) + .unwrap(); + assert_eq!(paused, PauseInfoResponse::Unpaused {}); + + // DAO cannot unpause itself + let res = app.execute_contract( + core_with_admin_addr.clone(), + core_with_admin_addr.clone(), + &ExecuteMsg::Unpause {}, + &[], + ); + assert!(res.is_err()); + // Admin can nominate a new admin. let res = app.execute_contract( Addr::unchecked("admin"), diff --git a/packages/dao-interface/src/msg.rs b/packages/dao-interface/src/msg.rs index 797865c2e..969288433 100644 --- a/packages/dao-interface/src/msg.rs +++ b/packages/dao-interface/src/msg.rs @@ -63,6 +63,8 @@ pub enum ExecuteMsg { /// Pauses the DAO for a set duration. /// When paused the DAO is unable to execute proposals Pause { duration: Duration }, + /// Unpauses the DAO + Unpause {}, /// Executed when the contract receives a cw20 token. Depending on /// the contract's configuration the contract will automatically /// add the token to its treasury. From 0b36544ddce1bf3a7291731fb9d2a526cddc3473 Mon Sep 17 00:00:00 2001 From: Gabriel Lopez Date: Fri, 8 Sep 2023 00:16:27 -0500 Subject: [PATCH 02/12] Do not explicitly exclude core contract for unpause The only way the DAO can unpause is through an admin that is not itself --- contracts/dao-dao-core/src/contract.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/dao-dao-core/src/contract.rs b/contracts/dao-dao-core/src/contract.rs index 225676432..5f657f3ad 100644 --- a/contracts/dao-dao-core/src/contract.rs +++ b/contracts/dao-dao-core/src/contract.rs @@ -128,7 +128,7 @@ pub fn execute( execute_proposal_hook(deps.as_ref(), info.sender, msgs) } ExecuteMsg::Pause { duration } => execute_pause(deps, env, info.sender, duration), - ExecuteMsg::Unpause {} => execute_unpause(deps, env, info.sender), + ExecuteMsg::Unpause {} => execute_unpause(deps, info.sender), ExecuteMsg::Receive(_) => execute_receive_cw20(deps, info.sender), ExecuteMsg::ReceiveNft(_) => execute_receive_cw721(deps, info.sender), ExecuteMsg::RemoveItem { key } => execute_remove_item(deps, env, info.sender, key), @@ -184,11 +184,11 @@ pub fn execute_pause( .add_attribute("until", until.to_string())) } -pub fn execute_unpause(deps: DepsMut, env: Env, sender: Addr) -> Result { +pub fn execute_unpause(deps: DepsMut, sender: Addr) -> Result { let admin = ADMIN.load(deps.storage)?; - // Only the admin may call this method excluding the core contract. - if sender != admin || sender == env.contract.address { + // Only the admin may call this method. + if sender != admin { return Err(ContractError::Unauthorized {}); } From 5a4aa71b3ac833fcb870de45c25831624e54f39b Mon Sep 17 00:00:00 2001 From: Gabriel Lopez Date: Sun, 10 Sep 2023 22:01:20 -0500 Subject: [PATCH 03/12] Update tests.rs --- contracts/dao-dao-core/src/tests.rs | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/contracts/dao-dao-core/src/tests.rs b/contracts/dao-dao-core/src/tests.rs index e390d93f6..b8312522b 100644 --- a/contracts/dao-dao-core/src/tests.rs +++ b/contracts/dao-dao-core/src/tests.rs @@ -1172,6 +1172,26 @@ fn test_admin_permissions() { } ); + // DAO cannot unpause itself + let res = app.execute_contract( + core_with_admin_addr.clone(), + core_with_admin_addr.clone(), + &ExecuteMsg::Unpause {}, + &[], + ); + assert!(res.is_err()); + + // Random person cannot pause the DAO + let res = app.execute_contract( + Addr::unchecked("random"), + core_with_admin_addr.clone(), + &ExecuteMsg::Pause { + duration: Duration::Height(10), + }, + &[], + ); + assert!(res.is_err()); + // Admin can unpause the DAO let res = app.execute_contract( Addr::unchecked("admin"), @@ -1187,15 +1207,6 @@ fn test_admin_permissions() { .unwrap(); assert_eq!(paused, PauseInfoResponse::Unpaused {}); - // DAO cannot unpause itself - let res = app.execute_contract( - core_with_admin_addr.clone(), - core_with_admin_addr.clone(), - &ExecuteMsg::Unpause {}, - &[], - ); - assert!(res.is_err()); - // Admin can nominate a new admin. let res = app.execute_contract( Addr::unchecked("admin"), From 106a791e6ae4620198c0f93066e102f069124870 Mon Sep 17 00:00:00 2001 From: Gabriel Lopez Date: Thu, 1 Feb 2024 09:49:36 -0600 Subject: [PATCH 04/12] Cargo fmt --- ci/integration-tests/src/helpers/helper.rs | 17 +++++++++-------- .../cw-vesting/src/suite_tests/tests.rs | 11 +++++++++-- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/ci/integration-tests/src/helpers/helper.rs b/ci/integration-tests/src/helpers/helper.rs index 3c685c22d..4d30c1670 100644 --- a/ci/integration-tests/src/helpers/helper.rs +++ b/ci/integration-tests/src/helpers/helper.rs @@ -118,15 +118,16 @@ pub fn create_dao( .unwrap(); let ProposalCreationPolicy::Module { addr: pre_propose } = chain - .orc - .query( - "dao_proposal_single", - &dao_proposal_single::msg::QueryMsg::ProposalCreationPolicy {} - ).unwrap() - .data() - .unwrap() + .orc + .query( + "dao_proposal_single", + &dao_proposal_single::msg::QueryMsg::ProposalCreationPolicy {}, + ) + .unwrap() + .data() + .unwrap() else { - panic!("expected pre-propose module") + panic!("expected pre-propose module") }; chain .orc diff --git a/contracts/external/cw-vesting/src/suite_tests/tests.rs b/contracts/external/cw-vesting/src/suite_tests/tests.rs index f93656c64..2738a3539 100644 --- a/contracts/external/cw-vesting/src/suite_tests/tests.rs +++ b/contracts/external/cw-vesting/src/suite_tests/tests.rs @@ -376,7 +376,12 @@ fn test_slash_while_cancelled_counts_against_owner() { assert_eq!(balance, distributable); let vest = suite.query_vest(); - let Status::Canceled { owner_withdrawable: pre_slash } = vest.status else { panic!("should be canceled") }; + let Status::Canceled { + owner_withdrawable: pre_slash, + } = vest.status + else { + panic!("should be canceled") + }; // register the slash. even though the time of the slash was // during the vest, the contract should deduct this from @@ -390,7 +395,9 @@ fn test_slash_while_cancelled_counts_against_owner() { .unwrap(); let vest = suite.query_vest(); - let Status::Canceled { owner_withdrawable } = vest.status else { panic!("should be canceled") }; + let Status::Canceled { owner_withdrawable } = vest.status else { + panic!("should be canceled") + }; assert_eq!(pre_slash - Uint128::new(10_000_000), owner_withdrawable); } From e918bf489c6db56efc60e70a25fca056110f4370 Mon Sep 17 00:00:00 2001 From: Gabriel Lopez Date: Thu, 1 Feb 2024 09:52:06 -0600 Subject: [PATCH 05/12] Random person cannot unpause dao test --- contracts/dao-dao-core/src/tests.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/contracts/dao-dao-core/src/tests.rs b/contracts/dao-dao-core/src/tests.rs index b8312522b..13bfc2dd9 100644 --- a/contracts/dao-dao-core/src/tests.rs +++ b/contracts/dao-dao-core/src/tests.rs @@ -1192,6 +1192,15 @@ fn test_admin_permissions() { ); assert!(res.is_err()); + // Random person cannot unpause the DAO + let res = app.execute_contract( + Addr::unchecked("random"), + core_with_admin_addr.clone(), + &ExecuteMsg::Unpause {}, + &[], + ); + assert!(res.is_err()); + // Admin can unpause the DAO let res = app.execute_contract( Addr::unchecked("admin"), From 862886029697ed7e1d9bf6f5e053a02bbfaf166b Mon Sep 17 00:00:00 2001 From: Gabriel Lopez Date: Thu, 1 Feb 2024 11:36:32 -0600 Subject: [PATCH 06/12] Make pause follow same authorization flow --- contracts/dao-dao-core/src/contract.rs | 10 ++--- contracts/dao-dao-core/src/tests.rs | 54 +++++++++++++++++--------- 2 files changed, 39 insertions(+), 25 deletions(-) diff --git a/contracts/dao-dao-core/src/contract.rs b/contracts/dao-dao-core/src/contract.rs index 5f657f3ad..85ae69200 100644 --- a/contracts/dao-dao-core/src/contract.rs +++ b/contracts/dao-dao-core/src/contract.rs @@ -106,12 +106,13 @@ pub fn execute( info: MessageInfo, msg: ExecuteMsg, ) -> Result { - // No actions can be performed while the DAO is paused. + // No actions can be performed while the DAO is paused except for unpause. match &msg { &ExecuteMsg::Unpause {} => { // Allow the unpause action to pass through } _ => { + // Check if the DAO is currently paused if let Some(expiration) = PAUSED.may_load(deps.storage)? { if !expiration.is_expired(&env.block) { return Err(ContractError::Paused {}); @@ -167,10 +168,7 @@ pub fn execute_pause( sender: Addr, pause_duration: Duration, ) -> Result { - let admin = ADMIN.load(deps.storage)?; - - // Only the core contract or admin may call this method. - if sender != env.contract.address && sender != admin { + if sender != env.contract.address { return Err(ContractError::Unauthorized {}); } @@ -187,7 +185,7 @@ pub fn execute_pause( pub fn execute_unpause(deps: DepsMut, sender: Addr) -> Result { let admin = ADMIN.load(deps.storage)?; - // Only the admin may call this method. + // Only the admin can unpause if sender != admin { return Err(ContractError::Unauthorized {}); } diff --git a/contracts/dao-dao-core/src/tests.rs b/contracts/dao-dao-core/src/tests.rs index 13bfc2dd9..3bec1419d 100644 --- a/contracts/dao-dao-core/src/tests.rs +++ b/contracts/dao-dao-core/src/tests.rs @@ -1117,6 +1117,17 @@ fn test_admin_permissions() { ); res.unwrap_err(); + // Admin cannot directly pause the DAO + let res = app.execute_contract( + Addr::unchecked("admin"), + core_with_admin_addr.clone(), + &ExecuteMsg::Pause { + duration: Duration::Height(10), + }, + &[], + ); + assert!(res.is_err()); + // Admin can call ExecuteAdminMsgs, here an admin pauses the DAO let res = app.execute_contract( Addr::unchecked("admin"), @@ -1134,7 +1145,7 @@ fn test_admin_permissions() { }, &[], ); - res.unwrap(); + assert!(res.is_ok()); let paused: PauseInfoResponse = app .wrap() @@ -1150,29 +1161,33 @@ fn test_admin_permissions() { // DAO unpauses after 10 blocks app.update_block(|block| block.height += 11); - // Admin can directly pause the DAO + // Check we are unpaused + let paused: PauseInfoResponse = app + .wrap() + .query_wasm_smart(core_with_admin_addr.clone(), &QueryMsg::PauseInfo {}) + .unwrap(); + assert_eq!(paused, PauseInfoResponse::Unpaused {}); + + // Admin pauses DAO again let res = app.execute_contract( Addr::unchecked("admin"), core_with_admin_addr.clone(), - &ExecuteMsg::Pause { - duration: Duration::Height(10), + &ExecuteMsg::ExecuteAdminMsgs { + msgs: vec![WasmMsg::Execute { + contract_addr: core_with_admin_addr.to_string(), + msg: to_binary(&ExecuteMsg::Pause { + duration: Duration::Height(10), + }) + .unwrap(), + funds: vec![], + } + .into()], }, &[], ); - res.unwrap(); - - let paused: PauseInfoResponse = app - .wrap() - .query_wasm_smart(core_with_admin_addr.clone(), &QueryMsg::PauseInfo {}) - .unwrap(); - assert_eq!( - paused, - PauseInfoResponse::Paused { - expiration: Expiration::AtHeight(start_height + 21) - } - ); + assert!(res.is_ok()); - // DAO cannot unpause itself + // DAO with admin cannot unpause itself let res = app.execute_contract( core_with_admin_addr.clone(), core_with_admin_addr.clone(), @@ -1201,15 +1216,16 @@ fn test_admin_permissions() { ); assert!(res.is_err()); - // Admin can unpause the DAO + // Admin can unpause the DAO directly let res = app.execute_contract( Addr::unchecked("admin"), core_with_admin_addr.clone(), &ExecuteMsg::Unpause {}, &[], ); - res.unwrap(); + assert!(res.is_ok()); + // Check we are unpaused let paused: PauseInfoResponse = app .wrap() .query_wasm_smart(core_with_admin_addr.clone(), &QueryMsg::PauseInfo {}) From c7217cf263fbad1623cf81a73c554ebdd13353eb Mon Sep 17 00:00:00 2001 From: Gabriel Lopez Date: Mon, 5 Feb 2024 23:33:14 -0600 Subject: [PATCH 07/12] Only check pause logic in execute_proposal_hook --- contracts/dao-dao-core/src/contract.rs | 25 ++++------- contracts/dao-dao-core/src/tests.rs | 59 ++++++++++++-------------- 2 files changed, 37 insertions(+), 47 deletions(-) diff --git a/contracts/dao-dao-core/src/contract.rs b/contracts/dao-dao-core/src/contract.rs index 85ae69200..23cfdcc5a 100644 --- a/contracts/dao-dao-core/src/contract.rs +++ b/contracts/dao-dao-core/src/contract.rs @@ -106,27 +106,12 @@ pub fn execute( info: MessageInfo, msg: ExecuteMsg, ) -> Result { - // No actions can be performed while the DAO is paused except for unpause. - match &msg { - &ExecuteMsg::Unpause {} => { - // Allow the unpause action to pass through - } - _ => { - // Check if the DAO is currently paused - if let Some(expiration) = PAUSED.may_load(deps.storage)? { - if !expiration.is_expired(&env.block) { - return Err(ContractError::Paused {}); - } - } - } - } - match msg { ExecuteMsg::ExecuteAdminMsgs { msgs } => { execute_admin_msgs(deps.as_ref(), info.sender, msgs) } ExecuteMsg::ExecuteProposalHook { msgs } => { - execute_proposal_hook(deps.as_ref(), info.sender, msgs) + execute_proposal_hook(deps.as_ref(), env, info.sender, msgs) } ExecuteMsg::Pause { duration } => execute_pause(deps, env, info.sender, duration), ExecuteMsg::Unpause {} => execute_unpause(deps, info.sender), @@ -168,6 +153,7 @@ pub fn execute_pause( sender: Addr, pause_duration: Duration, ) -> Result { + // Only the core contract may call this method. if sender != env.contract.address { return Err(ContractError::Unauthorized {}); } @@ -216,6 +202,7 @@ pub fn execute_admin_msgs( pub fn execute_proposal_hook( deps: Deps, + env: Env, sender: Addr, msgs: Vec>, ) -> Result { @@ -228,6 +215,12 @@ pub fn execute_proposal_hook( return Err(ContractError::ModuleDisabledCannotExecute { address: sender }); } + if let Some(expiration) = PAUSED.may_load(deps.storage)? { + if !expiration.is_expired(&env.block) { + return Err(ContractError::Paused {}); + } + } + Ok(Response::default() .add_attribute("action", "execute_proposal_hook") .add_messages(msgs)) diff --git a/contracts/dao-dao-core/src/tests.rs b/contracts/dao-dao-core/src/tests.rs index 3bec1419d..a2e88caea 100644 --- a/contracts/dao-dao-core/src/tests.rs +++ b/contracts/dao-dao-core/src/tests.rs @@ -1128,6 +1128,17 @@ fn test_admin_permissions() { ); assert!(res.is_err()); + // Random person cannot pause the DAO + let res = app.execute_contract( + Addr::unchecked("random"), + core_with_admin_addr.clone(), + &ExecuteMsg::Pause { + duration: Duration::Height(10), + }, + &[], + ); + assert!(res.is_err()); + // Admin can call ExecuteAdminMsgs, here an admin pauses the DAO let res = app.execute_contract( Addr::unchecked("admin"), @@ -1147,6 +1158,7 @@ fn test_admin_permissions() { ); assert!(res.is_ok()); + // Ensure we are paused for 10 blocks let paused: PauseInfoResponse = app .wrap() .query_wasm_smart(core_with_admin_addr.clone(), &QueryMsg::PauseInfo {}) @@ -1196,17 +1208,6 @@ fn test_admin_permissions() { ); assert!(res.is_err()); - // Random person cannot pause the DAO - let res = app.execute_contract( - Addr::unchecked("random"), - core_with_admin_addr.clone(), - &ExecuteMsg::Pause { - duration: Duration::Height(10), - }, - &[], - ); - assert!(res.is_err()); - // Random person cannot unpause the DAO let res = app.execute_contract( Addr::unchecked("random"), @@ -2465,27 +2466,23 @@ fn test_pause() { } ); - let err: ContractError = app - .execute_contract( - core_addr.clone(), - core_addr.clone(), - &ExecuteMsg::UpdateConfig { - config: Config { - dao_uri: None, - name: "The Empire Strikes Back Again".to_string(), - description: "haha lol we have pwned your DAO again".to_string(), - image_url: None, - automatically_add_cw20s: true, - automatically_add_cw721s: true, - }, + // This should actually be allowed to enable the admin to execute + let result = app.execute_contract( + core_addr.clone(), + core_addr.clone(), + &ExecuteMsg::UpdateConfig { + config: Config { + dao_uri: None, + name: "The Empire Strikes Back Again".to_string(), + description: "haha lol we have pwned your DAO again".to_string(), + image_url: None, + automatically_add_cw20s: true, + automatically_add_cw721s: true, }, - &[], - ) - .unwrap_err() - .downcast() - .unwrap(); - - assert!(matches!(err, ContractError::Paused { .. })); + }, + &[], + ); + assert!(result.is_ok()); let err: ContractError = app .execute_contract( From 2ba3817002948ca960b0b307752ad129fe05fa0c Mon Sep 17 00:00:00 2001 From: Gabriel Lopez Date: Wed, 7 Feb 2024 15:10:02 -0600 Subject: [PATCH 08/12] Add a helpful comment for paused check --- contracts/dao-dao-core/src/contract.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/dao-dao-core/src/contract.rs b/contracts/dao-dao-core/src/contract.rs index 23cfdcc5a..bc49bde4c 100644 --- a/contracts/dao-dao-core/src/contract.rs +++ b/contracts/dao-dao-core/src/contract.rs @@ -215,6 +215,7 @@ pub fn execute_proposal_hook( return Err(ContractError::ModuleDisabledCannotExecute { address: sender }); } + // If the DAO is paused, then proposal modules cannot execute messages until expiration if let Some(expiration) = PAUSED.may_load(deps.storage)? { if !expiration.is_expired(&env.block) { return Err(ContractError::Paused {}); From 54376180b1a648c7e375c4bbc01749fdf80c51e5 Mon Sep 17 00:00:00 2001 From: Gabriel Lopez Date: Fri, 15 Mar 2024 21:49:53 -0500 Subject: [PATCH 09/12] Move pause logic back up --- contracts/dao-dao-core/src/contract.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/contracts/dao-dao-core/src/contract.rs b/contracts/dao-dao-core/src/contract.rs index bc49bde4c..62136d1ae 100644 --- a/contracts/dao-dao-core/src/contract.rs +++ b/contracts/dao-dao-core/src/contract.rs @@ -106,12 +106,24 @@ pub fn execute( info: MessageInfo, msg: ExecuteMsg, ) -> Result { + // Check if the DAO is paused + if let Some(expiration) = PAUSED.may_load(deps.storage)? { + if !expiration.is_expired(&env.block) { + // If paused, then only allow messages from the Admin or DAO itself + if info.sender != env.contract.address + && Some(info.sender.clone()) != ADMIN.may_load(deps.storage)? + { + return Err(ContractError::Paused {}); + } + } + } + match msg { ExecuteMsg::ExecuteAdminMsgs { msgs } => { execute_admin_msgs(deps.as_ref(), info.sender, msgs) } ExecuteMsg::ExecuteProposalHook { msgs } => { - execute_proposal_hook(deps.as_ref(), env, info.sender, msgs) + execute_proposal_hook(deps.as_ref(), info.sender, msgs) } ExecuteMsg::Pause { duration } => execute_pause(deps, env, info.sender, duration), ExecuteMsg::Unpause {} => execute_unpause(deps, info.sender), @@ -202,7 +214,6 @@ pub fn execute_admin_msgs( pub fn execute_proposal_hook( deps: Deps, - env: Env, sender: Addr, msgs: Vec>, ) -> Result { @@ -215,13 +226,6 @@ pub fn execute_proposal_hook( return Err(ContractError::ModuleDisabledCannotExecute { address: sender }); } - // If the DAO is paused, then proposal modules cannot execute messages until expiration - if let Some(expiration) = PAUSED.may_load(deps.storage)? { - if !expiration.is_expired(&env.block) { - return Err(ContractError::Paused {}); - } - } - Ok(Response::default() .add_attribute("action", "execute_proposal_hook") .add_messages(msgs)) From 7434820cb8bb7bbc7cda49d98152888ac5bee01a Mon Sep 17 00:00:00 2001 From: Gabriel Lopez Date: Mon, 18 Mar 2024 11:59:33 -0500 Subject: [PATCH 10/12] Update tests.rs --- contracts/dao-dao-core/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/dao-dao-core/src/tests.rs b/contracts/dao-dao-core/src/tests.rs index cd9405f66..81dd040cb 100644 --- a/contracts/dao-dao-core/src/tests.rs +++ b/contracts/dao-dao-core/src/tests.rs @@ -1215,7 +1215,7 @@ fn test_admin_permissions() { &ExecuteMsg::ExecuteAdminMsgs { msgs: vec![WasmMsg::Execute { contract_addr: core_with_admin_addr.to_string(), - msg: to_binary(&ExecuteMsg::Pause { + msg: to_json_binary(&ExecuteMsg::Pause { duration: Duration::Height(10), }) .unwrap(), From 1c034cc63a883721bf9c422c6028785521082bb1 Mon Sep 17 00:00:00 2001 From: ismellike Date: Thu, 21 Mar 2024 21:59:53 -0500 Subject: [PATCH 11/12] Update contracts/dao-dao-core/src/contract.rs Co-authored-by: noah --- contracts/dao-dao-core/src/contract.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/dao-dao-core/src/contract.rs b/contracts/dao-dao-core/src/contract.rs index e3c0d30f1..1ee52a165 100644 --- a/contracts/dao-dao-core/src/contract.rs +++ b/contracts/dao-dao-core/src/contract.rs @@ -111,7 +111,7 @@ pub fn execute( if !expiration.is_expired(&env.block) { // If paused, then only allow messages from the Admin or DAO itself if info.sender != env.contract.address - && Some(info.sender.clone()) != ADMIN.may_load(deps.storage)? + && info.sender.clone() != ADMIN.load(deps.storage)? { return Err(ContractError::Paused {}); } From ba3e86faef060f4f040c514c96efb21a101398c4 Mon Sep 17 00:00:00 2001 From: Gabriel Lopez Date: Fri, 22 Mar 2024 22:59:26 -0500 Subject: [PATCH 12/12] Update dao-dao-core.json --- contracts/dao-dao-core/schema/dao-dao-core.json | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/contracts/dao-dao-core/schema/dao-dao-core.json b/contracts/dao-dao-core/schema/dao-dao-core.json index 5b504f602..80991cef8 100644 --- a/contracts/dao-dao-core/schema/dao-dao-core.json +++ b/contracts/dao-dao-core/schema/dao-dao-core.json @@ -290,6 +290,20 @@ }, "additionalProperties": false }, + { + "description": "Unpauses the DAO", + "type": "object", + "required": [ + "unpause" + ], + "properties": { + "unpause": { + "type": "object", + "additionalProperties": false + } + }, + "additionalProperties": false + }, { "description": "Executed when the contract receives a cw20 token. Depending on the contract's configuration the contract will automatically add the token to its treasury.", "type": "object",