Waterusdc and Vaultka Solana Programs - Vaultka

Both the ExecuteDeposit and ExecuteWithdraw instructions ensure that the received amount falls within the slippage range defined by the expected amount and slippage percentage, verifying that there hasn't been a significant exchange rate change between the request and execution.

However, the RequestDeposit and RequestWithdraw instructions do not allow users to set slippage limits. In volatile markets, outdated Pyth oracle prices, or incorrect manual JLP price settings, users might request a deposit or withdrawal that results in an unexpected output amount.

Depending on the slippage control settings in the ExecuteDeposit or ExecuteWithdraw instructions, the execution might fail, causing funds to become stuck, or it might succeed, with the user receiving less than expected.

In addition to the issue above, the amount in the ExecuteDeposit or ExecuteWithdraw instructions is verified only using the slippage control. However, the slippage control parameter can be adjusted by the admin user without any bounds. Invoking the ExecuteDeposit instruction with an incorrect amount and large slippage range can result in loses either for the user or the protocol.

1. Initialize the vaultkausdc and waterusdc programs

2. Set strategy parameters

3. Manually set the JLP token price to twice of the current price to simulate incorrect/outdated/volatile price

4. Request deposit

5. Set back the JLP token price to the current price

6. Execute deposit

7. Manually set the JLP token price to twice of the current price to simulate incorrect/outdated/volatile price

8. Set back the JLP token price to the current price

9. Execute deposit

Deposit request with incorrect price:

Execute deposit with correct price fails and with incorrect price completes successfully:

To address this issue, it is recommended to implement slippage control also for the RequestDeposit and RequestWithdraw instructions and limit the allowed range of slippage.

SOLVED: The Vaultka team solved the issue by adding a new input parameter to the RequestDeposit, RequestIncreaseCollateral and RequestWithdraw instructions so that the user is able to set the minimal output amount that he or she can accept. If the calculated output amount is less than the expected minimum, the instruction fails and the user has to repeat the request.

The vaultkausdc program implements manually the AccountsMeta structures, that are used during CPI (cross-program invocation) of Borrow and Repay instructions of the waterusdc program.

However, these AccountsMeta structs are implemented incorrectly and the token_program and usdc_mint accounts are marked as mutable instead of read-only.

This prevents the RequestDeposit and the ExecuteWithdraw instructions from being invoked due to cross-program invocation error.

vaultkausdc/src/lib.rs

impl<'info> ToAccountMetas for Borrow<'info> {
    fn to_account_metas(&self, _is_signer: Option<bool>) -> Vec<AccountMeta> {
        vec![
            AccountMeta::new(*self.borrower.key, true),
            AccountMeta::new(*self.vault.key, false),
            AccountMeta::new(*self.whitelisted_borrower.key, false),
            AccountMeta::new(*self.program_authority.key, false),
            AccountMeta::new(*self.receiver_ata.key, false),
            AccountMeta::new(*self.usdc_token_account.key, false),
            AccountMeta::new(*self.token_program.key, false), 
            AccountMeta::new(*self.usdc_mint.key, false), 
        ]
    }
}

impl<'info> ToAccountMetas for Repay<'info> {
    fn to_account_metas(&self, _is_signer: Option<bool>) -> Vec<AccountMeta> {
        vec![
            AccountMeta::new(*self.borrower.key, true),
            AccountMeta::new(*self.vault.key, false),
            AccountMeta::new(*self.whitelisted_borrower.key, false),
            AccountMeta::new(*self.program_authority_lending.key, false),
            AccountMeta::new(*self.usdc_token_account.key, false),
            AccountMeta::new(*self.borrower_ata.key, false),
            AccountMeta::new(*self.token_program.key, false), 
            AccountMeta::new(*self.usdc_mint.key, false) 
        ]
    }
}

In addition to this issue, the context structures Deposit, ExecuteDeposit, RequestIncreaseCollateral, RequestWithdraw and ExecuteWithdraw of the vaultkausdc program do not set correctly the mutability attribute of their accounts (some accounts are missing the mut attribute and are considered as read-only instead of mutable). It does not create a vulnerability in the program, however it does not allow the Anchor Typescript client to correctly generate the AccountMetas for the corresponding instruction. Therefore it is impossible to invoke these instructions using the generated Anchor Typescript client.

1. Initialize the vaultkausdc and waterusdc programs

2. Set strategy parameters

3. Request deposit

4. Execute deposit

5. Request increase collateral

6. Execute deposit

7. Request withdrawal

8. Execute withdrawal

To address this issue, it is recommended to set the accounts token_program and usdc_mint as read-only. Also review the mutability attribute for each account with the Deposit, ExecuteDeposit, RequestIncreaseCollateral, RequestWithdraw and ExecuteWithdraw instruction context structs.

SOLVED: The Vaultka team solved the issue by correctly setting the token_program and usdc_mint accounts as read-only and by correctly setting the mutability attribute to the corresponding accounts.

The RequestWithdraw instruction allows users to initiate withdrawal and close their positions. The instruction calculates, among other calculations also the current price of JLP tokens in USDC tokens based on the price of JLP and USDC tokens in USD.

However, the price of JLP tokens is incorrectly scaled and is 10 times greater than it is supposed to be (it is multiplied by 10 twice instead of only once). This results in the JLP/USDC price being 10 times greater, and consequently also the resulting expected amount of USDC tokens is 10 times greater. This prevents the invocation of the ExecuteWithdraw instruction, because the received amount of USDC tokens will not be within the accepted range of allowed slippage.

vaultkausdc/src/lib.rs

let usdc_price = get_pyth_price_helper(pyth_price_account_usdc, strategy.usdc_usd_feed)?;
let mut jlp_price = 0;
if price_control.is_manual_price_update{
    jlp_price = price_control.jlp_price * 10; // here is the JLP price multiplied by 10
    msg!("Manual JLP price: {}", jlp_price);
} else {
    jlp_price = get_pyth_price_helper(pyth_price_account_jlp, strategy.jlp_sol_feed)? * 10; // here is the JLP price multiplied by 10
    msg!("Pyth JLP price: {}", jlp_price);
}

let mut jlp_p: u128 = jlp_price as u128;
jlp_p = jlp_p * 10; // here is the JLP price multiplied by 10 for the second time
msg!("JLP price: {}", jlp_p);
let mut usdc_p: u128 = usdc_price as u128;
usdc_p = usdc_p * 10;
msg!("USDC price: {}", usdc_p);
let jlp_price_usdc: u128 = jlp_p.checked_mul(10u128.pow(6)).ok_or(ErrorCode::ReceivedAmountOperationError)?
                    .checked_div(usdc_p).ok_or(ErrorCode::ReceivedAmountOperationError)?;
msg!("JLP price in USDC: {}", jlp_price_usdc);

pos_amount = pos_amount.checked_mul(10u64.pow(3)).ok_or(ErrorCode::ReceivedAmountOperationError)?;
let pa_a = pos_amount as u128;
let expected_amount_out_usdc: u128 = pa_a.checked_mul(jlp_price_usdc).ok_or(ErrorCode::ReceivedAmountOperationError)?
    .checked_div(10u128.pow(9)).ok_or(ErrorCode::ReceivedAmountOperationError)?;

1. Initialize the vaultkausdc and waterusdc programs

2. Set strategy parameters

3. Request deposit

4. Execute deposit

5. Request withdrawal

6. Manually execute withdrawal

To address this issue, it is recommended to correct the calculation of the JLP/USDC price pair.

SOLVED: The Vaultka team solved the issue by removing the superfluous multiplication of the the JLP token price and thus corrected the price conversion.

The program relies on the Pyth oracle to retrieve the current USD prices of JLP and USDC tokens. However, the maximum age for the price feed update is currently set to 120,000 seconds (approximately 33.3 hours). If prices become outdated, such as during a Pyth oracle outage, and market volatility is high, both the user and the protocol risk losses due to incorrect exchange rates.

waterusdc/src/lib.rs

pub fn get_pyth_price_helper<'info>(price_account: &Account<'info, PriceUpdateV2>,feed_id: [u8; 32]) -> Result<u64> {
    let price_update = price_account;
    // get_price_no_older_than will fail if the price update is more than 30 seconds old
    let maximum_age: u64 = 120000; 
    // get_price_no_older_than will fail if the price update is for a different price feed.
    // This string is the id of the WIF/USD feed (close to JLP price). See https://pyth.network/developers/price-feed-ids for all available IDs.
    //let feed_id: [u8; 32] = get_feed_id_from_hex("0x4ca4beeca86f0d164160323817a4e42b10010a724c2217c6ee41b54cd4cc61fc")?;
    let price = price_update.get_price_no_older_than(&Clock::get()?, maximum_age, &feed_id)?;
    // Sample output:
    // The price is (7160106530699 ± 5129162301) * 10^-8
    msg!("The price is ({} ± {}) * 10^{}", price.price, price.conf, price.exponent);
    let final_price = price.price as u64;

    Ok(final_price)
}

To address this issue, it is recommended to reduce the maximum age parameter.

SOLVED: The Vaultka team solved the issue by decreasing the maximum age parameter to 120 seconds and thus reducing the risk of outdated price feed.

The SetJlpPrice instruction is intended to manually set the USD price of the JLP token by the admin and supposedly also by the authorized keeper.

However, the access control condition is flawed, as it always evaluates to true, leading to an IncorrectOwner error. Specifically, the code is checking whether the user.key is not equal to both admin.admin and strategy.keeper at the same time. This creates a situation where the condition will always be true, even if the user.key matches one of the two addresses, because a single user.key cannot simultaneously match both admin.admin and strategy.keeper.

waterusdc/src/lib.rs

// Set the JLP Price manually
pub fn set_jlp_price(ctx: Context<SetJLPPrice>, jlp_price: u64) -> ProgramResult {
    let strategy = &mut ctx.accounts.strategy;
    let admin = &ctx.accounts.admin;
    let price_control = &mut ctx.accounts.price_control;

    if *ctx.accounts.user.key != admin.admin || *ctx.accounts.user.key != strategy.keeper{
        return Err(ErrorCode::IncorrectOwner.into());
    }

    //MUST BE IN DECIMAL 8
    //ensure the new JLP price is not 0
    if jlp_price == 0 {
        return Err(ErrorCode::InvalidSetterData.into());
    }
    let min_jlp_price = price_control.jlp_price - (price_control.jlp_price * price_control.jlp_price_control_slippage / 100);
    let max_jlp_price = price_control.jlp_price + (price_control.jlp_price * price_control.jlp_price_control_slippage / 100);

    if jlp_price < min_jlp_price || jlp_price > max_jlp_price {
        return Err(ErrorCode::SlippageError.into());
    }

    price_control.jlp_price = jlp_price;

    Ok(())
}

To address this issue, it is recommended to correct the access control condition. Either to authorize only one defined account (admin or keeper) or change the logical operator from OR to AND.

SOLVED: The Vaultka team solved the issue by modifying the logical operator from OR to AND and thus authorizing the admin or keeper accounts to invoke the setJlpPrice instruction.

During the ExecuteWithdraw instruction, the program deducts various fees from the user's profit during the withdrawal process. The amount of fees depends on the following parameters:

• leverage

• fixed_fee_split

• mfee_percent

Although these parameters can be set within predefined limits, the resulting fees are not limited and certain parameter combinations may result in fees equal to or exceeding 100% of the profit. If the fees are less than or equal to 100% of the profit, the ExecuteWithdraw instruction will succeed, but the user risks receiving very little or no profit at all. If the fees exceed 100% of the profit, the ExecuteWithdraw instruction will fail due to overflow by subtraction, preventing the user from withdrawing their position.

waterusdc/src/lib.rs

fn get_profit_split(profit: u64, leverage: u64, fixed_fee_split: u64, mfee_percent: u64) -> Result<(u64,u64,u64)> {
    msg!("fixed_fee_split: {}", fixed_fee_split);
    msg!("leverage: {}", leverage);
    let fixed_split_leverage = fixed_fee_split * leverage;
    msg!("Fixed split leverage: {}", fixed_split_leverage);
    let fixed_split_adjusted = fixed_fee_split * 10;
    msg!("Fixed split adjusted: {}", fixed_split_adjusted);

    let split = fixed_split_leverage.checked_add(fixed_split_adjusted).ok_or(ErrorCode::ReceivedAmountOperationError)?;
    msg!("Split: {}", split);
    let to_water = profit.checked_mul(split).ok_or(ErrorCode::ReceivedAmountOperationError)?
                        .checked_div(10000).ok_or(ErrorCode::ReceivedAmountOperationError)?;
    msg!("To water: {}", to_water);
    let m_fee = profit.checked_mul(mfee_percent).ok_or(ErrorCode::ReceivedAmountOperationError)?
                    .checked_div(10000).ok_or(ErrorCode::ReceivedAmountOperationError)?;
    msg!("M fee: {}", m_fee);
    let to_user = profit.checked_sub(to_water.checked_add(m_fee).ok_or(ErrorCode::ReceivedAmountOperationError)?).ok_or(ErrorCode::ReceivedAmountOperationError)?;
    msg!("To user: {}", to_user);
    
    Ok((to_water, m_fee, to_user))
}

To address this issue, it is recommended to cap the fees to ensure they do not exceed a maximum percentage of the profit.

SOLVED: The Vaultka team solved the issue by limiting the maximal percentage of total fees and thus ensuring that the fees will always be less than 100 percent of the profit. This guarantees that a user always receives part of the profit and that no arithmetic issues can occur.

The program lacks functionality to close unnecessary PositionInfo and UserInfo accounts, resulting in rent lamports being permanently locked.

To address this issue, it is recommended to close the unneeded accounts and sent the rent back to the initializer.

ACKNOWLEDGED: The client acknowledged the finding.

The project is written in Rust and utilizes the Anchor framework to improve code clarity and security. While the program generally makes effective use of Rust and Anchor, some areas could be enhanced to better align with common Solana development conventions and best software engineering practices. Additionally, numerous formal issues, such as incorrect naming, unused variables, and inaccurate comments, need to be addressed.

To address this issue, it is recommended to implement following actions:

• Do not redefine accounts and instruction data structs (such as lending::cpi::accounts::Repay) for CPI calls.

  • Use automatically generated code from Anchor. Follow the corresponding Anchor CPI documentation.

• Avoid recalculating PDAs and corresponding bumps when not necessary.

  • Use ctx.bumps.program_authority instead of Pubkey::find_program_address(...) .

• To reload accounts after CPI completion, use the reload() method.

  • For example: ctx.accounts.jlp_token_account.reload()?;

• Avoid using msg!() macro if not necessary.

• Avoid truncating downcast.

  // instead of truncating downcast
  let variable_u64 = variable_u128 as u64;
  // prefer using 
  let variable_u64 = u64::try_from(variable_u128).map_err(...)?;

• Add programs as last accounts in the context struct.

• Preferably perform the access control checks in the context struct to separate it from the business logic.

  // For example in set_vault_keeper, instead of 
  if ctx.accounts.user.key() != vault_data.owner {}
  // use rather the has_one attribute
  #[account(mut, has_one = owner, seeds = [b"LENDING".as_ref()], bump)]
  pub lending_account: Account<'info, Lending>,    
  #[account(mut)]    
  pub owner: Signer<'info>, // rename user to owner
  
  // Instead of passing unchecked AccountInfo
  pub lending_program: AccountInfo<'info>,
  // prefer using dedicated Anchor types such as
  pub lending_program: Program<‘info, LendingProgram>

• Avoid separate variables declaration and initialization.

• Define seeds as constants and reuse them across accounts.

• Modularize the program and split it in multiple files.

• Do not use magic numbers.

• Avoid passing unnecessary (unused) accounts to instructions.

  • The System program in SetVaultKeeper, SetFeeParams and SetMaxUtilRate instructions is not needed.

  • The strategy account in ManualPriceUpdate, SetSlippageControl, SetJLPPriceSlippage instructions is not needed.

• Remove the invalid #[instruction(to_whitelist: Pubkey)] macro annotation from SetVaultKeeper struct.

• Use more descriptive names for accounts.

  • For example the account user in SetFeeParams, SetWhitelisted, SetMaxUtilRate, SetVaultKeeper or DisableWhitelisted instruction should rather be owner .

• Return correct errors:

  • After checked_div operations, only division by zero error can happen and not overflow or underflow.

  • Review all errors returned.

  • waterusdc/src/lib.rs

    pub fn repay(ctx: Context<Repay>, repay_amount: u64, debt_value: u64) -> Result<()> {
            let ra:u128;
            ra = repay_amount as u128;
            let dv:u128;
            dv = debt_value as u128;
    
            if ra <= 0 {
                return err!(ErrorCode::InvalidRepayAmount);
            }
    
            if dv <= 0 {
                return err!(ErrorCode::InvalidRepayAmount); // FIX incorrect error
            }
    
            let whitelisted = &ctx.accounts.whitelisted_borrower;
            if !whitelisted.whitelisted {
                return err!(ErrorCode::InvalidBorrowAmount); // FIX incorrect error
            }
    // ...
    }

• Rename incorrect variable names:

  • waterusdc/src/lib.rs

    pub fn set_strategy_params(
            ctx: Context<SetStrategyParams>,
            dtv_limit: u64,
            keeper: Pubkey,
            keeper_ata: Pubkey,
            leverage_limit: u64,
            fixed_fee_split: u64,
            mfee_percent: u64,
            sol_jlp_feed: [u8; 32], // FIX incorrect name 
            jlp_sol_feed: [u8; 32], // FIX incorrect name 
            jlp_mint: Pubkey,
            keeper_fees: u64,
            usdc_mint: Pubkey,
            maturity_time: i64) -> ProgramResult {...)

• Use cargo clippy and resolve all warnings.

• 
  

ACKNOWLEDGED: The client acknowledged the finding.