Solana LP Locker - UNCX Network

The change_owner and confirm_owner instructions implement a two-step ownership transfer mechanism. In the first step, change_owner accepts the new_admin public key as a parameter and sets it as the pending owner, provided it differs from the current admin. However, this instruction does not verify whether the new admin is already set as the current pending owner, nor does it prevent invalid values such as Pubkey::default.

change_owner.rs

pub fn handle_change_owner(ctx: Context<AdminIx>, new_admin_key: Pubkey) -> Result<()> {
    require_keys_neq!(ctx.accounts.config_account.config.admin_key, new_admin_key);

    ctx.accounts.config_account.set_new_admin_pending(new_admin_key);

The confirm_owner instruction (implemented as confirm_pending_admin) promotes the pending admin to active admin and resets the pending field. While the function includes a check to ensure that config.pending is Some, this is redundant because the instruction context (AdminIxConfirmOwnership) already enforces:

• That new_admin is a signer and matches the current config.pending.

• That admin_sign is a signer and matches config.admin_key.

Given these constraints, the presence of a None value in config.pending is structurally impossible at runtime, making the internal check unnecessary and dead code in practice.

admin_ix.rs

pub struct AdminIxConfirmOwnership<'info> {
    /// Signer of the Pending Admin Key
    #[account(constraint =&new_admin.key()==config_account.config.pending.as_ref().unwrap()@ UncxLpError::InvalidAccountError)]
    new_admin: Signer<'info>,
    //current admin
    #[account(constraint =admin_sign.key()==config_account.config.admin_key)]
    admin_sign: Signer<'info>,
    ///Configuration Account PDA
    #[account(mut,address=uncx_config_pda_key::ID)]
    pub config_account: Account<'info, ConfigurationAccount>,
}

confirm_pending_admin.rs

pub fn handle_confirming_pending_owner(ctx: Context<AdminIxConfirmOwnership>) -> Result<()> {

    ctx.accounts.config_account.confirm_pending_admin()?;

configuration.rs

pub fn confirm_pending_admin(&mut self)-> anchor_lang::prelude::Result<()> {
        let Some(pending_admin) = self.config.pending else {
            return err!(UncxLpError::NoPendingAdmin);
        };
        self.config.admin_key = pending_admin;
        //reset pending admin
        self.config.pending = None;

    Ok(())

A similar pattern occurs in the set_dev instruction. The current admin must provide the new developer's public key, and while the program checks that this key differs from the current developer, it does not validate against default values like Pubkey::default.

set_dev.rs

pub fn handle_set_dev(ctx: Context<AdminIx>, new_dev_addr: Pubkey) -> Result<()> {
    require_keys_neq!(ctx.accounts.config_account.config.dev_addr, new_dev_addr);

    ctx.accounts
        .config_account
        .set_developer_address(new_dev_addr);

Although it is ultimately the administrator's responsibility for supplying valid input and can recover from misconfigurations (e.g., via remove_pending_admin or another change_owner call), incorporating stricter validation and eliminating unreachable logic would improve code clarity, reduce potential confusion during audits, and align with robust development standards.

To improve the robustness, clarity, and maintainability of the role transition logic, the following adjustments are recommended:

• In change_owner, add a check to prevent reassigning the same address that is already pending, which would otherwise lead to redundant state transitions and reject default or invalid public keys (Pubkey::default()) to prevent accidental misconfiguration.

• In set_dev, similarly validate that the new developer key is not a default or zeroed address.

• Remove the Some check on config.pending and the corresponding error return in the confirm_pending_admin instruction if it is None. Since the instruction context enforces that the pending admin must sign the transaction, together with the current admin, and match the value of config.pending, it is guaranteed that config.pending is Some.

ACKNOWLEDGED: The UNCX team acknowledged this finding.

The LpFeeCalc struct encompasses several parameters for calculating accumulated LP token fees, including user_share. Based on its nomenclature and intended functionality, user_share ostensibly represents the user's proportionate stake in the locked liquidity. However, this parameter is redundant since the TokenLock struct already maintains the current_locked_amount, which inherently reflects the user's participation in the liquidity pool. Additionally, the user_share field in LpFeeCalc appears to be directly derived from current_locked_amount, resulting in unnecessary data duplication.

model.rs:

pub(crate) struct LpFeeCalc {
    pub(crate) last_k: U128,
    pub(crate) current_k: U128,
    pub(crate) current_total_supply: u64,
    pub(crate) last_total_supply: u64,
    pub(crate) user_share : u64,
}

Furthermore, the TokenLock struct includes coin_accrued_fee and pc_accrued_fee, which, according to their names, are intended to represent the fees claimed in both tokens. Nevertheless, these fields are neither utilized nor updated within the program, rendering them superfluous

token_lock.rs:

pub struct TokenLock {
    pub bump: u8,
    /// The raydium amm pool id/amm pair lp mint address
    // keeping lp-token first for easier indexing via gpa.
    pub amm_id: Pubkey,
    pub lp_mint: Pubkey,
    /// incremental global locker_count value.
    //stored earlier for easier indexing
    pub lock_global_id: u64,
    ///the data the token was locked at ,stored in unix timestamp
    pub lock_date: i64,
    ///the data the token is unlocked at ,stored in unix timestamp
    pub unlock_date: i64,
    /// the country code of the locker/business
    pub country_code: u8,
    //amount of lp locked at the time of creating the lock
    pub initial_lock_amount: u64,
    //amount of lp locked at any specific time after the initial lock
    pub current_locked_amount: u64,
    /// who can withdraw the lp from the lock after specified duration is complete aka owner
    pub lock_owner: Pubkey,
    /// last stored constant product
    pub last_coin_reserve : u64,
    pub last_pc_reserve : u64,
    /// last sync lp supply
    pub last_recorded_lp_supply: u64,
    /// fee claimed so far in terms of lp tokens
    pub fee_on_locked_lp_claimed: u64,
    /// fee claimed in token 0
    pub coin_accrued_fee: u64,
    pub pc_accrued_fee: u64,

The presence of these unused fields may lead to increased transaction costs due to unnecessary data storage and serialization during program operations.

Consider removing those fields in TokenLock if they are not expected to be consumed in the program.

NOT APPLICABLE: The UNCX team stated that the issue is not applicable.

In the handle_lp_fee_claim instruction handler, the fee calculation logic depends on current_locked_amount (the amount of currently locked LP tokens) being greater than zero.

However, the code does not perform an explicit check to ensure that current_locked_amount is indeed greater than zero before proceeding with the commission calculation. Although there is a condition that checks that the calculated commission is less than current_locked_amount and greater than zero, this does not replace a direct check that current_locked_amount is positive.

This lack of a specific error handler or a more explicit check makes debugging and understanding the failure more difficult. Users and developers may not immediately realize that the issue stems from a zero balance of locked liquidity, slowing down the resolution process and potentially causing confusion.

lock_fee_claim.rs:

pub fn handle_lp_fee_claim(
	ctx: Context<ClaimLockedLpFee>,
	lock_id: u64,
) -> Result<()> {
	let lp_locker_acc = ctx.accounts.lp_locker_acc.deref();
	let raydium_amm_info = RaydiumAmmV1Info::try_from(&*ctx.accounts)?;

	// Calculate max_lp_fee_claimable based on feature flag
	#[cfg(not(feature = "raydium-withdraw-check"))]
	let max_lp_fee_claimable = {
		let fee = LpFeeCalc::new(&raydium_amm_info, lp_locker_acc)?
			.calc_user_lp_fee(lp_locker_acc.current_locked_amount)
			.ok_or(UncxLpError::LpFeesCalcFailed)?;

To address this issue, consider to:

• Introduce a direct check at the beginning of the fee calculation process to ensure that current_locked_amount is greater than zero.

• Create a specific error case to explicitly handle this scenario where current_locked_amount is zero. This will provide developers and users with clear and actionable feedback about the issue.

NOT APPLICABLE: The UNCX team stated that the issue is not applicable.