Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the MANTRA DEX smart contract system. The audit took place from November 29, 2024 to January 13, 2025.

This audit was judged by 3docSec.

Final report assembled by Code4rena.

Summary

The C4 analysis yielded an aggregated total of 31 unique vulnerabilities. Of these vulnerabilities, 12 received a risk rating in the category of HIGH severity and 19 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 11 reports detailing issues with a risk rating of LOW severity or non-critical.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 MANTRA DEX repository, and is composed of 8 smart contracts written in the Rust programming language and includes 22,268 lines of Rust code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (12)

[H-01] Protocol allows creating broken tri-crypto CPMM pools

Submitted by carrotsmuggler, also found by 0xAlix2, Abdessamed, DadeKuma, DadeKuma, DadeKuma, gegul, LonnyFlash, and Tigerfrake

/contracts/pool-manager/src/manager/commands.rs#L75

Finding description and impact

The protocol allows the creation of constant product pools and stableswap pools. Stable-swap pools, as established by curve, can have any number of tokens and so the protocol allows for the creation of pools with 2 or more tokens.

Constant product market-makers (CPMM), however, can have multiple tokens as well; however, the protocol here uses the uniswap formula, which only works for 2-token pools. For pools with more than 2 tokens, this model does not work anymore, and invariants need to be established with different formulas with products of all tokens quantities, like shown in the balancer protocol.

The issue is that the protocol here does not check if the constant product pool being created has more than 2 tokens. Surprisingly, it is perfectly possible to create a constant product pool with 3 tokens, add/remove liquidity and even do swaps in them, even though the protocol was never designed to handle this.

The POC below will show how we can set up a 3-token CPMM pool, add liquidity and even do swaps in it. The issue is that these pools are completely broken and should not be allowed.

The compute_swap function in the helpers.rs contract calculates the number of output tokens given the number of input tokens.

// ask_amount = (ask_pool * offer_amount / (offer_pool + offer_amount)) - swap_fee - protocol_fee - burn_fee
let return_amount: Uint256 =
    Decimal256::from_ratio(ask_pool.mul(offer_amount), offer_pool + offer_amount)
        .to_uint_floor();

But these are only valid for 2-token uniswap-style pools. If there are more than 2 tokens involved, the invariant changes from being x * y = k to x * y * z = k, and the formula above does not work anymore. So for multi token pools, this formula should not be used, or x-y swaps can be arbitraged off of with y-z swaps and vice versa.

Furthermore, there is a check in the assert_slippage_tolerance function in the helpers contract:

if deposits.len() != 2 || pools.len() != 2 {
    return Err(ContractError::InvalidPoolAssetsLength {
        expected: 2,
        actual: deposits.len(),
    });
}

This explicitly shows that constant product pools are only allowed to have 2 tokens. However, if no slippage tolerance is specified, this check can be completely bypassed.

pub fn assert_slippage_tolerance(
    slippage_tolerance: &Option<Decimal>,
    deposits: &[Coin],
    pools: &[Coin],
    pool_type: PoolType,
    amount: Uint128,
    pool_token_supply: Uint128,
) -> Result<(), ContractError> {
    if let Some(slippage_tolerance) = *slippage_tolerance {
        //@audit check for number of tokens
    }

By never sending a slippage tolerance, users can create, add/remove liquidity and even do swaps in pools with more than 2 tokens following constant product algorithm. But these pools are completely broken and should not be allowed since the invariants are not functioning correctly

Proof of Concept

The POC below creates a CPMM pool with 3 tokens - uwhale, uluna and uusd. It is shown that liquidity can be added and swaps can be performed.

First, some helper functions are needed to check and print out the token balances.

fn print_diff(init_bal: [Uint128; 4], final_bal: [Uint128; 4]) -> [i128; 4] {
    let diffs = [
        final_bal[0].u128() as i128 - init_bal[0].u128() as i128,
        final_bal[1].u128() as i128 - init_bal[1].u128() as i128,
        final_bal[2].u128() as i128 - init_bal[2].u128() as i128,
        final_bal[3].u128() as i128 - init_bal[3].u128() as i128,
    ];

    println!("==Balance deltas==");
    if diffs[0] != 0 {
        println!("uwhale delta: {}", diffs[0]);
    }
    if diffs[1] != 0 {
        println!("uluna delta : {}", diffs[1]);
    }
    if diffs[2] != 0 {
        println!("uusd delta  : {}", diffs[2]);
    }
    if diffs[3] != 0 {
        println!("lp delta    : {}", diffs[3]);
    }
    println!("==Balance deltas==\n");

    diffs
}
fn calc_state(suite: &mut TestingSuite, creator: &str) -> [Uint128; 4] {
    let uwhale_balance = RefCell::new(Uint128::zero());
    let uluna_balance = RefCell::new(Uint128::zero());
    let uusd_balance = RefCell::new(Uint128::zero());
    let lp_shares = RefCell::new(Uint128::zero());

    suite.query_balance(&creator.to_string(), "uwhale".to_string(), |result| {
        *uwhale_balance.borrow_mut() = result.unwrap().amount;
    });

    suite.query_balance(&creator.to_string(), "uluna".to_string(), |result| {
        *uluna_balance.borrow_mut() = result.unwrap().amount;
    });

    suite.query_balance(&creator.to_string(), "uusd".to_string(), |result| {
        *uusd_balance.borrow_mut() = result.unwrap().amount;
    });

    suite.query_all_balances(&creator.to_string(), |balances| {
        for coin in balances.unwrap().iter() {
            if coin.denom.contains("o.whale.uluna") {
                *lp_shares.borrow_mut() = coin.amount;
            }
        }
    });

    let uwhale = *uwhale_balance.borrow();
    let uluna = *uluna_balance.borrow();
    let uusd = *uusd_balance.borrow();
    let lp = *lp_shares.borrow();
    [uwhale, uluna, uusd, lp]
}

And here’s the actual test:

fn const_prod_test() {
    let mut suite = TestingSuite::default_with_balances(
        vec![
            coin(1_000_000_000u128, "uwhale".to_string()),
            coin(1_000_000_000u128, "uluna".to_string()),
            coin(1_000_000_000u128, "uusd".to_string()),
            coin(1_000_000_000u128, "uom".to_string()),
        ],
        StargateMock::new("uom".to_string(), "8888".to_string()),
    );
    let creator = suite.creator();
    let _other = suite.senders[1].clone();
    let _unauthorized = suite.senders[2].clone();
    // Asset infos with uwhale and uluna

    let first_pool = vec![
        "uwhale".to_string(),
        "uluna".to_string(),
        "uusd".to_string(),
    ];

    let pool_fees = PoolFee {
        protocol_fee: Fee {
            share: Decimal::bps(50), // 0.5%
        },
        swap_fee: Fee {
            share: Decimal::bps(50), // 0.5%
        },
        burn_fee: Fee {
            share: Decimal::bps(50), // 0.5%
        },
        extra_fees: vec![],
    };

    // Create a pool
    suite.instantiate_default().add_one_epoch().create_pool(
        &creator,
        first_pool,
        vec![6u8, 6u8, 6u8],
        pool_fees.clone(),
        PoolType::ConstantProduct,
        Some("whale.uluna".to_string()),
        vec![coin(1000, "uusd"), coin(8888, "uom")],
        |result| {
            result.unwrap();
        },
    );

    println!("===Liq addition===");
    let initial_balances = calc_state(&mut suite, &creator.to_string());
    suite.provide_liquidity(
        &creator,
        "o.whale.uluna".to_string(),
        None,
        None,
        None,
        None,
        vec![
            Coin {
                denom: "uwhale".to_string(),
                amount: Uint128::from(1_000_000u128),
            },
            Coin {
                denom: "uluna".to_string(),
                amount: Uint128::from(1_000_000u128),
            },
            Coin {
                denom: "uusd".to_string(),
                amount: Uint128::from(1_000_000u128),
            },
        ],
        |result| {
            result.unwrap();
        },
    );
    let final_balances = calc_state(&mut suite, &creator.to_string());
    let diffs = print_diff(initial_balances, final_balances);

    let swap_operations = vec![amm::pool_manager::SwapOperation::MantraSwap {
        token_in_denom: "uwhale".to_string(),
        token_out_denom: "uusd".to_string(),
        pool_identifier: "o.whale.uluna".to_string(),
    }];

    println!("===Swap===");
    let initial_balances = calc_state(&mut suite, &creator.to_string());
    suite.execute_swap_operations(
        &creator,
        swap_operations,
        None,
        None,
        None,
        vec![coin(1000u128, "uwhale".to_string())],
        |result| {
            result.unwrap();
        },
    );
    let final_balances = calc_state(&mut suite, &creator.to_string());
    let diffs = print_diff(initial_balances, final_balances);
}

The test runs fine and here’s the output:

running 1 test
===Liq addition===
==Balance deltas==
uwhale delta: -1000000
uluna delta : -1000000
uusd delta  : -1000000
lp delta    : 999000
==Balance deltas==

===Swap===
==Balance deltas==
uwhale delta: -1000
uusd delta  : 987
==Balance deltas==

It shows:

1. Liquidity addition of 1e6 uwhale, 1e6 uluna 1e6 uusd and minting of 999k LP tokens.
2. Swapping 1e3 uwhale for 987 uusd.

While the swap is correctly functioning here, it doesn’t maintain the correct pool invariant and can be arbitraged off of when the pools grow imbalanced.

Recommended mitigation steps

Add an explicit check during pool creation to make sure constant product pools cannot have more than 2 tokens.

jvr0x (MANTRA) confirmed

[H-02] Logical error in validate_fees_are_paid can cause a DoS or allow users to bypass fees if denom_creation_fee includes multiple coins, including pool_creation_fee, and the user attempts to pay all fees using only pool_creation_fee

Submitted by 0xRajkumar, also found by 0xAlix2, carrotsmuggler, Egis_Security, Egis_Security, jasonxiale, Lambda, oakcobalt, Tigerfrake, Tigerfrake, and Tigerfrake

/contracts/pool-manager/src/helpers.rs#L561-L592

Finding description and Impact

When a user creates a pool, they must pay both denom_creation_fee and pool_creation_fee.

The denom_creation_fee can be paid using multiple coins or a single coin and may also include the same coin as pool_creation_fee. If multiple denom_creation_fee coins options are available, and one of them matches the coin used for pool_creation_fee, it can lead to issues.

Problem Scenario

The issue arises when the user attempts to pay both fees using the same coin.

1. Different Fee Amounts: If the user pays both fees in the same coin, with different amounts for denom_creation_fee and pool_creation_fee, they might add both amounts and send the total. When validating the pool_creation_fee, the check paid_pool_fee_amount == pool_creation_fee.amount will fail, causing a DoS.

            ensure!(
                paid_pool_fee_amount == pool_creation_fee.amount,
                ContractError::InvalidPoolCreationFee {
                    amount: paid_pool_fee_amount,
                    expected: pool_creation_fee.amount,
                }
            );

2. Same Fee Amounts: If both fees have the same amount and the user pays only once, they can bypass one of the fees entirely, resulting in a fee payment bypass.

            ensure!(
                paid_pool_fee_amount == pool_creation_fee.amount, //-> HERE It will pass
                ContractError::InvalidPoolCreationFee {
                    amount: paid_pool_fee_amount,
                    expected: pool_creation_fee.amount,
                }
            );

            total_fees.push(Coin {
                denom: pool_fee_denom.clone(),
                amount: paid_pool_fee_amount,
            });

            // Check if the user paid the token factory fee in any other of the allowed denoms
            let tf_fee_paid = denom_creation_fee.iter().any(|fee| {
                let paid_fee_amount = info
                    .funds
                    .iter()
                    .filter(|fund| fund.denom == fee.denom)
                    .map(|fund| fund.amount)
                    .try_fold(Uint128::zero(), |acc, amount| acc.checked_add(amount))
                    .unwrap_or(Uint128::zero());

                total_fees.push(Coin {
                    denom: fee.denom.clone(),
                    amount: paid_fee_amount,
                });

                paid_fee_amount == fee.amount  //-> HERE It will pass
            });

As both are equal, that’s why both checks will pass. The impact is High as it can cause a DoS and allow the bypass of one of the fees.

Proof of Concept

How it happens when amounts are different:

1. The user sends a transaction to create a pool by combining both fee amounts into a single payment.
2. The transaction reverts because the check paid_pool_fee_amount == pool_creation_fee.amount evaluates to false.
3. If the user attempts to bypass this, the next check for denom_creation_fee will also fail.

How it happens when amounts are the same:

1. The attacker will send only one amount because both checks (for pool_creation_fee and denom_creation_fee) will pass, as both amounts are equal. This allows the attacker to pay only once.

Recommended mitigation steps

We can verify whether the user is paying with one coin or multiple coins. If the user is paying with one coin, we can combine both amounts and perform the validation. Similarly, if the user is paying with multiple coins, we can apply the same approach. This will effectively mitigate the issue.

jvr0x (MANTRA) confirmed and commented:

It is valid. However, considering the chain only supports 1 token to pay for the token factory at the moment, I wouldn’t deem it as high, but low.

3docSec (judge) commented:

I see your point, and while I would agree if this were a bug bounty program (funds are not at risk in live contracts), I consider this a High, because what counts is the code in-scope and not the live config, unless the in-scope code is hardcoded to have only one token and can’t be changed by config.

[H-03] Multi-token stableswap pools allow 0 liquidity for tokens, creating bricked pools

Submitted by carrotsmuggler, also found by 0xAlix2, 0xRajkumar, Abdessamed, carrotsmuggler, and LonnyFlash

/contracts/pool-manager/src/liquidity/commands.rs#L46-L74

/contracts/pool-manager/src/liquidity/commands.rs#L234-L239

Finding description and impact

The stableswap pools allow anyone to create pools following the stableswap formula with any number of tokens. This can be higher than 2. The issue is that the initial provide_liquidity does not check if ALL tokens are provided.

The provide_liquidity function does a number of checks. For the ConstantProduct pools, the constant product part uses both deposits[0] and deposits[1] to calculate the initial number of shares, and uses a product of the two. So anyone being absent or 0 leads to reverts during the initial liquidity addition itself.

However, the stableswap pools do not check if the initial liquidity provided is non-zero for all the tokens. So if only 2 of the three tokens are provided, the transaction still goes through. The only check is that all the passed in tokens must be pool constituents.

ensure!(
        deposits.iter().all(|asset| pool_assets
            .iter()
            .any(|pool_asset| pool_asset.denom == asset.denom)),
        ContractError::AssetMismatch
    );

This leads to a broken pool, where further liquidity cannot be added anymore. This is because the pool is saved in a state where the pool has 0 liquidity for one of the tokens. Then in future liquidity additions, amount_times_coins value evaluates to 0 for those tokens, which eventually leads to a division by zero error in d_prod calculation.

let amount_times_coins: Vec<Uint128> = deposits
    .iter()
    .map(|coin| coin.amount.checked_mul(n_coins).unwrap())
    .collect();

// ...

for _ in 0..256 {
    let mut d_prod = d;
    for amount in amount_times_coins.clone().into_iter() {
        d_prod = d_prod
            .checked_mul(d)
            .unwrap()
            .checked_div(amount.into()) //@audit division by zero
            .unwrap();
// ...

Thus this leads to a broken pool and there is nothing in the contract preventing this.

Proof of Concept

Attached is a POC where a pool is created with 3 tokens [uwhale,uluna,uusd] but only 2 tokens are provided in the initial liquidity addition [uwhale, ulune]. Further liquidity additions revert due to division by zero error.

fn multiswap_test() {
    let mut suite = TestingSuite::default_with_balances(
        vec![
            coin(1_000_000_001u128, "uwhale".to_string()),
            coin(1_000_000_000u128, "uluna".to_string()),
            coin(1_000_000_001u128, "uusd".to_string()),
            coin(1_000_000_001u128, "uom".to_string()),
        ],
        StargateMock::new("uom".to_string(), "8888".to_string()),
    );
    let creator = suite.creator();
    let _other = suite.senders[1].clone();
    let _unauthorized = suite.senders[2].clone();

    let asset_infos = vec![
        "uwhale".to_string(),
        "uluna".to_string(),
        "uusd".to_string(),
    ];

    // Protocol fee is 0.01% and swap fee is 0.02% and burn fee is 0%
    let pool_fees = PoolFee {
        protocol_fee: Fee {
            share: Decimal::from_ratio(1u128, 1000u128),
        },
        swap_fee: Fee {
            share: Decimal::from_ratio(1u128, 10_000_u128),
        },
        burn_fee: Fee {
            share: Decimal::zero(),
        },
        extra_fees: vec![],
    };

    // Create a pool
    suite.instantiate_default().create_pool(
        &creator,
        asset_infos,
        vec![6u8, 6u8, 6u8],
        pool_fees,
        PoolType::StableSwap { amp: 100 },
        Some("whale.uluna.uusd".to_string()),
        vec![coin(1000, "uusd"), coin(8888, "uom")],
        |result| {
            result.unwrap();
        },
    );

    // Add liquidity with only 2 tokens
    suite.provide_liquidity(
        &creator,
        "o.whale.uluna.uusd".to_string(),
        None,
        None,
        None,
        None,
        vec![
            Coin {
                denom: "uwhale".to_string(),
                amount: Uint128::from(1_000_000u128),
            },
            Coin {
                denom: "uluna".to_string(),
                amount: Uint128::from(1_000_000u128),
            },
        ],
        |result| {
            result.unwrap();
        },
    );
}

The above test passes, showing liquidity can be added with 2 tokens only. Further liquidity provision reverts.

// Add liquidity again
suite.provide_liquidity(
    &creator,
    "o.whale.uluna.uusd".to_string(),
    None,
    None,
    None,
    None,
    vec![
        Coin {
            denom: "uwhale".to_string(),
            amount: Uint128::from(1_000_000u128),
        },
        Coin {
            denom: "uluna".to_string(),
            amount: Uint128::from(1_000_000u128),
        },
    ],
    |result| {
        result.unwrap();
    },
);

Output:

thread 'tests::integration_tests::provide_liquidity::multiswap_test' panicked at contracts/pool-manager/src/helpers.rs:737:22:
called `Result::unwrap()` on an `Err` value: DivideByZeroError
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tests::integration_tests::provide_liquidity::multiswap_test ... FAILED

Recommended mitigation steps

Add a check to make sure if total supply=0, every token of the pool is provided as liquidity.

jvr0x (MANTRA) confirmed

[H-04] Block gas limit can be hit due to loop depth

Submitted by carrotsmuggler, also found by 0xAlix2, Evo, and Lambda

/contracts/farm-manager/src/farm/commands.rs#L43-L94

Finding description and impact

The claim function iterates over the user positions and calculates the rewards in nested loops. The issue is that every blockchain, to combat against gas attacks of infinite loops, has a block gas limit. If this limit is exceeded, that transaction cannot be included in the chain. The implementation of the claim function here is of the order of N^3 and is thus highly susceptible to an out of gas error.

The claim function iterates over all the user’s positions.

let lp_denoms = get_unique_lp_asset_denoms_from_positions(open_positions);

for lp_denom in &lp_denoms {
    // calculate the rewards for the lp denom
    let rewards_response = calculate_rewards(
        deps.as_ref(),
        &env,
        lp_denom,
        &info.sender,
        current_epoch.id,
        true,
    )?;
    //...
}

Lets say the user has P positions, all of different lp_deonm values. Thus this loop is of the order of P. The calculate_rewards function then loops over all the farms of each lp_denom.

let farms = get_farms_by_lp_denom(
    deps.storage,
    lp_denom,
    None,
    Some(config.max_concurrent_farms),
)?;
//...
for farm in farms {
    // skip farms that have not started
    if farm.start_epoch > current_epoch_id {
        continue;
    }

    // compute where the user can start claiming rewards for the farm
    let start_from_epoch = compute_start_from_epoch_for_address(
        deps.storage,
        &farm.lp_denom,
        last_claimed_epoch_for_user,
        receiver,
    )?;
    //...
}

Say there are F farms, then this inner loop is of the order of F. Then for each farm, the reward is calculated by iterating over all the epochs from start_from_epoch up to the current_epoch.

for epoch_id in start_from_epoch..=until_epoch {
    if farm.start_epoch > epoch_id {
        continue;
    }
    //...
}

The start_from_epoch can be the very first deposit of the user, far back in time, if this is the first time the user is claiming rewards. Thus, this loop can run very long if the position is years old. Say the epoch loop is of the order of E.

Since these 3 loops are nested, the claim function is of the order of P*F*E. P and F are restricted by the config can can have maximum values of the order of 10. But E can be very large, and is actually the order of epoch number. So if epochs are only a few days long, the E can be of the order of 500 over a couple of years.

Thus the claim function can be of the order of 50_000. This is an issue since it requires a loop running 50_000 times along with reward calculations and even token transfers. This can be above the block gas limit and thus the transaction will fail.

There is no functionality to skip positions/farms/epochs. Thus users cannot claim rewards of only a few particular farms or epochs. This part of the code is also executed during the close_position function, which checks if rewards are 0. Thus, the close_position function can also fail due to the same issue, and users are thus forced to emergency withdraw and lose deposits as well as their rewards.

Thus users who join a bunch of different farms and keep their positions for a long time can hit the block gsa limit during the time of claiming rewards or closing positions.

The OOG issue due to large nesting depth is present in multiple instances in the code, this is only one example.

Proof of Concept

P, the number of open positions of a user, is restricted by limit of 100 stored in MAX_ITEMS_LIMIT. F is restricted by the max concurrent no of farms per lp_denom, which we can assume to be 10. E is of the order of epochs between the first deposit and the current epoch, which can be in the 100s if epochs are single days, or 100s if epochs are weeks.

Thus, P*F*E is of the order of 100*10*100 = 100_000; 100_000 iterations are required for the claim function on top of token transfers and math calculations. This can easily exceed the block gas limit.

Recommended mitigation steps

The order of the nested loops need to be decreased. This can be done in multiple ways.

1. Implement sushi-masterchef style reward accounting. This way the entire E number of epochs dont need to be looped over.
2. Implement a way to only process a given number of positions. This way P can also be restricted and users can claim in batches.

jvr0x (MANTRA) confirmed

[H-05] Farms can be created to start in past epochs

Submitted by Abdessamed, also found by 0xAlix2, carrotsmuggler, Lambda, and Tigerfrake

/contracts/farm-manager/src/helpers.rs#L128-L175

Finding description and impact

In the farming mechanism, users can claim rewards from active farms based on their locked LP token share. The rewards distribution must adhere to the following invariant:

At any given epoch, all users with locked LP tokens claim rewards from corresponding farms proportional to their share of the total LP tokens.

However, the current implementation allows the creation of farms with a start_epoch in the past. This breaks the invariant, as users who have already claimed rewards for past epochs will miss out on additional rewards assigned retroactively to those epochs. This issue arises because the validate_farm_epochs function does not enforce that the farm’s start epoch must be in the future relative to the current epoch:

/// Validates the farm epochs. Returns a tuple of (start_epoch, end_epoch) for the farm.
pub(crate) fn validate_farm_epochs(
    params: &FarmParams,
    current_epoch: u64,
    max_farm_epoch_buffer: u64,
) -> Result<(u64, u64), ContractError> {
     let start_epoch = params.start_epoch.unwrap_or(current_epoch + 1u64);

    ensure!(
        start_epoch > 0u64,
        ContractError::InvalidEpoch {
            which: "start".to_string()
        }
    );

    let preliminary_end_epoch = params.preliminary_end_epoch.unwrap_or(
        start_epoch
            .checked_add(DEFAULT_FARM_DURATION)
            .ok_or(ContractError::InvalidEpoch {
                which: "end".to_string(),
            })?,
    );

    // ensure that start date is before end date
    ensure!(
        start_epoch < preliminary_end_epoch,
        ContractError::FarmStartTimeAfterEndTime
    );

    // ensure the farm is set to end in a future epoch
    ensure!(
        preliminary_end_epoch > current_epoch,
        ContractError::FarmEndsInPast
    );

    // ensure that start date is set within buffer
    ensure!(
        start_epoch
            <= current_epoch.checked_add(max_farm_epoch_buffer).ok_or(
                ContractError::OverflowError(OverflowError {
                    operation: OverflowOperation::Add
                })
            )?,
        ContractError::FarmStartTooFar
    );

    Ok((start_epoch, preliminary_end_epoch))
}

The function lacks a check to ensure that start_epoch is not earlier than current_epoch + 1, allowing farms to be created retroactively. This leads to unfair rewards distribution.

Proof of Concept

The following test case demonstrates that a farm can be created in such a way that it starts in a past epoch, copy and paste the following test to /contracts/farm-manager/tests/integration.rs:

#[test]
fn poc_farm_can_be_created_in_the_past() {
    let lp_denom = format!("factory/{MOCK_CONTRACT_ADDR_1}/{LP_SYMBOL}").to_string();
    let invalid_lp_denom = format!("factory/{MOCK_CONTRACT_ADDR_2}/{LP_SYMBOL}").to_string();

    let mut suite = TestingSuite::default_with_balances(vec![
        coin(1_000_000_000u128, "uom".to_string()),
        coin(1_000_000_000u128, "uusdy".to_string()),
        coin(1_000_000_000u128, "uosmo".to_string()),
        coin(1_000_000_000u128, lp_denom.clone()),
        coin(1_000_000_000u128, invalid_lp_denom.clone()),
    ]);
    suite.instantiate_default();

    let creator = suite.creator().clone();
    let other = suite.senders[1].clone();
    let fee_collector = suite.fee_collector_addr.clone();

    for _ in 0..10 {
        suite.add_one_epoch();
    }
    // current epoch is 10

    // We can create a farm in a past epoch
    suite
        .manage_farm(
            &other,
            FarmAction::Fill {
                params: FarmParams {
                    lp_denom: lp_denom.clone(),
                    start_epoch: Some(1), // @audit Notice, start epoch in the past
                    preliminary_end_epoch: Some(28),
                    curve: None,
                    farm_asset: Coin {
                        denom: "uusdy".to_string(),
                        amount: Uint128::new(4_000u128),
                    },
                    farm_identifier: Some("farm_1".to_string()),
                },
            },
            vec![coin(4_000, "uusdy"), coin(1_000, "uom")],
            |result| {
                result.unwrap();
            },
        );
}

The transaction passes without reverting, creating a farm that starts in a past epoch.

Recommended mitigation steps

Ensure the start_epoch is always in the future relative to the current_epoch:

/// Validates the farm epochs. Returns a tuple of (start_epoch, end_epoch) for the farm.
pub(crate) fn validate_farm_epochs(
    params: &FarmParams,
    current_epoch: u64,
    max_farm_epoch_buffer: u64,
) -> Result<(u64, u64), ContractError> {
    let start_epoch = params.start_epoch.unwrap_or(current_epoch + 1u64);
+   assert!(start_epoch >= current_epoch + 1);
    // --SNIP
}

jvr0x (MANTRA) confirmed

[H-06] Stable swap pools don’t properly handle assets with different decimals, forcing LPs to receive wrong shares

Submitted by 0xAlix2, also found by 0x1982us, Abdessamed, carrotsmuggler, and oakcobalt

Stable swap pools in Mantra implement Curve’s stable swap logic, this is mentioned in the docs. Curve normalizes the tokens in a stable swap pool, by having something called rate multipliers where they’re used to normalize the tokens’ decimals. This is critical as it is used in D computation here.

The reflection of this in Mantra is compute_d, where it does something similar, here:

// sum(x_i), a.k.a S
let sum_x = deposits
    .iter()
    .fold(Uint128::zero(), |acc, x| acc.checked_add(x.amount).unwrap());

However, the issue is that amounts are not normalized from the caller, where this is called from compute_lp_mint_amount_for_stableswap_deposit:

#[allow(clippy::unwrap_used, clippy::too_many_arguments)]
pub fn compute_lp_mint_amount_for_stableswap_deposit(
    amp_factor: &u64,
    old_pool_assets: &[Coin],
    new_pool_assets: &[Coin],
    pool_lp_token_total_supply: Uint128,
) -> Result<Option<Uint128>, ContractError> {
    // Initial invariant
@>  let d_0 = compute_d(amp_factor, old_pool_assets).ok_or(ContractError::StableInvariantError)?;

    // Invariant after change, i.e. after deposit
    // notice that new_pool_assets already added the new deposits to the pool
@>  let d_1 = compute_d(amp_factor, new_pool_assets).ok_or(ContractError::StableInvariantError)?;

    // If the invariant didn't change, return None
    if d_1 <= d_0 {
        Ok(None)
    } else {
        let amount = Uint512::from(pool_lp_token_total_supply)
            .checked_mul(d_1.checked_sub(d_0)?)?
            .checked_div(d_0)?;
        Ok(Some(Uint128::try_from(amount)?))
    }
}

This messes up the whole shares calculation logic, as D would be way greater for LPs depositing tokens of higher decimals than other tokens in the same stable swap pool.

NB: This is handled for swaps, here.

Proof of Concept

Add the following in contracts/pool-manager/src/tests/integration_tests.rs:

The following test creates a stable swap pool with 3 assets, 2 of them have 6 decimals, while the 3rd has 18 decimals. Initially, the same amount * asset decimals of each asset is deposited, depositing the same amount of the 18 decimal token results in an exaggerated amount of shares minted to the LP.

To double check this, you can try changing uweth’s decimals to 6, and confirm that both test cases result in equal number of shares, unlike the current implementation, where the difference is huge.

fn setup() -> (TestingSuite, Addr, Addr, String) {
    let mut suite = TestingSuite::default_with_balances(
        vec![
            coin(1_000u128 * 10u128.pow(6), "uluna".to_string()),
            coin(1_000u128 * 10u128.pow(6), "uusd".to_string()),
            coin(1_000u128 * 10u128.pow(18), "uweth".to_string()),
            coin(10_000u128, "uom".to_string()),
        ],
        StargateMock::new("uom".to_string(), "8888".to_string()),
    );

    let creator = suite.creator();
    let user = suite.senders[1].clone();

    suite.instantiate_default().add_one_epoch().create_pool(
        &creator,
        vec!["uluna".to_string(), "uusd".to_string(), "uweth".to_string()],
        vec![6u8, 6u8, 18u8],
        PoolFee {
            protocol_fee: Fee {
                share: Decimal::zero(),
            },
            swap_fee: Fee {
                share: Decimal::zero(),
            },
            burn_fee: Fee {
                share: Decimal::zero(),
            },
            extra_fees: vec![],
        },
        PoolType::StableSwap { amp: 100 },
        Some("uluna.uusd.uweth".to_string()),
        vec![coin(1000, "uusd"), coin(8888, "uom")],
        |result| {
            result.unwrap();
        },
    );

    let lp_denom = suite.get_lp_denom("o.uluna.uusd.uweth".to_string());

    suite.provide_liquidity(
        &creator,
        "o.uluna.uusd.uweth".to_string(),
        None,
        None,
        None,
        None,
        vec![
            Coin {
                denom: "uluna".to_string(),
                amount: Uint128::from(10u128 * 10u128.pow(6)),
            },
            Coin {
                denom: "uusd".to_string(),
                amount: Uint128::from(10u128 * 10u128.pow(6)),
            },
            Coin {
                denom: "uweth".to_string(),
                amount: Uint128::from(10u128 * 10u128.pow(18)),
            },
        ],
        |result| {
            result.unwrap();
        },
    );

    (suite, creator, user, lp_denom)
}

#[test]
fn wrong_handling_of_decimals_on_stableswap_deposit_1() {
    let (mut suite, _, user, lp_denom) = setup();

    suite
        .provide_liquidity(
            &user,
            "o.uluna.uusd.uweth".to_string(),
            None,
            None,
            None,
            None,
            vec![
                Coin {
                    denom: "uluna".to_string(),
                    amount: Uint128::from(2u128 * 10u128.pow(6)),
                },
                Coin {
                    denom: "uweth".to_string(),
                    amount: Uint128::from(2u128 * 10u128.pow(18)),
                },
            ],
            |result| {
                result.unwrap();
            },
        )
        .query_balance(&user.to_string(), lp_denom.clone(), |result| {
            assert_eq!(
                result.unwrap().amount,
                Uint128::from(13_901_163_096_216u128)
            );
        });
}

#[test]
fn wrong_handling_of_decimals_on_stableswap_deposit_2() {
    let (mut suite, _, user, lp_denom) = setup();

    suite
        .provide_liquidity(
            &user,
            "o.uluna.uusd.uweth".to_string(),
            None,
            None,
            None,
            None,
            vec![
                Coin {
                    denom: "uluna".to_string(),
                    amount: Uint128::from(2u128 * 10u128.pow(6)),
                },
                Coin {
                    denom: "uusd".to_string(),
                    amount: Uint128::from(2u128 * 10u128.pow(6)),
                },
            ],
            |result| {
                result.unwrap();
            },
        )
        .query_balance(&user.to_string(), lp_denom.clone(), |result| {
            assert_eq!(result.unwrap().amount, Uint128::from(9_054_673_799_013u128));
        });
}

Recommended mitigation steps

Whenever computing D, make sure all the deposits/amounts are in the “non-decimal” value, i.e., without decimals. For example, 100e6 should just be sent as 100, just like how it’s done in compute_swap. This should be added in compute_d.

jvr0x (MANTRA) confirmed

[H-07] User cannot claim rewards or close_position, due to vulnerable division by zero handling

Submitted by oakcobalt, also found by 0xAlix2, Daniel526, Lambda, and Tigerfrake

A user cannot claim rewards or close_position, due to vulnerable division by zero handling in the claim -> calculate_rewards flow.

In calculate_rewards, a user’s reward per farm per epoch is based on the user_share (user_weight/contract_weights); contract_weights can be zero.

The main vulnerability is division by zero handling is not done at the site of division; i.e., no check on contract_weights is non-zero before using it as a denominator in checked_mul_floor. (Flows: claim -> calculate_rewards).

//contracts/farm-manager/src/farm/commands.rs
pub(crate) fn calculate_rewards(
...
) -> Result<RewardsResponse, ContractError> {
...
        for epoch_id in start_from_epoch..=until_epoch {
...
            let user_weight = user_weights[&epoch_id];
            let total_lp_weight = contract_weights
                .get(&epoch_id)
                .unwrap_or(&Uint128::zero())
                .to_owned();
            //@audit contract_weights or total_lp_weight can be zero, when used as a fraction with checked_mul_floor, this causes division by zero error.
|>          let user_share = (user_weight, total_lp_weight);

            let reward = farm_emissions
                .get(&epoch_id)
                .unwrap_or(&Uint128::zero())
                .to_owned()
|>              .checked_mul_floor(user_share)?;
...

/contracts/farm-manager/src/farm/commands.rs#L205

Current contract attempts to handle this at the source; clear the users LAST_CLAIMED_EPOCH when a user closes a position. This is also vulnerable because when the user has active positions in other lp-denoms, LAST_CLAIMED_EPOCH cannot be cleared for the user. Back in calcualte_rewards, this means the epoch iteration will still start at (LAST_CLAIMED_EPOCH + 1) which includes the epoch where contract_weights is zero. (Flows: close_position -> reconcile_user_state).

//contracts/farm-manager/src/position/helpers.rs
pub fn reconcile_user_state(
    deps: DepsMut,
    receiver: &Addr,
    position: &Position,
) -> Result<(), ContractError> {
    let receiver_open_positions = get_positions_by_receiver(
        deps.storage,
        receiver.as_ref(),
        Some(true),
        None,
        Some(MAX_ITEMS_LIMIT),
    )?;

    // if the user has no more open positions, clear the last claimed epoch
    //@audit-info note: LAST_CLAIMED_EPOCH will not be cleared for the user when the user has open positions in other lp_denom
    if receiver_open_positions.is_empty() {
|>      LAST_CLAIMED_EPOCH.remove(deps.storage, receiver);
    }
...

/contracts/farm-manager/src/position/helpers.rs#L215

Impact

Users’ rewards will be locked and unclaimable. Since pending rewards have to be claimed before close_position, users cannot close any positions without penalty.

Proof of Concept

Suppose a user has two positions: position1 (lp_denom1) and position2(lp_denom2).

1. User calls close_position to close position1.
2. In close_position→ update_weights, contract weight for lp_denom1 becomes 0 in the following epoch.
3. In close_position→ reconcile_user_state, LAST_CLAIMED_EPOCH.remove is skipped due to user has other positions.
4. After a few epochs, user create position3 (lp_denom1).
5. After a few epochs, user calls claims. claim tx reverts due to division by zero.
6. Now, all of the rewards of the user are locked.

Coded PoC:

In contracts/farm-manager/tests/integration.rs, add test_query_rewards_divide_by_zero_cause_rewards_locked and run cargo test test_query_rewards_divide_by_zero_cause_rewards_locked.

//contracts/farm-manager/tests/integration.rs

#[test]
fn test_query_rewards_divide_by_zero_cause_rewards_locked() {
    let lp_denom_1 = format!("factory/{MOCK_CONTRACT_ADDR_1}/1.{LP_SYMBOL}").to_string();
    let lp_denom_2 = format!("factory/{MOCK_CONTRACT_ADDR_1}/2.{LP_SYMBOL}").to_string();

    let mut suite = TestingSuite::default_with_balances(vec![
        coin(1_000_000_000u128, "uom".to_string()),
        coin(1_000_000_000u128, "uusdy".to_string()),
        coin(1_000_000_000u128, "uosmo".to_string()),
        coin(1_000_000_000_000, lp_denom_1.clone()),
        coin(1_000_000_000_000, lp_denom_2.clone()),
    ]);

    let alice = suite.creator();
    let bob = suite.senders[1].clone();
    let carol = suite.senders[2].clone();

    suite.instantiate_default();

    // create overlapping farms
    suite
        .manage_farm(
            &alice,
            FarmAction::Fill {
                params: FarmParams {
                    lp_denom: lp_denom_1.clone(),
                    start_epoch: None,
                    preliminary_end_epoch: None,
                    curve: None,
                    farm_asset: Coin {
                        denom: "uusdy".to_string(),
                        amount: Uint128::new(8_888u128),
                    },
                    farm_identifier: None,
                },
            },
            vec![coin(8_888u128, "uusdy"), coin(1_000, "uom")],
            |result| {
                result.unwrap();
            },
        )
        .manage_farm(
            &alice,
            FarmAction::Fill {
                params: FarmParams {
                    lp_denom: lp_denom_2.clone(),
                    start_epoch: None,
                    preliminary_end_epoch: Some(20),
                    curve: None,
                    farm_asset: Coin {
                        denom: "uusdy".to_string(),
                        amount: Uint128::new(666_666u128),
                    },
                    farm_identifier: None,
                },
            },
            vec![coin(666_666u128, "uusdy"), coin(1_000, "uom")],
            |result| {
                result.unwrap();
            },
        );

    // creator and other fill two positions - one in a different lp_denom farm.
    suite.manage_position(
        &bob,
        PositionAction::Create {
            identifier: Some("creator_position".to_string()),
            unlocking_duration: 86_400,
            receiver: None,
        },
        vec![coin(1_000, lp_denom_1.clone())],
        |result| {
            result.unwrap();
        },
    );

    suite.manage_position(
        &bob,
        PositionAction::Create {
            identifier: Some("creator_another_position".to_string()),
            unlocking_duration: 86_400,
            receiver: None,
        },
        vec![coin(1_000, lp_denom_2.clone())],
        |result| {
            result.unwrap();
        },
    );

    suite
        .add_one_epoch()
        .add_one_epoch()
        .add_one_epoch()
        .add_one_epoch()
        .add_one_epoch()
        .query_current_epoch(|result| {
            let epoch_response = result.unwrap();
            assert_eq!(epoch_response.epoch.id, 5);
        });

    suite.query_rewards(&bob, |result| {
        result.unwrap();
    });
    let farm_manager = suite.farm_manager_addr.clone();
    suite
        .claim(&bob, vec![], |result| {
            result.unwrap();
        })
        .manage_position(
            &bob,
            PositionAction::Close {
                identifier: "u-creator_position".to_string(),
                lp_asset: None,
            },
            vec![],
            |result| {
                result.unwrap();
            },
        )
        .query_lp_weight(&farm_manager, &lp_denom_1, 6, |result| {
            result.unwrap(); //? is this querying the contract weight at epoch 6? which should be 0.
        })
        .query_rewards(&bob, |result| {
            let rewards_response = result.unwrap();
            match rewards_response {
                RewardsResponse::RewardsResponse { total_rewards, .. } => {
                    assert!(total_rewards.is_empty());
                }
                _ => {
                    panic!("Wrong response type, should return RewardsResponse::RewardsResponse")
                }
            } //@audit Medium: Incorrect test logic for divide_by_zero edge case, causing invalid test results. Vulnerable divide_by_zero impact still exists.
        }) // because user lp_weight cleared + last_claim epoch is cleared due to all of creator positions closed.
        .query_lp_weight(&bob, &lp_denom_1, 4, |result| {
            result.unwrap_err();
        })
        .query_lp_weight(&bob, &lp_denom_1, 5, |result| {
            result.unwrap_err();
        })
        .query_lp_weight(&bob, &lp_denom_1, 6, |result| {
            result.unwrap_err();
        })
        .query_lp_weight(&bob, &lp_denom_1, 7, |result| {
            result.unwrap_err();
        });

    suite
        .add_one_epoch() //6
        .add_one_epoch(); //7

    // open a new position
    suite.manage_position(
        &bob,
        PositionAction::Create {
            identifier: Some("creator_a_third_position".to_string()),
            unlocking_duration: 86_400,
            receiver: None,
        },
        vec![coin(2_000, lp_denom_1.clone())],
        |result| {
            result.unwrap();
        },
    );
    suite.add_one_epoch().query_current_epoch(|result| {
        let epoch_response = result.unwrap();
        assert_eq!(epoch_response.epoch.id, 8);
    });

    suite.query_rewards(&bob, |result| {
        let err = result.unwrap_err().to_string();
        // println!("err: {:?}", err);
        assert_eq!(
            err,
            "Generic error: Querier contract error: Cannot divide by zero"
        );
    });
}

running 1 test
test test_query_rewards_divide_by_zero_cause_rewards_locked ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 40 filtered out; finished in 0.01s

Recommended mitigation steps

Consider handling division by zero in calculate_rewards directly by skip the epoch iteration when contract_weights is 0.

jvr0x (MANTRA) confirmed

[H-08] Stableswap pool can be skewed free of fees

Submitted by carrotsmuggler, also found by Abdessamed

/contracts/pool-manager/src/liquidity/commands.rs#L257-L265

Finding description and impact

Stableswap pools are designed to work around a set pricepoint. If the price of the pool deviates away from that point, the pool can incur large slippage.

Normally in constant product AMMs (CPMMs), slippage is observed at every point in the curve. However, for a user to change the price drastically, they need to do a large swap. This costs them swap fees. In CPMMs, adding liquidity does not change the price since they always have to be added at specific ratios.

For stableswaps, this is not true. In stableswap pools, liquidity can be added in at any ratio. This means that a user can add liquidity at a ratio far from the current price, which will change the ratio of funds in the pool, leading to large slippage for all users. Stableswap pools protect against this by using fees.

If we look at the curve protocol, we see that if liquidity is added at a ratio far from the current price, the difference between the liquidity addition price and ideal price is computed. The contract can be found here.

ideal_balance = D1 * old_balances[i] / D0
    difference = 0
    new_balance = new_balances[i]

    if ideal_balance > new_balance:
        difference = unsafe_sub(ideal_balance, new_balance)
    else:
        difference = unsafe_sub(new_balance, ideal_balance)

This basically is a measure of how much the pool is being skewed due to this liquidity addition. The user is then made to pay swap fees for this skew they introduced.

_dynamic_fee_i = self._dynamic_fee(xs, ys, base_fee)
fees.append(unsafe_div(_dynamic_fee_i * difference, FEE_DENOMINATOR))
self.admin_balances[i] += unsafe_div(fees[i] * admin_fee, FEE_DENOMINATOR)
new_balances[i] -= fees[i]

So, If a user adds liquidity at the current pool price, difference will be 0 and they wont be charged fees. But if they add liquidity at a skewed price, they will be charged a fee which is equal to the swap fee on the skew they introduced.

This basically makes them equivalent to CPMMs, where to change the price you need to pay swap fees. In stableswap pools like on curve, you pay swap fees if you change the price during liquidity addition.

The issue is that in the stableswap implementation in the codebase, this fee isn’t charged. So users skewing the stableswap pool can basically do it for free, pay no swap fees and only lose out on some slippage.

let d_0 = compute_d(amp_factor, old_pool_assets).ok_or(ContractError::StableInvariantError)?;
let d_1 = compute_d(amp_factor, new_pool_assets).ok_or(ContractError::StableInvariantError)?;

if d_1 <= d_0 {
    Ok(None)
} else {
    let amount = Uint512::from(pool_lp_token_total_supply)
        .checked_mul(d_1.checked_sub(d_0)?)?
        .checked_div(d_0)?;
    Ok(Some(Uint128::try_from(amount)?))
}

Here new_pool_assets is just old_pool_assets+deposits. So a user can add liquidity at any ratio, and not the penalty for it. This can be used by any user to manipulate the pool price or highly skew the pool composition.

Attached is a POC showing a user doing the same. This shows that the pools can be manipulated very easily at very low costs, and users are at risk of losing funds due to high slippage.

Proof of Concept

In the following POC, the following steps take place:

1. A 2-token stableswap pool is created with uwhale and uluna.
2. 1e6 of each token is added as liquidity. This is Liq addition event.
3. A user adds 2e6 of uwhale and no uluna. This is Liq addition 2.
4. The user then removes the liquidity. This is Liq removal.

First let’s look at the output from the POC:

running 1 test
===Liq addition===
==Balance deltas==
uwhale delta: -1000000
uluna delta : -1000000
lp delta    : 1999000
==Balance deltas==

===Liq addition 2===
==Balance deltas==
uwhale delta: -2000000
lp delta    : 1993431
==Balance deltas==

===Liq Removal===
==Balance deltas==
uwhale delta: 1497532
uluna delta : 499177
lp delta    : -1993431
==Balance deltas==

Lets assume uwhale and uluna are both 1 USD each. In step 3, the user added 2e6 of whale, so added in 2e6 usd. In step 4, the user removed 1497532+499177=1996709 usd. So the user recovered 99.84% of their funds. They only lost 0.16% to slippage.

In step 4 we see the impact. When the user removes liquidity, the liquidity comes out at a ratio of 1:0.33. Thus, the pool is highly skewed and the price of the pool will be reported incorrectly and users will incur high slippage.

So at the cost of just 0.16% of their funds, the user was able to skew the pool by a massive amount. Even though the swap fees are 10% in the POC, the user was able to do this paying only 0.16%. This level of low-cost manipulation would be impossible even for CPMM pools, which are known to be more manipulatable than stableswap pools.

Below are some of the helper functions used in the POC:

fn print_diff(init_bal: [Uint128; 4], final_bal: [Uint128; 4]) -> [i128; 4] {
    let diffs = [
        final_bal[0].u128() as i128 - init_bal[0].u128() as i128,
        final_bal[1].u128() as i128 - init_bal[1].u128() as i128,
        final_bal[2].u128() as i128 - init_bal[2].u128() as i128,
        final_bal[3].u128() as i128 - init_bal[3].u128() as i128,
    ];

    println!("==Balance deltas==");
    if diffs[0] != 0 {
        println!("uwhale delta: {}", diffs[0]);
    }
    if diffs[1] != 0 {
        println!("uluna delta : {}", diffs[1]);
    }
    if diffs[2] != 0 {
        println!("uusd delta  : {}", diffs[2]);
    }
    if diffs[3] != 0 {
        println!("lp delta    : {}", diffs[3]);
    }
    println!("==Balance deltas==\n");

    diffs
}
fn calc_state(suite: &mut TestingSuite, creator: &str) -> [Uint128; 4] {
    let uwhale_balance = RefCell::new(Uint128::zero());
    let uluna_balance = RefCell::new(Uint128::zero());
    let uusd_balance = RefCell::new(Uint128::zero());
    let lp_shares = RefCell::new(Uint128::zero());

    suite.query_balance(&creator.to_string(), "uwhale".to_string(), |result| {
        *uwhale_balance.borrow_mut() = result.unwrap().amount;
    });

    suite.query_balance(&creator.to_string(), "uluna".to_string(), |result| {
        *uluna_balance.borrow_mut() = result.unwrap().amount;
    });

    suite.query_balance(&creator.to_string(), "uusd".to_string(), |result| {
        *uusd_balance.borrow_mut() = result.unwrap().amount;
    });

    suite.query_all_balances(&creator.to_string(), |balances| {
        for coin in balances.unwrap().iter() {
            if coin.denom.contains("o.whale.uluna.uusd") {
                *lp_shares.borrow_mut() = coin.amount;
            }
        }
    });

    let uwhale = *uwhale_balance.borrow();
    let uluna = *uluna_balance.borrow();
    let uusd = *uusd_balance.borrow();
    let lp = *lp_shares.borrow();
    [uwhale, uluna, uusd, lp]
}

Attached is the full POC code.

fn twopool_test() {
    let mut suite = TestingSuite::default_with_balances(
        vec![
            coin(1_000_000_001u128, "uwhale".to_string()),
            coin(1_000_000_000u128, "uluna".to_string()),
            coin(1_000_000_001u128, "uusd".to_string()),
            coin(1_000_000_001u128, "uom".to_string()),
        ],
        StargateMock::new("uom".to_string(), "8888".to_string()),
    );
    let creator = suite.creator();
    let _other = suite.senders[1].clone();
    let _unauthorized = suite.senders[2].clone();
    // Asset infos with uwhale and uluna

    let asset_infos = vec![
        "uwhale".to_string(),
        "uluna".to_string(),
    ];

    let pool_fees = PoolFee {
        protocol_fee: Fee {
            share: Decimal::zero(),
        },
        swap_fee: Fee {
            share: Decimal::from_ratio(1u128, 1000u128),
        },
        burn_fee: Fee {
            share: Decimal::zero(),
        },
        extra_fees: vec![],
    };

    // Create a pool
    suite.instantiate_default().create_pool(
        &creator,
        asset_infos,
        vec![6u8, 6u8],
        pool_fees,
        PoolType::StableSwap { amp: 100 },
        Some("whale.uluna.uusd".to_string()),
        vec![coin(1000, "uusd"), coin(8888, "uom")],
        |result| {
            result.unwrap();
        },
    );

    // Initial liquidity
    println!("===Liq addition===");
    let initial_balances = calc_state(&mut suite, &creator.to_string());
    suite.provide_liquidity(
        &creator,
        "o.whale.uluna.uusd".to_string(),
        None,
        None,
        None,
        None,
        vec![
            Coin {
                denom: "uwhale".to_string(),
                amount: Uint128::from(1_000_000u128),
            },
            Coin {
                denom: "uluna".to_string(),
                amount: Uint128::from(1_000_000u128),
            },
        ],
        |result| {
            result.unwrap();
        },
    );
    let final_balances = calc_state(&mut suite, &creator.to_string());
    let diffs = print_diff(initial_balances, final_balances);

    let lp_shares = RefCell::new(Coin::new(0u128, "".to_string()));
    suite.query_all_balances(&creator.to_string(), |balances| {
        for coin in balances.unwrap().iter() {
            if coin.denom.contains("o.whale.uluna.uusd") {
                *lp_shares.borrow_mut() = coin.clone();
            }
        }
    });

    //single sided liquidity
    println!("===Liq addition 2===");
    let initial_balances = calc_state(&mut suite, &creator.to_string());
    suite.provide_liquidity(
        &creator,
        "o.whale.uluna.uusd".to_string(),
        None,
        None,
        None,
        None,
        vec![
            Coin {
                denom: "uwhale".to_string(),
                amount: Uint128::from(2_000_000u128),
            },
            Coin {
                denom: "uluna".to_string(),
                amount: Uint128::from(0u128),
            },
        ],
        |result| {
            result.unwrap();
        },
    );
    let final_balances = calc_state(&mut suite, &creator.to_string());
    let diffs = print_diff(initial_balances, final_balances);

    println!("===Liq Removal===");
    let initial_balances = calc_state(&mut suite, &creator.to_string());
    suite.withdraw_liquidity(
        &creator,
        "o.whale.uluna.uusd".to_string(),
        vec![Coin {
            denom: lp_shares.borrow().denom.to_string(),
            amount: Uint128::from(diffs[3].max(0) as u128),
        }],
        |result| {
            result.unwrap();
        },
    );
    let final_balances = calc_state(&mut suite, &creator.to_string());
    print_diff(initial_balances, final_balances);
}

Recommended mitigation steps

Similar to curve, add swap fees based on the skewness introduced in the stableswap pools during liquidity addition.

jvr0x (MANTRA) confirmed

[H-09] Attackers can force the rewards to be stuck in the contract with malicious x/tokenfactory denoms

Submitted by peachtea, also found by Audinarey, carrotsmuggler, Egis_Security, and p0wd3r

Attackers can fund rewards of LP tokens with tokens created from the x/tokenfactory module and abuse the MsgForceTransfer message to prevent the contract from successfully distributing rewards. This would also prevent the contract owner from closing the malicious farm. As a result, rewards that are accrued to the users will be stuck in the contract, causing a loss of rewards.

Proof of Concept

When a user claims pending rewards of their LP tokens, all of their rewards are aggregated together and sent within a BankMsg::Send message.

/contracts/farm-manager/src/farm/commands.rs#L102-L107

These rewards can be funded externally via the FarmAction::Fill message for a particular LP asset.

One thing to note is that the reward must be a Coin, which means it must be a native token recognized by the Cosmos SDK module.

https://github.com/code-423n4/2024-11-mantra-dex/blob/26714ea59dab7ecfafca9db1138d60adcf513588/packages/amm/src/farm_manager.rs#L186-L187

The Mantra DEX contract will be deployed in the Mantra chain, which is running in parallel as another competition here. The Mantra chain implements a x/tokenfactory module to allow token creators to create native tokens.

https://github.com/MANTRA-Chain/mantrachain/blob/v1.0.2/x/tokenfactory/keeper/msg_server.go

One of the features in the x/tokenfactory module is that token creators can call the MsgForceTransfer to forcefully transfer funds from one account to another account, effectively reducing its balance.

https://github.com/MANTRA-Chain/mantrachain/blob/v1.0.2/x/tokenfactory/keeper/msg_server.go#L149

This allows an attacker to perform a denial of service of the rewards pending in the contract by supplying a tokenfactory denom, and then forcefully transfer funds from the contract in order to cause an “insufficient funds” error.

1. The attacker creates an x/tokenfactory denom from the Mantra chain.
2. The attacker mints some of the tokens and supplies them to an LP token with FarmAction::Fill.
3. The attacker calls MsgForceTransfer to transfer all the tokens forcefully from the contract.
4. When users want to claim their rewards, the transaction will fail due to an insufficient funds error. Since all the rewards are aggregated into a single BankMsg::Send, other legitimate rewards that are accrued for the user will be stuck and cannot be withdrawn.
5. At this point, the contract owner notices it and sends the FarmAction::Close messages to close the farm created by the attacker. However, because the close_farms function will automatically refund the unclaimed farm.farm_asset.amount to the attacker (see here), the transaction will fail due to an insufficient funds error.

Recommended mitigation steps

To mitigate this attack, consider modifying the close_farms function so the messages are dispatched as SubMsg::reply_on_error when refunding the rewards to the farm owner. Within the reply handler, simply return an Ok(Response::default()) if an error occurred during BankMsg::Send. This will prevent the attack because the contract owner will still have the power to close malicious farms even though the attacker reduced the contract’s balance.

https://docs.rs/cosmwasm-std/latest/cosmwasm_std/struct.SubMsg.html#method.reply_on_error

jvr0x (MANTRA) confirmed

3docSec (judge) commented:

Marking this one as primary, because it highlights the two impacts in this group:

• Malicious pools brick claiming of legitimate pools’ rewards.
• Malicious pools can’t be closed.

It is, however, recommended to take into consideration also the S-377 mitigation of letting users opt-out from malicious pools without requiring admin intervention

[H-10] Incorrect slippage_tolerance handling in stableswap provide_liquidty function

Submitted by carrotsmuggler, also found by oakcobalt

/contracts/pool-manager/src/helpers.rs#L437-L438

Finding description and impact

The provide_liquidity function is used to add liquidity to the dex pools. This function implements a slippage tolerance check via the assert_slippage_tolerance function.

helpers::assert_slippage_tolerance(
    &slippage_tolerance,
    &deposits,
    &pool_assets,
    pool.pool_type.clone(),
    share,
    total_share,
)?;

This function implements slippage tolerance in two sub-functions, one for stableswap and one for constant product. This function basically compares the ratio of liquidity of the deposit to the ratio of pool liquidity.

For the constant product pool, the slippage tolerance checks both the 1/0 ratio and 0/1 ratios, where 0 and 1 represent the two tokens of the pool.

if Decimal256::from_ratio(deposits[0], deposits[1]) * one_minus_slippage_tolerance
    > Decimal256::from_ratio(pools[0], pools[1])
    || Decimal256::from_ratio(deposits[1], deposits[0])
        * one_minus_slippage_tolerance
        > Decimal256::from_ratio(pools[1], pools[0])
{
    return Err(ContractError::MaxSlippageAssertion);
}

But for stableswap, it only does a one-sided check.

if pool_ratio * one_minus_slippage_tolerance > deposit_ratio {
    return Err(ContractError::MaxSlippageAssertion);
}

The situation is best described for the scenario where slippage_tolerance is set to 0. This means the pool should ONLY accept liquidity in the ratio of the pool liquidity. This is enforced for constant product pools correctly. However, for stableswap pools, this is incorrect.

If slippage_tolerance is set to 0, then one_minus_slippage_tolerance is 1. Thus, the inequality check above makes sure that the pool_ratio is always less than or equal to the deposit_ratio for the transaction to go through. However, the deposit_ratio can be be either higher or lower than than the pool_ratio, depending on the components of the liquidity addition. The inequality above only checks for one case (less than equals) and misses the other check (greater than equals).

This means even with slippage_tolerance set to 0, the stableswap pool will accept liquidity that is not in the ratio of the pool liquidity.

Furthermore, for the case where the deposit_ratio is higher than the pool_ratio, there is no slippage restriction on the pool at all.

The entire reason slippage_tolerance exists, is so that the user can specify the exact amount of lp tokens they expect out of the pool. However, the protocol does not implement a minimum_amount_out like on curve, and instead uses this slippage_tolerance value. This means the slippage_tolerance value is crucial to ensure that the depositor is not leaking any value. However, below shown is a situation where if the depositor adds liquidity in certain compositions, they can leak any amount of value.

A POC is run to generate the numbers given here.

Lets say a pool is created and liquidity is provided with 1e6 whale and 2e6 luna tokens. It is quite common to have stableswap pools similarly imbalanced, so this is a usual scenario. Now a user deposits 1e4 whale and 1e5 luna tokens in this pool.

At the end, the pool composition becomes 1.01e6 whale and 2.1e6 luna tokens. The initial liquidity addition created 2997146 lp tokens and the second liquidity addition creates 109702 lp tokens, for a total of 3106848 lp tokens.

These numbers come from running the POC below, which has the output:

running 1 test
===Liq addition===
==Balance deltas==
uwhale delta: -1000000
uluna delta : -2000000
lp delta    : 2997146
==Balance deltas==

===Liq addition 2===
==Balance deltas==
uwhale delta: -10000
uluna delta : -100000
lp delta    : 109702
==Balance deltas==

So during the slippage check on the second deposit, pool_sum = 1e6+2e6 + 1e5+1e4 = 3.11e6, and the deposit_sum = 1e5+1e4 = 1.1e5.

pool_ratio = 3.11e6/3106848 = 1.001014533
deposit_ratio = 1.1e5/109702 =1.00271645

Now, even if slippagetolerance is set to 0, since `poolratio<deposit_ratio, the transaction goes through. However, the issue is that in the second liquidity addition, the user could have received less than109702` lp tokens and the transaction would have still gone through.

Say the user receives only 108000 tokens. Then, total pool lp_tokens = 2997146+108000 = 3105146

pool_ratio = 3.11e6/3105146 = 1.001563212
deposit_ratio = 1.1e5/108000 = 1.018518519

This transaction will also pass, since deposit_ratio > pool_ratio. However, we can clearly see that the liquidity depositor has lost 1.55% of their deposit. So even with slippage_tolerance set to 0, the stableswap pool can accept liquidity that is not in the ratio of the pool liquidity, and depositors can eat large amounts of slippage.

Proof of Concept

A POC was used to generate the results of the liquidity addition. First, a couple helper functions,

fn print_diff(init_bal: [Uint128; 4], final_bal: [Uint128; 4]) -> [i128; 4] {
    let diffs = [
        final_bal[0].u128() as i128 - init_bal[0].u128() as i128,
        final_bal[1].u128() as i128 - init_bal[1].u128() as i128,
        final_bal[2].u128() as i128 - init_bal[2].u128() as i128,
        final_bal[3].u128() as i128 - init_bal[3].u128() as i128,
    ];

    println!("==Balance deltas==");
    if diffs[0] != 0 {
        println!("uwhale delta: {}", diffs[0]);
    }
    if diffs[1] != 0 {
        println!("uluna delta : {}", diffs[1]);
    }
    if diffs[2] != 0 {
        println!("uusd delta  : {}", diffs[2]);
    }
    if diffs[3] != 0 {
        println!("lp delta    : {}", diffs[3]);
    }
    println!("==Balance deltas==\n");

    diffs
}
fn calc_state(suite: &mut TestingSuite, creator: &str) -> [Uint128; 4] {
    let uwhale_balance = RefCell::new(Uint128::zero());
    let uluna_balance = RefCell::new(Uint128::zero());
    let uusd_balance = RefCell::new(Uint128::zero());
    let lp_shares = RefCell::new(Uint128::zero());

    suite.query_balance(&creator.to_string(), "uwhale".to_string(), |result| {
        *uwhale_balance.borrow_mut() = result.unwrap().amount;
    });

    suite.query_balance(&creator.to_string(), "uluna".to_string(), |result| {
        *uluna_balance.borrow_mut() = result.unwrap().amount;
    });

    suite.query_balance(&creator.to_string(), "uusd".to_string(), |result| {
        *uusd_balance.borrow_mut() = result.unwrap().amount;
    });

    suite.query_all_balances(&creator.to_string(), |balances| {
        for coin in balances.unwrap().iter() {
            if coin.denom.contains("o.whale.uluna.uusd") {
                *lp_shares.borrow_mut() = coin.amount;
            }
        }
    });

    let uwhale = *uwhale_balance.borrow();
    let uluna = *uluna_balance.borrow();
    let uusd = *uusd_balance.borrow();
    let lp = *lp_shares.borrow();
    [uwhale, uluna, uusd, lp]
}

The actual POC to generate the numbers.

fn twopool_test() {
    let mut suite = TestingSuite::default_with_balances(
        vec![
            coin(1_000_000_001u128, "uwhale".to_string()),
            coin(1_000_000_000u128, "uluna".to_string()),
            coin(1_000_000_001u128, "uusd".to_string()),
            coin(1_000_000_001u128, "uom".to_string()),
        ],
        StargateMock::new("uom".to_string(), "8888".to_string()),
    );
    let creator = suite.creator();
    let _other = suite.senders[1].clone();
    let _unauthorized = suite.senders[2].clone();
    // Asset infos with uwhale and uluna

    let asset_infos = vec!["uwhale".to_string(), "uluna".to_string()];

    let pool_fees = PoolFee {
        protocol_fee: Fee {
            share: Decimal::zero(),
        },
        swap_fee: Fee {
            share: Decimal::from_ratio(1u128, 1000u128),
        },
        burn_fee: Fee {
            share: Decimal::zero(),
        },
        extra_fees: vec![],
    };

    // Create a pool
    suite.instantiate_default().create_pool(
        &creator,
        asset_infos,
        vec![6u8, 6u8],
        pool_fees,
        PoolType::StableSwap { amp: 100 },
        Some("whale.uluna.uusd".to_string()),
        vec![coin(1000, "uusd"), coin(8888, "uom")],
        |result| {
            result.unwrap();
        },
    );

    // Initial liquidity
    println!("===Liq addition===");
    let initial_balances = calc_state(&mut suite, &creator.to_string());
    suite.provide_liquidity(
        &creator,
        "o.whale.uluna.uusd".to_string(),
        None,
        None,
        None,
        None,
        vec![
            Coin {
                denom: "uwhale".to_string(),
                amount: Uint128::from(1_000_000u128),
            },
            Coin {
                denom: "uluna".to_string(),
                amount: Uint128::from(2_000_000u128),
            },
        ],
        |result| {
            result.unwrap();
        },
    );
    let final_balances = calc_state(&mut suite, &creator.to_string());
    let diffs = print_diff(initial_balances, final_balances);

    let lp_shares = RefCell::new(Coin::new(0u128, "".to_string()));
    suite.query_all_balances(&creator.to_string(), |balances| {
        for coin in balances.unwrap().iter() {
            if coin.denom.contains("o.whale.uluna.uusd") {
                *lp_shares.borrow_mut() = coin.clone();
            }
        }
    });

    // liquidity 2
    println!("===Liq addition 2===");
    let initial_balances = calc_state(&mut suite, &creator.to_string());
    suite.provide_liquidity(
        &creator,
        "o.whale.uluna.uusd".to_string(),
        None,
        None,
        None,
        None,
        vec![
            Coin {
                denom: "uwhale".to_string(),
                amount: Uint128::from(1_000u128),
            },
            Coin {
                denom: "uluna".to_string(),
                amount: Uint128::from(10_000u128),
            },
        ],
        |result| {
            result.unwrap();
        },
    );
    let final_balances = calc_state(&mut suite, &creator.to_string());
    let diffs = print_diff(initial_balances, final_balances);
}

Recommended mitigation steps

For stableswap, the slippage_tolerance should be checked against the difference in the price ratios, so abs(pool_ratio - deposit_ratio). This way both sides of the inequality are checked.

jvr0x (MANTRA) confirmed

[H-11] Stableswap does disjoint swaps, breaking the underlying invariant

Submitted by carrotsmuggler, also found by 0x1982us, Abdessamed, Abdessamed, and LonnyFlash

/contracts/pool-manager/src/helpers.rs#L117-L124

/contracts/pool-manager/src/helpers.rs#L39-L88

Finding description and impact

In stableswap pools, the invariant that is preserved is a combination of a CPMM and a constant price model. For pools with more than 2 tokens, every token balance is used to compute the invariant.

This is shown in the curve protocol, where the invariant is calculated correctly.

The invariant is made up of two parts, the stable part and the constant product part:

Note: please see scenario in warden’s original submission.

This lets every token in the pool stay in parity with the others, and reduces the slippage. The issue is that in the current implementation, instead of summing or taking the product of all the tokens of the pools, the protocol only takes the sum/product of the ask and offer tokens.

For example, in the compute_swap function,

let new_pool = calculate_stableswap_y(
    n_coins,
    offer_pool,
    ask_pool,
    offer_amount,
    amp,
    ask_precision,
    StableSwapDirection::Simulate,
)?;

Only the ask and offer amounts token amounts are sent in. In the internal calculate_stableswap_y function, the invariant is calculated using these two only.

let pool_sum = match direction {
    StableSwapDirection::Simulate => offer_pool.checked_add(offer_amount)?,
    StableSwapDirection::ReverseSimulate => ask_pool.checked_sub(offer_amount)?,

Here’s the curve stableswap code for comparison,

for _i in range(N_COINS):
    if _i == i:
        _x = x
    elif _i != j:
        _x = xp_[_i]
    else:
        continue
    S_ += _x

The sum_invariant D is calculated only with the two tokens in question, ignoring the third or fourth tokens in the pool; while the actual invariant requires a sum of ALL the tokens in the pool. Similarly, calculating in calculate_stableswap_d also calculates the sum using only 2 token balances.

let sum_pools = offer_pool.checked_add(ask_pool)?;

The n_coins used in the calculations, however, is correct and equal to the number of tokens in the pool. This is enforced since the reserves length is used, which is set up correctly during pool creation.

n_coins: Uint256::from(pool_info.assets.len() as u128),

Thus, the S and D calculated are incorrect. This also influences the outcome of the newton-raphson iterations, since both these quantities are used there.

The result of this is that if a pool has three tokens A, B, C then A-B swaps ignore the liquidity of C. This is because the S and D calculations will never touch the liquidity of C, since they only deal with the ask and offer tokens.

So for tricrypto pools, the invariant preserved in A-B swaps is different from the invariant preserved in B-C swaps.

The result are swaps with worse slippage profiles. In normal stableswap pools, the pool tries to maintain all the tokens in parity with each other, giving higher slippage if the pool as a whole is imbalanced. So A-B swaps will have lots of slippage if token C is available in a drastically different amount. However, in this case, the pool only cares about the ask and offer tokens, so the slippage will be lower than expected, leading to arbitrage opportunities. This allows the pools to be more manipulatable.

Proof of Concept

It is evident from the code snippets above that only the ask and offer token amounts are used for invariant calculations. Other token amounts are not used. The protocol does support pools with 2+ tokens and for those cases, the invariant is incorrect.

Recommended mitigation steps

Implement the correct invariant for stableswap, by also including the third/other token amounts in the sum and D calculations.

jvr0x (MANTRA) confirmed

[H-12] Pool creators can manipulate the slippage calculation for liquidity providers

Submitted by DadeKuma, also found by 0x1982us

Pool creation is permissionless, and users can create a pool by specifying asset denoms. The issue is that they can put a different order of the same token denoms, which should result in the same pool, but in fact, it does not.

This ultimately cause the slippage mechanism to use the inverse ratio instead of the correct one, as in other parts of the codebase these values are always ordered, which will cause a loss of funds for the users that provide liquidity as they use the inverted slippage.

Proof of Concept

Users can create a pool by calling pool_manager::create_pool and specifying the asset_denoms.

They can call this function with the same denoms, but in a different order. In theory, this should result in the same pool, but this isn’t the case.

Suppose Bob adds liquidity on a [uwhale, uluna] pool instead of a [uluna, uwhale] pool. The slippage tolerance check is triggered:

    // assert slippage tolerance
    helpers::assert_slippage_tolerance(
        &slippage_tolerance,
        &deposits,
        &pool_assets,
        pool.pool_type.clone(),
        share,
        total_share,
    )?;

/contracts/pool-manager/src/liquidity/commands.rs#L271

This results in the following issue: supposing a k-product pool with n = 2 tokens, we calculate the slippage check:

    PoolType::ConstantProduct => {
	    if deposits.len() != 2 || pools.len() != 2 {
	        return Err(ContractError::InvalidPoolAssetsLength {
	            expected: 2,
	            actual: deposits.len(),
	        });
	    }
      //@audit-info added these logs to check the actual ratios
      println!("------> Slippage ratios, d1: {}, p1: {}, d2: {}, p2: {}", deposits[0], pools[0], deposits[1], pools[1]);
      println!("1st ratio check: {} > {}", Decimal256::from_ratio(deposits[0], deposits[1]) * one_minus_slippage_tolerance, Decimal256::from_ratio(pools[0], pools[1]));
      println!("2nd ratio check: {} > {}", Decimal256::from_ratio(deposits[1], deposits[0]) * one_minus_slippage_tolerance, Decimal256::from_ratio(pools[1], pools[0]));
	    if Decimal256::from_ratio(deposits[0], deposits[1]) * one_minus_slippage_tolerance
->	        > Decimal256::from_ratio(pools[0], pools[1])
	        || Decimal256::from_ratio(deposits[1], deposits[0])
	            * one_minus_slippage_tolerance
->	            > Decimal256::from_ratio(pools[1], pools[0])
	    {
	        return Err(ContractError::MaxSlippageAssertion);
	    }
	}

/contracts/pool-manager/src/helpers.rs#L452

deposits are always sorted but the pool order is determined on creation. As the pool has [uwhale, uluna] instead of [uluna, uwhale] denominations, the slippage checks are inverted.