Stader Labs LunaX Contracts - Stader Labs

The staking contract implemented a switch to go into a pause-like state called “Protocol Inactive” by using the config.active setting. This check was not implemented consistently across all the functions accessible for unprivileged users, but only on the deposit one.

This state could be desirable for some reasons, where external users' interference would like to be kept to the minimum among all features of the contract.

Code Location

Affected assets

contracts/staking/src/contract.rs:486 redeem_rewards function
contracts/staking/src/contract.rs:523 swap_rewards function
contracts/staking/src/contract.rs:486 reinvest function
contracts/staking/src/contract.rs:615 reimburse_slashing function
contracts/staking/src/contract.rs:715 undelegate_stake function
contracts/staking/src/contract.rs:810 reconcile_funds function
contracts/staking/src/contract.rs:888 withdraw_funds_to_wallet function
contracts/staking/src/contract.rs:976 claim_airdrops function

SOLVED: A new "operations control" mechanism has been implemented to have a more granular control over paused operations, covering all listed functionalities.

This issue was fixed in commit 368f9a665af2fd6d512d2687e40f8699b7a6e164.

The instantiate function did not set the cw20_token_contract address, as done with other contract addresses required in the configuration. Instead, it relied on update_config being called post initialization, which could cause undesirable situations if this address is not set right after deployment.

It is worth noting that the update_config function only allowed to set the CW20 address if it contained the initial value Addr::unchecked("0"). This effectively forbade any future change after the first update.

Code Location

contracts/staking/src/contract.rs

airdrop_registry_contract: deps
    .api
    .addr_validate(msg.airdrops_registry_contract.as_str())?,
airdrop_withdrawal_contract: deps
    .api
    .addr_validate(msg.airdrop_withdrawal_contract.as_str())?,
reward_contract: deps.api.addr_validate(msg.reward_contract.as_str())?,
cw20_token_contract: Addr::unchecked("0"),

protocol_fee_contract: deps.api.addr_validate(msg.protocol_fee_contract.as_str())?,

contracts/staking/src/contract.rs

pub fn update_config(
    deps: DepsMut,
    info: MessageInfo,
    env: Env,
    update_config: ConfigUpdateRequest,
) -> Result<Response, ContractError> {
    let mut config = CONFIG.load(deps.storage)?;
    validate(&config, &info, &env, vec![Verify::SenderManager])?;

    if let Some(cw20_contract) = update_config.cw20_token_contract {
        if config.cw20_token_contract == Addr::unchecked("0") {
            config.cw20_token_contract = deps.api.addr_validate(cw20_contract.as_str())?;
        }
    }

ACKNOWLEDGED: Stader Labs acknowledged this finding.

The reimburse_slashing function of the staking contract does not update the total_staked and exchange_rate state variables. As detailed in the comment of line 635, most functionalities perform a check_slashing at the beginning or update these values. However, this is not the case of query_user_info and query_compute_deposit_breakdown.

If a user queries those functions after a reimburse_slashing has been performed, they would receive erroneous information due to the lack of update.

Code Location

Affected assets

contracts/staking/src/contract.rs:615 reimburse_slashing function

SOLVED: Added a RedeemRewards message from the staking contract as a result of the reimburse_slashing function, which effectively updates the exchange rate at the end of the transaction.

This issue was fixed on commit 24ec25b51e52e98c1128cd77c379d94b3e9dab58s.

The use of the unwrap and expect function is very useful for testing environments because a value is forcibly demanded to get an error (aka panic!) if the "Option" does not have "Some" value or "Result". Nevertheless, leaving unwrap or expect functions in production environments is a bad practice because not only will this cause the program to crash out, or panic!, but also (in case of unwrap) no helpful messages are shown to help the user solve, or understand the reason of the error.

Code Location

Affected assets

contracts/reward/src/contract.rs: #L93, 168, 171, 217
contracts/staking/src/contract.rs: #L266, 361, 371, 400, 433, 579, 632, 696, 701, 743, 773, 785, 830, 836, 867, 880, 946, 956, 1020, 1108, 1121, 1137
contracts/staking/src/helpers.rs: #L90, 98, 122, 163

SOLVED: Most of the instances highlighted below has been fixed using the recommendations or deemed secure due to previous checks. Instances related to the result of checked_add operations have not been modified, as the risk of overflow in those cases is minimal, since the entire supply of Terra supply will not cause the value of uint128 to overflow.

This issue was fixed in commit 7478e89ee9a0f72b89690573fd2dee956d2408fa.

Some mathematical operations that could cause unexpected behavior under specific circumstances were found on the codebase. Although no effective arithmetic over/underflow were found and the overflow-checks = true flag was set on Cargo.toml, it is still recommended to avoid unchecked math as much as possible to follow best-practices and limit the risk of future updates introducing an actual vulnerability.

Code Location

Affected assets

packages/stader-utils/src/coin_utils.rs:173:    let c_u256: Decimal = (b_u256 * a_u256).into();
packages/stader-utils/src/coin_utils.rs:181:    let c_u256: Decimal = (b_u256 + a_u256).into();
packages/stader-utils/src/coin_utils.rs:189:    let c_u256: Decimal = (a_u256 - b_u256).into();
packages/stader-utils/src/coin_utils.rs:243:                Uint128::new(coin.amount.u128() + existing_coin.u128()),
packages/stader-utils/src/coin_utils.rs:278:                Uint128::new(existing_coin.u128() - coin.amount.u128()),
packages/stader-utils/src/coin_utils.rs:432:        coin.amount.u128() * ratio.numerator() / ratio.denominator(),
packages/stader-utils/src/coin_utils.rs:437:    (num * dec.numerator() / dec.denominator()) as u128

ACKNOWLEDGED: Since the affected instances were not exploitable and therefore did not pose a direct risk to in-scope contracts, Stader Labs acknowledged the potential risks outlined above.

Multiple “ToDo” comments and commented code instances were found on the codebase. Although incomplete code does not directly cause a security vulnerability or affect the audit’s outcome as far as it is functional, having development comments could simplify the process for an attacker to find a valid attack surface within the contract.

In addition, it shows that the audited code will be different from the released one, which could cause that new vulnerabilities were to be introduced in the code after the audit.

Code Location

Affected assets

staking/src/helpers.rs:29:// TODO: bchain99 - write unit-tests to validate.
staking/src/contract.rs:1092:// TODO - GM. Test this
staking/src/contract.rs:1101:    // TODO - GM. Will converting u64 to string for batch id start work?
staking/src/contract.rs:521:// TODO - GM. Does swap have a fixed cost or a linear cost?
reward/src/contract.rs:77:// TODO - GM. Does swap have a fixed cost or a linear cost? Useful to make this permissionless.
reward/src/contract.rs:94:    // let denoms: Vec<String> = total_rewards
reward/src/contract.rs:95:    //     .iter()
reward/src/contract.rs:96:    //     .map(|item| item.denom.clone())
reward/src/contract.rs:97:    //     .collect();

SOLVED: Suggested mitigation steps have been implemented by the Stader Labs team in commit 07878e2eefa1e66a9a25ad3dc234340a355b5ad0.