MP-3345. MP-3348. Don't allow red-bank users to create alternative account ids.

contracts/red-bank/src/deposit.rs

@@ -39,6 +39,13 @@ pub fn deposit(
     let params_addr = &addresses[&MarsAddressType::Params];
     let credit_manager_addr = &addresses[&MarsAddressType::CreditManager];
 
+    // Don't allow red-bank users to create alternative account ids.
+    // Only allow credit-manager contract to create them.
+    // Even if account_id contains empty string we won't allow it.
+    if account_id.is_some() && info.sender != credit_manager_addr {
+        return Err(ContractError::Mars(MarsError::Unauthorized {}));
+    }
+
     let user_addr: Addr;
     let user = match on_behalf_of.as_ref() {
         // A malicious user can permanently disable the lend action in credit-manager contract by performing the following steps:

contracts/red-bank/src/withdraw.rs

@@ -1,6 +1,6 @@
 use cosmwasm_std::{DepsMut, Env, MessageInfo, Response, Uint128};
 use mars_interest_rate::{get_scaled_liquidity_amount, get_underlying_liquidity_amount};
-use mars_red_bank_types::{address_provider, address_provider::MarsAddressType};
+use mars_red_bank_types::{address_provider, address_provider::MarsAddressType, error::MarsError};
 use mars_utils::helpers::build_send_asset_msg;
 
 use crate::{
@@ -21,6 +21,32 @@ pub fn withdraw(
     account_id: Option<String>,
     liquidation_related: bool,
 ) -> Result<Response, ContractError> {
+    let config = CONFIG.load(deps.storage)?;
+
+    let addresses = address_provider::helpers::query_contract_addrs(
+        deps.as_ref(),
+        &config.address_provider,
+        vec![
+            MarsAddressType::Oracle,
+            MarsAddressType::Incentives,
+            MarsAddressType::RewardsCollector,
+            MarsAddressType::Params,
+            MarsAddressType::CreditManager,
+        ],
+    )?;
+    let rewards_collector_addr = &addresses[&MarsAddressType::RewardsCollector];
+    let incentives_addr = &addresses[&MarsAddressType::Incentives];
+    let oracle_addr = &addresses[&MarsAddressType::Oracle];
+    let params_addr = &addresses[&MarsAddressType::Params];
+    let credit_manager_addr = &addresses[&MarsAddressType::CreditManager];
+
+    // Don't allow red-bank users to create alternative account ids.
+    // Only allow credit-manager contract to create them.
+    // Even if account_id contains empty string we won't allow it.
+    if account_id.is_some() && info.sender != credit_manager_addr {
+        return Err(ContractError::Mars(MarsError::Unauthorized {}));
+    }
+
     let withdrawer = User(&info.sender);
     let acc_id = account_id.clone().unwrap_or("".to_string());
 
@@ -54,25 +80,6 @@ pub fn withdraw(
         None => withdrawer_balance_before,
     };
 
-    let config = CONFIG.load(deps.storage)?;
-
-    let addresses = address_provider::helpers::query_contract_addrs(
-        deps.as_ref(),
-        &config.address_provider,
-        vec![
-            MarsAddressType::Oracle,
-            MarsAddressType::Incentives,
-            MarsAddressType::RewardsCollector,
-            MarsAddressType::Params,
-            MarsAddressType::CreditManager,
-        ],
-    )?;
-    let rewards_collector_addr = &addresses[&MarsAddressType::RewardsCollector];
-    let incentives_addr = &addresses[&MarsAddressType::Incentives];
-    let oracle_addr = &addresses[&MarsAddressType::Oracle];
-    let params_addr = &addresses[&MarsAddressType::Params];
-    let credit_manager_addr = &addresses[&MarsAddressType::CreditManager];
-
     // if withdraw is part of the liquidation in credit manager we need to use correct pricing for the assets
     let liquidation_related = info.sender == credit_manager_addr && liquidation_related;
 

contracts/red-bank/tests/tests/test_credit_accounts.rs

@@ -17,7 +17,7 @@ fn deposit_and_withdraw_for_credit_account_works() {
 
     let funded_amt = 1_000_000_000_000u128;
     let provider = Addr::unchecked("provider"); // provides collateral to be borrowed by others
-    let credit_manager = Addr::unchecked("credit_manager");
+    let credit_manager = mock_env.credit_manager.clone();
     let account_id = "111".to_string();
 
     // setup red-bank

contracts/red-bank/tests/tests/test_deposit.rs

@@ -598,29 +598,49 @@ fn depositing_on_behalf_of_credit_manager() {
         ..
     } = setup_test();
 
-    // disable the market
-    deps.querier.set_redbank_params(
+    let err = execute(
+        deps.as_mut(),
+        mock_env(),
+        mock_info(depositor_addr.as_str(), &coins(123, denom)),
+        ExecuteMsg::Deposit {
+            account_id: None,
+            on_behalf_of: Some("credit_manager".to_string()),
+        },
+    )
+    .unwrap_err();
+    assert_eq!(err, ContractError::Mars(MarsError::Unauthorized {}));
+}
+
+#[test]
+fn depositing_with_account_id_by_non_credit_manager_user() {
+    let TestSuite {
+        mut deps,
         denom,
-        AssetParams {
-            credit_manager: CmSettings {
-                whitelisted: false,
-                hls: None,
-            },
-            red_bank: RedBankSettings {
-                deposit_enabled: true,
-                borrow_enabled: true,
-            },
-            ..th_default_asset_params()
+        depositor_addr,
+        ..
+    } = setup_test();
+
+    // non-credit-manager user cannot deposit with account_id (even with empty string)
+    let err = execute(
+        deps.as_mut(),
+        mock_env(),
+        mock_info(depositor_addr.as_str(), &coins(123, denom)),
+        ExecuteMsg::Deposit {
+            account_id: Some("".to_string()),
+            on_behalf_of: None,
         },
-    );
+    )
+    .unwrap_err();
+    assert_eq!(err, ContractError::Mars(MarsError::Unauthorized {}));
 
+    // non-credit-manager user cannot deposit with account_id
     let err = execute(
         deps.as_mut(),
         mock_env(),
         mock_info(depositor_addr.as_str(), &coins(123, denom)),
         ExecuteMsg::Deposit {
-            account_id: None,
-            on_behalf_of: Some("credit_manager".to_string()),
+            account_id: Some("1234".to_string()),
+            on_behalf_of: None,
         },
     )
     .unwrap_err();

contracts/red-bank/tests/tests/test_withdraw.rs

@@ -17,6 +17,7 @@ use mars_red_bank::{
 };
 use mars_red_bank_types::{
     address_provider::MarsAddressType,
+    error::MarsError,
     incentives,
     red_bank::{Collateral, Debt, ExecuteMsg, Market},
 };
@@ -939,3 +940,45 @@ fn withdraw_if_oracle_circuit_breakers_activated() {
     let res = red_bank.withdraw_with_acc_id(&mut mock_env, &user, "uosmo", None, None, Some(true));
     assert_err_with_str(res, expected_msg);
 }
+
+#[test]
+fn withdrawing_with_account_id_by_non_credit_manager_user() {
+    let TestSuite {
+        mut deps,
+        denom,
+        withdrawer_addr,
+        ..
+    } = setup_test();
+
+    // non-credit-manager user cannot withdraw with account_id (even with empty string)
+    let err = execute(
+        deps.as_mut(),
+        mock_env(),
+        mock_info(withdrawer_addr.as_str(), &[]),
+        ExecuteMsg::Withdraw {
+            denom: denom.to_string(),
+            amount: Some(Uint128::from(2000u128)),
+            recipient: None,
+            account_id: Some("".to_string()),
+            liquidation_related: None,
+        },
+    )
+    .unwrap_err();
+    assert_eq!(err, ContractError::Mars(MarsError::Unauthorized {}));
+
+    // non-credit-manager user cannot withdraw with account_id
+    let err = execute(
+        deps.as_mut(),
+        mock_env(),
+        mock_info(withdrawer_addr.as_str(), &[]),
+        ExecuteMsg::Withdraw {
+            denom: denom.to_string(),
+            amount: Some(Uint128::from(2000u128)),
+            recipient: None,
+            account_id: Some("1234".to_string()),
+            liquidation_related: None,
+        },
+    )
+    .unwrap_err();
+    assert_eq!(err, ContractError::Mars(MarsError::Unauthorized {}));
+}