Merge remote-tracking branch 'origin/audit-fixes/root' into audit-fix…

…es/12-miscellaneous

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/error.rs

@@ -42,6 +42,9 @@ pub enum AccountError {
     #[error("The provided module {0} can't be installed on an Abstract account")]
     ModuleNotInstallable(String),
 
+    #[error("The module {0} needs an init msg to be installed but not was provided")]
+    InitMsgMissing(String),
+
     #[error("The name of the proposed module can not have length 0.")]
     InvalidModuleName {},
 
@@ -78,8 +81,11 @@ pub enum AccountError {
     NotWhitelisted {},
 
     // ** Sub Account ** //
-    #[error("Removing sub account failed")]
-    SubAccountRemovalFailed {},
+    #[error("Sub account doesn't exist")]
+    SubAccountDoesntExist {},
+
+    #[error("Sub account is expected to be the caller")]
+    SubAccountIsNotCaller {},
 
     #[error("Register of sub account failed")]
     SubAccountRegisterFailed {},
@@ -106,13 +112,19 @@ pub enum AccountError {
     #[error("The caller ({caller}) is not the owner account's account ({account}). Only account can create sub-accounts for itself.", )]
     SubAccountCreatorNotAccount { caller: String, account: String },
 
+    #[error("You can't chain admin calls")]
+    CantChainAdminCalls {},
+
     #[error("Abstract Account Address ({abstract_account}) doesn't match to the Contract address ({contract})   ")]
     AbstractAccountInvalidAddress {
         abstract_account: String,
         contract: String,
     },
 
-    #[error("Abstract Account doesn't have Authentication")]
+    #[error("No auth methods capabilities on this account (xion feature disabled)")]
+    NoAuthMethods {},
+
+    #[error("Abstract Account don't have Authentication")]
     AbstractAccountNoAuth {},
 
     #[cfg(feature = "xion")]

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(),
@@ -92,6 +91,9 @@ pub fn admin_execute(
 ) -> AccountResult {
     ownership::assert_nested_owner(deps.storage, &deps.querier, &info.sender)?;
 
+    if CALLING_TO_AS_ADMIN.exists(deps.storage) {
+        return Err(AccountError::CantChainAdminCalls {});
+    }
     CALLING_TO_AS_ADMIN.save(deps.storage, &addr)?;
 
     let msg = SubMsg::reply_on_success(
@@ -119,26 +121,29 @@ 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"))]
     {
-        Ok(AccountResponse::action("add_auth"))
+        Err(AccountError::NoAuthMethods {})
     }
 }
 
-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"))]
     {
-        Ok(AccountResponse::action("remove_auth"))
+        Err(AccountError::NoAuthMethods {})
     }
 }
 
@@ -149,8 +154,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,17 +186,113 @@ 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 {
 
-        use cosmwasm_std::{testing::MOCK_CONTRACT_ADDR, wasm_execute, CosmosMsg};
+        use abstract_sdk::namespaces::OWNERSHIP_STORAGE_KEY;
+        use abstract_std::objects::{gov_type::GovernanceDetails, ownership::Ownership};
+        use cosmwasm_std::{
+            testing::MOCK_CONTRACT_ADDR, wasm_execute, Addr, Binary, CosmosMsg, DepsMut, Empty,
+            Env, Response, WasmMsg,
+        };
+        use cw_storage_plus::Item;
+
+        use crate::contract::AccountResult;
 
         use super::*;
+        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");
+            }
+        }
+
+        #[coverage_helper::test]
+        fn admin_actions_not_chained() -> 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 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)?;
+
+            let msg = ExecuteMsg::AdminExecute {
+                addr: env.contract.address.to_string(),
+                msg: to_json_binary(&ExecuteMsg::<Empty>::AdminExecute {
+                    addr: env.contract.address.to_string(),
+                    msg: Binary::new(vec![]),
+                })?,
+            };
+
+            let info = message_info(&env.contract.address, &[]);
+            let env = mock_env_validated(deps.api);
+
+            let res = execute(deps.as_mut(), env.clone(), info, msg).unwrap();
+            // We simulate it's still an admin call
+            AUTH_ADMIN.save(deps.as_mut().storage, &true)?;
+            let res = execute_from_res(deps.as_mut(), env, res);
+            assert_eq!(res, Err(AccountError::CantChainAdminCalls {}));
+            Ok(())
+        }
 
         #[coverage_helper::test]
         fn only_whitelisted_can_execute() -> anyhow::Result<()> {