## Summary A pool owner is able to close the pool even if the pool is attached to the shared escrow account which might lead to inconsistencies and liquidity imbalances ## Finding Description As part of the AMM program, a user is able to create different type of pools, for users to buy NFTs from, sell NFTs to or to trade NFTs. When creating a pool, there is an option to provide `Pubkey` of a shared escrow account. The description of the shared escrow field is as follows: "_If an escrow account is present, it means it's a shared-escrow pool where liquidity is shared with other pools_." This means that the funds provided to this pool do not belong ONLY to this specific pool. Closing the pool is one of the operations an admin is permitted to perform (in addition to creating, editing, depositing SOL, and withdrawing SOL). When closing the pool, it is crucial to ensure that the pool does not contain any NFTs. Additionally, it is important to verify that the pool is not linked to the shared escrow account, as noted in multiple comments in the code as a reminder. For example, see `close_pool.rs`: `/! Close a pool if it has no NFTs and is not attached to a shared escrow.` or in the comment for the instruction in `lib.rs`: `/// Close a pool if it has no NFTs and is not attached to a shared escrow.` The pool PDA within the `ClosePool` struct correctly checks if `pool.nfts_held == 0` but does not verify whether the pool is attached to the shared escrow account. ```rust /// Instruction accounts. #[derive(Accounts)] pub struct ClosePool<'info> { /// The rent payer to refund pool rent to. /// CHECK: has_one in pool #[account(mut)] pub rent_payer: UncheckedAccount<'info>, /// The owner must sign to close the pool. /// CHECK: has_one = owner in pool #[account(mut)] pub owner: Signer<'info>, /// The pool to close. #[account(mut, seeds = [ b"pool", owner.key().as_ref(), pool.pool_id.as_ref(), ], bump = pool.bump[0], has_one = rent_payer @ ErrorCode::WrongRentPayer, constraint = pool.version == CURRENT_POOL_VERSION @ ErrorCode::WrongPoolVersion, constraint = pool.nfts_held == 0 @ ErrorCode::ExistingNfts, )] pub pool: Box>, /// The Solana system program. pub system_program: Program<'info, System>, } ``` Closing the account means sending its balance to the owner and the rent to the specified rent payer. On the other side, if we examine the `WithdrawSOL` struct in `withdraw_sol.rs`, we can see that it is impossible for the owner to withdraw any SOL if the pool is attached to the shared escrow. ```rust /// Instruction accounts. #[derive(Accounts)] pub struct WithdrawSol<'info> { /// The owner of the pool and will receive the SOL. #[account(mut)] pub owner: Signer<'info>, /// The pool from which the SOL will be withdrawn. #[account( mut, seeds = [ b"pool", owner.key().as_ref(), pool.pool_id.as_ref(), ], bump = pool.bump[0], constraint = pool.version == CURRENT_POOL_VERSION @ ErrorCode::WrongPoolVersion, constraint = pool.config.pool_type == PoolType::Token || pool.config.pool_type == PoolType::Trade @ ErrorCode::WrongPoolType, constraint = pool.shared_escrow == Pubkey::default() @ ErrorCode::PoolOnSharedEscrow, )] pub pool: Box>, /// The Solana system program. pub system_program: Program<'info, System>, } ``` In addition, the owner could bypass this check by closing the pool directly, allowing him to withdraw any amount of SOL that may be present. ## Impact Explanation It may cause inconsistent liquidity state/unforeseen events if the SOL from the shared escrow account is transferred to the pool while buying and selling NFTs ## Likelihood Explanation Considering that any owner of the pool can do it at any moment, it is highly likely that it will happen, whether it is on purpose or not. ## Recommendation Add missing escrow check to `close_pool.rs`: ```rust /// The pool to close. #[account(mut, seeds = [ b"pool", owner.key().as_ref(), pool.pool_id.as_ref(), ], bump = pool.bump[0], has_one = rent_payer @ ErrorCode::WrongRentPayer, constraint = pool.version == CURRENT_POOL_VERSION @ ErrorCode::WrongPoolVersion, constraint = pool.nfts_held == 0 @ ErrorCode::ExistingNfts, constraint = pool.shared_escrow == Pubkey::default() @ ErrorCode::PoolOnSharedEscrow // add this here )] pub pool: Box>, /// The Solana system program. pub system_program: Program<'info, System>, } ``` It is also important to add this check in the `pool.rs` (in the state account) when trying to auto-close the pool because this function is called directly after the buy/sell events. ```rust /// A utitilty function that tries to autoclose a pool if it is possible. pub fn try_autoclose_pool<'info>( pool: &Account<'info, Pool>, rent_payer: AccountInfo<'info>, owner: AccountInfo<'info>, ) -> Result<()> { match pool.config.pool_type { PoolType::Trade => (), // Cannot be auto-closed PoolType::Token => { // Not enough SOL to purchase another NFT, so we can close the pool. if pool.currency == Pubkey::default() && pool.amount < pool.current_price(TakerSide::Sell)? && pool.shared_escrow == Pubkey::default() // <= check this line { close_pool(pool, rent_payer, owner)?; } } PoolType::NFT => { // No more NFTs to sell, so we can close the pool. if pool.nfts_held == 0 && pool.shared_escrow == Pubkey::default() { // <= check this line close_pool(pool, rent_payer, owner)?; } } } Ok(()) } ``` After adding these lines, it will also be necessary to implement functionality to detach the shared escrow account so that the pool can be closed in the future. Currently, `edit_pool.rs` does not offer the option to update the `shared_escrow` account.

avoloder

Public reviews