Rujira Staking - THORChain

The instantiate function in contracts/rujira-staking/src/contract.rs does not adequately validate all user-provided fields in the InstantiateMsg, which may lead to configuration errors during deployment. While critical fields like addresses and fee percentages are validated, others such as bond_denom, revenue_denom, and receipt_token_metadata are only partially checked. In particular, bond_denom and revenue_denom are only validated for non-emptiness, allowing incorrect formats such as uppercase letters, extra spaces, or special characters. Additionally, the metadata for the receipt token is passed to TokenFactory::create_msg(...) without validation, which could result in user-facing display issues or malformed tokens. These shortcomings could cause operational problems despite the assumption that the deployer is non-malicious.

Code Location

The instantiate function invokes config.validate() and constructs the token creation message using unchecked user input; however, it lacks proper format validation for denoms and metadata fields, which may lead to misconfiguration or malformed token definitions:

#[cfg_attr(not(feature = "library"), entry_point)]
pub fn instantiate(
    deps: DepsMut,
    env: Env,
    _info: MessageInfo,
    msg: InstantiateMsg,
) -> Result<Response, ContractError> {
    set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?;
    let config = Config::new(deps.api, msg.clone())?;
    config.validate()?;
    config.save(deps.storage)?;
    init(deps.storage)?;
    let share_denom = TokenFactory::new(&env, format!("staking-{}", config.bond_denom).as_str());
    Ok(Response::default().add_message(share_denom.create_msg(msg.receipt_token_metadata)))
}

It is recommended to implement stricter validation in the validate method of the Config struct within the rujira-staking contract, enforcing a proper format for denoms using a regex such as ^[a-z0-9]{3,64}$ and validating the contents of receipt_token_metadata to ensure that the token name, symbol, and decimals meet expected formatting and length constraints.

RISK ACCEPTED: The THORChain team accepted this risk and stated the following:

Regarding the validation comments, the emitted Cosmos SDK message is validated, specifically using the logic in cosmos-sdk/x/bank/types/metadata.go. This includes validation of the denom string via sdk.ValidateDenom(denom), which ensures that only permitted SDK tokens are accepted.

The execute function in contracts/rujira-staking/src/contract.rs attempts to read the amount of bond_denom sent by the user using the may_pay helper. However, the result is wrapped in unwrap_or_default(), which causes the contract to silently fallback to zero if an error occurs. This suppresses important validation — specifically, if the user sends multiple coins (a case correctly rejected by may_pay), the fallback allows the transaction to continue and consume gas before ultimately failing in deeper message branches.

Code Location

The following line bypasses validation by ignoring the result of may_pay, defaulting to zero even when multiple coins are sent:

#[cfg_attr(not(feature = "library"), entry_point)]
pub fn execute(
    deps: DepsMut,
    env: Env,
    info: MessageInfo,
    msg: ExecuteMsg,
) -> Result<Response, ContractError> {
    let config = Config::load(deps.storage)?;
    let bond_amount_sent = may_pay(&info, config.bond_denom.as_str()).unwrap_or_default();

It is recommended to remove unwrap_or_default() and propagate the result of may_pay to avoid masking fund validation failures. If this change is made, the may_pay function must also be adjusted to return an error only when multiple coins are sent, and to return zero when no funds are provided or when a single coin with a different denom is sent. This ensures early rejection of invalid transactions while maintaining compatibility with valid branches such as LiquidMsg::Unbond.

ACKNOWLEDGED: The THORChain team acknowledged this issue and stated the following:

The distribution must occur before any user action to ensure that new joiners are not allocated existing fees. Not all user actions involve sending funds. This balance check was simply a convenient way to retrieve the token amount supplied. It is only used to deduct from the current contract balance in order to compute the net effect of the new deposit. This logic could have been implemented using an iter().find(...) with a default value of 0.

The status function in contracts/rujira-staking/src/state.rs exposes two revenue-related fields: account_revenue and pending_revenue. However, both the calculation and the naming of these fields raise concerns:

1. The computation of revenue_surplus is inconsistent with internal accounting logic:

It is defined as:

let revenue_surplus = revenue_balance.checked_sub(account.pending)?; 

This omits subtracting PENDING_SWAP, which is properly accounted for in the distribute function. As a result, status() may report more revenue than is actually available for future distribution, especially in periods of high activity or when swaps are queued.

2. The field names account_revenue and pending_revenue appear semantically inverted or misleading:

• account_revenue shows account.pending, which reflects revenue already distributed to users but not yet claimed — a more accurate label might be pending_rewards or assigned_rewards.

• pending_revenue shows revenue_surplus, which represents undistributed contract-held revenue — better described as unallocated_rewards or undistributed_revenue.

If these values are queried by external systems, such as dashboards, indexers, or accounting tools, these inconsistencies may lead to integrity issues, incorrect assumptions, or flawed financial reporting. For example, if the contract holds 1,000 units of revenue_denom, with 600 units assigned to accounts (account.pending) and 200 units marked as PENDING_SWAP, the true unallocated surplus is only 200. However, the current logic would report a pending_revenue of 400, which is twice the actual amount that could be distributed.

It is recommended to:

• Adjust the computation of revenue_surplus in status() to also subtract PENDING_SWAP, mirroring the internal logic used in distribute().

• Rename the StatusResponse fields to clearly reflect their meanings, such as pending_rewards for user-assigned but unclaimed revenue, and undistributed_rewards for revenue still held by the contract but not yet allocated.

• Optionally document these fields explicitly if they are intended to be consumed by off-chain systems.

These changes will ensure consistency with the contract's internal logic, reduce ambiguity for developers, and avoid integrity issues in external integrations.

SOLVED: The THORChain team solved the issue in the specified commit id.

The LiquidMsg::Unbond branch inside the execute_liquid function of contracts/rujira-staking/src/contract.rs allows unbonding operations to proceed even when the returned token amount is zero. In this case, the contract burns the user's share tokens but performs no actual transfer of the underlying bond tokens. This could happen in rare edge cases — for example, due to rounding errors or unexpected internal state. Although unlikely, it may lead to silent value loss for the user without any visible error.

Code Location

The returned value is not validated before constructing the response:

fn execute_liquid(
    storage: &mut dyn Storage,
    env: &Env,
    info: MessageInfo,
    config: &Config,
    msg: LiquidMsg,
) -> Result<Response, ContractError> {
    let share_denom = TokenFactory::new(env, format!("staking-{}", config.bond_denom).as_str());

    match msg {
        LiquidMsg::Bond {} => {
            let amount = must_pay(&info, config.bond_denom.as_str())?;
            let shares = execute_liquid_bond(storage, amount)?;
            Ok(Response::default()
                .add_event(event_liquid_bond(info.sender.clone(), amount, shares))
                .add_message(share_denom.mint_msg(shares, info.sender)))
        }
        LiquidMsg::Unbond {} => {
            let shares = must_pay(&info, share_denom.denom().as_str())?;
            let returned = execute_liquid_unbond(storage, shares)?;
            Ok(Response::default()
                .add_event(event_liquid_unbond(info.sender.clone(), shares, returned))
                .add_message(share_denom.burn_msg(shares))
                .add_message(BankMsg::Send {
                    to_address: info.sender.to_string(),
                    amount: coins(returned.u128(), config.bond_denom.clone()),
                }))
        }
    }
}

It is recommended to verify that the returned value is strictly greater than zero before proceeding. If the amount is zero, the transaction should be canceled early by returning an appropriate error. This prevents value loss and avoids executing transactions that have no meaningful effect.

ACKNOWLEDGED: The THORChain team acknowledged this issue and stated the following:

If a zero amount is sent, the SDK throws an "invalid coins" error. Therefore, it should be impossible to receive a zero return when performing an unbond operation.