Hawksight security record

The `create_state` function is designed to initialize a globally unique `state` account. This `state` account will be used to identify the project owner and to ensure that only the owner can create a `strategy` and set the `token_per_second` of the farm. ```rust pub struct CreateState<'info> { #[account( init, seeds = [b"state".as_ref()], bump, payer = authority, space = 8 + size_of::<StateAccount>() )] pub state: Account<'info, StateAccount>, ... } ``` ```rust pub fn create_state(&mut self, bump: u8) -> Result<()> { let state = &mut self.state; state.authority = self.authority.key(); state.bump = bump; state.reward_mint = self.reward_mint.key(); state.reward_vault = self.reward_vault.key(); emit!(StateCreated {}); Ok(()) } ``` Any given program_id can only have one `state` account. This may allow malicious users to pre-empt the creation of `state` by posting `create_state` transactions first, thus making the program unavailable. Additionally, if a malicious user manages to call `create_state` prior to admin of the program, the malicious attacker would then have a parallel running program who's state is controlled by them. The user could use this to impersonate the real program to exploit users.

The unit tests here are not enough. Testing programs are a very important aspect of proving program correctness, preventing regressions, and release engineering. Without tests, there is no way to know if the program works as expected. The fact that code was shipped for review with such major flaw, is considered a signal of some problems with release engineering procedures, which may lead to shipping not intended changes to production.

In the `farm` account, the `asset_infos` array is initialized by `weights` and `remaining_accounts`. - `mints` vector is initialized by iterating over `ctx.remaining_accounts`. ```rust for mint in _ctx.remaining_accounts.iter(){ require!(*mint.owner == token::ID, ErrorCode::InvalidMint); Mint::unpack(&mint.to_account_info().try_borrow_data()?)?; mints.push(mint.key()); } ``` - `asset_size` is defined by the length of `weights`. ```rust let asset_size = weights.len(); let asset_count = asset_size.try_into().unwrap(); ``` - `mints` is used to calculate the `farm`'s `seed`. ```rust for mint in mints.iter() { seeds.push(mint.as_ref()); } ``` - `farm.asset_infos` is filled by iterating over the elements of `mints` and `weights` up to `asset_size`. ```rust for i in 0 .. asset_size{ total_weight += u128::from(weights[i]); farm.asset_infos.push(FarmAssetInfo{ weight: weights[i], mint: mints[i] }) } ``` However, the length of `weights` and `remaining_accounts`/`mints` are not checked during this initialization process. This makes it possible that the farm's `seed` and its `asset_infos` to not match, making the service unusable.

The function `change_token_per_second` updates the farms `token_per_second` parameter. The function provides no validation for when the function should be called. ```rust impl<'info> ChangeTokenPerSecondMulti<'info>{ pub fn change_token_per_second(&mut self, token_per_seconds: Vec<u128> ) -> Result<()> { let farm = &mut self.farm; let farm_key = farm.key(); farm.validate_address(farm_key)?; let asset_count:u8 = token_per_seconds.len().try_into().unwrap(); require!( asset_count == farm.asset_count, ErrorCode::InvalidAssetCount); let mut index:usize = 0; for token_per_second in token_per_seconds.iter() { farm.reward_infos[index].token_per_second = *token_per_second; index += 1; } Ok(()) } } ``` In the `state.rs`, the `update()` function will calculate the rewards that have been generated for a farm. ```rust pub fn update<'info>(&mut self, clock: &Sysvar<'info, Clock>) -> Result<()> { let seconds = u128::try_from( clock .unix_timestamp .checked_sub(self.last_reward_time) .unwrap(), ) .unwrap(); let mut reward_per_share: u128 = 0; if self.amount > 0 && seconds > 0 { reward_per_share = u128::from(self.token_per_second) .checked_mul(seconds) .unwrap() .checked_mul(ACC_PRECISION) .unwrap() .checked_div(u128::from(self.amount)) .unwrap(); } self.acc_reward_per_share = self .acc_reward_per_share .checked_add(reward_per_share) .unwrap(); self.last_reward_time = clock.unix_timestamp; Ok(()) } ``` Neither `update` nor `change_token_per_second` have any validation for _when_ `token_per_second` should be changed. In practice, this could mean someone calling `change_token_per_second` prior to when they are supposed to, which would result in unexpected results when calling `update`. The logic of when `token_per_second` should change is totally off-chain and in the hands of the owner, without any validation. Both human error and malicious intent are risks with the current architecture of this function.

Note that with the `UST` price now below $0.05, and no longer has the role of a stable coin. This may easily affect functionality of UST in the project. ```rust pub const UST_MAINNET: Pubkey = static_pubkey::static_pubkey!("9vMJfxuKxXBoEa7rM12mYLMwTacLMLDJqHozw96WQL8i"); pub const STABLE_MINTS: [Pubkey; 2] = [USDC_MAINNET, UST_MAINNET]; ```

After withdrawing reward, the user's `last_action` is set as `FinishAction`, however the users `last_amount` is not reset to 0 after processing. For comparison, `stake_to_farm()` and `redeem_stable_token()`, after a `FinishAction` reset the `last_amount` to zero. This could cause undefined behavior in the future. For example ```rust=919 pub fn withdraw_reward(&mut self) -> Result<()> { ... for asset_index in 0..usize::from(farm.asset_count) { ... user_asset.calculate_reward_debt(&farm_reward)?; user_asset.last_action = Action::FinishAction as u8; user_asset.last_amount = 0; // reset as zero before FinishAction } ... } ```

`total_weight` will be used in the future to assign `amount` to different `asset_info.last_amount`. ```rust user.asset_infos[index].last_amount = u128::from(new_amount) .checked_mul(u128::from(asset_info.weight)) .unwrap() .checked_div(farm.total_weight) .unwrap() .try_into() .unwrap(); ``` But the instruction doesn't have a validation over the weights and the total weight. It is important to make sure that `total_weight` is not equal to 0 in order to prevent the construction of unavailable `farm` accounts. ```rust pub fn create_farm(&mut self, ... ) -> Result<()> { for i in 0 .. asset_size{ total_weight += u128::from(weights[i]); ... } farm.total_weight = total_weight; } ```

By adding an empty `bump` constraint to the `#[account(...)]` macro, you signal Anchor to find the canonical bump on its own for the initialization of the account. The user defined `bump` will be written to storage, but will not be used for account initialization. ```rust = 21 pub fn create_state(&mut self, bump: u8, ) -> Result<()> { ... state.bump = bump; ... } ``` Anywhere that the storage `state.bump` is used will result in incorrect validation of account addresses. Reference material on bump creation can be found [here](https://docs.rs/anchor-lang/latest/anchor_lang/context/struct.Context.html#structfield.bumps) and [here](https://www.anchor-lang.com/docs/pdas)

`MAX_ASSET_COUNT` is used to determine the total size that is to be allocated for accounts in `create_or_allocate_account_raw`. ```rust pub const MAX_ASSET_COUNT:usize = 10; ``` However, when this upper limit is not checked when allocating `asset_infos` in `farm`. This may result in the allocated space of the account being depleted. ```rust for i in 0..asset_size { total_weight += u128::from(weights[i]); farm.asset_infos.push(FarmAssetInfo { weight: weights[i], mint: mints[i], }) } ```

`Mint` is the only basis for identifying different types of tokens in Solana, and it is important to make sure that the mint in the a given Token Accounts data, which is input by the user, is correct. The current program only checks whether the address of the Token Account is constructed by the right seeds and program id, via the `check_token_account` function. It is best practice to include mint and ownership checks when interacting with the SPL Token program. For example, a validation that guarantees the Token Accounts mint address is the same as the one expected in the `strategy`. ```rust require!(token_account.mint == strategy.src_mint, ErrorCode::CustomErrorCode); ``` This would guarantee the token account was not created incorrectly.