Anchor contest - WatchPug's results

Lines of code

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/money-market-contracts/contracts/oracle/src/contract.rs#L106-L113

Vulnerability details

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/money-market-contracts/contracts/oracle/src/contract.rs#L106-L113

The implementation only takes two attributes: asset and price. And the last_updated_time of the record will always be set to the current block.time.

This makes it possible for the price feeds to be disrupted when the network is congested, or the endpoint is down for a while, or the feeder bot handled the message queue inappropriately, as a result, the transactions with stale prices get accepted as fresh prices.

Since the price feeds are essential to the protocol, that can result in users' positions being liquidated wrongfully and case fund loss to users.

PoC

Given:

• feeder i connected to an endpoint currently experiencing degraded performance;
• ETH price is $10,000;
• The max_ltv ratio of ETH is 60%.

1. Alice borrowed 5,000 USDC with 1 ETH as collateral;
2. ETH price dropped to $9,000, to avoid liquidation, Alice repaid 1,000 USD;
3. The price of ETH dropped to $8,000; feeder tries to updateMainFeedData() with the latest price: $8,000, however, since the network is congested, the transactions were not get packed timely;
4. ETH price rebound to $10,000; Alice borrowed another 1,000 USDC;
5. The txs send by feeder at step 3 finally got packed, the protocol now believes the price of ETH has suddenly dropped to $8,000, as a result, Alice's position got liquidated.

Recommendation

Change to:

pub fn feed_prices(
    deps: DepsMut,
    env: Env,
    info: MessageInfo,
    prices: Vec<(String, Decimal256, u64)>,
) -> Result<Response, ContractError> {
    let mut attributes = vec![attr("action", "feed_prices")];
    let sender_raw = deps.api.addr_canonicalize(info.sender.as_str())?;
    for price in prices {
        let asset: String = price.0;
        let mut updated_time: u64 = price.2;
        let price: Decimal256 = price.1;

        // Check feeder permission
        let feeder = read_feeder(deps.storage, &asset)?;
        if feeder != sender_raw {
            return Err(ContractError::Unauthorized {});
        }

        let config: Config = read_config(deps.storage)?;
        if env.block.time.seconds() > updated_time {
            // reject stale price
            if env.block.time.seconds() - updated_time > config.valid_period {
                return Err(ContractError::InvalidInputs {});
            }
        } else {
            // reject future timestamp, graceFuturePeriod can be set to 3, which means < 3s is allowed
            if updated_time - env.block.time.seconds() > config.grace_future_period {
                return Err(ContractError::InvalidInputs {});
            }
            updated_time = env.block.time.seconds();
        }

        attributes.push(attr("asset", asset.to_string()));
        attributes.push(attr("price", price.to_string()));

        store_price(
            deps.storage,
            &asset,
            &PriceInfo {
                last_updated_time: updated_time,
                price,
            },
        )?;
    }

    Ok(Response::new().add_attributes(attributes))
}

Lines of code

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L167-L175

Vulnerability details

In the current implementation of CrossAnchorBridge, all require that "Check that token is a whitelisted token" is commented out.

As a result, users may send transcations with the non-whitelisted tokens and as they can not be processd properly on the Terra side, the funds can be frozen on the brige contract or on the Terra side.

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L167-L175

function handleStableToken(
        address token,
        uint256 amount,
        uint8 opCode
    ) internal {
        // Check that `token` is a whitelisted stablecoin token.
        // require(whitelistedStableTokens[token]);
        handleToken(token, amount, opCode);
    }

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L250-L253

function redeemStable(address token, uint256 amount) external {
        // require(whitelistedAnchorStableTokens[token]);
        handleToken(token, amount, OP_CODE_REDEEM_STABLE);
    }

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L255-L258

function lockCollateral(address token, uint256 amount) external {
        // require(whitelistedCollateralTokens[token]);
        handleToken(token, amount, OP_CODE_LOCK_COLLATERAL);
    }

Recommendation

Uncomment the whitelist codes.

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.

Lines of code

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/money-market-contracts/contracts/market/src/borrow.rs#L216-L234

Vulnerability details

While claim_rewards from the money-market, it calls the distributor_contract#spend() to send the rewards.

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/money-market-contracts/contracts/market/src/borrow.rs#L216-L234

let messages: Vec<CosmosMsg> = if !claim_amount.is_zero() {
    vec![CosmosMsg::Wasm(WasmMsg::Execute {
        contract_addr: deps
            .api
            .addr_humanize(&config.distributor_contract)?
            .to_string(),
        funds: vec![],
        msg: to_binary(&FaucetExecuteMsg::Spend {
            recipient: if let Some(to) = to {
                to.to_string()
            } else {
                borrower.to_string()
            },
            amount: claim_amount.into(),
        })?,
    })]
} else {
    vec![]
};

However, the distributor_contract#spend() function have a spend_limit config and it will revert if the amount is larger than the spend_limit.

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/anchor-token-contracts/contracts/distributor/src/contract.rs#L153-L155

if config.spend_limit < amount {
    return Err(StdError::generic_err("Cannot spend more than spend_limit"));
}

As a result, users won't be able to claim their rewards anymore once the amount of the rewards excess the spend_limit config on distributor_contract.

Recommendation

Consider removing the spend_limit or allowing users to specify an amount when claim_rewards.

[L] Tokens with fee on transfer are not supported

There are ERC20 tokens that charge fee for every transfer() or transferFrom().

In the current implementation, CrossAnchorBridge.sol#handleToken() assumes that the received amount is the same as the transfer amount. This means tokens with fee on transfer are not supported.

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L177-L212

Recommendation

Make sure tokens with fee-on-transfer do not get whitelisted.

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[S] parseIncomingTokenTransferInfo can be optimized

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L269-L296

    function parseIncomingTokenTransferInfo(bytes memory encoded)
        public
        pure
        returns (IncomingTokenTransferInfo memory incomingTokenTransferInfo)
    {
        uint256 index = 0;

        incomingTokenTransferInfo.chainId = encoded.toUint16(index);
        index += 2;

        incomingTokenTransferInfo.tokenRecipientAddress = encoded.toBytes32(
            index
        );
        index += 32;

        incomingTokenTransferInfo.tokenTransferSequence = encoded.toUint64(
            index
        );
        index += 8;

        incomingTokenTransferInfo.instructionSequence = encoded.toUint64(index);
        index += 8;

        require(
            encoded.length == index,
            "invalid IncomingTokenTransferInfo encoded length"
        );
    }

• In the check of require(encoded.length == index), the variable index can be replaced with constant value to avoid mload.
• Check can be done earlier to save gas for revert txs.
• Avoiding one arithmetic operation (index += 8 at L290).

Recommandation

Change to:

function parseIncomingTokenTransferInfo(bytes memory encoded)
    public
    pure
    returns (IncomingTokenTransferInfo memory incomingTokenTransferInfo)
{
    require(
        encoded.length == 50,
        "invalid IncomingTokenTransferInfo encoded length"
    );

    uint256 index;

    incomingTokenTransferInfo.chainId = encoded.toUint16(index);
    index += 2;

    incomingTokenTransferInfo.tokenRecipientAddress = encoded.toBytes32(
        index
    );
    index += 32;

    incomingTokenTransferInfo.tokenTransferSequence = encoded.toUint64(
        index
    );
    index += 8;

    incomingTokenTransferInfo.instructionSequence = encoded.toUint64(index);
}

[M] Setting uint variables to 0 is redundant

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L149-L149

uint8 i = 0;

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L274-L274

uint256 index = 0;

Setting uint8, uint256 variables to 0 is redundant as they default to 0.

[S] Cache array length in for loops can save gas

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

• https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L149-L149

        for (uint8 i = 0; i < _collateralTokens.length; i++) 

[M] ++i is more efficient than i++

Using ++i is more gas efficient than i++, especially in for loops.

For example:

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L149-L149

        for (uint8 i = 0; i < _collateralTokens.length; i++) 

Change to:

        for (uint8 i = 0; i < _collateralTokens.length; ++i) 

[M] Use short reason strings can save gas

Every reason string takes at least 32 bytes.

Use short reason strings that fits in 32 bytes or it will become more expensive.

Instances include:

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L292-L295

        require(
            encoded.length == index,
            "invalid IncomingTokenTransferInfo encoded length"
        );

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L327-L331

        require(
            incomingTokenTransferInfoVM.emitterAddress ==
                TERRA_ANCHOR_BRIDGE_ADDRESS,
            "message does not come from terra anchor bridge"
        );

[M] Unused constant variable

Unused constant variables in contracts increase contract size and gas usage at deployment.

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L104-L104

    uint8 private constant FLAG_NO_ASSC_TRANSFER = 0x00; // 0000 0000

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L115-L115

    uint8 private constant OP_CODE_CLAIM_REWARDS = 2 | FLAG_OUTGOING_TRANSFER;

In the contract, the constant variable FLAG_NO_ASSC_TRANSFER``OP_CODE_CLAIM_REWARDS are set once but have never been read, therefore it can be removed.