From 61b6d0e6d6be617ba2b0f894d423954ee8e8e4ef Mon Sep 17 00:00:00 2001 From: piobab Date: Thu, 25 Jul 2024 17:08:49 +0200 Subject: [PATCH] MP-2543. Deposit cap improvement (#425) * Deposit cap check ommited if amount after TX <= before TX for Swap and ProvideLiquidity. * Update test cases. * Update schema. * Fix typo. * Improve comments in tests. * Add test to cover missing line coverage. * Review comments. * Update comment. --- contracts/credit-manager/src/deposit.rs | 79 ++++++++- contracts/credit-manager/src/execute.rs | 46 +++-- .../tests/tests/test_deposit_cap.rs | 165 ++++++++++++++++-- packages/types/src/credit_manager/execute.rs | 4 +- .../mars-credit-manager.json | 16 +- .../MarsCreditManager.types.ts | 4 +- 6 files changed, 276 insertions(+), 38 deletions(-) diff --git a/contracts/credit-manager/src/deposit.rs b/contracts/credit-manager/src/deposit.rs index 2cf71a71..54d663a1 100644 --- a/contracts/credit-manager/src/deposit.rs +++ b/contracts/credit-manager/src/deposit.rs @@ -1,6 +1,6 @@ -use std::collections::BTreeSet; +use std::collections::BTreeMap; -use cosmwasm_std::{Coin, Coins, Deps, DepsMut, Response}; +use cosmwasm_std::{Coin, Coins, Deps, DepsMut, Response, Uint128}; use mars_types::params::TotalDepositResponse; use crate::{ @@ -48,12 +48,15 @@ fn assert_sent_fund(expected: &Coin, received_coins: &Coins) -> ContractResult<( /// Given a list of denoms, assert that the total deposited amount of each denom /// across Red Bank and Rover does not exceed its deposit cap recorded in the /// params contract. -pub fn assert_deposit_caps(deps: Deps, denoms: BTreeSet) -> ContractResult { +pub fn assert_deposit_caps( + deps: Deps, + denom_deposits: BTreeMap>, +) -> ContractResult { let params = PARAMS.load(deps.storage)?; let mut response = Response::new().add_attribute("action", "callback/assert_deposit_caps"); - for denom in denoms { + for (denom, deposited_opt) in denom_deposits { // Asset is not found (not whitelisted) and it doesn't count towards the cap and Health Factor let params_opt = params.query_asset_params(&deps.querier, &denom)?; if params_opt.is_none() { @@ -66,6 +69,16 @@ pub fn assert_deposit_caps(deps: Deps, denoms: BTreeSet) -> ContractResu cap, } = params.query_total_deposit(&deps.querier, &denom)?; + // - If there is None in the map, it means that the deposit cap should be enforced. It is related to the Deposit action. + // - If there is Some in the map, it means that the deposit amount should be compared (value before and after the TX). + // It is related to the SwapExactIn and ProvideLiquidity actions. + if let Some(deposited) = deposited_opt { + // amount is lower than or equal to the previous deposit amount so it is fine + if amount <= deposited { + continue; + } + } + if amount > cap { return Err(ContractError::AboveAssetDepositCap { new_value: Coin { @@ -84,3 +97,61 @@ pub fn assert_deposit_caps(deps: Deps, denoms: BTreeSet) -> ContractResu Ok(response) } + +/// Update the total deposit amount for the asset in the denom_deposits map +/// The function either resets the deposit amount to None for Deposit actions +/// or updates the deposit amount based on the received coins and existing parameters. +pub fn update_or_reset_denom_deposits( + deps: Deps, + denom_deposits: &mut BTreeMap>, + denom: &str, + received_coins: &Coins, + deposit_action: bool, +) -> ContractResult<()> { + // Overwrite the previous amount for Deposit action. + // It means that we don't compare deposits before and after the TX. + // + // Strictly enforce the deposit cap to prevent any increase in total value. + // Even if we use the funds for operations within the account (such as liquidation), + // and withdraw them at the end (resulting in zero net inflow), temporary funds + // could still be used for malicious actions. + if deposit_action { + denom_deposits.insert(denom.to_string(), None); + + return Ok(()); + } + + // Check if the denomination is already in the list. + // This ensures that a Deposit action (which does not have an associated amount) + // is not overwritten by a subsequent Swap or ProvideLiquidity action. + // By confirming the existence of the denomination in the list, we maintain + // the integrity of the original Deposit action. + if denom_deposits.contains_key(denom) { + return Ok(()); + } + + // Load the params + let params = PARAMS.load(deps.storage)?; + + // Asset is not found (not whitelisted) and it doesn't count towards the cap and Health Factor + let params_opt = params.query_asset_params(&deps.querier, denom)?; + if params_opt.is_none() { + return Ok(()); + } + + // Check total deposit amount for the asset + let total_deposit_amt = params.query_total_deposit(&deps.querier, denom)?.amount; + + // Check if the asset was sent in the TX + let received_amt = received_coins.amount_of(denom); + + // Total deposit amount is the sum of all deposits for the asset across Red Bank and Rover. + // If the asset was sent in the TX, the Credit Manager balance already includes the deposited amount + // so we need to subtract it from the total deposit amount to see previous state. + let new_total_deposit_amt = total_deposit_amt.checked_sub(received_amt).unwrap_or_default(); + + // Update the total deposit amount for the asset + denom_deposits.insert(denom.to_string(), Some(new_total_deposit_amt)); + + Ok(()) +} diff --git a/contracts/credit-manager/src/execute.rs b/contracts/credit-manager/src/execute.rs index 4a3825f7..8e0dd5be 100644 --- a/contracts/credit-manager/src/execute.rs +++ b/contracts/credit-manager/src/execute.rs @@ -1,7 +1,8 @@ -use std::collections::BTreeSet; +use std::collections::BTreeMap; use cosmwasm_std::{ - to_json_binary, Addr, Coins, CosmosMsg, DepsMut, Env, MessageInfo, Response, StdResult, WasmMsg, + to_json_binary, Addr, Coins, CosmosMsg, DepsMut, Env, MessageInfo, Response, StdResult, + Uint128, WasmMsg, }; use mars_types::{ account_nft::ExecuteMsg as NftExecuteMsg, @@ -15,7 +16,7 @@ use crate::{ borrow::borrow, claim_astro_lp_rewards::claim_lp_rewards, claim_rewards::claim_rewards, - deposit::{assert_deposit_caps, deposit}, + deposit::{assert_deposit_caps, deposit, update_or_reset_denom_deposits}, error::{ContractError, ContractResult}, health::{assert_max_ltv, query_health_state}, hls::assert_hls_rules, @@ -151,13 +152,10 @@ pub fn dispatch_actions( None }; - // We use a Set to record all denoms whose deposited amount may go up as the + // We use a Map to record all denoms whose deposited amount may go up as the // result of any action. We invoke the AssertDepositCaps callback in the end // to make sure that none of the deposit cap is exceeded. // - // Additionally, we use a BTreeSet (instead of a Vec or HashSet) to ensure - // uniqueness and determininism. - // // There are a few actions that may result in an asset's deposit amount // going up: // - Deposit: we check the deposited denom @@ -172,14 +170,22 @@ pub fn dispatch_actions( // Note that Borrow/Lend/Reclaim does not impact total deposit amount, // because they simply move assets between Red Bank and Rover. We don't // check these actions. - let mut denoms_for_cap_check = BTreeSet::new(); + // If there is None in the map, it means that the deposit cap should be enforced, + // otherwise it should compare deposit amount before and after the TX. + let mut denoms_for_cap_check: BTreeMap> = BTreeMap::new(); for action in actions { match action { Action::Deposit(coin) => { response = deposit(&mut deps, response, account_id, &coin, &mut received_coins)?; - // check the deposit cap of the deposited denom - denoms_for_cap_check.insert(coin.denom); + // add the denom to the map to check the deposit cap in the end of the TX + update_or_reset_denom_deposits( + deps.as_ref(), + &mut denoms_for_cap_check, + &coin.denom, + &received_coins, + true, + )?; } Action::Withdraw(coin) => callbacks.push(CallbackMsg::Withdraw { account_id: account_id.to_string(), @@ -285,8 +291,14 @@ pub fn dispatch_actions( min_receive, route, }); - // check the deposit cap of the swap output denom - denoms_for_cap_check.insert(denom_out); + // add the output denom to the map to check the deposit cap in the end of the TX + update_or_reset_denom_deposits( + deps.as_ref(), + &mut denoms_for_cap_check, + &denom_out, + &received_coins, + false, + )?; } Action::ExitVault { vault, @@ -324,8 +336,14 @@ pub fn dispatch_actions( slippage, }); - // check the deposit cap of the LP output denom - denoms_for_cap_check.insert(lp_token_out); + // add the LP output denom to the map to check the deposit cap in the end of the TX + update_or_reset_denom_deposits( + deps.as_ref(), + &mut denoms_for_cap_check, + &lp_token_out, + &received_coins, + false, + )?; } Action::WithdrawLiquidity { lp_token, diff --git a/contracts/credit-manager/tests/tests/test_deposit_cap.rs b/contracts/credit-manager/tests/tests/test_deposit_cap.rs index 556aa0d0..446dc5e4 100644 --- a/contracts/credit-manager/tests/tests/test_deposit_cap.rs +++ b/contracts/credit-manager/tests/tests/test_deposit_cap.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; -use cosmwasm_std::{Addr, Coin, Coins, Decimal, StdResult, Uint128}; +use cosmwasm_std::{coin, Addr, Coin, Coins, Decimal, StdResult, Uint128}; use mars_credit_manager::error::ContractError; use mars_testing::multitest::helpers::{coin_info, ASTRO_LP_DENOM}; use mars_types::{ @@ -13,6 +13,7 @@ use test_case::test_case; use super::helpers::{AccountToFund, MockEnv}; #[test_case( + vec![], [].into(), vec![ Action::Deposit(Coin { @@ -28,6 +29,7 @@ use super::helpers::{AccountToFund, MockEnv}; "no deposit cap" )] #[test_case( + vec![], [("uatom", 100)].into(), vec![ Action::Deposit(Coin { @@ -43,6 +45,25 @@ use super::helpers::{AccountToFund, MockEnv}; "deposit cap exceeded" )] #[test_case( + // initial balance, deposit cap already exceeded by another user + vec![coin(101, "uatom")], + [("uatom", 100)].into(), + vec![ + Action::Deposit(Coin { + denom: "uatom".into(), + amount: Uint128::new(9), + }), + Action::Deposit(Coin { + denom: "uosmo".into(), + amount: Uint128::new(456), + }), + Action::RefundAllCoinBalances {} + ], + false; + "deposit cap exceeded even with refunding" +)] +#[test_case( + vec![], [("uatom", 100)].into(), vec![ // this first action exceeds deposit cap... @@ -71,6 +92,7 @@ use super::helpers::{AccountToFund, MockEnv}; "a deposit action causes cap to be exceeded but a follow up swap action saves it" )] #[test_case( + vec![], // in our specific test setup, 123 uatom swaps to 1337 uosmo // we set the cap to 1000 uosmo which should be exceeded [("uatom", 200), ("uosmo", 1000)].into(), @@ -98,6 +120,74 @@ use super::helpers::{AccountToFund, MockEnv}; "a deposit action is below cap but a follow up swap action exceeds the cap" )] #[test_case( + // initial balance, deposit cap already exceeded by another user + vec![coin(100, "uosmo")], + // in our specific test setup, 123 uatom swaps to 1337 uosmo + // we set the cap to 0 uosmo which should be exceeded + [("uatom", 200), ("uosmo", 0)].into(), + vec![ + Action::Deposit(Coin { + denom: "uatom".into(), + amount: Uint128::new(123), + }), + Action::SwapExactIn { + coin_in: ActionCoin { + denom: "uatom".into(), + amount: ActionAmount::AccountBalance, + }, + denom_out: "uosmo".into(), + min_receive: Uint128::zero(), + route: Some(SwapperRoute::Osmo(OsmoRoute{swaps: vec![ + OsmoSwap { + pool_id: 101, + to: "uosmo".into(), + } + ]})) + }, + Action::RefundAllCoinBalances {} + ], + true; + "a deposit action is below cap but a follow up swap action try to exceed the cap, refunding saves it" +)] +#[test_case( + // initial balance, deposit cap already exceeded by another user + vec![coin(101, "uosmo")], + // in our specific test setup, 123 uatom swaps to 1337 uosmo + // we set the cap to 100 uosmo which should be exceeded + [("uatom", 200), ("uosmo", 100)].into(), + vec![ + Action::Deposit(Coin { + denom: "uatom".into(), + amount: Uint128::new(123), + }), + Action::Deposit(Coin { + denom: "uosmo".into(), + amount: Uint128::new(10), + }), + Action::SwapExactIn { + coin_in: ActionCoin { + denom: "uatom".into(), + amount: ActionAmount::AccountBalance, + }, + denom_out: "uosmo".into(), + min_receive: Uint128::zero(), + route: Some(SwapperRoute::Osmo(OsmoRoute{swaps: vec![ + OsmoSwap { + pool_id: 101, + to: "uosmo".into(), + } + ]})) + }, + Action::RefundAllCoinBalances {} + ], + false; + // Deposit action is more important than Swap / ProvideLiquidity actions for the same asset. + // It means that if Deposit action set a denom to be checked for deposit cap, then all other actions + // for this denom can't save it by comparing deposits before and after the TX. + "a deposit action is above cap, swapping to deposited coin and refunding cannot save it" +)] +#[test_case( + vec![], [("uosmo", 1000), ("ujake", 1000), (ASTRO_LP_DENOM, 1000)].into(), vec![ Action::Deposit(Coin { @@ -122,12 +212,42 @@ use super::helpers::{AccountToFund, MockEnv}; false; "LP deposit cap exceeded" )] +#[test_case( + // initial balance, deposit cap already exceeded by another user + vec![coin(1001, ASTRO_LP_DENOM)], + [("uosmo", 1000), ("ujake", 1000), (ASTRO_LP_DENOM, 1000)].into(), + vec![ + Action::Deposit(Coin { + denom: "uosmo".into(), + amount: Uint128::new(101), + }), + Action::Deposit(Coin { + denom: "ujake".into(), + amount: Uint128::new(456), + }), + Action::ProvideLiquidity { coins_in: vec![ + ActionCoin { + denom: "uosmo".into(), + amount: ActionAmount::AccountBalance, + }, + ActionCoin { + denom: "ujake".into(), + amount: ActionAmount::AccountBalance, + }], + lp_token_out: ASTRO_LP_DENOM.to_string(), slippage: Decimal::percent(5) }, + Action::RefundAllCoinBalances {} + ], + true; + "LP deposit cap when refunding saves it" +)] fn asserting_deposit_cap( + init_deposits: Vec, deposit_caps: HashMap<&'static str, u128>, actions: Vec, exp_ok: bool, ) { let user = Addr::unchecked("user"); + let another_user = Addr::unchecked("another_user"); // compute how much coins need to be sent to the contract in order to update // the credit account @@ -148,14 +268,37 @@ fn asserting_deposit_cap( } // set up mock environment - let mut mock = MockEnv::new() - .set_params(¶ms) - .fund_account(AccountToFund { - addr: user.clone(), - funds: send_funds.clone(), - }) - .build() - .unwrap(); + let mut mock_builder = MockEnv::new().set_params(¶ms).fund_account(AccountToFund { + addr: user.clone(), + funds: send_funds.clone(), + }); + + if !init_deposits.is_empty() { + // fund the user with some base deposits + mock_builder = mock_builder.fund_account(AccountToFund { + addr: another_user.clone(), + funds: init_deposits.clone(), + }); + } + + let mut mock = mock_builder.build().unwrap(); + + // prepare base state of the account + if !init_deposits.is_empty() { + let account_id = mock.create_credit_account(&another_user).unwrap(); + for coin in init_deposits { + mock.update_credit_account( + &account_id, + &another_user, + vec![Action::Deposit(coin.clone())], + &[coin], + ) + .unwrap(); + } + } + + // register an account + let account_id = mock.create_credit_account(&user).unwrap(); // set deposit caps for uosmo and uatom // the `uosmo_info` and `uatom_info` functions set the cap to Uint128::MAX, @@ -169,9 +312,6 @@ fn asserting_deposit_cap( }); } - // register an account - let account_id = mock.create_credit_account(&user).unwrap(); - // attempt to execute the actions let result = mock.update_credit_account(&account_id, &user, actions, &send_funds); @@ -179,6 +319,7 @@ fn asserting_deposit_cap( assert!(result.is_ok()); } else { let err: ContractError = result.unwrap_err().downcast().unwrap(); + println!("err: {:?}", err); // if errors, we make sure the error is the AboveAssetDepositCap error // and not any other error assert!(matches!(err, ContractError::AboveAssetDepositCap { .. })); diff --git a/packages/types/src/credit_manager/execute.rs b/packages/types/src/credit_manager/execute.rs index 02ef7b97..2f3252a6 100644 --- a/packages/types/src/credit_manager/execute.rs +++ b/packages/types/src/credit_manager/execute.rs @@ -1,4 +1,4 @@ -use std::collections::BTreeSet; +use std::collections::BTreeMap; use cosmwasm_schema::cw_serde; use cosmwasm_std::{to_json_binary, Addr, Coin, CosmosMsg, Decimal, StdResult, Uint128, WasmMsg}; @@ -275,7 +275,7 @@ pub enum CallbackMsg { /// Assert that the total deposit amounts of the given denoms across Red /// Bank and Rover do not exceed their respective deposit caps. AssertDepositCaps { - denoms: BTreeSet, + denoms: BTreeMap>, }, /// Adds coin to a vault strategy EnterVault { diff --git a/schemas/mars-credit-manager/mars-credit-manager.json b/schemas/mars-credit-manager/mars-credit-manager.json index 8d815502..a8980cdb 100644 --- a/schemas/mars-credit-manager/mars-credit-manager.json +++ b/schemas/mars-credit-manager/mars-credit-manager.json @@ -1179,11 +1179,17 @@ ], "properties": { "denoms": { - "type": "array", - "items": { - "type": "string" - }, - "uniqueItems": true + "type": "object", + "additionalProperties": { + "anyOf": [ + { + "$ref": "#/definitions/Uint128" + }, + { + "type": "null" + } + ] + } } }, "additionalProperties": false diff --git a/scripts/types/generated/mars-credit-manager/MarsCreditManager.types.ts b/scripts/types/generated/mars-credit-manager/MarsCreditManager.types.ts index 562e9b0c..fc054df7 100644 --- a/scripts/types/generated/mars-credit-manager/MarsCreditManager.types.ts +++ b/scripts/types/generated/mars-credit-manager/MarsCreditManager.types.ts @@ -291,7 +291,9 @@ export type CallbackMsg = } | { assert_deposit_caps: { - denoms: string[] + denoms: { + [k: string]: Uint128 | null + } } } | {