[WP-H4] `anchor_basset_reward` pending yields can be stolen

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/anchor-bAsset-contracts/contracts/anchor_basset_reward/src/user.rs#L80-L123

Vulnerability details

For yield farming aggregators, if the pending yield on an underlying strategy can be harvested and cause a surge of rewards to all existing investors, especially if the harvest can be triggered permissionlessly. Then the attacker can amplify the attack using a flash loan.

This is a well-known attack vector on Ethereum.

The root cause for this attack vector is that the pending yield is not settled to the existing users before issuing shares to new deposits.

In the current implementation of anchor_basset_reward/src/user.rs#execute_increase_balance() before L105, the state.global_index is not being upadted first.

• Expected: the pending rewards (current_global_index - state.global_index) belongs to old user balance,
• Current implementation: the pending rewards (current_global_index - state.global_index) will be multiplied by the new user balance after increase_balance when calculate rewards.

Because new user balance > old user balance, the user will take a part of the rewards belonging to other existing users.

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/anchor-bAsset-contracts/contracts/anchor_basset_reward/src/user.rs#L80-L123

pub fn execute_increase_balance(
    deps: DepsMut,
    _env: Env,
    info: MessageInfo,
    address: String,
    amount: Uint128,
) -> StdResult<Response<TerraMsgWrapper>> {
    let config = read_config(deps.storage)?;
    let owner_human = deps.api.addr_humanize(&config.hub_contract)?;
    let address_raw = deps.api.addr_canonicalize(&address)?;
    let sender = info.sender;

    let token_address = deps
        .api
        .addr_humanize(&query_token_contract(deps.as_ref(), owner_human)?)?;

    // Check sender is token contract
    if sender != token_address {
        return Err(StdError::generic_err("unauthorized"));
    }

    let mut state: State = read_state(deps.storage)?;
    let mut holder: Holder = read_holder(deps.storage, &address_raw)?;

    // get decimals
    let rewards = calculate_decimal_rewards(state.global_index, holder.index, holder.balance)?;

    holder.index = state.global_index;
    holder.pending_rewards = decimal_summation_in_256(rewards, holder.pending_rewards);
    holder.balance += amount;
    state.total_balance += amount;

    store_holder(deps.storage, &address_raw, &holder)?;
    store_state(deps.storage, &state)?;

    let attributes = vec![
        attr("action", "increase_balance"),
        attr("holder_address", address),
        attr("amount", amount),
    ];

    let res = Response::new().add_attributes(attributes);
    Ok(res)
}

PoC

Given:

• the reward balance of anchor_basset_reward increased

The attacker can:

1. bond a large amount of asset tokens
   • anchor_basset_hub will trigger anchor_basset_token to mint basset tokens to the attacker
   • anchor_basset_token will trigger anchor_basset_reward to IncreaseBalance for the attacker
   • anchor_basset_reward will use the old state.global_index to update the attacker's holder.index
2. UpdateGlobalIndex on anchor_basset_hub
   • anchor_basset_hub will trigger anchor_basset_reward's execute_update_global_index() and increase global_index for all users

As of now, the attacker can get a large share of the pending yield. The attacker can claim the rewards and exit.

This process can be done in one transaction by using a smart contract, and the impact can be amplified by using a flash loan.

Recommendation

Consider changing to a similar approach like anchor_beth_reward/src/user.rs#L114, update state.global_index before changing the user's balance.

And/or, transfer rewards and update global_index in one transaction.

[GalloDaSballo]

Seems like loss of yield due to frontrun

[albertchon]

Indeed a bug, although this bug can be mitigated in practice by continuously executing UpdateGlobalIndex, doing so shouldn't necessarily be a requirement for the functioning of the system.