CALC - Manager/Scheduler/Strategy - THORChain

The distribution action aggregates balances for each entry in denoms, but does not validate or deduplicate duplicate entries. When Distribution.denoms contains the same denom more than once, execute_unsafe queries the balance for each occurrence and adds them into a Coins accumulator, which merges by denom and sums amounts. This double-counts the available balance.

The subsequent pro‑rata split uses this inflated balance, producing Bank/Wasm/Deposit messages that exceed the contract’s real balance per denom; the chain will reject the transaction, effectively producing a DoS in that strategy step and preventing progress until configuration is fixed. The missing duplicate-denom validation in init allows the bad input to pass unchecked.

Code Location

Code of execute_unsafe function from packages/calc-rs/src/actions/distribution.rs file:

pub fn execute_unsafe(
    self,
    deps: Deps,
    env: &Env,
) -> StdResult<(Vec<CosmosMsg>, Distribution)> {
    let mut balances = Coins::default();

    for denom in &self.denoms {
        balances.add(
            deps.querier
                .query_balance(env.contract.address.as_ref(), denom)?,
        )?;
    }

    if balances.is_empty() {
        return Ok((vec![], self));
    }

It is recommended to reject or eliminate duplicate entries in denoms during init by validating for uniqueness (for example, using a set). Alternatively, construct the balance map from a set of unique denoms before aggregation to ensure each denom is queried and included only once.

SOLVED: The Calc team solved the issue by using a HashSet to verify uniqueness.

The Distribution action prevents THORChain deposits if any denom in denoms lack a hyphen, by evaluating has_native_denoms as any !d.contains('-'). This misclassifies rune (which has no hyphen) as non-secured and rejects Recipient::Deposit. However, the THORChain deposit process explicitly supports rune within MsgDeposit::into_cosmos_msg, treating rune as a secured asset.

As a result, strategies that attempt to deposit RUNE via Distribution are blocked at init (causing a functional denial of service) and behave inconsistently with the Thorchain module.

Code Location

Code of Distribution::initfunction from packages/calc-rs/src/actions/distribution.rsfile:

fn init(self, deps: Deps, _env: &Env, affiliates: &[Affiliate]) -> StdResult<Distribution> {
    if self.denoms.is_empty() {
        return Err(StdError::generic_err("Denoms cannot be empty"));
    }

    if self.destinations.is_empty() {
        return Err(StdError::generic_err("Destinations cannot be empty"));
    }

    let has_native_denoms = self.denoms.iter().any(|d| !d.contains('-'));
    let mut total_shares = Uint128::zero();

    for destination in self.destinations.iter() {
        if destination.shares.is_zero() {
            return Err(StdError::generic_err("Destination shares cannot be zero"));
        }

        match &destination.recipient {
            Recipient::Bank { address, .. } | Recipient::Contract { address, .. } => {
                deps.api.addr_validate(address.as_ref()).map_err(|_| {
                    StdError::generic_err(format!("Invalid destination address: {address}"))
                })?;
            }
            Recipient::Deposit { memo } => {
                if has_native_denoms {
                    return Err(StdError::generic_err(format!(
                        "Only secured assets can be deposited with memo {memo}"
                    )));
                }
            }
        }

        total_shares += destination.shares;
    }

It is recommended to align the secured-asset check with the Thorchain route by treating rune as secured, e.g., compute has_native_denoms with d.to_lowercase() != "rune" && !d.contains('-'), or directly check with a helper consistent with is_secured_asset.

SOLVED: The Calc team solved the issue by adding RUNE as a secure asset.

Both the FIN and THORChain swap routes implement a LinearScalar adjustment intended to resize swaps proportionally to deviations in price. However, the implementation computes the base price as receive-per-input (base_receive_amount / swap_amount) while computing the current price as input-per-receive (swap_amount / expected_receive_amount). These two metrics are mathematical inverses, and directly comparing them produces distorted deltas.

This inconsistency causes the adjustment logic to measure price differences incorrectly:

• When prices fall, the calculation overstates the delta, causing the algorithm to over-scale swap amounts (executing larger trades than intended).

• When prices rise, the delta is understated, causing the algorithm to under-scale swap amounts (executing smaller trades than intended).

As a result, strategies using this adjustment deviate from their intended risk/reward calibration. Although this issue does not cause immediate loss of funds or security compromise, it represents a logic flaw in swap sizing and can materially affect strategy performance.

Code Location

Code of FinRoute::adjust function from packages/calc-rs/src/actions/swaps/fin.rs file:

SwapAmountAdjustment::LinearScalar {
    base_receive_amount,
    minimum_swap_amount,
    scalar,
} => {
    let swap_balance = deps.querier.query_balance(
        env.contract.address.clone(),
        quote.swap_amount.denom.clone(),
    )?;

    let new_swap_amount = Coin::new(
        min(swap_balance.amount, quote.swap_amount.amount),
        quote.swap_amount.denom.clone(),
    );

    let simulation = deps.querier.query_wasm_smart::<SimulationResponse>(
        self.pair_address.clone(),
        &QueryMsg::Simulate(new_swap_amount.clone()),
    )?;

    let expected_receive_amount =
        Coin::new(simulation.returned, quote.swap_amount.denom.clone());

    let base_price =
        Decimal::from_ratio(base_receive_amount.amount, quote.swap_amount.amount);

    let current_price =
        Decimal::from_ratio(quote.swap_amount.amount, expected_receive_amount.amount);

    let price_delta = base_price.abs_diff(current_price) / base_price;
    let scaled_price_delta = price_delta * scalar;

    let scaled_swap_amount = if current_price < base_price {
        new_swap_amount
            .amount
            .mul_floor(Decimal::one().saturating_add(scaled_price_delta))
    } else {
        new_swap_amount
            .amount
            .mul_floor(Decimal::one().saturating_sub(scaled_price_delta))
    };

Code of ThorchainRoute::adjust function from packages/calc-rs/src/actions/swaps/thor.rs file:

SwapAmountAdjustment::LinearScalar {
    base_receive_amount,
    minimum_swap_amount,
    scalar,
} => {
    let quote = get_swap_quote(deps, &route)?;

    let base_price =
        Decimal::from_ratio(base_receive_amount.amount, route.swap_amount.amount);

    let current_price =
        Decimal::from_ratio(route.swap_amount.amount, quote.expected_amount_out);

    let price_delta = base_price.abs_diff(current_price) / base_price;
    let scaled_price_delta = price_delta * scalar;

    let scaled_swap_amount = if current_price < base_price {
        route
            .swap_amount
            .amount
            .mul_floor(Decimal::one().saturating_add(scaled_price_delta))
    } else {
        route
            .swap_amount
            .amount
            .mul_floor(Decimal::one().saturating_sub(scaled_price_delta))
    };

It is recommended to calculate both prices using the same metric, for example:

• current_receive_per_input = expected_receive_amount / swap_amount

• base_receive_per_input = base_receive_amount / original_swap_amount

• then define price_delta = |current − base| / base.

SOLVED: The Calc team resolved this issue by refactoring the adjustment function and correcting the inaccurate calculation.

In ThorchainRoute::adjust under the LinearScalar branch, the function requests an L1 swap quote and computes a scaled_swap_amount without first capping by the contract’s actual on-chain input balance. Moreover, it calls get_swap_quote using the uncapped route.swap_amount.amount.

This creates two issues:

1. Unrealistic price basis for scaling. If the wallet’s balance is lower than the requested amount, the quote is computed for a size the contract cannot execute. The resulting “current price” (and thus the delta and scaling) may be materially different from the one corresponding to the executable size. This produces mis-sized adjustments.

2. Execution failure / liveness hit. The adjusted new_swap_amount can exceed the available balance. When the step proceeds to deposit/swap on THORChain (e.g., MsgDeposit), the tx fails due to insufficient funds, halting the strategy step and requiring retries. No funds are lost, but the strategy suffers unnecessary failures and potential keeper costs.

It is recommended to cap the swap size by the contract’s actual on-chain balance before requesting the quote and again after computing the scaled amount. Use the capped amount consistently for price calculations, scaling, and rescaling minimum_receive_amount, and add unit tests to cover insufficient-balance cases.

SOLVED: The Calc team addressed this issue by refactoring the adjust function and implementing the missing check.

Several components derive prices or decisions from the FIN orderbook using only the top-of-book level (book[0]) or a mid-price built from the best levels. In thin markets, an attacker can cheaply post a tiny (low-liquidity) quote at an extreme price to temporarily become the best level. If the code trusts that single level:

• Offset strategies may reset prices and withdraw/re-place orders unnecessarily (churn, gas).

• Conditions (e.g., AssetValueRatio, CanSwap) can flip true/false spuriously, triggering or suppressing actions.

• Slippage checks and route selection may be skewed toward suboptimal choices.

While this does not directly steal funds, it degrades execution quality, increases costs, and creates noisy, manipulable behavior.

Code Location

Code of get_new_price function from packages/calc-rs/src/actions/limit_orders/fin_limit_order.rs:

pub fn get_new_price(
    &self,
    deps: Deps,
    pair_address: &Addr,
    side: &Side,
) -> StdResult<Decimal> {
    Ok(match self {
        PriceStrategy::Fixed(price) => *price,
        PriceStrategy::Offset {
            direction, offset, ..
        } => {
            let book_response = deps.querier.query_wasm_smart::<BookResponse>(
                pair_address.clone(),
                &QueryMsg::Book {
                    limit: Some(10),
                    offset: None,
                },
            )?;

            let book = if side == &Side::Base {
                book_response.base
            } else {
                book_response.quote
            };

            if book.is_empty() {
                return Err(StdError::generic_err("Order book is empty"));
            }

            let price = book[0].price;

Code of is_satisfied function from packages/calc-rs/src/conditions/asset_value_ratio.rs:

let price = match self.oracle.clone() {
    PriceSource::Fin { address } => {
        let book_response = deps.querier.query_wasm_smart::<BookResponse>(
            &address,
            &QueryMsg::Book {
                limit: Some(1),
                offset: None,
            },
        )?;

        let pair = deps
            .querier
            .query_wasm_smart::<ConfigResponse>(address, &QueryMsg::Config {})?;

        let mid_price = (book_response.base[0].price + book_response.quote[0].price)
            / Decimal::from_ratio(2u128, 1u128);

        if pair.denoms.base() == self.numerator {
            Decimal::one() / mid_price
        } else {
            mid_price
        }
    }

Code of validate_adjusted function from packages/calc-rs/src/actions/swaps/fin.rs:

let book_response = deps.querier.query_wasm_smart::<BookResponse>(
    self.pair_address.clone(),
    &QueryMsg::Book {
        limit: Some(1),
        offset: None,
    },
)?;

let mid_price = (book_response.base[0].price + book_response.quote[0].price)
    / Decimal::from_ratio(2u128, 1u128);

let pair = deps
    .querier
    .query_wasm_smart::<ConfigResponse>(self.pair_address.clone(), &QueryMsg::Config {})?;

let spot_price = if route.swap_amount.denom == pair.denoms.base() {
    Decimal::one() / mid_price
} else {
    mid_price
};

It is recommended to harden price derivation against top-of-book spoofing:

• Require minimal liquidity at the level before considering it (absolute or relative to intended trade size).

• Aggregate multiple levels (e.g., VWAP across 3–5 levels) or use a median filter, then quantize to tick.

  
  

SOLVED: The Calc team solved this finding by implementing some new functions for price calculation. These commits represent improvements over the initial fix:

a0758c174bffa1ebd0b8bf28f66da679e22f3ae5

7121e1a945a2e3832e505c77df8831f03deef7c1

092d6b9b3d877dc37f63b8f8d91e988f32206cf3 (last version)

Several FIN-related pricing paths lack defensive checks that can lead to transaction reverts (panics):

• Division-by-zero risks: Decimal::from_ratio panics when denominator is zero. If a simulation returns 0 (illiquid pair/rounding), or a computed mid price is 0 then inversions can panic and revert the whole tx.

• Empty orderbook access: validate_adjusted uses base[0] and quote[0] without non-empty guards, which can panic/Err on thin markets.

• Unbounded percentage offsets: PriceStrategy::Offset uses Decimal::percent(100 ± offset) without bounding offset; values ≥100 (below) collapse to 0, and very large values can explode multipliers.

Additionally, the same gap applies to AssetValueRatio with FIN as the price source, so equivalent guards should be added (or treat the case as not satisfied). These issues do not directly enable theft, but they can cause step-level DoS, brittle behavior under illiquidity, and degraded execution quality.

Code Location

Code of adjust function from packages/calc-rs/src/actions/swaps/fin.rs file:

let simulation = deps.querier.query_wasm_smart::<SimulationResponse>(
    self.pair_address.clone(),
    &QueryMsg::Simulate(new_swap_amount.clone()),
)?;
let expected_receive_amount =
    Coin::new(simulation.returned, quote.swap_amount.denom.clone());

let base_price =
    Decimal::from_ratio(base_receive_amount.amount, quote.swap_amount.amount);

let current_price =
    Decimal::from_ratio(quote.swap_amount.amount, expected_receive_amount.amount); // denom may be 0

Code of validate_adjusted function from packages/calc-rs/src/actions/swaps/fin.rs file:

let book_response = deps.querier.query_wasm_smart::<BookResponse>(
    self.pair_address.clone(),
    &QueryMsg::Book { limit: Some(1), offset: None },
)?;
let mid_price = (book_response.base[0].price + book_response.quote[0].price)   // requires both non-empty
    / Decimal::from_ratio(2u128, 1u128);

let pair = deps
    .querier
    .query_wasm_smart::<ConfigResponse>(self.pair_address.clone(), &QueryMsg::Config {})?;

let spot_price = if route.swap_amount.denom == pair.denoms.base() {
    Decimal::one() / mid_price   // mid_price must be > 0
} else {
    mid_price
};

Code of get_new_price function from packages/calc-rs/src/actions/limit_orders/fin_limit_order.rs file:

match offset.clone() {
    Offset::Exact(offset) => match direction {
        Direction::Above => price.saturating_add(offset),
        Direction::Below => price.saturating_sub(offset),
    },
    Offset::Percent(offset) => {
        match direction {
            Direction::Above => price
                .saturating_mul(Decimal::percent(100u64.saturating_add(offset))),
            Direction::Below => price
                .saturating_mul(Decimal::percent(100u64.saturating_sub(offset))),
        }
    }
}

It is recommended to:

• Guard zero denominators and convert to explicit StdError:

  • If simulation.returned == 0, return Err before calling Decimal::from_ratio(..., 0).

  • Ensure mid_price > 0 before inversion; if not, return Err.

• Add non-empty checks for both orderbook sides before indexing [0] in validate_adjusted; treat empty sides as a recoverable Err.

• Bound percentage offsets:

  • Enforce Offset::Percent in [0, 100] (or a documented max). For Direction::Below, require offset < 100 to avoid zeroing; for Above, cap multiplier to a sane max.

• Emit attributes on handled errors for observability.

SOLVED: The Calc team addressed this issue by implementing the recommended fixes. Changes related to the asset_value_ratio.rs file were committed in this commit.

Some of these fixes are not applicable, as certain functions were refactored during the resolution of another finding.

The current logic on Fin Limit Orders withdraws on any partial fill (filled > 0) or on a price reset, which can be beneficial in asynchronous execution contexts (unknown next run via trigger/owner), because it consolidates proceeds and avoids stale positions.

However, doing so systematically may introduce avoidable churn (extra gas and operational noise) and temporary exposure gaps (no pro‑rata participation while withdrawing/replacing) when the target price has not changed materially. This is primarily a policy/design trade‑off rather than a correctness bug. Making the withdraw behavior configurable (e.g., thresholds/timeout) would balance liveness and efficiency across different market conditions and execution cadences.

Code Location

Code of saturating_withdraw function from packages/calc-rs/src/actions/limit_orders/fin_limit_order.rs file:

pub fn saturating_withdraw(
    self,
    deps: Deps,
) -> StdResult<FinLimitOrderState<WithdrawingOrder>> {
    let new_price = self.config.strategy.get_new_price(
        deps,
        &self.config.pair_address,
        &self.config.side,
    )?;

    let should_withdraw = self.state.filled.gt(&Uint128::zero())
        || self
            .config
            .strategy
            .should_reset(self.state.price, new_price);

    if should_withdraw {
        let withdrawing_order = self.withdraw()?;

It is recommended to make withdraw behavior configurable to suit asynchronous execution, for example:

• Only when should_reset(current, new) is true (price change per strategy), or

• When fully filled (remaining == 0), or

• When partial fill exceeds a configurable min_fill_ratio (e.g., filled / offer >= X%), or

• When the order is older than a configurable timeout (blocks/seconds).

• For Fixed strategies, avoid withdrawing on small partial fills if price is unchanged; prefer full‑fill or timeout criteria.

SOLVED: The Calc team addressed this issue by updating the requirements that must be checked before proceeding with a withdrawal. An edge case related to this remediation was fixed in this commit.

The get_new_price function from FIN Limit Order requests 10 orderbook levels from FIN (limit: Some(10)) from FIN, but uses only utilizes the first entry (book[0]).

This approach incurs additional query/gas cost without providing any robustness advantages. If only the top-of-book data is required, the query should request should be optimized to retrieve a single level. Conversely, if robustness against top-of-book manipulation or minimal quote sizes is necessary, the code should explicitly aggregate multiple levels (e.g., volume-weighted) and then quantize the result to the pair’s tick size, as previously recommended.

Code Location

Code of get_new_price function from packages/calc-rs/src/actions/limit_orders/fin_limit_order.rs file:

pub fn get_new_price(
    &self,
    deps: Deps,
    pair_address: &Addr,
    side: &Side,
) -> StdResult<Decimal> {
    Ok(match self {
        PriceStrategy::Fixed(price) => *price,
        PriceStrategy::Offset {
            direction, offset, ..
        } => {
            let book_response = deps.querier.query_wasm_smart::<BookResponse>(
                pair_address.clone(),
                &QueryMsg::Book {
                    limit: Some(10),    // over-fetch: only the first level is used
                    offset: None,
                },
            )?;

            let book = if side == &Side::Base {
                book_response.base
            } else {
                book_response.quote
            };

It is recommended to:

• If only top-of-book is required, change the query to limit: Some(1) to reduce gas.

• If more robust pricing is needed, aggregate across N levels (e.g., volume-weighted average across the first 3–5 levels) and then quantize the result to the pair’s tick.

SOLVED: The Calc team resolved this issue by modifying the requested quantity. However, this issue is now obsolete, as this functionality has been replaced by a new method of calculating the price.

The StrategyQueryMsg::Balances query aggregates balances by calling balances() on each node. Currently, only Action::LimitOrder implements balances, returning the remaining bid and filled amounts (ask) associated with FIN limit orders. Other actions (Swap, Distribute) return empty balances.

As a result, the query does not reflect the strategy contract’s full holdings (e.g., free native balances, funds held for swaps or pending distributions), which may mislead operators relying on this endpoint for accounting or monitoring.

Code Location

Code of Action::balances from packages/calc-rs/src/actions/action.rs:

fn balances(&self, deps: Deps, env: &Env) -> StdResult<Coins> {
    match self {
        Action::LimitOrder(limit_order) => limit_order.balances(deps, env),
        _ => Ok(Coins::default()),
    }
}

It is recommended to:

• Document that Balances reports only node-exposed positions (currently FIN limit orders).

• Extend Action::balances for Swap and Distribute to include relevant denoms, or

• Add a separate query (e.g., AllBalances) that aggregates bank::all_balances of the strategy contract plus external positions (like orders), ensuring no double-counting.

SOLVED: The Calc team solved this issue by querying all the denoms in the contract.