NT Bundle - Neutral Trade

The NTBundle program exhibits significant centralization risks due to the level of control granted to the bundle manager. Specifically, the bundle manager has the authority to create strategies and configure receivers with allocations (allocation_bps) that can access up to 100% of the underlying balance of the bundle without any form of additional control, checks, or guarantees, as shown in the snippets below:

lib.rs

    pub fn add_strategy(
        ctx: Context<AddStrategy>,
        allocation_bps: u16,
        is_raw_transfer_receiver: bool,
    ) -> Result<()> {
        let receiver_account = &mut ctx.accounts.receiver_account;
        let bundle_account = &mut ctx.accounts.bundle_account;

        // Ensure the caller is the manager
        if ctx.accounts.manager.key() != bundle_account.manager {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }

        // Initialize the receiver account
        receiver_account.strategy_key = ctx.accounts.strategy_key.key();
        receiver_account.allocation_bps = allocation_bps;
        receiver_account.manager = ctx.accounts.manager.key();
        receiver_account.allowed = true;
        receiver_account.is_raw_transfer_receiver = is_raw_transfer_receiver;

        emit!(StrategyAdded {
            strategy_key: ctx.accounts.strategy_key.key(),
            allocation_bps,
        });

        Ok(())
    }

This design introduces the following centralization risks:

• Systemic Failure: A malicious or compromised bundle manager could configure receivers to drain the entire bundle balance, leading to complete loss of funds.

• Single Point of Failure: If for any reason a Receiver can't access the network and has under their control a significant amount of underlying balance of the Bundle, then the users are exposed to important lost of opportunities due to lack of access to their funds.

Consider the next group of recommendations to reduce the mentioned risks:

• Introduce Allocation Caps, such as a maximum allocation limit for receivers (e.g., no single receiver can be allocated more than 50% of the bundle's underlying balance).

• Add Decentralized Oversight, such as a multisig or governance-based approval process for creating or updating strategies and receivers, ensuring that no single entity has unchecked control.

• Require the receivers to provide a collateral to back the funds while the distribution process is executed.

• Introduce a global allocation cap, for instance, that the receivers can't access to more than the 50% of the total of the underlying balance of the Bundle

RISK ACCEPTED: The Neutral Trade team accepted the risk of this finding.

Multiple functions introduce an inconsistent constraint regarding the signer of the instruction. Particularly, the initialize_vault_depositor introduce such inconsistency.

The mentioned function is designed to initialize the Drift Protocol Vault Depositor within the NTBundle program. This depositor serves as a critical component that allows the NTBundle program to integrate with Drift Vaults, enabling interactions such as fund deposits, withdrawals, and other vault-related functionalities.

However, the function introduces an inconsistency in its role constraints:

• The bundle_account enforces that the manager is the bundle_account.manager (via the has_one = manager constraint), as shown in the snippet below:

state.rs

    #[account(
        mut,
        seeds = [BUNDLE_SEED],
        bump,
        has_one = manager
    )]
    pub bundle_account: Account<'info, Bundle>,

• The function logic requires the manager to also match bundle_account.keeper , as shown in the snippet below:

lib.rs

        pub fn initialize_vault_depositor<'info>(
        ctx: Context<'_, '_, '_, 'info, InitializeDriftVaultDepositor<'info>>,
    ) -> Result<()> {
        let bundle_data = &mut ctx.accounts.bundle_account;
        //ensure caller is keeper
        if ctx.accounts.manager.key() != bundle_data.keeper {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }

This inconsistency contradicts the Neutral Trade's specification that manager and keeper are distinct roles. As a result:

• The initialize_vault_depositor function cannot be executed successfully because it effectively enforces that the manager and keeper are the same entity.

• Consequently, the Drift Vault Depositor cannot be initialized, blocking the integration of the NTBundle program with Drift Vaults.

This same inconsistency can be found in 2 other functions:

• call_drift_vault_deposit

• withdraw

As the Neutral trade team mentioned that these functions should be called by the keeper, consider deleting the has_one = manager in the bundle account for the InitializeDriftVaultDepositor , CallDriftVaultDeposit and WithdrawContext context accounts, highlighted in the snippet below:

state.rs

    #[account(
        mut,
        seeds = [BUNDLE_SEED],
        bump,
        has_one = manager
    )]
    pub bundle_account: Account<'info, Bundle>,

SOLVED: The Neutral Trade team solved the issue by deleting the has_one = manager constraint to the mentioned entry points.

The initialize_bundle function accepts critical accounts, such as the pod mint (representing the shares) and USDC mint, as function parameters instead of passing them as context accounts. These parameters are directly assigned to the bundle_account without leveraging Anchor’s built-in account validation features, such as ensuring the accounts are valid Mint accounts, as we can see in the snippet below:

lib.rs

    pub fn initialize_bundle(
        ctx: Context<InitializeBundle>,
        neutral_fee_incrementer: Pubkey,
        w_fee_receiver_account: Pubkey,
        usdc_mint: Pubkey,
        keeper: Pubkey,
        pod_mint: Pubkey,
    ) -> Result<()> {
        let bundle_account = &mut ctx.accounts.bundle_account;
        let oracle_data = &mut ctx.accounts.oracle_data;
        let now = Clock::get()?.unix_timestamp;

        require!(
            neutral_fee_incrementer != Pubkey::default(),
            ErrorCode::InvalidNeutralFeeIncrementer
        );
        require!(
            w_fee_receiver_account != Pubkey::default(),
            ErrorCode::WrongReceiver
        );
        require!(usdc_mint != Pubkey::default(), ErrorCode::WrongReceiver);
        require!(keeper != Pubkey::default(), ErrorCode::WrongReceiver);
        require!(pod_mint != Pubkey::default(), ErrorCode::WrongReceiver);
        require!(
            ctx.accounts.manager.key() != Pubkey::default(),
            ErrorCode::WrongReceiver
        );

        bundle_account.bundle_underlying_balance = 0;
        bundle_account.manager = *ctx.accounts.manager.key;
        bundle_account.withdraw_fee = 500;

        bundle_account.w_fee_receiver_account = w_fee_receiver_account;
        bundle_account.neutral_fee_incrementer = neutral_fee_incrementer;
        bundle_account.usdc_mint = usdc_mint;
        bundle_account.keeper = keeper;
        bundle_account.vault_pod_token_mint = pod_mint;
        bundle_account.twap_period = 86400;

        oracle_data.average_external_equity = 0;
        oracle_data.last_update_time = now;
        oracle_data.last_equity_value = 0;
        oracle_data.cumulative_equity = 0;

        emit!(InitializedVault {
            manager: ctx.accounts.manager.key()
        });

        Ok(())
    }

The key issues identified revolve around the lack of proper validation and the unnecessary flexibility in handling critical mint accounts. Firstly, Anchor provides robust validation for mint accounts through the Account<'info, Mint> type, ensuring that the provided accounts are valid Solana SPL token mints. However, by passing these values as parameters in the form of Pubkey, the program bypasses these safeguards, leaving it exposed to invalid or malicious inputs. Secondly, while the function performs basic checks to ensure the provided Pubkey values are not the default Pubkey, it fails to validate whether the usdc_mint or pod_mint are valid mint accounts. Finally, both the pod mint and USDC mint represent fixed, program-level configurations that do not change over time. Allowing them to be passed as parameters introduces unnecessary risks and complexity, making the program less secure and more error-prone.

The impact of these issues can be categorized into three main areas. Security Risk: Malicious actors could provide invalid or unauthorized mint accounts, potentially leading to the redirection or misappropriation of funds. Operational Failures: If an incorrect or invalid mint is supplied, the program may fail to distribute shares or process transactions as intended, disrupting its functionality. Complexity and Fragility: Allowing these values to be passed as parameters introduces unnecessary complexity, increasing the likelihood of misconfiguration or human error during initialization, ultimately making the program more error-prone and less reliable.

Consider implementing the next steps:

1. Define the hardcoded values for usdc_mint and pod_mint:

lib.rs

const EXPECTED_USDC_MINT: Pubkey = Pubkey::new_from_array([/* insert USDC mint address bytes */]);
const EXPECTED_POD_MINT: Pubkey = Pubkey::new_from_array([/* insert Pod mint address bytes */]);

2. Then, update the context to validate these addresses:

lib.rs

#[derive(Accounts)]
pub struct InitializeBundle<'info> {
    #[account(mut)]
    pub manager: Signer<'info>,
    #[account(init, space = 8 + Bundle::INIT_SPACE, payer = manager, seeds = [BUNDLE_SEED], bump)]
    pub bundle_account: Account<'info, Bundle>,
    #[account(init, space = 8 + OracleData::INIT_SPACE, payer = manager, seeds = [ORACLE_SEED], bump)]
    pub oracle_data: Account<'info, OracleData>,
    #[account(
        constraint = pod_mint.key() == EXPECTED_POD_MINT,
        constraint = pod_mint.owner == spl_token::id(),
        constraint = pod_mint.decimals == EXPECTED_POD_DECIMALS // replace with the expected decimals of the POD mint
    )]
    pub pod_mint: Account<'info, Mint>, // Validate Pod Mint against hardcoded address
    #[account(
        constraint = usdc_mint.key() == EXPECTED_USDC_MINT,
        constraint = usdc_mint.owner == spl_token::id(),
        constraint = usdc_mint.decimals == 6
    )]
    pub usdc_mint: Account<'info, Mint>, // Validate USDC Mint against hardcoded address
    pub system_program: Program<'info, System>,
}

3. Update the initialize_bundle function:

lib.rs

pub fn initialize_bundle(
    ctx: Context<InitializeBundle>,
    neutral_fee_incrementer: Pubkey,
    w_fee_receiver_account: Pubkey,
    keeper: Pubkey,
) -> Result<()> {
    let bundle_account = &mut ctx.accounts.bundle_account;
    let oracle_data = &mut ctx.accounts.oracle_data;
    let now = Clock::get()?.unix_timestamp;

    require!(
        neutral_fee_incrementer != Pubkey::default(),
        ErrorCode::InvalidNeutralFeeIncrementer
    );
    require!(
        w_fee_receiver_account != Pubkey::default(),
        ErrorCode::WrongReceiver
    );
    require!(keeper != Pubkey::default(), ErrorCode::WrongReceiver);
    require!(
        ctx.accounts.manager.key() != Pubkey::default(),
        ErrorCode::WrongReceiver
    );

    bundle_account.bundle_underlying_balance = 0;
    bundle_account.manager = *ctx.accounts.manager.key;
    bundle_account.withdraw_fee = 500;

    bundle_account.w_fee_receiver_account = w_fee_receiver_account;
    bundle_account.neutral_fee_incrementer = neutral_fee_incrementer;
    bundle_account.usdc_mint = *ctx.accounts.usdc_mint.to_account_info().key;
    bundle_account.keeper = keeper;
    bundle_account.vault_pod_token_mint = *ctx.accounts.pod_mint.to_account_info().key;
    bundle_account.twap_period = 86400;

    oracle_data.average_external_equity = 0;
    oracle_data.last_update_time = now;
    oracle_data.last_equity_value = 0;
    oracle_data.cumulative_equity = 0;

    emit!(InitializedVault {
        manager: ctx.accounts.manager.key()
    });

    Ok(())
}

This way, both the USDC token mint and the POD token mint are properly validated according to Anchor checks.

PARTIALLY SOLVED: The Neutral Trade team partially solved the issue by including a hardcoded value for the USDC mint and expected decimals for the pod token mint, but didn't include the hardcoded value for the POD token mint. This is due to the fact that the POD token mint has not been deployed yet.

The change_manager and the set_fees functions sets the manager of the bundle and the treasury, accordingly, as shown in the snippets below:

lib.rs

    pub fn change_manager(ctx: Context<ChangerManager>, new_manager: Pubkey) -> Result<()> {
        let bundle = &mut ctx.accounts.bundle_account;
        if ctx.accounts.user.key() != bundle.manager {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }

        bundle.manager = new_manager;

        emit!(NewManagerSet {
            manager: new_manager
        });

        Ok(())
    }

    pub fn set_fees(ctx: Context<SetFees>, treasury: Pubkey, withdraw_fee: u64) -> Result<()> {
        let bundle = &mut ctx.accounts.bundle_account;
        if ctx.accounts.user.key() != bundle.manager {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }
        if withdraw_fee > 5000 {
            return Err(ErrorCode::ExcessiveFeePercentage.into());
        }

        if treasury == Pubkey::new_from_array([0u8; 32]) {
            return Err(ErrorCode::WrongReceiver.into());
        }
        bundle.withdraw_fee = withdraw_fee;
        bundle.w_fee_receiver_account = treasury;

        emit!(ChangedCoreParam {
            withdraw_fee,
            treasury,
        });

        Ok(())
    }

Neither of these function validate that the new authority account is a valid new account.

The mentioned functions directly overwrite the respective authority without implementing a two-step ownership transfer mechanism. This could allow an incorrect ownership transfer without a formal verification that the next owner is correct.

A two-step ownership transfer process typically requires the following:

1. The current authority nominates a new authority.

2. The nominated authority confirms the acceptance of the role before the transfer is finalized.

The recommended way to update the admin of a platform functionality is to use a two-step ownership transfer pattern, with 2 potential implementations:

• First, implement a function that designates a "new owner candidate." The ownership is not transferred until the new owner "accepts" it by sending a signed transaction.

• Require the new_admin to be a signer of the transaction.

ACKNOWLEDGED: The Neutral Trade team acknowledged this finding.

The Neutral Trade platform manages user funds through a Solana program that interacts with Drift to invest user funds and update the NAV (Net Asset Value). Additionally, it provides funds to receivers, who manage the funds at their discretion and engage in external profit opportunities. Once these receivers generate returns, they return the principal plus profits to the platform, which increases the bundle_underlying_balance and, consequently, the NAV.

The core functions involved in these operations include:

• update_oracle: This function updates the average_external_equity, which affects the NAV calculation. It ensures that the new equity value does not exceed a 50% increase from the current value, preventing manipulation:

  
  

programs/ntbundle/src/lib.rs

    pub fn update_oracle(ctx: Context<SetOracleData>, new_equity: u128) -> Result<()> {
        let oracle_data = &mut ctx.accounts.oracle_data;
        let bundle_data = &mut ctx.accounts.bundle_account;

        //ensure only the keeper can update the oracle
        if ctx.accounts.user.key() != bundle_data.keeper {
            return Err(ErrorCode::NotAuthorized.into());
        }

        //if current equity exceeds the current equity by 50% then revert
        let current_equity = oracle_data.average_external_equity;
        let current_equity_10_percent = current_equity.checked_mul(50).ok_or(ErrorCode::MathError)?.checked_div(100).ok_or(ErrorCode::MathError)?;
        msg!("Current equity: {}", current_equity);
        msg!("Current equity 10%: {}", current_equity_10_percent);
        if new_equity > current_equity_10_percent.checked_add(current_equity).ok_or(ErrorCode::MathError)? {
            return Err(ErrorCode::OracleDataExceeded.into());
        }

        let average_external_equity = update_oracle_data(oracle_data, new_equity)?;
        msg!("Average equity: {}", average_external_equity);

        emit!(OracleUpdated {
            cumulative_equity: oracle_data.cumulative_equity,
            average_external_equity: average_external_equity,
        });
    
        Ok(())
    }

• fetch_total_equity: This function calculates the total NAV by summing the average_external_equity, bundle_underlying_balance, and receivers_borrowed_amount:

  
  

programs/ntbundle/src/lib.rs

fn fetch_total_equity(oracle_data: &OracleData, bundle_data: &Bundle) -> Result<u128> {
    let underlying_balance = bundle_data.bundle_underlying_balance;
    msg!("Underlying balance: {}", underlying_balance);
    let current_equity = oracle_data.average_external_equity;
    msg!("Current external equity: {}", current_equity);
    let current_borrowed_amount = bundle_data.receivers_borrowed_amount;
    msg!("Current borrowed amount: {}", current_borrowed_amount);

    let total_equity = underlying_balance
        .checked_add(current_equity)
        .ok_or(ErrorCode::MathError)? // Handle the Option from checked_add
        .checked_add(current_borrowed_amount)
        .ok_or(ErrorCode::MathError)?; // Handle the Option from checked_sub
    Ok(total_equity)
}

• call_drift_vault_deposit & withdraw: These functions interact with Drift’s vaults to invest or withdraw funds, respectively. They also update the NAV by modifying the average_external_equity.

• perform_refill: Receivers manage user funds and deploy them into profit-generating opportunities. When these funds return with profits, the bundle_underlying_balance increases, indirectly increasing NAV:

  
  

programs/ntbundle/src/lib.rs

    pub fn perform_refill(ctx: Context<Refill>, refill_amount: u64) -> Result<()> {
        let ra: u128 = refill_amount as u128;
        let bundle = &mut ctx.accounts.bundle_account;
        let receiver = &ctx.accounts.receiver_account;
        if ra <= 0 {
            return err!(ErrorCode::RefillAmountInvalid);
        }

        //ensure the receiver is a raw transfer receiver
        if !receiver.is_raw_transfer_receiver {
            return Err(ErrorCode::RawTransferReceiver.into());
        }
    
        // Ensure the receiver is allowed
        if !receiver.allowed {
            return Err(ErrorCode::MustBeInWhitelist.into());
        }

        //ensure the refill amount isnt greater than the amount borrowed
        if bundle.receivers_borrowed_amount < ra {
            return Err(ErrorCode::RefillAmountExceeded.into());
        }

        bundle.receivers_borrowed_amount = bundle
            .receivers_borrowed_amount
            .checked_sub(ra)
            .ok_or(ErrorCode::MathError)?;
            
        // Handle the transfer
        let net_amount_transfer_accounts = TransferChecked {
            from: ctx.accounts.receiver_ata.to_account_info(),
            to: ctx.accounts.bundle_usdc_account.to_account_info(),
            authority: ctx.accounts.receiver.to_account_info(),
            mint: ctx.accounts.usdc_mint.to_account_info(),
        };
        let net_amount_context = CpiContext::new(
            ctx.accounts.token_program.to_account_info(),
            net_amount_transfer_accounts,
        );
        anchor_spl::token::transfer_checked(net_amount_context, refill_amount, 6)?;
    
        refill(bundle, ra)?;
    
        Ok(())
    }

• allocate_funds: Users call this function to buy shares (pod tokens) by depositing funds. The function updates the bundle_underlying_balance, which is factored into the NAV calculation:

  
  

programs/ntbundle/src/lib.rs

    pub fn allocate_funds(ctx: Context<AllocateFunds>, amount: u64) -> Result<()> {
        let oracle_data = &mut ctx.accounts.oracle_data;
        let bundle_account = &mut ctx.accounts.bundle_account;

        if amount <= 0 {
            return err!(ErrorCode::AllocationAmountZero);
        }

        if bundle_account.is_distribution_started {
            return Err(ErrorCode::DistributionAlreadyStarted.into());
        }

        // Derive bump seed
        let (_program_authority, program_authority_bump) = Pubkey::find_program_address(&[AUTHORITY_SEED], &ctx.program_id);
        let authority_seeds = &[AUTHORITY_SEED.as_ref(), &[program_authority_bump]];
        let signer_seeds = &[&authority_seeds[..]];

        let net_amount_transfer_accounts = TransferChecked {
            from: ctx.accounts.user_token_account.to_account_info(),
            to: ctx.accounts.bundle_usdc_account.to_account_info(),
            authority: ctx.accounts.user.to_account_info(),
            mint: ctx.accounts.usdc_mint.to_account_info(),
        };
        let net_amount_context = CpiContext::new(
            ctx.accounts.token_program.to_account_info(),
            net_amount_transfer_accounts,
        );
        anchor_spl::token::transfer_checked(net_amount_context, amount, 6)?;

        let deposit_fee = amount.checked_mul(bundle_account.deposit_fee)
            .ok_or(ErrorCode::MathError)?.checked_div(10000)
            .ok_or(ErrorCode::MathError)?;
        msg!("Deposit fee: {}", deposit_fee);
        let deposit_sub_fee = amount.checked_sub(deposit_fee).ok_or(ErrorCode::MathError)?;
        msg!("Deposit sub fee: {}", deposit_sub_fee);

        if deposit_fee > 0 {
            //transfer the deposit fee to the treasury
            let net_amount_transfer_accounts = TransferChecked {
                from: ctx.accounts.bundle_usdc_account.to_account_info(),
                to: ctx.accounts.treasury_address.to_account_info(),
                authority: ctx.accounts.program_authority.to_account_info(),
                mint: ctx.accounts.usdc_mint.to_account_info(),
            };
            let net_amount_context = CpiContext::new_with_signer(
                ctx.accounts.token_program.to_account_info(),
                net_amount_transfer_accounts,
                signer_seeds,
            );
            anchor_spl::token::transfer_checked(net_amount_context, deposit_fee, 6)?;
        }

        let da: u128 = deposit_sub_fee as u128;
        let shares: u128;

        let current_pod_total_supply = fetch_current_pod_total_supply(&ctx.accounts.vault_pod_token_mint)?;
        msg!("Current pod total supply: {}", current_pod_total_supply);
        let total_assets = fetch_total_equity(oracle_data, bundle_account)?;
        msg!("Total assets: {}", total_assets);
        if current_pod_total_supply == 0 {
            shares = da;
        } else {
            shares = da.checked_mul(current_pod_total_supply)
            .ok_or(ErrorCode::MathError)?.checked_div(total_assets)
            .ok_or(ErrorCode::MathError)?;
        }

        msg!("SA: {}", shares);

        bundle_account.bundle_underlying_balance = bundle_account
            .bundle_underlying_balance
            .checked_add(da)
            .ok_or(ErrorCode::MathError)?;
        msg!("VB: {}", bundle_account.bundle_underlying_balance);

        let cpi_accounts = MintTo {
            mint:  ctx.accounts.vault_pod_token_mint.to_account_info(),
            to: ctx.accounts.user_pod_token_account.to_account_info(),
            authority: ctx.accounts.program_authority.to_account_info(),
        };
        let cpi_program = ctx.accounts.token_program.to_account_info();
        let cpi_context = CpiContext::new_with_signer(
            cpi_program,
            cpi_accounts,
            signer_seeds,
        );
        let _ = anchor_spl::token::mint_to(cpi_context, shares as u64);
        msg!("MTU: {}", shares);
        
        let account_balance = fetch_account_balance(&ctx.accounts.bundle_usdc_account)?;
        msg!("Bundle Balance: {}", account_balance);

        emit!(Allocated {
            from: ctx.accounts.user.key(),
            to: ctx.accounts.bundle_account.key(),
            amount: da,
            timestamp: Clock::get()?.unix_timestamp,
        });

        Ok(())
    }

• redeem_collateral: This function allows users to redeem their shares by converting their pod tokens back into USDC based on the current NAV:

  
  

programs/ntbundle/src/lib.rs

    pub fn redeem_collateral(ctx: Context<RedeemCollateral>, redeem_amount: u64) -> Result<()> {
        let bundle_account = &mut ctx.accounts.bundle_account;
        let oracle_data = &mut ctx.accounts.oracle_data;
        let wa: u128 = redeem_amount as u128;
        let total_assets = fetch_total_equity(oracle_data, bundle_account)?;
        msg!("Total assets: {}", total_assets);
        let user_token_balance = fetch_account_balance(&ctx.accounts.user_pod_token_account)? as u128;
        msg!("UTB: {}", user_token_balance);

        //prevent withdraw if distribution is started
        //if someone is trying to withdraw while we are distributing, it will lower the total
        //underlying usdc balance available that was assigned in start_distribution
        //and the keeper wont have enough to distribute the full amount
        //this wont be a blocker as the backend will distribute automatically then set is_distribution_started to false once done
        if bundle_account.is_distribution_started {
            return Err(ErrorCode::DistributionAlreadyStarted.into());
        }

        let current_pod_total_supply = fetch_current_pod_total_supply(&ctx.accounts.vault_pod_token_mint)?;
        msg!("Current pod total supply: {}", current_pod_total_supply);

        let max_withdraw = user_token_balance.checked_mul(total_assets)
        .ok_or(ErrorCode::MathError)?
        .checked_div(current_pod_total_supply)
        .ok_or(ErrorCode::MathError)?;
        msg!("100% WD: {}", max_withdraw);

        if wa > max_withdraw || wa <= 0 {
           return err!(ErrorCode::AmountMustBeBelowMax);
        }

        let bundle_underlying_balance = bundle_account.bundle_underlying_balance;
        msg!("VB: {}", bundle_underlying_balance);
        if bundle_underlying_balance < wa || wa <= 0 {
            return err!(ErrorCode::RedemptionAmountExceeded);
        }

        let computed_user_shares:u128;
        computed_user_shares = wa.checked_mul(current_pod_total_supply)
            .ok_or(ErrorCode::MathError)?
            .checked_div(total_assets)
            .ok_or(ErrorCode::MathError)?;

        if computed_user_shares == 0 {
            return err!(ErrorCode::RedemptionAmountExceeded);
        }
        msg!("CUS: {}", computed_user_shares);

        bundle_account.bundle_underlying_balance = bundle_account
            .bundle_underlying_balance
            .checked_sub(wa)
            .ok_or(ErrorCode::MathError)?;

        let w_fee = redeem_amount.checked_mul(bundle_account.withdraw_fee)
            .ok_or(ErrorCode::MathError)?
            .checked_div(10000)
            .ok_or(ErrorCode::MathError)?;
        msg!("FC: {}", w_fee);

        let with_min_fee = redeem_amount.checked_sub(w_fee).ok_or(ErrorCode::MathError)?;
        msg!("UWG: {}", with_min_fee);

        let (_program_authority, program_authority_bump) = Pubkey::find_program_address(&[AUTHORITY_SEED], &ctx.program_id);
        let authority_seeds = &[AUTHORITY_SEED.as_ref(), &[program_authority_bump]];
        let signer_seeds = &[&authority_seeds[..]];

        let burn_accounts = Burn {
            mint: ctx.accounts.vault_pod_token_mint.to_account_info(),
            from: ctx.accounts.user_pod_token_account.to_account_info(),
            authority: ctx.accounts.user.to_account_info()
        };
        let burn_ctx = CpiContext::new_with_signer(ctx.accounts.token_program.to_account_info(), burn_accounts, signer_seeds);
        anchor_spl::token::burn(burn_ctx, computed_user_shares as u64)?;

        let net_amount_transfer_accounts = TransferChecked {
            from: ctx.accounts.bundle_usdc_account.to_account_info(),
            to: ctx.accounts.user_token_account.to_account_info(),
            authority: ctx.accounts.program_authority.to_account_info(),
            mint: ctx.accounts.usdc_mint.to_account_info(),
        };
        let net_amount_context = CpiContext::new_with_signer(
            ctx.accounts.token_program.to_account_info(),
            net_amount_transfer_accounts,
            signer_seeds,
        );
        anchor_spl::token::transfer_checked(net_amount_context, with_min_fee, 6)?;

        let fee_transfer_accounts = TransferChecked {
            from: ctx.accounts.bundle_usdc_account.to_account_info(),
            to: ctx.accounts.treasury_address.to_account_info(),
            authority: ctx.accounts.program_authority.to_account_info(),
            mint: ctx.accounts.usdc_mint.to_account_info(),
        };
        let fee_amount_context = CpiContext::new_with_signer(
            ctx.accounts.token_program.to_account_info(),
            fee_transfer_accounts,
            signer_seeds,
        );
        anchor_spl::token::transfer_checked(fee_amount_context, w_fee, 6)?;

        emit!(Redeemed {
            from: ctx.accounts.bundle_account.key(),
            to: ctx.accounts.user.key(),
            amount: wa,
            timestamp: Clock::get()?.unix_timestamp,
        });

        Ok(())
    }

A potential arbitrage opportunity exists if a malicious user can accurately predict when the NAV update occurs. By timing their allocate_funds and redeem_collateral calls strategically, they could profit from fluctuations in the NAV. However, this scenario is highly unlikely due to multiple factors:

1. Frequent NAV Updates: NAV updates occur at least once daily and potentially more frequently if interactions with Drift or receivers' fund settlements happen multiple times per day. This reduces the feasibility of accurately predicting NAV movements.

2. Unpredictability of Receiver Profit Events: Unlike Drift interactions, which may follow a pattern, receiver profit opportunities are highly dynamic and unpredictable. Since receivers engage in external profit strategies, there is no fixed pattern to exploit.

3. Front-Running in Solana is Nearly Impossible: Unlike Ethereum, where front-running is a common problem due to transaction mempools, Solana does not have a public mempool. Transactions are executed almost immediately upon submission, and block producers do not have a window to reorder or sandwich transactions. This makes classic front-running attacks extremely difficult or practically impossible on Solana.

As an impact, we can mention:

• Users engaging in arbitrage could profit at the expense of long-term investors.

• The fairness of the platform could be questioned if repeated arbitrage is observed.

• In extreme cases, frequent arbitrage could slightly affect the returns of other investors.

While the identified arbitrage opportunity exists, it is unlikely to be a significant issue due to the difficulty in accurately predicting NAV movements. The unpredictable nature of receiver profits, the frequent NAV updates, and Solana’s front-running-resistant design make this attack vector impractical. The proposed mitigation measures (fees and frequent NAV updates) help minimize the risk and maintain fairness for all platform users.

Arbitrage is a common issue in financial markets and decentralized finance (DeFi). In many cases, it is an accepted phenomenon, and mitigation strategies depend on the specific use case of the platform. In this case, the mitigation consists of:

• Implementing deposit and withdrawal fees: These fees will reduce the profit margins of potential arbitrageurs, disincentivizing short-term exploitation of NAV updates.

• Frequent NAV Updates: The client has designed the system to update NAV daily at a minimum. This reduces the window of opportunity for arbitrageurs and ensures that share prices reflect current market conditions. Since the Drift vault interactions (call_drift_vault_deposit and withdraw) can happen in an irregular basis, receiver settlements are irregular, and update_oracle is also called daily, the NAV will remain up to date.

• Solana’s Design Prevents Front-Running: Since Solana does not have a public mempool and transactions are executed almost instantly, front-running attacks are practically impossible. Arbitrageurs cannot front-run a NAV update or strategically place transactions in the same way they could on Ethereum or other blockchains with open mempools.

PARTIALLY SOLVED: The Neutral Trade team partially solved the issue by adding a deposit fee. As previously mentioned, arbitrage is a common challenge in financial markets, including DeFi. The widely accepted approach is to mitigate it by implementing tailored mechanisms suited to the specific issue at hand. In this case, introducing a deposit fee will effectively diminish the profit potential for arbitrageurs, thereby reducing the likelihood of the platform becoming a consistent target for such activities.

The NTBundle program does not track the global allocation basis points (BPS), which is the cumulative sum of all allocation BPS values assigned to receivers. This omission leads to a critical issue where multiple receivers can collectively exceed the 100% allocation limit (10,000 BPS).

Specifically:

1. The add_strategy function allows the bundle manager to create strategies and assign an arbitrary allocation_bps to each receiver without validating the cumulative total of all allocations, as shown in the snippet below:

lib.rs

    pub fn add_strategy(
        ctx: Context<AddStrategy>,
        allocation_bps: u16,
        is_raw_transfer_receiver: bool,
    ) -> Result<()> {
        let receiver_account = &mut ctx.accounts.receiver_account;
        let bundle_account = &mut ctx.accounts.bundle_account;

        // Ensure the caller is the manager
        if ctx.accounts.manager.key() != bundle_account.manager {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }

        // Initialize the receiver account
        receiver_account.strategy_key = ctx.accounts.strategy_key.key();
        receiver_account.allocation_bps = allocation_bps;
        receiver_account.manager = ctx.accounts.manager.key();
        receiver_account.allowed = true;
        receiver_account.is_raw_transfer_receiver = is_raw_transfer_receiver;

        emit!(StrategyAdded {
            strategy_key: ctx.accounts.strategy_key.key(),
            allocation_bps,
        });

        Ok(())
    }

2. The handle_distribution_conditions function, called by distribute_to_receivers, validates that a single receiver's allocation_bps does not exceed 10,000. However, it does not account for the total allocation across all receivers.

lib.rs

fn handle_distribution_conditions<'info>(
    bundle_data: &mut Bundle,
    allocation_bps: u16,
    receiver_key: Pubkey,
) -> Result<u64> {
    // Ensure the distribution is started
    if !bundle_data.is_distribution_started {
        return Err(ErrorCode::DistributionNotStarted.into());
    }

    // Validate allocation basis points
    if allocation_bps > 10000 {
        return Err(ErrorCode::InvalidAllocations.into());
    }

    // Check if receiver has already been allocated
    if bundle_data.allocated_receivers.contains(&receiver_key) {
        return Err(ErrorCode::ReceiverAlreadyAllocated.into()); // or a new specific error
    }

    let available = bundle_data.distribution_base_amount;
    if available == 0 {
        return Err(ErrorCode::AllocationAmountZero.into());
    }

    // Calculate the share amount
    let share_amount = (available as u128)
        .checked_mul(allocation_bps as u128)
        .ok_or(ErrorCode::MathError)?
        .checked_div(10000)
        .ok_or(ErrorCode::MathError)? as u64;

    if share_amount == 0 || share_amount as u128 > bundle_data.distribution_base_amount {
        return Err(ErrorCode::RedemptionAmountExceeded.into());
    }

    // Update the bundle data
    bundle_data.bundle_underlying_balance = bundle_data
        .bundle_underlying_balance
        .checked_sub(share_amount as u128)
        .ok_or(ErrorCode::MathError)?;
    bundle_data.left_to_distribute = bundle_data
        .left_to_distribute
        .checked_sub(share_amount as u128)
        .ok_or(ErrorCode::MathError)?;
    msg!("Left to distribute: {}", bundle_data.left_to_distribute);
    msg!(
        "Bundle underlying balance: {}",
        bundle_data.bundle_underlying_balance
    );

    // Track the receiver
    bundle_data.allocated_receivers.push(receiver_key);

    msg!(
        "Distribution updated: VB={}, LD={}",
        bundle_data.bundle_underlying_balance,
        bundle_data.left_to_distribute
    );

    emit!(DistributedToReceivers {
        amount: share_amount,
        from: bundle_data.manager,
        receiver: receiver_key,
        timestamp: Clock::get()?.unix_timestamp,
    });

    Ok(share_amount)
}

3. If the total allocation of all receivers exceeds 10,000 BPS, the last receiver(s) to participate in the distribution will not receive funds, as the remaining allocation is already exhausted.

This creates a situation where:

• The last receiver(s) to be distributed will fail due to insufficient remaining allocation.

• The protocol does not enforce fairness or guarantee fund distribution for all receivers when the global allocation exceeds 10,000 BPS.

• Introduce Global Allocation Tracking:

  • Add a field in the bundle_account to store the cumulative sum of all allocation_bps values assigned to receivers.

state.rs

pub struct Bundle {
    pub global_allocation_bps: u16,
    // other fields...
}

• Validate Allocations in add_strategy:

  • Ensure that adding a new strategy does not cause the global_allocation_bps to exceed 10,000.

lib.rs

if bundle_account.global_allocation_bps + allocation_bps > 10000 {
    return Err(ErrorCode::ExceedsGlobalAllocation.into());
}

bundle_account.global_allocation_bps += allocation_bps;

• Update Allocations Safely:

  • If update_allocations or other functions modify an existing receiver's allocation, ensure that adjustments to the global_allocation_bps remain within limits.

SOLVED: The Neutral Trade team solved the issue by implementing a global parameter that tracks the sum of all the allocations of the receivers.

The add_strategy and update_allocations functions are critical for managing Receivers within the bundle system. These functions serve the following purposes:

1. add_strategy:

   • Creates a new Receiver account with the specified allocation_bps (basis points) and other configuration settings.

   • Associates the receiver with a bundle, enabling it to participate in distributions.

2. update_allocations:

   • Updates the allocation_bps for an existing Receiver.

   • Allows dynamic adjustment of allocation percentages for receivers in a bundle.

Both functions allow for the configuration and reconfiguration of receivers that participate in fund distributions. However, neither function validates that the allocation_bps value is within the maximum allowed limit of 10,000 bps (100%), which can lead to invalid receiver setups, as shown in the snippets below:

lib.rs

    pub fn add_strategy(
        ctx: Context<AddStrategy>,
        allocation_bps: u16,
        is_raw_transfer_receiver: bool,
    ) -> Result<()> {
        let receiver_account = &mut ctx.accounts.receiver_account;
        let bundle_account = &mut ctx.accounts.bundle_account;

        // Ensure the caller is the manager
        if ctx.accounts.manager.key() != bundle_account.manager {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }

        // Initialize the receiver account
        receiver_account.strategy_key = ctx.accounts.strategy_key.key();
        receiver_account.allocation_bps = allocation_bps;
        receiver_account.manager = ctx.accounts.manager.key();
        receiver_account.allowed = true;
        receiver_account.is_raw_transfer_receiver = is_raw_transfer_receiver;

        emit!(StrategyAdded {
            strategy_key: ctx.accounts.strategy_key.key(),
            allocation_bps,
        });

        Ok(())
    }

    pub fn update_allocations(
        ctx: Context<UpdateAllocations>,
        new_allocation_bps: u16,
    ) -> Result<()> {
        let receiver_account = &mut ctx.accounts.receiver_account;

        // Ensure caller is the manager
        if ctx.accounts.manager.key() != receiver_account.manager {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }

        // Update allocation
        receiver_account.allocation_bps = new_allocation_bps;

        emit!(AllocationsUpdated {
            strategy_key: receiver_account.strategy_key,
            allocation_bps: new_allocation_bps,
        });

        Ok(())
    }

As a result, Receivers with allocation_bps greater than 10,000 bps can exist, creating a state where the receiver's allocation exceeds the total available allocation for distribution, and when the distribute_to_receivers function is called, it will fail for receivers with invalid allocation_bps, as the handle_distribution_conditions function will return an error (ErrorCode::InvalidAllocations).

It is recommended the following:

• Add Validation in add_strategy:

  • Validate the allocation_bps value when creating a receiver to ensure it does not exceed 10,000 bps.

lib.rs

if allocation_bps > 10000 {
    return Err(ErrorCode::InvalidAllocations.into());
}

• Add Validation in update_allocations:

  • Ensure that new_allocation_bps is validated when updating a receiver's allocation.

lib.rs

if new_allocation_bps > 10000 {
    return Err(ErrorCode::InvalidAllocations.into());
}

SOLVED: The Neutral Trade team solved the issue by implementing the globally tracked sum of all the allocations for the receivers. This way, the need to check that a receiver allocation is bigger than 10,000 becomes redundant.

The remove_strategy function disables a receiver account by setting its allowed field to false:

lib.rs

    pub fn remove_strategy(ctx: Context<EditStrategy>) -> Result<()> {
        let receiver_account = &mut ctx.accounts.receiver_account;

        //ensure sender is the manager
        if ctx.accounts.manager.key() != receiver_account.manager {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }

        receiver_account.allowed = false;

        emit!(StrategyRemoved {
            strategy_key: receiver_account.strategy_key,
        });

        Ok(())
    }

This function is designed to prevent a receiver from participating in further operations, such as receiving funds during distribution or performing a refill. However, there is no complementary functionality to re-enable the receiver account by resetting the allowed field to true. Once a receiver is disabled, it remains permanently unusable, which introduces unnecessary limitations and reduces the protocol's flexibility.

Consider creating a function that sets the allowed field of a receiver to true:

lib.rs

    pub fn enable_strategy(ctx: Context<EditStrategy>) -> Result<()> {
        let receiver_account = &mut ctx.accounts.receiver_account;

        //ensure sender is the manager
        if ctx.accounts.manager.key() != receiver_account.manager {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }

        receiver_account.allowed = true;

        emit!(StrategyAllowed {
            strategy_key: receiver_account.strategy_key,
        });

        Ok(())
    }

SOLVED: The Neutral Trade team solved the issue by adding an enable_strategy entry point.

The update_allocations function is responsible for updating the allocation basis points (bps) of a receiver. This allocation determines the share of resources assigned to the receiver during distributions. On the other hand, the remove_strategy function allows the receiver manager to disable the Strategy be used.

In both functions, a validation ensures that the signer is the manager of the receiver_account, as shown in the snippet below:

lib.rs

    pub fn update_allocations(
        ctx: Context<UpdateAllocations>,
        new_allocation_bps: u16,
    ) -> Result<()> {
        let receiver_account = &mut ctx.accounts.receiver_account;

        // Ensure caller is the manager
        if ctx.accounts.manager.key() != receiver_account.manager {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }

        // Update allocation
        receiver_account.allocation_bps = new_allocation_bps;

        emit!(AllocationsUpdated {
            strategy_key: receiver_account.strategy_key,
            allocation_bps: new_allocation_bps,
        });

        Ok(())
    }

    pub fn remove_strategy(ctx: Context<EditStrategy>) -> Result<()> {
        let receiver_account = &mut ctx.accounts.receiver_account;

        //ensure sender is the manager
        if ctx.accounts.manager.key() != receiver_account.manager {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }

        receiver_account.allowed = false;

        emit!(StrategyRemoved {
            strategy_key: receiver_account.strategy_key,
        });

        Ok(())
    }

However, Neutral trade has stated that it should be the bundle manager, not the receiver manager, who is responsible for updating the allocation of a receiver. The current state of the NTBundle program sets the value of the receiver manager to the same value of the bundle manager, but the bundle manager can be updated and the receiver manager can't.

If for some reason the bundle manager key needs to be updated due to a leakage or lost of the key, the Receiver allocation bps will be exposed to lost of control.

Replace the validation logic to ensure that the caller matches the manager field in the bundle_account rather than the receiver_account, in both functions:

lib.rs

    pub fn update_allocations(
        ctx: Context<UpdateAllocations>,
        new_allocation_bps: u16,
    ) -> Result<()> {
        let receiver_account = &mut ctx.accounts.receiver_account;
        let bundle_account = &mut ctx.accounts.bundle_account;

        // Ensure caller is the manager
        if ctx.accounts.manager.key() != bundle_account.manager {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }

        // Update allocation
        receiver_account.allocation_bps = new_allocation_bps;

        emit!(AllocationsUpdated {
            strategy_key: receiver_account.strategy_key,
            allocation_bps: new_allocation_bps,
        });

        Ok(())
    }

    pub fn remove_strategy(ctx: Context<EditStrategy>) -> Result<()> {
        let receiver_account = &mut ctx.accounts.receiver_account;

        //ensure sender is the manager
        if ctx.accounts.manager.key() != receiver_account.manager {
            return Err(ErrorCode::UnauthorizedManagerAction.into());
        }

        receiver_account.allowed = false;

        emit!(StrategyRemoved {
            strategy_key: receiver_account.strategy_key,
        });

        Ok(())
    }

SOLVED: The Neutral Trade team solved the issue by changing the manager check to the bundle account manager in the mentioned entry points.

In the distribute_to_receivers function, there is redundant validation for the allocation_bps value of the receiver. The function retrieves allocation_bps from the receiver account, but later in the function, it validates that allocation_bps matches receiver.allocation_bps:

lib.rs

    pub fn distribute_to_receivers(ctx: Context<DistributeToReceivers>) -> Result<()> {
        let bundle_data = &mut ctx.accounts.bundle_account;
        let receiver = &ctx.accounts.receiver_account;
        let allocation_bps = receiver.allocation_bps;

        //ensure the receiver is a raw transfer receiver
        if !receiver.is_raw_transfer_receiver {
            return Err(ErrorCode::RawTransferReceiver.into());
        }

        //ensure the receiver is allowed
        if !receiver.allowed {
            return Err(ErrorCode::MustBeInWhitelist.into());
        }

        // Ensure caller is the keeper
        if ctx.accounts.manager.key() != bundle_data.keeper {
            return Err(ErrorCode::NotAuthorized.into());
        }

        // Ensure the passed-in allocation_bps matches the stored allocation_bps
        if allocation_bps != receiver.allocation_bps {
            return Err(ErrorCode::InvalidAllocations.into());
        }

As an impact, we can mention the next situations:

• Code Inefficiency: The redundant check increases code complexity without adding value, as it will never fail.

• Maintenance Overhead: Unnecessary validations can confuse developers and lead to longer debugging or modification times.

• Readability Issues: Redundant logic reduces code readability and may obscure the intent of the function.

Delete the code that redundantly validates the allocation bps of the Receiver account.

SOLVED: The Neutral Trade team solved the issue by modifying deleting the mentioned validations.

In the perform_refill function, there are two separate validations to ensure that the refill_amount (ra) does not exceed the receivers_borrowed_amount in the bundle_account. These validations are redundant as they perform the same check using equivalent logic:

lib.rs

    pub fn perform_refill(ctx: Context<Refill>, refill_amount: u64) -> Result<()> {
        let ra: u128 = refill_amount as u128;
        let bundle = &mut ctx.accounts.bundle_account;
        let receiver = &ctx.accounts.receiver_account;
        if ra <= 0 {
            return err!(ErrorCode::RefillAmountInvalid);
        }

        //ensure the receiver is a raw transfer receiver
        if !receiver.is_raw_transfer_receiver {
            return Err(ErrorCode::RawTransferReceiver.into());
        }

        // Ensure the receiver is allowed
        if !receiver.allowed {
            return Err(ErrorCode::MustBeInWhitelist.into());
        }

        //ensure the refill amount isnt greater than the amount borrowed
        if bundle.receivers_borrowed_amount < ra {
            return Err(ErrorCode::RefillAmountExceeded.into());
        }

        // Ensure the refill amount isn't greater than the amount borrowed
        if ra > bundle.receivers_borrowed_amount {
            return Err(ErrorCode::RefillAmountExceeded.into());
        }

As an impact, we can mention the next situations:

• Code Inefficiency: The redundant check increases code complexity without adding value, as it will never fail.

• Maintenance Overhead: Unnecessary validations can confuse developers and lead to longer debugging or modification times.

• Readability Issues: Redundant logic reduces code readability and may obscure the intent of the function.

Consider removing one of the mentioned validations of the perform_refill function.

SOLVED: The Neutral Trade team solved the issue by modifying deleting the mentioned validations.