Anchor contest - BondiPestControl's results

Lines of code

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L135-L155

Vulnerability details

Impact

Upon deployment/initialisation, an array of collateral tokens are added to a whitelist, meaning that these tokens are the only tokens that should be deposited/redeemed. Any unsupported tokens should correctly revert so as to avoid instances where Terra does not support the token. This can result in lost funds for users. There are several examples where require(whitelistedAnchorStableTokens[token]); is commented out in the contract where it should likely be un-commented.

Proof of Concept

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider un-commenting any part of the code where whitelisting should be enforced.

Lines of code

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/anchor-bEth-contracts/contracts/anchor_beth_reward/src/user.rs#L187-L212

Vulnerability details

Impact

Rewards are dispersed to users as a percentage of the user's balance vs total balance (of bEth). Rewards are accumulated each time a user calls execute_decrease_balance(), execute_increase_balance() or execute_claim_rewards() as these functions will in term call update_global_index() before any balance adjustments.

There is an attack vector where a user may arbitrarily increase their bEth balance either by flashloan of purchase of wormhole eth, then converting it bEth via anchor_beth_converter. The attacker may then have a significant portion of the total balance. This use can then call the contract which pushes rewards to anchor_beth_reward this will increase the reward_balance of the contract. Since the attacker controls a large portion of the bEth balance they will receive a higher portion of the rewards. They may then convert the bEth back to wormhole eth and close their position.

The result is the user has not contributed anything to the system but still gained a high portion of the rewards.

Proof of Concept

The logic for update_global_index() does not account for users suddenly increasing the balance.

fn update_global_index(state: &mut State, reward_balance: Uint128) -> StdResult<()> {
    // Zero staking balance check
    if state.total_balance.is_zero() {
        // nothing balance, skip update
        return Ok(());
    }

    // No change check
    if state.prev_reward_balance == reward_balance {
        // balance didnt change, skip update
        return Ok(());
    }

    // claimed_rewards = current_balance - prev_balance;
    let claimed_rewards = reward_balance.checked_sub(state.prev_reward_balance)?;

    // update state
    state.prev_reward_balance = reward_balance;
    // global_index += claimed_rewards / total_balance;
    state.global_index = decimal_summation_in_256(
        state.global_index,
        Decimal::from_ratio(claimed_rewards, state.total_balance),
    );

    Ok(())
}

Steps: a) Flashloan (or buy) a significant portion of wormhole Eth b) Convert to Anchor bEth via anchor_beth_converter c) Push rewards on anchor_beth_rewards d) anchor_beth_rewards::execute_claim_rewards() e) Convert Anchor bEth to wormhole Eth f) Repay flashloan (or sell wormhole eth)

Recommended Mitigation Steps

There are multiple possible mitigations to this issue.

First, option is to only allow the global_index to be updated once per block. In addition to this, cap the amount of rewards that may be paid per block (keeping the remaining rewards for the next block). This would reduce the effectiveness of the attack and limit the amount they may earn per block.

Another option is to induce a wait time before the user may begin earning rewards. However, this would require a second transaction from the user to begin collection their reward which may hurt UX.

Lines of code

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-token-contracts/contracts/gov/src/contract.rs#L543-L580 https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-token-contracts/contracts/gov/src/contract.rs#L582-L665 https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-token-contracts/contracts/gov/src/staking.rs#L15-L57 https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-token-contracts/contracts/gov/src/contract.rs#L364-L455

Vulnerability details

Impact

Polls are created by targeting the receive_cw20 function which is queried whenever the contract receives tokens. By setting the hook message to Cw20HookMsg::CreatePoll, the sender is able to create a poll, assuming the amount sent satisfies the minimum deposit amount for poll creation. Users can also choose to call ExecuteMsg::SnapshotPoll or have it handled automatically when a user casts a vote on the newly created poll.

The snapshot simply sets a_poll.staked_amount, which represents the total staked amount within the governance contract at a given block. However, during the voting period, other users can stake tokens and effectively have an increasing influence over the outcome of a given poll. There are no check-pointed balances to ensure that a certain user had staked tokens at the time the poll had its snapshot taken.

This can be abused to skew poll results in favour of users who stake their Anchor tokens after a poll has had its snapshot taken.

Proof of Concept

Let's assume the share to token exchange rate is 1:1 such that if a user deposits 100 Anchor tokens, they receive 100 shares in return.

Consider the following scenario:

• There are a total of 100 Anchor tokens in the Governance contract.
• Alice creates a poll and executes ExecuteMsg::SnapshotPoll such that a_poll.staked_amount == 100.
• Bob deposits 10 Anchor tokens through the Cw20HookMsg::StakeVotingTokens hook message which increases the contract's total balance to 110 and shares to 110 as the exchange rate is 1:1 upon minting and redeeming shares.
• At this point, the target poll has a a_poll.staked_amount == 100, even though there are really 110 Anchor tokens staked.
• As a result, if Bob votes on a poll, they have a 10% degree of influence on the outcome of the poll, even though they have less than 10% of the total staked tokens (i.e. 10/110).
• Therefore, poll voters are actually incentivised to stake tokens after a poll has had its snapshot taken in order to maximise their voting power.

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider implementing a check-pointing mechanism such that when a user casts a vote, the user's staked balance is checked at the block height upon which the snapshot was taken instead of checking its most up-to-date staked balance. This check-pointing behaviour is implemented on Ethereum which has a more restrictive block space. The mechanism will simply store the staker's balance on each stake/unstake action. When user's wish to vote, the protocol will check the balance at a specific block (i.e. the snapshotted block). An example implementation can be found here.

Lines of code

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/anchor-bAsset-contracts/contracts/anchor_basset_reward/src/user.rs#L219 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

Impact

If a users balance increases to more than about 3 * 10^20 then the user will be unable to call

• execute_increase_balance()
• execute_decrease_balance()
• execute_claim_rewards()

Due to an overflow in calculate_decimal_rewards() which results in the transaction failing. The impact is a user will no longer be able to claim their rewards or transfer the bAsset balance (since execute_decrease_balance() fails they cannot transfer or burn bAsset). Thus, their bAsset and rewards are locked in the contract.

Whether 3 * 10^20 is a feasible amount of depends on both the value of the bonded asset and the amount of decimals set in the bAsset constructor.

For example bEth has 6 decimal places and thus we would need over 10^14 bEth for this to be a valid issue which is more than the total supply.

However, if bAssets has 18 decimal places, which is common for many crypto currencies and may be required for assets which have a high dollar value to avoid rounding issues, then that would only need ~300 bAsset to overflow this value and lock their bAsset and rewards in the contract. Thus, it is quite likely that there will be some bonded assets which may reach this overflow condition.

Proof of Concept

The issue occurs due to the following line.

let decimal_balance = Decimal::from_ratio(user_balance, Uint128::new(1));

Here user_balance is converted to a Decimal which has a maximum value of (2^128 - 1) / 10^18 ~= 3 * 10 ^ 20. If user_balance is larger than the maximum value then we will overflow.

Since overflow-checks = true is set in the Cargo.toml file, if this value is overflowed at any point the function will panic reverting the transaction.

The invalid state in which a user has more balance may be reached through the function execute_increase_balance() since holder.balance += amount; and state.total_balance += amount; add Uint128 values which may handle 38 decimal places and thus no overflows occur.

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)
}

Recommended Mitigation Steps

This issue may be mitigated by instead using a precision for decimals that varies depending on the number of decimals bAsset is using.

For example if the aim is to always have 18 decimals worth of precision rather using the struct Decimals which simply multiplies the values by 10^18 it may be more desirable to use our own decimal functions.

To convert_to_decimals(x) we do:

• if bAsset.decimals() < 18 multiply x by 10^(18 - bAasset.deciamls())
• if bAssets.decimals() > 18 divide x by 10^(bAasset.deciamls() - 18).
• else do nothing (x is already has base 10^18)

Then convert_from_decimals(x) we do the opposite of above e.g.

• if bAsset.decimals() > 18 multiply x by 10^(18 - bAasset.deciamls())
• if bAssets.decimals() < 18 divide x by 10^(bAasset.deciamls() - 18).
• else do nothing (x is already has base 10^18)

After we have adjusted the base we would need to implement functions for multiplication (addition and subtraction may remain unchanged). Multiplication would be performed as follows:

For a and b in decimal form do:

a * b => Uint128(Uint256(a) * Uint256(b) / 10^18)

If only one of a or b is in decimal form do:

a * b => a * b