QA Report

[code423n4]

Codebase Impressions & Summary

This audit contest is probably the largest in codebase size to date, consisting of various components such as tokens, airdrop, governance, staking, liquidity mining, reward distributions, a money market and cross-chain bridge functionality. While these components aren’t complex when reviewed in isolation, the code complexity becomes a little higher because of the various interactions with each other.

Overall, the codebase quality is very high. Inline comments provided are helpful in describing why some checks were not performed (mainly because they are done elsewhere).

The level of documentation is also high, where the documentation portal and READMEs are adequate and kept up-to-date with the codebase.

Tests ran without issues, and could be easily modified to test out POCs and potential vulnerabilities.

The findings made involve the lack of sanity checks for some key config values (eg. their values should not exceed 100%). While the likelihood of the creation of governance polls to modify these values is low because the proposer loses his ANC deposit if quorum isn’t met, the impact of such polls, should they be successful, is high.

Low Severity Findings

L01: CrossAnchorBridge: Decide if whitelisting feature is to be kept or removed

Line References

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

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

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

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

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

Description

The whitelisted mappings are defined and set to true for certain token addresses, but the lines of code where they are used are commented out.

Recommended Mitigation Steps

Decide whether to keep the whitelisting requirement. Either comment out the remaining lines / remove them entirely, or uncomment the lines where they are used.

L02: Incorrect opcode specification documentation

Line References

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/cross-chain-contracts/README.md#op-code-specification

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/cross-chain-contracts/terra/contracts/wormhole-bridge/src/contract.rs#L99-L125

Description

It is unclear if the full opcode values include the flags as well. If so, they should be 8 bits in length, which isn’t the case. Their decimal representations are also incorrect.

| Opcode | Flags | Full Opcode | Decimal |
|:-------|:------|:------------|:-------|
| Deposit Stable | `FLAG_BOTH_TRANSFERS` | `0b110000` | `192` |
| Redeem Stable | `FLAG_BOTH_TRANSFERS` | `0b110001` | `193` |
| Repay Stable | `FLAG_INCOMING_TRANSFER` | `0b1000000` | `64` |
| Lock Collateral | `FLAG_INCOMING_TRANSFER` | `0b1000001` | `65` |
| Unlock Collateral | `FLAG_OUTGOING_TRANSFER` | `0b0100000` | `32` |
| Borrow Stable | `FlAG_OUTGOING_TRANSFER` | `0b0100001` | `33` |
| Claim Rewards | `FLAG_OUTGOING_TRANSFER` | `0b0100010` | `34` |

The opcodes were also not updated in the comments of the wormhole bridge terra contract.

Recommended Mitigation Steps

Update the comments to reflect the latest changes.

| Opcode | Flags | Full Opcode | Decimal |
|:-------|:------|:------------|:-------|
| Deposit Stable | `FLAG_BOTH_TRANSFERS` | `0b11000000` | `192` |
| Redeem Stable | `FLAG_BOTH_TRANSFERS` | `0b11000001` | `193` |
| Repay Stable | `FLAG_INCOMING_TRANSFER` | `0b10000000` | `128` |
| Lock Collateral | `FLAG_INCOMING_TRANSFER` | `0b10000001` | `129` |
| Unlock Collateral | `FLAG_OUTGOING_TRANSFER` | `0b01000000` | `64` |
| Borrow Stable | `FlAG_OUTGOING_TRANSFER` | `0b01000001` | `65` |
| Claim Rewards | `FLAG_OUTGOING_TRANSFER` | `0b01000010` | `66` |

/*
struct Instruction {
  uint8 op_code; // [1 byte]
  bytes32 sender_address; // [32 bytes]
  one_of {
    // [deposit_stable] opcode = 192
    uint64 sequence; // [8 bytes]

    // [repay_stable] opcode = 128
    uint64 sequence; // [8 bytes]

    // [unlock_collateral] opcode = 64
    bytes32 collorateral_token_address; // [32 bytes]
    uint128 amount; // [16 bytes]

    // [borrow_stable] opcode = 65
    uint256 amount; // [16 bytes]

    // [claim rewards] opcode = 66
    // N/A for now

    // [redeem_stable] opcode = 193
    uint64 sequence; // [8 bytes]

    // [lock_collateral] opcode = 129
    uint64 sequence; // [8 bytes]
  }
}
*/

L03: Slightly imprecise tax calculation

Line References

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-bAsset-contracts/packages/basset/src/tax_querier.rs#L12-L15

https://docs.anchorprotocol.com/developers-ethereum/fees#terra-blockchain-tax

Description

The tax amount is calculated as amount * DENOM / DENOM + tax_rate instead of amount * tax_rate / DENOM, which means the tax percentage isn’t as precise (less tax is actually charged).

We see this in the test case:

// normal tax
assert_eq!(
  deduct_tax(&deps.as_mut().querier, Coin::new(50000000u128, "uusd")).unwrap(),
  Coin {
    denom: "uusd".to_string(),
    amount: Uint128::new(49504950u128)
  }
);

Given an amount of 50_000_000, assuming a tax rate of 1%,the amount after tax would be 0.99 * 50_000_000 = 49500000, but the calculated amount is 50_000_000 * 100 / 101 ~= 49504950.

Recommended Mitigation Steps

Update the calculation to be

(coin.amount.checked_sub(coin.amount.multiply_ratio(
    Uint128::new(1) * tax_rate,
    DECIMAL_FRACTION,
)))?,

L04: Duplicate vesting schedules can be added

Line References

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-token-contracts/contracts/staking/src/contract.rs

Description

In the LP staking contract, it is possible to instantiate / add duplicate vesting schedules.

Proof of Concept

#[test]
fn test_instantiate_duplicate_schedules() {
  let mut deps = mock_dependencies(&[]);

  let msg = InstantiateMsg {
    anchor_token: "reward0000".to_string(),
    staking_token: "staking0000".to_string(),
    distribution_schedule: vec![
      (
        mock_env().block.time.seconds() + 100,
	mock_env().block.time.seconds() + 200,
	Uint128::from(1000000u128),
      ),
      (
        mock_env().block.time.seconds() + 100,
	mock_env().block.time.seconds() + 200,
	Uint128::from(1000000u128),
      ),
    ],
  };

  let info = mock_info("addr0000", &[]);
  let _res = instantiate(deps.as_mut(), mock_env(), info, msg).unwrap();
}

Recommendation

De-duplicate the new vesting schedules when it is added. It is easier done if the schedules are sorted prior.

Non-Critical Findings

NC01: money-market-contracts: Market max_borrow_factor is not capped

Line References

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/money-market-contracts/contracts/market/src/contract.rs#L66

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/money-market-contracts/contracts/market/src/contract.rs#L321-L323

Description

There is no check to ensure that max_borrow_factor is less than 100% (Decimal::One()). It is therefore possible to set a borrow factor of > 1. However, the consequence of it is negligible because it would be the same as setting the value to 100% (can’t borrow if there is no liquidity left).

Proof of Concept

Change the instantiation and update config test values to a value greater than 100%, such as Decimal256::MAX.

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/money-market-contracts/contracts/market/src/testing/tests.rs#L36

let msg = InstantiateMsg {
  owner_addr: "owner".to_string(),
  stable_denom: "uusd".to_string(),
  aterra_code_id: 123u64,
  anc_emission_rate: Decimal256::one(),
  max_borrow_factor: Decimal256::MAX,
};

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/money-market-contracts/contracts/market/src/testing/tests.rs#L215

let msg = ExecuteMsg::UpdateConfig {
  owner_addr: None,
  interest_model: Some("interest2".to_string()),
  distribution_model: Some("distribution2".to_string()),
  max_borrow_factor: Some(Decimal256::percent(120)),
};

Recommended Mitigation Steps

Ensure max_borrow_factor doesn’t exceed Decimal256::one().

// add error in market/src/error.rs
pub enum ContractError {
  #[error("Setting greater than theoretical borrow factor")]
  MaxTheoreticalBorrowFactorExceeded {},
  ...
}

// in market/src/contract.rs
// before store_config() in L54
if msg.max_borrow_factor > Decimal256::one() {
  return Err(ContractError::MaxTheoreticalBorrowFactorExceeded {});
}

// in update_config()
if let Some(max_borrow_factor) = max_borrow_factor {
  if max_borrow_factor > Decimal256::one() {
    return Err(ContractError::MaxTheoreticalBorrowFactorExceeded {});
  }
  config.max_borrow_factor = max_borrow_factor;
}

NC02: Incorrect description of Safe Ratio in documentation

Reference

https://docs.anchorprotocol.com/protocol/anchor-governance/modify-liquidation-parameters

Description

The docs say that

“A low Safe Ratio value allows for the fast liquidation of collaterals while incurring a high price impact for the collateral, while a low Safe Ratio value enforces liquidations with lower collateral price impact, albeit with slower collateral liquidation.”

The second statement describes a high safe ratio.

Recommended Mitigation Steps

Change to “while a high Safe Ratio value enforces liquidations with lower collateral price impact, albeit with slower collateral liquidation.”

NC03: Typos / Grammar

Description

Do a CMD / CTRL + F for the following statements.

Recommended Mitigation Steps

1. form → from
   • // load price form the oracle → // load price from the oracle
   • // load balance form the token contract → // load price from the token contract
2. WITHDARW → WITHDRAW
   • ALICE CAN ONLY WITHDARW 40 UST → ALICE CAN ONLY WITHDRAW 40 UST
3. // cannot liquidation collaterals → // cannot liquidate collaterals
4. // if the token contract is already register we cannot change the address → // if the token contract is already registered we cannot change the address
   • Note the double spacing between is and already
5. inistantiation → instantiation
   • // cannot register the token at the inistantiation → // cannot register token at instantiation

NC04: Outstanding TODO

Line Reference

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/money-market-contracts/contracts/overseer/src/contract.rs#L395

Description

There’s 1 remaining TODO that may not have been resolved.

// TODO: Should this become a reply? If so which SubMsg to make reply_on?

Suggestions

S01: Change interest rate model to jump rate model

Reference

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/money-market-contracts/contracts/interest_model/src/contract.rs#L114-L135

https://github.com/compound-finance/compound-protocol/blob/master/contracts/JumpRateModel.sol

https://docs.benqi.fi/benqi-liquidity-market/protocol-parameters

Description

The current interest rate model used is a simple model that scales linearly with market utilization. A better interest rate model that Compound and its forks are using is the jump rate model, because it is more efficient at incentivizing liquidity at higher utilization rates.

Recommendation

Change the interest rate model to the jump rate model, where a kink is introduced to increase the interest multiplier after a specified market utilization rate.

[GalloDaSballo]

L01: CrossAnchorBridge: Decide if whitelisting feature is to be kept or removed -> Dup of #44

[GalloDaSballo]

This looks like the most complete report