diff --git a/contracts/red-bank/src/contract.rs b/contracts/red-bank/src/contract.rs index 061eae5cf..409729cc6 100644 --- a/contracts/red-bank/src/contract.rs +++ b/contracts/red-bank/src/contract.rs @@ -50,9 +50,18 @@ pub fn execute( } ExecuteMsg::Deposit { account_id, + on_behalf_of, } => { let sent_coin = cw_utils::one_coin(&info)?; - deposit::deposit(deps, env, info, sent_coin.denom, sent_coin.amount, account_id) + deposit::deposit( + deps, + env, + info, + on_behalf_of, + sent_coin.denom, + sent_coin.amount, + account_id, + ) } ExecuteMsg::Withdraw { denom, diff --git a/contracts/red-bank/src/deposit.rs b/contracts/red-bank/src/deposit.rs index 711f4e0ef..16efe17b5 100644 --- a/contracts/red-bank/src/deposit.rs +++ b/contracts/red-bank/src/deposit.rs @@ -1,6 +1,9 @@ -use cosmwasm_std::{DepsMut, Env, MessageInfo, Response, Uint128}; +use cosmwasm_std::{Addr, DepsMut, Env, MessageInfo, Response, Uint128}; use mars_interest_rate::get_scaled_liquidity_amount; -use mars_red_bank_types::address_provider::{self, MarsAddressType}; +use mars_red_bank_types::{ + address_provider::{self, MarsAddressType}, + error::MarsError, +}; use crate::{ error::ContractError, @@ -14,12 +17,11 @@ pub fn deposit( deps: DepsMut, env: Env, info: MessageInfo, + on_behalf_of: Option, denom: String, deposit_amount: Uint128, account_id: Option, ) -> Result { - let mut market = MARKETS.load(deps.storage, &denom)?; - let config = CONFIG.load(deps.storage)?; let addresses = address_provider::helpers::query_contract_addrs( @@ -29,11 +31,32 @@ pub fn deposit( MarsAddressType::Incentives, MarsAddressType::RewardsCollector, MarsAddressType::Params, + MarsAddressType::CreditManager, ], )?; let rewards_collector_addr = &addresses[&MarsAddressType::RewardsCollector]; let incentives_addr = &addresses[&MarsAddressType::Incentives]; let params_addr = &addresses[&MarsAddressType::Params]; + let credit_manager_addr = &addresses[&MarsAddressType::CreditManager]; + + 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: + // 1.) Wait for a new asset XXX to be listed and makes sure there is no coin lent out for XXX from the credit-manager to red-bank. + // 2.) Calls deposit on red-bank and sends 1 XXX and deposits on behalf of credit-manager. + // 3.) A user wants to lend out XXX from credit-manager but the call fails because TOTAL_LENT_SHARES is never initialized + // because this query red_bank.query_lent(&deps.querier, &env.contract.address, &coin.denom)? returns one. + Some(address) if address == credit_manager_addr.as_str() => { + return Err(ContractError::Mars(MarsError::Unauthorized {})); + } + Some(address) => { + user_addr = deps.api.addr_validate(address)?; + User(&user_addr) + } + None => User(&info.sender), + }; + + let mut market = MARKETS.load(deps.storage, &denom)?; let asset_params = query_asset_params(&deps.querier, params_addr, &denom)?; @@ -68,7 +91,7 @@ pub fn deposit( let deposit_amount_scaled = get_scaled_liquidity_amount(deposit_amount, &market, env.block.time.seconds())?; - response = User(&info.sender).increase_collateral( + response = user.increase_collateral( deps.storage, &market, deposit_amount_scaled, @@ -86,6 +109,7 @@ pub fn deposit( Ok(response .add_attribute("action", "deposit") .add_attribute("sender", &info.sender) + .add_attribute("on_behalf_of", user) .add_attribute("denom", denom) .add_attribute("amount", deposit_amount) .add_attribute("amount_scaled", deposit_amount_scaled)) diff --git a/contracts/red-bank/tests/tests/test_deposit.rs b/contracts/red-bank/tests/tests/test_deposit.rs index 6593a5363..c954ecc07 100644 --- a/contracts/red-bank/tests/tests/test_deposit.rs +++ b/contracts/red-bank/tests/tests/test_deposit.rs @@ -17,6 +17,7 @@ use mars_red_bank::{ }; use mars_red_bank_types::{ address_provider::MarsAddressType, + error::MarsError, incentives, red_bank::{Collateral, ExecuteMsg, Market}, }; @@ -113,6 +114,7 @@ fn depositing_with_no_coin_sent() { mock_info(depositor_addr.as_str(), &[]), ExecuteMsg::Deposit { account_id: None, + on_behalf_of: None, }, ) .unwrap_err(); @@ -135,6 +137,7 @@ fn depositing_with_multiple_coins_sent() { mock_info(depositor_addr.as_str(), &sent_coins), ExecuteMsg::Deposit { account_id: None, + on_behalf_of: None, }, ) .unwrap_err(); @@ -158,6 +161,7 @@ fn depositing_to_non_existent_market() { mock_info(depositor_addr.as_str(), &coins(123, false_denom)), ExecuteMsg::Deposit { account_id: None, + on_behalf_of: None, }, ) .unwrap_err(); @@ -195,6 +199,7 @@ fn depositing_to_disabled_market() { mock_info(depositor_addr.as_str(), &coins(123, denom)), ExecuteMsg::Deposit { account_id: None, + on_behalf_of: None, }, ) .unwrap_err(); @@ -252,6 +257,7 @@ fn depositing_above_cap(amount_to_deposit: u128, deposit_cap: u128, exp_ok: bool mock_info(depositor_addr.as_str(), &coins(amount_to_deposit, denom)), ExecuteMsg::Deposit { account_id: None, + on_behalf_of: None, }, ); @@ -295,6 +301,7 @@ fn depositing_without_existing_position() { mock_info(depositor_addr.as_str(), &coins(deposit_amount, denom)), ExecuteMsg::Deposit { account_id: None, + on_behalf_of: None, }, ) .unwrap(); @@ -323,6 +330,7 @@ fn depositing_without_existing_position() { vec![ attr("action", "deposit"), attr("sender", &depositor_addr), + attr("on_behalf_of", &depositor_addr), attr("denom", denom), attr("amount", deposit_amount.to_string()), attr("amount_scaled", expected_mint_amount), @@ -385,6 +393,7 @@ fn depositing_with_existing_position() { mock_info(depositor_addr.as_str(), &coins(deposit_amount, denom)), ExecuteMsg::Deposit { account_id: None, + on_behalf_of: None, }, ) .unwrap(); @@ -421,3 +430,199 @@ fn depositing_with_existing_position() { } ); } + +#[test] +fn depositing_on_behalf_of() { + let TestSuite { + mut deps, + denom, + depositor_addr, + initial_market, + } = setup_test(); + + let deposit_amount = 123456u128; + let on_behalf_of_addr = Addr::unchecked("jake"); + + // compute expected market parameters + let block_time = 10000300; + let expected_params = + th_get_expected_indices_and_rates(&initial_market, block_time, Default::default()); + let expected_mint_amount = compute_scaled_amount( + Uint128::from(deposit_amount), + expected_params.liquidity_index, + ScalingOperation::Truncate, + ) + .unwrap(); + let expected_reward_amount_scaled = compute_scaled_amount( + expected_params.protocol_rewards_to_distribute, + expected_params.liquidity_index, + ScalingOperation::Truncate, + ) + .unwrap(); + + let res = execute( + deps.as_mut(), + mock_env_at_block_time(block_time), + mock_info(depositor_addr.as_str(), &coins(deposit_amount, denom)), + ExecuteMsg::Deposit { + account_id: None, + on_behalf_of: Some(on_behalf_of_addr.clone().into()), + }, + ) + .unwrap(); + + // NOTE: For this test, the accrued protocol reward is non-zero, so we do expect a message to + // update the index of the rewards collector. + assert_eq!( + res.messages, + vec![ + SubMsg::new(WasmMsg::Execute { + contract_addr: MarsAddressType::Incentives.to_string(), + msg: to_binary(&incentives::ExecuteMsg::BalanceChange { + user_addr: Addr::unchecked(MarsAddressType::RewardsCollector.to_string()), + account_id: None, + denom: initial_market.denom.clone(), + user_amount_scaled_before: Uint128::zero(), + total_amount_scaled_before: initial_market.collateral_total_scaled, + }) + .unwrap(), + funds: vec![] + }), + SubMsg::new(WasmMsg::Execute { + contract_addr: MarsAddressType::Incentives.to_string(), + msg: to_binary(&incentives::ExecuteMsg::BalanceChange { + user_addr: on_behalf_of_addr.clone(), + account_id: None, + denom: initial_market.denom.clone(), + user_amount_scaled_before: Uint128::zero(), + // NOTE: New collateral shares were minted to the rewards collector first, so + // for the depositor this should be initial total supply + rewards shares minted + total_amount_scaled_before: initial_market.collateral_total_scaled + + expected_reward_amount_scaled, + }) + .unwrap(), + funds: vec![] + }) + ] + ); + + // depositor should not have created a new collateral position + let opt = COLLATERALS.may_load(deps.as_ref().storage, (&depositor_addr, "", denom)).unwrap(); + assert!(opt.is_none()); + + // the recipient should have created a new collateral position + let collateral = + COLLATERALS.load(deps.as_ref().storage, (&on_behalf_of_addr, "", denom)).unwrap(); + assert_eq!( + collateral, + Collateral { + amount_scaled: expected_mint_amount, + enabled: true, + } + ); +} + +#[test] +fn depositing_on_behalf_of_cannot_enable_collateral() { + let TestSuite { + mut deps, + denom, + depositor_addr, + .. + } = setup_test(); + + deps.querier.set_oracle_price(denom, Decimal::one()); + + let on_behalf_of_addr = Addr::unchecked("jake"); + + let block_time = 10000300; + + // 'on_behalf_of_addr' deposit funds to their own account + execute( + deps.as_mut(), + mock_env_at_block_time(block_time), + mock_info(on_behalf_of_addr.as_str(), &coins(1u128, denom)), + ExecuteMsg::Deposit { + account_id: None, + on_behalf_of: None, + }, + ) + .unwrap(); + + // 'on_behalf_of_addr' should have collateral enabled + let collateral = + COLLATERALS.load(deps.as_ref().storage, (&on_behalf_of_addr, "", denom)).unwrap(); + assert!(collateral.enabled); + + // 'on_behalf_of_addr' disables asset as collateral + execute( + deps.as_mut(), + mock_env_at_block_time(block_time), + mock_info(on_behalf_of_addr.as_str(), &[]), + ExecuteMsg::UpdateAssetCollateralStatus { + denom: denom.to_string(), + enable: false, + }, + ) + .unwrap(); + + // verify asset is disabled as collateral for 'on_behalf_of_addr' + let collateral = + COLLATERALS.load(deps.as_ref().storage, (&on_behalf_of_addr, "", denom)).unwrap(); + assert!(!collateral.enabled); + + // 'depositor_addr' deposits a small amount of funds to 'on_behalf_of_addr' to enable his asset as collateral + execute( + deps.as_mut(), + mock_env_at_block_time(block_time), + mock_info(depositor_addr.as_str(), &coins(1u128, denom)), + ExecuteMsg::Deposit { + account_id: None, + on_behalf_of: Some(on_behalf_of_addr.to_string()), + }, + ) + .unwrap(); + + // 'on_behalf_of_addr' doesn't have the asset enabled as collateral + let collateral = + COLLATERALS.load(deps.as_ref().storage, (&on_behalf_of_addr, "", denom)).unwrap(); + assert!(!collateral.enabled); +} + +#[test] +fn depositing_on_behalf_of_credit_manager() { + let TestSuite { + mut deps, + denom, + depositor_addr, + .. + } = setup_test(); + + // disable the market + deps.querier.set_redbank_params( + denom, + AssetParams { + credit_manager: CmSettings { + whitelisted: false, + hls: None, + }, + red_bank: RedBankSettings { + deposit_enabled: true, + borrow_enabled: true, + }, + ..th_default_asset_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 {})); +} diff --git a/contracts/red-bank/tests/tests/test_liquidate.rs b/contracts/red-bank/tests/tests/test_liquidate.rs index c062fb63d..d94f2548d 100644 --- a/contracts/red-bank/tests/tests/test_liquidate.rs +++ b/contracts/red-bank/tests/tests/test_liquidate.rs @@ -746,6 +746,7 @@ fn response_verification() { mock_info(provider.as_str(), &[coin(1000000, "uusdc")]), ExecuteMsg::Deposit { account_id: None, + on_behalf_of: None, }, ) .unwrap(); @@ -755,6 +756,7 @@ fn response_verification() { mock_info(provider.as_str(), &[coin(1000000, "untrn")]), ExecuteMsg::Deposit { account_id: None, + on_behalf_of: None, }, ) .unwrap(); @@ -766,6 +768,7 @@ fn response_verification() { mock_info(liquidatee.as_str(), &[coin(10000, "uosmo")]), ExecuteMsg::Deposit { account_id: None, + on_behalf_of: None, }, ) .unwrap(); @@ -775,6 +778,7 @@ fn response_verification() { mock_info(liquidatee.as_str(), &[coin(900, "uatom")]), ExecuteMsg::Deposit { account_id: None, + on_behalf_of: None, }, ) .unwrap(); diff --git a/integration-tests/tests/test_oracles.rs b/integration-tests/tests/test_oracles.rs index 2fc5b0dcf..bd74cb96f 100644 --- a/integration-tests/tests/test_oracles.rs +++ b/integration-tests/tests/test_oracles.rs @@ -953,6 +953,7 @@ fn redbank_should_fail_if_no_price() { &red_bank_addr, &Deposit { account_id: None, + on_behalf_of: None, }, &[coin(1_000_000, "uatom")], depositor, @@ -1015,6 +1016,7 @@ fn redbank_quering_oracle_successfully() { &red_bank_addr, &Deposit { account_id: None, + on_behalf_of: None, }, &[coin(1_000_000, "uatom")], depositor, diff --git a/packages/testing/src/integration/mock_env.rs b/packages/testing/src/integration/mock_env.rs index 52003578e..95ab2e404 100644 --- a/packages/testing/src/integration/mock_env.rs +++ b/packages/testing/src/integration/mock_env.rs @@ -355,6 +355,7 @@ impl RedBank { self.contract_addr.clone(), &red_bank::ExecuteMsg::Deposit { account_id, + on_behalf_of: None, }, &[coin], ) diff --git a/packages/types/src/red_bank/msg.rs b/packages/types/src/red_bank/msg.rs index b80f7ec8c..f3a93e256 100644 --- a/packages/types/src/red_bank/msg.rs +++ b/packages/types/src/red_bank/msg.rs @@ -56,6 +56,9 @@ pub enum ExecuteMsg { Deposit { /// Credit account id (Rover) account_id: Option, + + /// Address that will receive the coins + on_behalf_of: Option, }, /// Withdraw native coins diff --git a/schemas/mars-red-bank/mars-red-bank.json b/schemas/mars-red-bank/mars-red-bank.json index 6bafea4e5..06be95ce6 100644 --- a/schemas/mars-red-bank/mars-red-bank.json +++ b/schemas/mars-red-bank/mars-red-bank.json @@ -196,6 +196,13 @@ "string", "null" ] + }, + "on_behalf_of": { + "description": "Address that will receive the coins", + "type": [ + "string", + "null" + ] } }, "additionalProperties": false diff --git a/scripts/types/generated/mars-red-bank/MarsRedBank.client.ts b/scripts/types/generated/mars-red-bank/MarsRedBank.client.ts index a48ed7d52..d6c5cad7f 100644 --- a/scripts/types/generated/mars-red-bank/MarsRedBank.client.ts +++ b/scripts/types/generated/mars-red-bank/MarsRedBank.client.ts @@ -445,8 +445,10 @@ export interface MarsRedBankInterface extends MarsRedBankReadOnlyInterface { deposit: ( { accountId, + onBehalfOf, }: { accountId?: string + onBehalfOf?: string }, fee?: number | StdFee | 'auto', memo?: string, @@ -668,8 +670,10 @@ export class MarsRedBankClient extends MarsRedBankQueryClient implements MarsRed deposit = async ( { accountId, + onBehalfOf, }: { accountId?: string + onBehalfOf?: string }, fee: number | StdFee | 'auto' = 'auto', memo?: string, @@ -681,6 +685,7 @@ export class MarsRedBankClient extends MarsRedBankQueryClient implements MarsRed { deposit: { account_id: accountId, + on_behalf_of: onBehalfOf, }, }, fee, diff --git a/scripts/types/generated/mars-red-bank/MarsRedBank.react-query.ts b/scripts/types/generated/mars-red-bank/MarsRedBank.react-query.ts index b7502eb79..bb32ebe4a 100644 --- a/scripts/types/generated/mars-red-bank/MarsRedBank.react-query.ts +++ b/scripts/types/generated/mars-red-bank/MarsRedBank.react-query.ts @@ -636,6 +636,7 @@ export interface MarsRedBankDepositMutation { client: MarsRedBankClient msg: { accountId?: string + onBehalfOf?: string } args?: { fee?: number | StdFee | 'auto' diff --git a/scripts/types/generated/mars-red-bank/MarsRedBank.types.ts b/scripts/types/generated/mars-red-bank/MarsRedBank.types.ts index 9552b2fdf..5595744e7 100644 --- a/scripts/types/generated/mars-red-bank/MarsRedBank.types.ts +++ b/scripts/types/generated/mars-red-bank/MarsRedBank.types.ts @@ -43,6 +43,7 @@ export type ExecuteMsg = | { deposit: { account_id?: string | null + on_behalf_of?: string | null } } | {