From 8bade3d294eecbd8b05782cdbac81a1bc6102308 Mon Sep 17 00:00:00 2001 From: piobab Date: Fri, 8 Sep 2023 12:27:46 +0200 Subject: [PATCH] Don't allow red-bank users to create alternative account ids. --- contracts/red-bank/src/deposit.rs | 7 +++ contracts/red-bank/src/withdraw.rs | 47 +++++++++-------- .../tests/tests/test_credit_accounts.rs | 2 +- .../red-bank/tests/tests/test_deposit.rs | 50 +++++++++++++------ .../red-bank/tests/tests/test_withdraw.rs | 43 ++++++++++++++++ 5 files changed, 113 insertions(+), 36 deletions(-) diff --git a/contracts/red-bank/src/deposit.rs b/contracts/red-bank/src/deposit.rs index 16efe17b5..e3fe60668 100644 --- a/contracts/red-bank/src/deposit.rs +++ b/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: diff --git a/contracts/red-bank/src/withdraw.rs b/contracts/red-bank/src/withdraw.rs index 8e7318886..890feec52 100644 --- a/contracts/red-bank/src/withdraw.rs +++ b/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, liquidation_related: bool, ) -> Result { + 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; diff --git a/contracts/red-bank/tests/tests/test_credit_accounts.rs b/contracts/red-bank/tests/tests/test_credit_accounts.rs index 8a5b42d70..edbce8428 100644 --- a/contracts/red-bank/tests/tests/test_credit_accounts.rs +++ b/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 diff --git a/contracts/red-bank/tests/tests/test_deposit.rs b/contracts/red-bank/tests/tests/test_deposit.rs index c954ecc07..81f9928e1 100644 --- a/contracts/red-bank/tests/tests/test_deposit.rs +++ b/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(); diff --git a/contracts/red-bank/tests/tests/test_withdraw.rs b/contracts/red-bank/tests/tests/test_withdraw.rs index 2d6584396..d5d7af200 100644 --- a/contracts/red-bank/tests/tests/test_withdraw.rs +++ b/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 {})); +}