Stader Labs SD Token Staking - Stader Labs

The staking contract has missing bounds on the emission_rate and unbonding_period variables. This can lead to unexpected contract behavior.

The bounds of these parameters should be enforced to avoid potential errors in the current or future uses of these variables.

Code Location

contracts/staking/src/contract.rs

pub fn instantiate(
    deps: DepsMut,
    env: Env,
    _info: MessageInfo,
    msg: InstantiateMsg,
) -> Result<Response, ContractError> {
    set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?;

    let state = State {
        emission_rate: msg.emission_rate,
        last_exchange_rate_updated_block: env.block.height,
        next_undelegate_id: 0,
    };
    STATE.save(deps.storage, &state)?;

    let config = Config {
        manager: deps
            .api
            .addr_validate(msg.manager.to_lowercase().as_str())?,
        xsd_cw20_contract: Addr::unchecked("0"),
        sd_cw20_contract: deps
            .api
            .addr_validate(msg.sd_cw20_contract.to_lowercase().as_str())?,
        rewards_pool_contract: deps
            .api
            .addr_validate(msg.rewards_pool_contract.to_lowercase().as_str())?,
        lock_contract: deps
            .api
            .addr_validate(msg.lock_contract.to_lowercase().as_str())?,
        unbonding_period: msg.unbonding_period,
    };
    CONFIG.save(deps.storage, &config)?;

    Ok(Response::new().add_attribute("method", "instantiate"))
}

contracts/staking/src/contract.rs

pub fn update_config(
    deps: DepsMut,
    info: MessageInfo,
    _env: Env,
    update_config_request: UpdateConfigRequest,
) -> Result<Response, ContractError> {
    let mut config = CONFIG.load(deps.storage)?;
    let mut state = STATE.load(deps.storage)?;
    let sending_manager = info.sender.to_string().to_lowercase();

    if sending_manager.ne(&config.manager) {
        return Err(ContractError::Unauthorized {});
    }

    if let Some(sd_cw20) = update_config_request.sd_cw20_contract {
        config.sd_cw20_contract = deps.api.addr_validate(sd_cw20.to_lowercase().as_str())?;
    }

    if let Some(xsd_cw20) = update_config_request.xsd_cw20_contract {
        if config.xsd_cw20_contract.eq(&Addr::unchecked("0")) {
            config.xsd_cw20_contract = deps.api.addr_validate(xsd_cw20.to_lowercase().as_str())?;
        }
    }

    if let Some(lock) = update_config_request.lock_contract {
        config.lock_contract = deps.api.addr_validate(lock.to_lowercase().as_str())?;
    }

    if let Some(rpc) = update_config_request.rewards_pool_contract {
        config.rewards_pool_contract = deps.api.addr_validate(rpc.to_lowercase().as_str())?;
    }

    if let Some(er) = update_config_request.emission_rate {
        state.emission_rate = er;
    }

    if let Some(up) = update_config_request.unbonding_period {
        config.unbonding_period = up;
    }

    CONFIG.save(deps.storage, &config)?;
    STATE.save(deps.storage, &state)?;

    Ok(Response::new().add_attribute("method", "update_config"))
}

SOLVED: The issue was fixed with the above recommendation in commits:

• 2f1aa9ae1db519d452fa3456fa912a562041b666
• ea46b4313fc5ce14d9b8adedb290a1a4b4b28829

The set_manager function of the staking contract takes user input directly as an address without performing any validation. This could lead to failed messages or the storage of incorrect information.

However, the impact in this particular case is very limited since the address submitted in the set_manager function must execute the accept_manager function to finish the process, protecting against potential mistakes such as those mentioned.

Code Location

contracts/staking/src/contract.rs

pub fn set_manager(
    deps: DepsMut,
    info: MessageInfo,
    _env: Env,
    manager: String,
) -> Result<Response, ContractError> {
    let config = CONFIG.load(deps.storage)?;
    if info.sender.ne(&config.manager) {
        return Err(ContractError::Unauthorized {});
    }

    TMP_MANAGER_STORE.save(
        deps.storage,
        &TmpManagerStore {
            manager: manager.clone(),
        },
    )?;

    Ok(Response::new()
        .add_attribute("method", "set_manager")
        .add_attribute("new_manager", manager))

SOLVED: The issue was fixed with the above recommendation in commit 0f3fbb9237c43f381987e113fa418d4cd1afbb57.

The instantiate function did not set the staking_contract address, as it did for other required contract addresses in the configuration. Instead, it relied on update_config being called post initialization. If the stacking address is not set by calling update_config before executing transfer_rewards, an attempt to transfer to address "0" will end the panic! macro and unnecessary consumption of gas fee per transaction.

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

Code Location

contracts/rewards-pool/src/contract.rs

pub fn instantiate(
    deps: DepsMut,
    _env: Env,
    _info: MessageInfo,
    msg: InstantiateMsg,
) -> Result<Response, ContractError> {
    let config = Config {
        staking_contract: Addr::unchecked("0"),
        sd_cw20_contract: deps
            .api
            .addr_validate(msg.sd_cw20_contract.to_lowercase().as_str())?,
        manager: deps
            .api
            .addr_validate(msg.manager.to_lowercase().as_str())?,
    };

contracts/lock/src/contract.rs

pub fn instantiate(
    deps: DepsMut,
    _env: Env,
    _info: MessageInfo,
    msg: InstantiateMsg,
) -> Result<Response, ContractError> {
    let config = Config {
        manager: deps
            .api
            .addr_validate(msg.manager.to_lowercase().as_str())?,
        sd_cw20_contract: deps
            .api
            .addr_validate(msg.sd_cw20_contract.to_lowercase().as_str())?,
        staking_contract: Addr::unchecked("0"),
    };
    set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?;
    CONFIG.save(deps.storage, &config)?;

ACKNOWLEDGED: The Stader Labs team acknowledged this finding.