Brokkr Protocol P1 Contracts - Brokkr

When a user unstake or claim BRO rewards in staking-v1 contract, his information is totally removed from STAKERS item if the following conditions are true:

• Total staked amount is zero
• Pending BRO reward is zero

Under the mentioned circumstances, the user won't be able to claim his bBRO rewards, even if the amount is greater than zero, i.e.: he totally loses all his bBRO tokens.

Code Location

User's information is totally removed from STAKERS item when he unstakes:

contracts/staking-v1/src/commands.rs

// decrease stake amount
state.total_stake_amount = state.total_stake_amount.checked_sub(amount)?;
staker_info.unlocked_stake_amount = staker_info.unlocked_stake_amount.checked_sub(amount)?;

if staker_info.pending_bro_reward.is_zero() && staker_info.total_staked()?.is_zero() {
    remove_staker_info(deps.storage, &sender_addr_raw);
} else {
    store_staker_info(deps.storage, &sender_addr_raw, &staker_info)?;
}

\color{black}\color{white}User's information is totally removed from STAKERS item when he claims his BRO rewards:

contracts/staking-v1/src/commands.rs

staker_info.pending_bro_reward = Uint128::zero();
staker_info.unlock_expired_lockups(&env.block)?;

if staker_info.total_staked()?.is_zero() {
    remove_staker_info(deps.storage, &sender_addr_raw);
} else {
    store_staker_info(deps.storage, &sender_addr_raw, &staker_info)?;
}

SOLVED: The issue was fixed in commit f9bbc72a85ff872b36691c4992d7a86439a4bba2.

When users stake, the compute_normal_bbro_reward function is called to calculate the bBRO rewards accrued. Because of the calculation of bbro_reward value depends on the subtraction between last_distribution_block and last_balance_update, it is possible the following attack scenario:

\textbf{\underline{Attack scenario}:}

1. Just after a reward distribution, an attacker stakes 10_000000 BRO (Block height: 200).

2. Attacker waits until there is another reward distribution and he front-runs the transaction to additionally stake 80_000000 BRO before the distribution of BRO tokens (Block height: 700).

3. The attacker unstakes 80_000000 BRO just after the reward distribution (could be even in the same block!).

4. The calculation of his bBRO reward will consider that 90_000000 BRO has been staked for 500 blocks (700 - 200). However, only 10_000000 BRO of the total has been staked for 500 blocks. This situation gives the attacker an unfair advantage.

Code Location

contracts/staking-v1/src/state.rs

let stake_amount = self.total_staked()?;

if stake_amount.is_zero() || state.last_distribution_block < self.last_balance_update {
    return Ok(());
}

let epoch_info = query_epoch_info(querier, epoch_manager_contract)?;

let epochs_staked = Uint128::from(state.last_distribution_block - self.last_balance_update)
    .checked_div(Uint128::from(epoch_info.epoch))?;

let bbro_per_epoch_reward =
    stake_amount.checked_div(epoch_info.epochs_per_year())? * epoch_info.bbro_emission_rate;

let bbro_reward = bbro_per_epoch_reward.checked_mul(epochs_staked)?;
self.pending_bbro_reward = self.pending_bbro_reward.checked_add(bbro_reward)?;
self.last_balance_update = current_block;

Ok(())

PARTIALLY SOLVED: The Brokkr team claimed that contracts will be launched to mainnet with the following parameters:

• epoch = 17,280 blocks (1 day)
• unstake_period_blocks = 241,920 blocks (14 days)

Because of this setup, the attack vector mentioned above is not profitable unless the rewards are distributed after 14 days, which is highly unlikely, especially with the use of an off-chain trigger (out-of-scope of this audit).

If the above-mentioned parameters are subsequently incorrectly changed with the update_config function, this security issue could arise again, so it has been marked as "Partially Solved".

consult_price function in contracts/oracle/src/queries.rs calculates the return value using the price_average stored in oracle contract's storage (PRICE_LAST). However, the function does not previously validate if the price has been updated within a reasonable timeframe.

As a consequence, prices calculated in this TWAP oracle can rapidly become stale if users do not bond tokens frequently enough or if Brokkr's off-chain trigger does not work correctly (out-of-scope for this audit), which could affect negatively users' operations or protocol funds.

Code Location

price_average is extracted from oracle contract's storage (PRICE_LAST) without validating if the price has been updated within a reasonable timeframe:

contracts/oracle/src/queries.rs

let config = load_config(deps.storage)?;
let price_last = load_price_cumulative_last(deps.storage)?;

let price_average = if config.asset_infos[0].equal(&asset) {
    price_last.price_0_average
} else if config.asset_infos[1].equal(&asset) {
    price_last.price_1_average
} else {
    return Err(StdError::generic_err("Invalid asset info"));
};

\color{black}\color{white}Return value of consult_price function is calculated using price_average, even if this value is stale:

contracts/oracle/src/queries.rs

} else {
    let price_precision = Uint256::from(10_u128.pow(TWAP_PRECISION.into()));
    Uint256::from(amount) * price_average / Decimal256::from_uint256(price_precision)
};

Ok(ConsultPriceResponse {
    amount: consult_price.into(),
})

SOLVED: The issue was fixed in commit 44a54bddf48a4e28b711449ddcb13d9bf31afdb9.

An incorrect use of the update_config function in contracts can set owner to an invalid address and inadvertently lose control of the contracts, which cannot be undone in any way. Currently, the owner of the contracts can change owner address using the aforementioned function in a single transaction and without confirmation from the new address.

The affected smart contracts are the following:

• airdrop
• bonding-v1
• oracle
• staking-v1
• vesting

Code Location

contracts/airdrop/src/commands.rs

pub fn update_config(deps: DepsMut, owner: Option<String>) -> Result<Response, ContractError> {
    let mut config = load_config(deps.storage)?;

    if let Some(owner) = owner {
        config.owner = deps.api.addr_canonicalize(&owner)?;
    }

contracts/bonding-v1/src/commands.rs

pub fn update_config(
    deps: DepsMut,
    owner: Option<String>,
    lp_token: Option<String>,
    rewards_pool_contract: Option<String>,
    treasury_contract: Option<String>,
    astroport_factory: Option<String>,
    oracle_contract: Option<String>,
    ust_bonding_reward_ratio: Option<Decimal>,
    ust_bonding_discount: Option<Decimal>,
    lp_bonding_discount: Option<Decimal>,
    min_bro_payout: Option<Uint128>,
    vesting_period_blocks: Option<u64>,
    lp_bonding_enabled: Option<bool>,
) -> Result<Response, ContractError> {
    let mut config = load_config(deps.storage)?;

    if let Some(owner) = owner {
        config.owner = deps.api.addr_canonicalize(&owner)?;
    }

contracts/oracle/src/commands.rs

pub fn update_config(
    deps: DepsMut,
    owner: Option<String>,
    price_update_interval: Option<u64>,
) -> Result<Response, ContractError> {
    let mut config = load_config(deps.storage)?;

    if let Some(owner) = owner {
        config.owner = deps.api.addr_canonicalize(&owner)?;
    }

contracts/staking-v1/src/commands.rs

pub fn update_config(
    deps: DepsMut,
    owner: Option<String>,
    paused: Option<bool>,
    unstake_period_blocks: Option<u64>,
    min_staking_amount: Option<Uint128>,
    min_lockup_period_epochs: Option<u64>,
    max_lockup_period_epochs: Option<u64>,
    base_rate: Option<Decimal>,
    linear_growth: Option<Decimal>,
    exponential_growth: Option<Decimal>,
) -> Result<Response, ContractError> {
    let mut config = load_config(deps.storage)?;

    if let Some(owner) = owner {
        config.owner = deps.api.addr_canonicalize(&owner)?;
    }

contracts/vesting/src/commands.rs

pub fn update_config(
    deps: DepsMut,
    owner: Option<String>,
    genesis_time: Option<u64>,
) -> Result<Response, ContractError> {
    let mut config = load_config(deps.storage)?;
    if let Some(owner) = owner {
        config.owner = deps.api.addr_canonicalize(&owner)?;
    }

SOLVED: The issue was fixed in commit 79549c38936e99a89a1fa7aa7e38456032f47389.

instantiate and update_config functions in staking-v1 contract do not validate that values of base_rate, linear_growth or exponential_growth are less or equal than a threshold (e.g.: 0.1) predefined in the contract.

The aforementioned values are used to calculate premium bBRO rewards. If those values are not correctly set, the premium bBRO rewards for users could be much higher than expected.

Code Location

instantiate function does not validate that base_rate, linear_growth or exponential_growth are less or equal than a predefined threshold:

contracts/staking-v1/src/contract.rs

store_config(
    deps.storage,
    &Config {
        owner: deps.api.addr_canonicalize(&msg.owner)?,
        paused: false,
        bro_token: deps.api.addr_canonicalize(&msg.bro_token)?,
        rewards_pool_contract: deps.api.addr_canonicalize(&msg.rewards_pool_contract)?,
        bbro_minter_contract: deps.api.addr_canonicalize(&msg.bbro_minter_contract)?,
        epoch_manager_contract: deps.api.addr_canonicalize(&msg.epoch_manager_contract)?,
        unstake_period_blocks: msg.unstake_period_blocks,
        min_staking_amount: msg.min_staking_amount,
        lockup_config: LockupConfig {
           min_lockup_period_epochs: msg.min_lockup_period_epochs,
           max_lockup_period_epochs: msg.max_lockup_period_epochs,
           base_rate: msg.base_rate,
           linear_growth: msg.linear_growth,
           exponential_growth: msg.exponential_growth,
        },
    },
)?;

\color{black}\color{white}update_config function does not validate that base_rate, linear_growth or exponential_growth are less or equal than a predefined threshold:

contracts/staking-v1/src/commands.rs

if let Some(base_rate) = base_rate {
    config.lockup_config.base_rate = base_rate;
}

if let Some(linear_growth) = linear_growth {
    config.lockup_config.linear_growth = linear_growth;
}

if let Some(exponential_growth) = exponential_growth {
    config.lockup_config.exponential_growth = exponential_growth;
}

SOLVED: The issue was fixed in commit 032d729b4cddd49c990fdb5d9e78c608d21f0d25.

instantiate and update_config functions in bonding-v1 contract do not validate that values of ust_bonding_reward_ratio, ust_bonding_discount or lp_bonding_discount are less or equal than 1.

The aforementioned values are used to calculate rewards distribution and amounts of BRO tokens to claim. If those values are not correctly set, operations will throw error messages and won't allow legitimate users to claim their rewards, thus generating a denial of service (DoS) in Brokkr protocol.

Code Location

instantiate function does not validate that ust_bonding_discount or lp_bonding_discount are less or equal than 1:

contracts/bonding-v1/src/contract.rs

if msg.ust_bonding_reward_ratio > Decimal::from_str("1.0")?
    || msg.ust_bonding_reward_ratio <= Decimal::zero()
{
    return Err(ContractError::InvalidUstBondRatio {});
}

store_config(
    deps.storage,
    &Config {
        owner: deps.api.addr_canonicalize(&msg.owner)?,
        bro_token: deps.api.addr_canonicalize(&msg.bro_token)?,
        lp_token: deps.api.addr_canonicalize(&msg.lp_token)?,
        rewards_pool_contract: deps.api.addr_canonicalize(&msg.rewards_pool_contract)?,
        treasury_contract: deps.api.addr_canonicalize(&msg.treasury_contract)?,
        astroport_factory: deps.api.addr_canonicalize(&msg.astroport_factory)?,
        oracle_contract: deps.api.addr_canonicalize(&msg.oracle_contract)?,
        ust_bonding_reward_ratio: msg.ust_bonding_reward_ratio,
        ust_bonding_discount: msg.ust_bonding_discount,
        lp_bonding_discount: msg.lp_bonding_discount,
        min_bro_payout: msg.min_bro_payout,
        vesting_period_blocks: msg.vesting_period_blocks,
        lp_bonding_enabled: msg.lp_bonding_enabled,
    },
)?;

\color{black}\color{white}update_config function does not validate that ust_bonding_reward_ratio, ust_bonding_discount or lp_bonding_discount are less or equal than 1:

contracts/bonding-v1/src/commands.rs

if let Some(ust_bonding_reward_ratio) = ust_bonding_reward_ratio {
    config.ust_bonding_reward_ratio = ust_bonding_reward_ratio;
}

if let Some(ust_bonding_discount) = ust_bonding_discount {
    config.ust_bonding_discount = ust_bonding_discount;
}

if let Some(lp_bonding_discount) = lp_bonding_discount {
    config.lp_bonding_discount = lp_bonding_discount;
}

SOLVED: The issue was fixed in commit e80b7dc97ff20b683cd27d7a4cdaa6d7d60c1076.

instantiate and update_config functions in staking-v1 contract do not validate that min_lockup_period_epochs is less than max_lockup_peri od_epochs.

The aforementioned values are used to validate the lockup period when staking locked BRO tokens or locking a previous staked amount. If those values are not correctly set, operations will throw error messages and won't allow legitimate users to stake or lock BRO tokens, thus generating a denial of service (DoS) in Brokkr protocol.

Code Location

instantiate function does not validate that min_lockup_period_epochs is less than max_lockup_period_epochs:

contracts/staking-v1/src/contract.rs

store_config(
    deps.storage,
    &Config {
        owner: deps.api.addr_canonicalize(&msg.owner)?,
        paused: false,
        bro_token: deps.api.addr_canonicalize(&msg.bro_token)?,
        rewards_pool_contract: deps.api.addr_canonicalize(&msg.rewards_pool_contract)?,
        bbro_minter_contract: deps.api.addr_canonicalize(&msg.bbro_minter_contract)?,
        epoch_manager_contract: deps.api.addr_canonicalize(&msg.epoch_manager_contract)?,
        unstake_period_blocks: msg.unstake_period_blocks,
        min_staking_amount: msg.min_staking_amount,
        lockup_config: LockupConfig {
           min_lockup_period_epochs: msg.min_lockup_period_epochs,
           max_lockup_period_epochs: msg.max_lockup_period_epochs,
           base_rate: msg.base_rate,
           linear_growth: msg.linear_growth,
           exponential_growth: msg.exponential_growth,
        },
    },
)?;

\color{black}\color{white}update_config function does not validate that min_lockup_period_epochs is less than max_lockup_period_epochs:

contracts/staking-v1/src/commands.rs

if let Some(min_lockup_period_epochs) = min_lockup_period_epochs {
    config.lockup_config.min_lockup_period_epochs = min_lockup_period_epochs;
}

if let Some(max_lockup_period_epochs) = max_lockup_period_epochs {
    config.lockup_config.max_lockup_period_epochs = max_lockup_period_epochs;
}

SOLVED: The issue was fixed in commit b9c1e4ad60fa79e030737e5374a8b027c147d091.

When registering new vesting accounts, the vesting contract sets the last claimed time to the configured genesis time. That parameter could be greater than the expiration date of some vesting schedules, such that vesting_info.last_claim_time > vesting_info.end_time, which makes it unclaimable. However, it is possible for the administrator to update genesis time and set new vesting schedules.

Code Location

packages/services/src/vesting.rs,

if current_time > schedule.end_time && self.last_claim_time < schedule.end_time {
    claimable_amount += schedule.bro_amount;
}

\textbf{ACKNOWLEDGED:} The Brokkr team acknowledged this finding.

When calculating the return values in get_share_in_assets function every time users provide LP tokens to bonding-v1 contract, the "multiply before divide" principle is not followed, which could generate slight rounding issues in some edge scenarios.

Code Location

contracts/bonding-v1/src/commands.rs

fn get_share_in_assets(
    querier: &QuerierWrapper,
    bro_pool: &Asset,
    ust_pool: &Asset,
    lp_amount: Uint128,
    lp_token_addr: Addr,
) -> StdResult<(Uint128, Uint128)> {
    let total_share = query_supply(querier, lp_token_addr)?;
    let share_ratio = Decimal::from_ratio(lp_amount, total_share);

    Ok((bro_pool.amount * share_ratio, ust_pool.amount * share_ratio))
}

\textbf{ACKNOWLEDGED:} The Brokkr team acknowledged this finding.