From 500a75f21fc06c3faaf4d2926125f678aaed6cba Mon Sep 17 00:00:00 2001 From: Bob van der Helm <34470358+bobthebuidlr@users.noreply.github.com> Date: Tue, 25 Jun 2024 11:12:49 +0200 Subject: [PATCH] fix issues oak audit no 16 (#417) --- Cargo.lock | 1 + .../credit-manager/src/stake_astro_lp.rs | 6 +- .../tests/tests/test_stake_astro_lp.rs | 23 ++++++ contracts/vault/Cargo.toml | 1 + contracts/vault/src/error.rs | 7 ++ contracts/vault/src/instantiate.rs | 9 ++- .../vault/tests/tests/test_instantiate.rs | 80 +++++++++++++++++++ .../vault/tests/tests/test_performance_fee.rs | 11 +-- 8 files changed, 131 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 923887e2..7e69ec10 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2814,6 +2814,7 @@ dependencies = [ "mars-owner 2.0.0", "mars-testing", "mars-types", + "mars-utils 2.0.0", "osmosis-std 0.22.0", "test-case", "thiserror", diff --git a/contracts/credit-manager/src/stake_astro_lp.rs b/contracts/credit-manager/src/stake_astro_lp.rs index 2d22a7a0..92067749 100644 --- a/contracts/credit-manager/src/stake_astro_lp.rs +++ b/contracts/credit-manager/src/stake_astro_lp.rs @@ -5,7 +5,7 @@ use mars_types::{ }; use crate::{ - error::ContractResult, + error::{ContractError, ContractResult}, state::{COIN_BALANCES, INCENTIVES}, utils::{decrement_coin_balance, increment_coin_balance}, }; @@ -27,6 +27,10 @@ pub fn stake_lp(deps: DepsMut, account_id: &str, lp_coin: ActionCoin) -> Contrac ActionAmount::AccountBalance => coin_balance, }; + if new_amount.is_zero() { + return Err(ContractError::NoAmount); + }; + let updated_coin = coin(new_amount.u128(), lp_coin.denom.as_str()); decrement_coin_balance(deps.storage, account_id, &updated_coin)?; diff --git a/contracts/credit-manager/tests/tests/test_stake_astro_lp.rs b/contracts/credit-manager/tests/tests/test_stake_astro_lp.rs index 446cc1a8..ad1d2b59 100644 --- a/contracts/credit-manager/tests/tests/test_stake_astro_lp.rs +++ b/contracts/credit-manager/tests/tests/test_stake_astro_lp.rs @@ -1,4 +1,5 @@ use cosmwasm_std::{coins, Addr, Coin, Uint128}; +use mars_credit_manager::error::ContractError; use mars_testing::multitest::helpers::AccountToFund; use mars_types::credit_manager::{Action, ActionAmount, ActionCoin}; @@ -28,6 +29,28 @@ fn stake_claims_rewards() { amount: Uint128::new(200), }; + // should fail when trying to stake 0 amount + let err: ContractError = mock + .update_credit_account( + &account_id, + &user, + vec![ + Action::Deposit(lp_coin.clone()), + Action::StakeAstroLp { + lp_token: ActionCoin { + denom: lp_denom.to_string(), + amount: ActionAmount::Exact(Uint128::new(0)), + }, + }, + ], + &[lp_coin.clone()], + ) + .unwrap_err() + .downcast() + .unwrap(); + + assert_eq!(err, ContractError::NoAmount); + // stake mock.update_credit_account( &account_id, diff --git a/contracts/vault/Cargo.toml b/contracts/vault/Cargo.toml index 5d86a550..873d90b5 100644 --- a/contracts/vault/Cargo.toml +++ b/contracts/vault/Cargo.toml @@ -29,6 +29,7 @@ cw-paginate = { workspace = true } cw-vault-standard = { workspace = true } mars-owner = { workspace = true } mars-types = { workspace = true } +mars-utils = { workspace = true } thiserror = { workspace = true } osmosis-std = { workspace = true } diff --git a/contracts/vault/src/error.rs b/contracts/vault/src/error.rs index 77396889..ae85776e 100644 --- a/contracts/vault/src/error.rs +++ b/contracts/vault/src/error.rs @@ -4,6 +4,7 @@ use cosmwasm_std::{ }; use cw_utils::PaymentError; use mars_owner::OwnerError; +use mars_utils::error::ValidationError; #[derive(Debug, PartialEq, thiserror::Error)] pub enum ContractError { @@ -34,6 +35,9 @@ pub enum ContractError { #[error(transparent)] Payment(#[from] PaymentError), + #[error(transparent)] + Validation(#[from] ValidationError), + #[error("{0}")] Generic(String), @@ -71,6 +75,9 @@ pub enum ContractError { #[error("Withdrawal interval not passed")] WithdrawalIntervalNotPassed {}, + + #[error("Invalid cooldown period, expected value greater than 0")] + ZeroCooldownPeriod, } pub type ContractResult = Result; diff --git a/contracts/vault/src/instantiate.rs b/contracts/vault/src/instantiate.rs index 74d5c1b6..9e747552 100644 --- a/contracts/vault/src/instantiate.rs +++ b/contracts/vault/src/instantiate.rs @@ -1,8 +1,9 @@ use cosmwasm_std::{DepsMut, Env, MessageInfo, Response}; use mars_owner::OwnerInit; +use mars_utils::helpers::validate_native_denom; use crate::{ - error::ContractResult, + error::{ContractError, ContractResult}, msg::InstantiateMsg, performance_fee::PerformanceFeeState, state::{ @@ -42,6 +43,10 @@ pub fn init( DESCRIPTION.save(deps.storage, &desc)?; } + if msg.cooldown_period == 0 { + return Err(ContractError::ZeroCooldownPeriod {}); + } + COOLDOWN_PERIOD.save(deps.storage, &msg.cooldown_period)?; // initialize performance fee state @@ -53,6 +58,8 @@ pub fn init( let vault_token = TokenFactoryDenom::new(env.contract.address.to_string(), msg.vault_token_subdenom); VAULT_TOKEN.save(deps.storage, &vault_token)?; + + validate_native_denom(&msg.base_token)?; BASE_TOKEN.save(deps.storage, &msg.base_token)?; Ok(vault_token.instantiate()?) diff --git a/contracts/vault/tests/tests/test_instantiate.rs b/contracts/vault/tests/tests/test_instantiate.rs index f263e546..13dca05e 100644 --- a/contracts/vault/tests/tests/test_instantiate.rs +++ b/contracts/vault/tests/tests/test_instantiate.rs @@ -3,6 +3,7 @@ use std::str::FromStr; use anyhow::Result as AnyResult; use cosmwasm_std::{coin, Addr, Decimal}; use cw_multi_test::Executor; +use mars_utils::error::ValidationError; use mars_vault::{ error::ContractError, msg::{InstantiateMsg, VaultInfoResponseExt}, @@ -146,6 +147,85 @@ fn cannot_instantiate_with_invalid_performance_fee() { ); } +#[test] +fn cannot_instantiate_with_zero_cooldown_period() { + let fund_manager = Addr::unchecked("fund-manager"); + let mut mock = MockEnv::new() + .fund_account(AccountToFund { + addr: fund_manager.clone(), + funds: vec![coin(1_000_000_000, "untrn")], + }) + .build() + .unwrap(); + let credit_manager = mock.rover.clone(); + + let contract_code_id = mock.app.store_code(mock_managed_vault_contract()); + let res = mock.app.instantiate_contract( + contract_code_id, + fund_manager, + &InstantiateMsg { + base_token: "uusdc".to_string(), + vault_token_subdenom: "fund".to_string(), + title: None, + subtitle: None, + description: None, + credit_manager: credit_manager.to_string(), + cooldown_period: 0, + performance_fee_config: PerformanceFeeConfig { + fee_rate: Decimal::from_str("0.000046287042457350").unwrap(), + withdrawal_interval: 1563, + }, + }, + &[coin(10_000_000, "untrn")], // Token Factory fee for minting new denom. Configured in the Token Factory module in `mars-testing` package. + "mock-managed-vault", + None, + ); + + assert_vault_err(res, ContractError::ZeroCooldownPeriod {}); +} + +#[test] +fn cannot_instantiate_with_invalid_base_denom() { + let fund_manager = Addr::unchecked("fund-manager"); + let mut mock = MockEnv::new() + .fund_account(AccountToFund { + addr: fund_manager.clone(), + funds: vec![coin(1_000_000_000, "untrn")], + }) + .build() + .unwrap(); + let credit_manager = mock.rover.clone(); + + let contract_code_id = mock.app.store_code(mock_managed_vault_contract()); + let res = mock.app.instantiate_contract( + contract_code_id, + fund_manager, + &InstantiateMsg { + base_token: "!*jadfaefc".to_string(), + vault_token_subdenom: "fund".to_string(), + title: None, + subtitle: None, + description: None, + credit_manager: credit_manager.to_string(), + cooldown_period: 24, + performance_fee_config: PerformanceFeeConfig { + fee_rate: Decimal::zero(), + withdrawal_interval: 0, + }, + }, + &[coin(10_000_000, "untrn")], // Token Factory fee for minting new denom. Configured in the Token Factory module in `mars-testing` package. + "mock-managed-vault", + None, + ); + + assert_vault_err( + res, + ContractError::Validation(ValidationError::InvalidDenom { + reason: "First character is not ASCII alphabetic".to_string(), + }), + ); +} + fn assert_vault_err(res: AnyResult, err: ContractError) { match res { Ok(_) => panic!("Result was not an error"), diff --git a/contracts/vault/tests/tests/test_performance_fee.rs b/contracts/vault/tests/tests/test_performance_fee.rs index c3a3d2c1..f54be5d5 100644 --- a/contracts/vault/tests/tests/test_performance_fee.rs +++ b/contracts/vault/tests/tests/test_performance_fee.rs @@ -124,7 +124,7 @@ fn cannot_withdraw_zero_performance_fee() { &mut mock.app, &fund_manager, &credit_manager, - 0, + 1, PerformanceFeeConfig { fee_rate: Decimal::from_str("0.0000208").unwrap(), withdrawal_interval: 60, @@ -171,7 +171,7 @@ fn cannot_withdraw_if_withdrawal_interval_not_passed() { &mut mock.app, &fund_manager, &credit_manager, - 0, + 1, PerformanceFeeConfig { fee_rate: Decimal::from_str("0.0000208").unwrap(), withdrawal_interval: performance_fee_interval, @@ -277,7 +277,7 @@ fn performance_fee_correctly_accumulated() { &mut mock.app, &fund_manager, &credit_manager, - 0, + 1, PerformanceFeeConfig { fee_rate: Decimal::from_str("0.0000208").unwrap(), withdrawal_interval: 60, @@ -399,6 +399,9 @@ fn performance_fee_correctly_accumulated() { // -- FOURTH ACTION -- + let unlock_vault_tokens = Uint128::new(10_000_000_000_000); + execute_unlock(&mut mock, &user, &managed_vault_addr, unlock_vault_tokens, &[]).unwrap(); + // move by 144 hours mock.increment_by_time(144 * 60 * 60); @@ -408,8 +411,6 @@ fn performance_fee_correctly_accumulated() { let pnl = calculate_pnl(&mut mock, &fund_acc_id, Decimal::from_str("4.9375").unwrap()); assert_eq!(pnl, Uint128::new(450_000_000)); - let unlock_vault_tokens = Uint128::new(10_000_000_000_000); - execute_unlock(&mut mock, &user, &managed_vault_addr, unlock_vault_tokens, &[]).unwrap(); execute_redeem( &mut mock, &user,