Audit Fixes 1: XION Authentication fix and test (#546)

Co-authored-by: Interchain Adair <32375605+adairrr@users.noreply.github.com>

Author: Kayanski

framework/contracts/account/src/contract.rs

@@ -269,24 +269,24 @@ pub fn execute(mut deps: DepsMut, env: Env, info: MessageInfo, msg: ExecuteMsg)
 
             match msg {
                 // ## Execution ##
-                ExecuteMsg::Execute { msgs } => execute_msgs(deps, &info.sender, msgs),
+                ExecuteMsg::Execute { msgs } => execute_msgs(deps, env, &info.sender, msgs),
                 ExecuteMsg::AdminExecute { addr, msg } => {
                     let addr = deps.api.addr_validate(&addr)?;
                     admin_execute(deps, info, addr, msg)
                 }
                 ExecuteMsg::ExecuteWithData { msg } => {
-                    execute_msgs_with_data(deps, &info.sender, msg)
+                    execute_msgs_with_data(deps, env, &info.sender, msg)
                 }
                 ExecuteMsg::ExecuteOnModule {
                     module_id,
                     exec_msg,
                     funds,
-                } => execute_on_module(deps, info, module_id, exec_msg, funds),
+                } => execute_on_module(deps, env, info, module_id, exec_msg, funds),
                 ExecuteMsg::AdminExecuteOnModule { module_id, msg } => {
                     admin_execute_on_module(deps, info, module_id, msg)
                 }
                 ExecuteMsg::IcaAction { action_query_msg } => {
-                    ica_action(deps, info, action_query_msg)
+                    ica_action(deps, env, info, action_query_msg)
                 }
 
                 // ## Configuration ##
@@ -354,10 +354,9 @@ pub fn execute(mut deps: DepsMut, env: Env, info: MessageInfo, msg: ExecuteMsg)
                     unreachable!("Update status case is reached above")
                 }
                 ExecuteMsg::AddAuthMethod { add_authenticator } => {
-                    add_auth_method(deps, env, add_authenticator)
+                    add_auth_method(deps, env, info, add_authenticator)
                 }
-                #[allow(unused)]
-                ExecuteMsg::RemoveAuthMethod { id } => remove_auth_method(deps, env, id),
+                ExecuteMsg::RemoveAuthMethod { id } => remove_auth_method(deps, env, info, id),
             }
         }
     }?;

framework/contracts/account/src/execution.rs

@@ -16,20 +16,15 @@ use crate::{
 };
 
 /// Check that sender either whitelisted or governance
-pub(crate) fn assert_whitelisted_or_owner(deps: &mut DepsMut, sender: &Addr) -> AccountResult<()> {
-    #[cfg(feature = "xion")]
-    {
-        if let Some(is_admin) = crate::state::AUTH_ADMIN.may_load(deps.storage)? {
-            // Clear auth if it was set
-            crate::state::AUTH_ADMIN.remove(deps.storage);
-            if is_admin {
-                return Ok(());
-            }
-        }
-    }
+pub(crate) fn assert_whitelisted_owner_or_self(
+    deps: &mut DepsMut,
+    env: &Env,
+    sender: &Addr,
+) -> AccountResult<()> {
     let whitelisted_modules = WHITELISTED_MODULES.load(deps.storage)?;
     if whitelisted_modules.0.contains(sender)
         || ownership::assert_nested_owner(deps.storage, &deps.querier, sender).is_ok()
+        || sender == env.contract.address
     {
         Ok(())
     } else {
@@ -41,10 +36,11 @@ pub(crate) fn assert_whitelisted_or_owner(deps: &mut DepsMut, sender: &Addr) ->
 /// Permission: Module
 pub fn execute_msgs(
     mut deps: DepsMut,
+    env: Env,
     msg_sender: &Addr,
     msgs: Vec<CosmosMsg<Empty>>,
 ) -> AccountResult {
-    assert_whitelisted_or_owner(&mut deps, msg_sender)?;
+    assert_whitelisted_owner_or_self(&mut deps, &env, msg_sender)?;
 
     Ok(AccountResponse::action("execute_module_action").add_messages(msgs))
 }
@@ -53,10 +49,11 @@ pub fn execute_msgs(
 /// Permission: Module
 pub fn execute_msgs_with_data(
     mut deps: DepsMut,
+    env: Env,
     msg_sender: &Addr,
     msg: CosmosMsg<Empty>,
 ) -> AccountResult {
-    assert_whitelisted_or_owner(&mut deps, msg_sender)?;
+    assert_whitelisted_owner_or_self(&mut deps, &env, msg_sender)?;
 
     let submsg = SubMsg::reply_on_success(msg, FORWARD_RESPONSE_REPLY_ID);
 
@@ -67,6 +64,7 @@ pub fn execute_msgs_with_data(
 /// This is a simple wrapper around [`ExecuteMsg::Execute`](abstract_std::account::ExecuteMsg::Execute).
 pub fn execute_on_module(
     deps: DepsMut,
+    env: Env,
     info: MessageInfo,
     module_id: String,
     exec_msg: Binary,
@@ -75,6 +73,7 @@ pub fn execute_on_module(
     let module_addr = load_module_addr(deps.storage, &module_id)?;
     execute_msgs(
         deps,
+        env,
         &info.sender,
         vec![CosmosMsg::Wasm(WasmMsg::Execute {
             contract_addr: module_addr.into(),
@@ -119,10 +118,12 @@ pub fn admin_execute_on_module(
 pub fn add_auth_method(
     _deps: DepsMut,
     _env: Env,
+    _info: MessageInfo,
     #[allow(unused_mut)] mut _auth: crate::msg::Authenticator,
 ) -> AccountResult {
     #[cfg(feature = "xion")]
     {
+        ownership::assert_nested_owner(_deps.storage, &_deps.querier, &_info.sender)?;
         abstract_xion::execute::add_auth_method(_deps, &_env, &mut _auth).map_err(Into::into)
     }
     #[cfg(not(feature = "xion"))]
@@ -131,9 +132,10 @@ pub fn add_auth_method(
     }
 }
 
-pub fn remove_auth_method(_deps: DepsMut, _env: Env, _id: u8) -> AccountResult {
+pub fn remove_auth_method(_deps: DepsMut, _env: Env, _info: MessageInfo, _id: u8) -> AccountResult {
     #[cfg(feature = "xion")]
     {
+        ownership::assert_nested_owner(_deps.storage, &_deps.querier, &_info.sender)?;
         abstract_xion::execute::remove_auth_method(_deps, _env, _id).map_err(Into::into)
     }
     #[cfg(not(feature = "xion"))]
@@ -149,8 +151,13 @@ pub fn remove_auth_method(_deps: DepsMut, _env: Env, _id: u8) -> AccountResult {
 /// It then fires a smart-query on that address of type [`QueryMsg::IcaAction`](abstract_ica::msg::QueryMsg).
 ///
 /// The resulting `Vec<CosmosMsg>` are then executed on the account contract.
-pub fn ica_action(mut deps: DepsMut, msg_info: MessageInfo, action_query: Binary) -> AccountResult {
-    assert_whitelisted_or_owner(&mut deps, &msg_info.sender)?;
+pub fn ica_action(
+    mut deps: DepsMut,
+    env: Env,
+    msg_info: MessageInfo,
+    action_query: Binary,
+) -> AccountResult {
+    assert_whitelisted_owner_or_self(&mut deps, &env, &msg_info.sender)?;
 
     let ica_client_address = ACCOUNT_MODULES
         .may_load(deps.storage, ICA_CLIENT)?
@@ -176,11 +183,45 @@ mod test {
     use crate::contract::execute;
     use crate::error::AccountError;
     use crate::test_common::mock_init;
+    use abstract_sdk::namespaces::OWNERSHIP_STORAGE_KEY;
     use abstract_std::account::{state::*, *};
+    use abstract_std::objects::gov_type::GovernanceDetails;
+    use abstract_std::objects::ownership::Ownership;
     use abstract_std::{account, IBC_CLIENT};
     use abstract_testing::prelude::*;
-    use cosmwasm_std::testing::*;
     use cosmwasm_std::{coins, CosmosMsg, SubMsg};
+    use cosmwasm_std::{testing::*, Addr};
+    use cw_storage_plus::Item;
+
+    #[coverage_helper::test]
+    fn abstract_account_can_execute_on_itself() -> anyhow::Result<()> {
+        let mut deps = mock_dependencies();
+        deps.querier = abstract_mock_querier(deps.api);
+        mock_init(&mut deps)?;
+
+        let env = mock_env_validated(deps.api);
+        // We set the contract as owner.
+        // We can't make it through execute msgs, because of XION signatures are too messy to reproduce in tests
+        let ownership = Ownership {
+            owner: GovernanceDetails::AbstractAccount {
+                address: env.contract.address.clone(),
+            }
+            .verify(deps.as_ref())?,
+            pending_owner: None,
+            pending_expiry: None,
+        };
+        const OWNERSHIP: Item<Ownership<Addr>> = Item::new(OWNERSHIP_STORAGE_KEY);
+        OWNERSHIP.save(deps.as_mut().storage, &ownership)?;
+
+        // Module calls nested admin calls on account, making it admin
+        let info = message_info(&env.contract.address, &[]);
+
+        let msg = ExecuteMsg::Execute { msgs: vec![] };
+
+        execute(deps.as_mut(), env.clone(), info, msg).unwrap();
+
+        Ok(())
+    }
 
     mod execute_action {
 

framework/contracts/account/src/modules.rs

@@ -10,7 +10,7 @@ use abstract_std::{
         module::{Module, ModuleInfo, ModuleVersion},
         module_factory::ModuleFactoryContract,
         module_reference::ModuleReference,
-        ownership::{self},
+        ownership,
         registry::RegistryContract,
         salt::generate_instantiate_salt,
         storage_namespaces,
@@ -349,7 +349,6 @@ mod tests {
     use abstract_std::objects::dependency::Dependency;
     use abstract_testing::prelude::*;
     use cosmwasm_std::{testing::*, Addr, Order, StdError, Storage};
-    use ownership::GovOwnershipError;
 
     fn load_account_modules(storage: &dyn Storage) -> Result<Vec<(String, Addr)>, StdError> {
         ACCOUNT_MODULES
@@ -384,6 +383,8 @@ mod tests {
     }
 
     mod update_module_addresses {
+        use abstract_std::objects::ownership::GovOwnershipError;
+
         use super::*;
 
         #[coverage_helper::test]

framework/contracts/account/src/modules/migration.rs

@@ -8,7 +8,7 @@ use abstract_std::{
         dependency::Dependency,
         module::ModuleInfo,
         module_reference::ModuleReference,
-        ownership::{self},
+        ownership,
         registry::{RegistryContract, RegistryError},
         storage_namespaces,
     },

framework/contracts/account/tests/xion.rs

@@ -1,15 +1,34 @@
 #![cfg(feature = "xion")]
 
+use abstract_account::contract::execute;
 use abstract_account::contract::instantiate;
+use abstract_account::contract::AccountResult;
+use abstract_account::error::AccountError;
+use abstract_account::msg::ExecuteMsg;
+use abstract_account::state::AUTH_ADMIN;
+use abstract_std::account;
 use abstract_std::account::InstantiateMsg;
+use abstract_std::account::InternalConfigAction;
+use abstract_std::objects::ownership::GovOwnershipError;
+use abstract_std::objects::ownership::GovernanceDetails;
+use abstract_std::objects::ownership::Ownership;
+use abstract_std::objects::storage_namespaces::OWNERSHIP_STORAGE_KEY;
+use abstract_std::objects::AccountId;
+use abstract_std::objects::AccountTrace;
 use abstract_std::registry::state::LOCAL_ACCOUNT_SEQUENCE;
+use abstract_testing::abstract_mock_querier;
 use abstract_testing::abstract_mock_querier_builder;
+use abstract_testing::mock_env_validated;
 use abstract_testing::prelude::AbstractMockAddrs;
+use abstract_testing::prelude::*;
 use abstract_xion::testing::util;
 use abstract_xion::testing::wrap_message;
 use base64::{engine::general_purpose, Engine as _};
-use cosmwasm_std::testing::{message_info, mock_dependencies, mock_env};
-use cosmwasm_std::{Addr, Api, Binary};
+use cosmwasm_std::testing::{message_info, mock_dependencies, mock_env, MockApi};
+use cosmwasm_std::CosmosMsg;
+use cosmwasm_std::Response;
+use cosmwasm_std::{wasm_execute, Addr, Api, Binary, DepsMut, Empty, Env, OwnedDeps, WasmMsg};
+use cw_storage_plus::Item;
 
 #[test]
 fn test_derive_addr() {
@@ -123,3 +142,116 @@ fn test_init_sign_arb() {
     )
     .unwrap();
 }
+
+/// Initialize the account with the test owner as the owner
+pub(crate) fn mock_init(
+    deps: &mut OwnedDeps<MockStorage, MockApi, MockQuerier, Empty>,
+) -> AccountResult {
+    let abstr = AbstractMockAddrs::new(deps.api);
+
+    let info = message_info(&abstr.owner, &[]);
+    let env = mock_env_validated(deps.api);
+
+    abstract_account::contract::instantiate(
+        deps.as_mut(),
+        env,
+        info,
+        account::InstantiateMsg {
+            code_id: 1,
+            account_id: Some(AccountId::new(1, AccountTrace::Local).unwrap()),
+            owner: GovernanceDetails::Monarchy {
+                monarch: abstr.owner.to_string(),
+            },
+            namespace: None,
+            name: Some("test".to_string()),
+            description: None,
+            link: None,
+            install_modules: vec![],
+            authenticator: None,
+        },
+    )
+}
+
+const OWNERSHIP: Item<Ownership<Addr>> = Item::new(OWNERSHIP_STORAGE_KEY);
+
+#[test]
+fn xion_account_auth_itself() -> anyhow::Result<()> {
+    let mut deps = mock_dependencies();
+    deps.querier = abstract_mock_querier(deps.api);
+    mock_init(&mut deps)?;
+
+    let env = mock_env_validated(deps.api);
+    // We set the contract as owner.
+    // We can't make it through execute msgs, because of XION signatures are too messy to reproduce in tests
+    let ownership = Ownership {
+        owner: GovernanceDetails::AbstractAccount {
+            address: env.contract.address.clone(),
+        }
+        .verify(deps.as_ref())?,
+        pending_owner: None,
+        pending_expiry: None,
+    };
+    OWNERSHIP.save(deps.as_mut().storage, &ownership)?;
+
+    let whitelisted = deps.api.addr_make("whitelisted");
+    let not_whitelisted_yet = deps.api.addr_make("not_whitelisted");
+
+    // We whitelist a module
+    AUTH_ADMIN.save(deps.as_mut().storage, &true)?;
+    execute(
+        deps.as_mut(),
+        env.clone(),
+        message_info(&env.contract.address, &[]),
+        ExecuteMsg::UpdateInternalConfig(InternalConfigAction::UpdateWhitelist {
+            to_add: vec![whitelisted.to_string()],
+            to_remove: vec![],
+        }),
+    )?;
+
+    // Module calls nested admin calls on account, making it admin
+    let info = message_info(&whitelisted, &[]);
+
+    let msg = ExecuteMsg::Execute {
+        msgs: vec![wasm_execute(
+            &env.contract.address,
+            &ExecuteMsg::Execute {
+                msgs: vec![wasm_execute(
+                    &env.contract.address,
+                    &ExecuteMsg::UpdateInternalConfig(InternalConfigAction::UpdateWhitelist {
+                        to_add: vec![not_whitelisted_yet.to_string()],
+                        to_remove: vec![],
+                    }),
+                    vec![],
+                )?
+                .into()],
+            },
+            vec![],
+        )?
+        .into()],
+    };
+
+    let res = execute(deps.as_mut(), env.clone(), info, msg).unwrap();
+
+    // Execute all messages
+    let res = execute_from_res(deps.as_mut(), &env, res).unwrap();
+    // This should error because this action is triggered at the top by an external module
+    let res = execute_from_res(deps.as_mut(), &env, res).unwrap_err();
+
+    assert_eq!(res, AccountError::Ownership(GovOwnershipError::NotOwner));
+    Ok(())
+}
+
+fn execute_from_res(deps: DepsMut, env: &Env, res: Response) -> AccountResult<Response> {
+    // Execute all messages
+    let info = message_info(&env.contract.address, &[]);
+    if let CosmosMsg::Wasm(WasmMsg::Execute {
+        contract_addr: _,
+        msg,
+        funds: _,
+    }) = res.messages[0].msg.clone()
+    {
+        execute(deps, env.clone(), info, from_json(&msg)?).map_err(Into::into)
+    } else {
+        panic!("Wrong message received");
+    }
+}