Turbos audit report (version 2.0)

# Turbos audit report (version 2.0) ## [H-01] pool.move: Incorrect `fee_growth_above` condition ### Code Snippet ```rust fun next_fee_growth_inside<CoinTypeA, CoinTypeB, FeeType>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, tick_lower_index: I32, tick_upper_index: I32, tick_current_index: I32, _ctx: &mut TxContext, ): (u128, u128) { let tick_lower = get_tick(pool, tick_lower_index); let tick_upper = get_tick(pool, tick_upper_index); // calculate fee growth below let fee_growth_below_a; let fee_growth_below_b; if (!tick_lower.initialized) { fee_growth_below_a = pool.fee_growth_global_a; fee_growth_below_b = pool.fee_growth_global_b; } else if (i32::gte(tick_current_index, tick_lower_index)) { fee_growth_below_a = tick_lower.fee_growth_outside_a; fee_growth_below_b = tick_lower.fee_growth_outside_b; } else { fee_growth_below_a = math_u128::wrapping_sub(pool.fee_growth_global_a, tick_lower.fee_growth_outside_a); fee_growth_below_b = math_u128::wrapping_sub(pool.fee_growth_global_b, tick_lower.fee_growth_outside_b); }; // calculate fee growth above let fee_growth_above_a; let fee_growth_above_b; // @audit-issue Should check tick_upper.initialized instead if (!tick_lower.initialized) { fee_growth_above_a = 0; fee_growth_above_b = 0; } else if (i32::lt(tick_current_index, tick_upper_index)) { fee_growth_above_a = tick_upper.fee_growth_outside_a; fee_growth_above_b = tick_upper.fee_growth_outside_b; } else { fee_growth_above_a = math_u128::wrapping_sub(pool.fee_growth_global_a, tick_upper.fee_growth_outside_a); fee_growth_above_b = math_u128::wrapping_sub(pool.fee_growth_global_b, tick_upper.fee_growth_outside_b); }; let fee_growth_inside_a = math_u128::wrapping_sub( math_u128::wrapping_sub(pool.fee_growth_global_a, fee_growth_below_a), fee_growth_above_a ); let fee_growth_inside_b = math_u128::wrapping_sub( math_u128::wrapping_sub(pool.fee_growth_global_b, fee_growth_below_b), fee_growth_above_b ); (fee_growth_inside_a, fee_growth_inside_b) } ``` ### Bug Description The condition if (`!tick_lower.initialized`) is used to determine if the fee growth above should be set to zero. However, this condition should be checking `tick_upper.initialized` instead. The current logic incorrectly assumes that the lower tick's initialization status is relevant for calculating the fee growth above the current tick range. ### Recommendation Fix the condition to check the upper tick's initialization status: ```rust // calculate fee growth above let fee_growth_above_a; let fee_growth_above_b; if (!tick_upper.initialized) { // <-- FIXED fee_growth_above_a = 0; fee_growth_above_b = 0; } else if (i32::lt(tick_current_index, tick_upper_index)) { fee_growth_above_a = tick_upper.fee_growth_outside_a; fee_growth_above_b = tick_upper.fee_growth_outside_b; } else { fee_growth_above_a = math_u128::wrapping_sub(pool.fee_growth_global_a, tick_upper.fee_growth_outside_a); fee_growth_above_b = math_u128::wrapping_sub(pool.fee_growth_global_b, tick_upper.fee_growth_outside_b); }; ``` ### [H-02] pool.move: `check_ticks()` function lacks tick spacing validation ### Code snippet ```rust fun check_ticks( tick_lower_index: I32, tick_upper_index: I32 ) { assert!(i32::lt(tick_lower_index, tick_upper_index), EInvildTick); assert!(i32::gte(tick_lower_index, i32::neg_from(MAX_TICK_INDEX)), EInvildTick); assert!(i32::lte(tick_upper_index, i32::from(MAX_TICK_INDEX)), EInvildTick); } ``` ### Bug description The `check_ticks()` function only validates that: - Lower tick is less than upper tick - Ticks are within `MAX_TICK_INDEX` bounds However, it does not validate that the ticks are properly spaced according to the pool's tick_spacing parameter. While `flip_tick()` does this validation, the `mint()` function can create positions with ticks that don't align with the pool's tick spacing. ### PoC ```rust // Assume pool.tick_spacing = 60 // Attacker can create position with invalid ticks: let tick_lower = 10; // Should be multiple of 60 let tick_upper = 100; // Should be multiple of 60 pool::mint( pool, owner, i32::from(tick_lower), // Will pass check_ticks() but is invalid spacing i32::from(tick_upper), // Will pass check_ticks() but is invalid spacing liquidity_delta, clock, ctx ); ``` ### Recommendation Add tick spacing validation in `check_ticks()`: ```rust fun check_ticks( tick_lower_index: I32, tick_upper_index: I32, tick_spacing: u32 ) { assert!(i32::lt(tick_lower_index, tick_upper_index), EInvildTick); assert!(i32::gte(tick_lower_index, i32::neg_from(MAX_TICK_INDEX)), EInvildTick); assert!(i32::lte(tick_upper_index, i32::from(MAX_TICK_INDEX)), EInvildTick); // Add tick spacing validation assert!(i32::eq(i32::mod_euclidean(tick_lower_index, tick_spacing), i32::zero()), EInvildTickIndex); assert!(i32::eq(i32::mod_euclidean(tick_upper_index, tick_spacing), i32::zero()), EInvildTickIndex); } ``` And update the call in `modify_position()`: ```rust fun modify_position<CoinTypeA, CoinTypeB, FeeType>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, owner: address, tick_lower_index: I32, tick_upper_index: I32, liquidity_delta: I128, clock: &Clock, ctx: &mut TxContext, ) { check_ticks(tick_lower_index, tick_upper_index, pool.tick_spacing); // ... rest of function } ``` ## [H-03] position_manager.move: Incorrect slippage check condition in `decrease_liquidity_with_return_()` ### Code snippet ```rust public fun decrease_liquidity_with_return_<CoinTypeA, CoinTypeB, FeeType>( ... ): (Coin<CoinTypeA>, Coin<CoinTypeB>) { ... // @audit-issue amount_b_min >= amount_b_min is always true assert!(amount_a >= amount_a_min && amount_b_min >= amount_b_min, EPriceSlippageCheck); ... } ``` ### Bug description A critical vulnerability exists in the decrease_liquidity_with_return_ function where the price slippage check is incorrectly implemented, potentially allowing transactions to proceed even when the output amount is below the minimum specified threshold. The bug is in the following line: ```rust assert!(amount_a >= amount_a_min && amount_b_min >= amount_b_min, EPriceSlippageCheck); ``` The issue is that instead of comparing `amount_b` with `amount_b_min`, the code compares `amount_b_min` with itself (`amount_b_min >= amount_b_min`). This comparison will always evaluate to true, effectively nullifying the slippage protection for token B. ### PoC Consider this scenario: 1. User calls `decrease_liquidity_with_return_()` with: - `amount_b_min` = 100 - Actual `amount_b` received = 50 2. The check `amount_b_min >= amount_b_min` will always return true (100 >= 100) 3. Transaction proceeds despite user receiving 50% less of token B than their minimum specified amount ### Recommendation Fix the assertion to properly compare the actual amount received with the minimum amount: ```rust assert!(amount_a >= amount_a_min && amount_b >= amount_b_min, EPriceSlippageCheck); ``` ## [H-04] Missing deadline check in flash swap ### Code snippet ```rust public fun flash_swap<CoinTypeA, CoinTypeB, FeeType>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, recipient: address, a_to_b: bool, amount_specified: u128, amount_specified_is_input: bool, sqrt_price_limit: u128, clock: &Clock, versioned: &Versioned, ctx: &mut TxContext, ): (Coin<CoinTypeA>, Coin<CoinTypeB>, FlashSwapReceipt<CoinTypeA, CoinTypeB>) { check_version(versioned); assert!(pool.unlocked, EPoolLocked); // No deadline check here ... } public fun repay_flash_swap<CoinTypeA, CoinTypeB, FeeType>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, coin_a: Coin<CoinTypeA>, coin_b: Coin<CoinTypeB>, receipt: FlashSwapReceipt<CoinTypeA, CoinTypeB>, versioned: &Versioned ) { // No deadline check here either ... } ``` ### Bug description While regular swaps through the router have deadline checks, flash swaps can be executed at any time with no deadline protection. This means: 1. Flash swap transactions can be held by validators indefinitely 2. The repayment can also be delayed indefinitely 3. No protection against sandwich attacks or unfavorable price movements ### Recommendation Add deadline parameter to flash swap operations: ```rust public fun flash_swap<CoinTypeA, CoinTypeB, FeeType>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, recipient: address, a_to_b: bool, amount_specified: u128, amount_specified_is_input: bool, sqrt_price_limit: u128, deadline: u64, // Add deadline parameter clock: &Clock, versioned: &Versioned, ctx: &mut TxContext, ): (Coin<CoinTypeA>, Coin<CoinTypeB>, FlashSwapReceipt<CoinTypeA, CoinTypeB>) { check_version(versioned); assert!(pool.unlocked, EPoolLocked); assert!(clock::timestamp_ms(clock) <= deadline, ETransactionToOld); ... } // Also modify FlashSwapReceipt to include deadline struct FlashSwapReceipt<phantom CoinTypeA, phantom CoinTypeB> { pool_id: ID, a_to_b: bool, pay_amount: u64, deadline: u64, // Add deadline } public fun repay_flash_swap<CoinTypeA, CoinTypeB, FeeType>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, coin_a: Coin<CoinTypeA>, coin_b: Coin<CoinTypeB>, receipt: FlashSwapReceipt<CoinTypeA, CoinTypeB>, clock: &Clock, // Add clock parameter versioned: &Versioned ) { assert!(clock::timestamp_ms(clock) <= receipt.deadline, ETransactionToOld); ... } ``` ## [H-05] position_manager.move: `burn_position_nft_with_return_()` lacks a few important checks before buring ### Code snippet ```rust public fun burn_position_nft_with_return_<CoinTypeA, CoinTypeB, FeeType>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, positions: &mut Positions, nft: TurbosPositionNFT, versioned: &Versioned, ctx: &mut TxContext ): TurbosPositionBurnNFT { pool::check_version(versioned); let pool_id = position_nft::pool_id(&nft); assert!(object::id(pool) == position_nft::pool_id(&nft), EInvalidPool); let nft_address = object::id_address(&nft); let position_inner = dof::borrow_mut<address, Position>(&mut positions.id, nft_address); let tick_spacing = pool::get_pool_tick_spacing(pool); let mod = i32::from(TICK_SIZE % tick_spacing); //check full range assert!(i32::eq(i32::sub(position_inner.tick_lower_index, mod), i32::neg_from(TICK_SIZE)), EInvalidBurnTickRange); assert!(i32::eq(i32::add(position_inner.tick_upper_index, mod), i32::from(TICK_SIZE)), EInvalidBurnTickRange); let position_id = position_nft::position_id(&nft); let burn_nft = TurbosPositionBurnNFT{ id: object::new(ctx), name: string::utf8(b"Proof of Turbos Position Burn"), description: string::utf8(b"Proof of Turbos Position Burn"), img_url: url::new_unsafe(string::to_ascii(string::utf8(b"https://app.turbos.finance/icon/turbos-position-burn-nft.png"))), position_nft: nft, position_id: position_id, pool_id: pool_id, coin_type_a: type_name::get<CoinTypeA>(), coin_type_b: type_name::get<CoinTypeB>(), fee_type: type_name::get<FeeType>(), }; event::emit(BurnPositionEvent { nft_address: nft_address, position_id: position_id, pool_id: pool_id, burn_nft_address: object::id_address(&burn_nft), }); burn_nft } ``` ### Bug description The `burn_position_nft_with_return_()` function in position_manager.move converts a position NFT to a burn NFT without verifying that all assets have been withdrawn from the position. This could result in permanent loss of: - Active liquidity in the pool - Uncollected trading fees - Uncollected rewards ### PoC ```rust public fun burn_position_nft_with_return_<CoinTypeA, CoinTypeB, FeeType>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, positions: &mut Positions, nft: TurbosPositionNFT, versioned: &Versioned, ctx: &mut TxContext ) -> TurbosPositionBurnNFT { // Only checks pool version and tick range // Does NOT check for remaining assets pool::check_version(versioned); let nft_address = object::id_address(&nft); let position_inner = dof::borrow_mut<address, Position>(&mut positions.id, nft_address); // Position could still have: // - position_inner.liquidity > 0 // - position_inner.tokens_owed_a > 0 // - position_inner.tokens_owed_b > 0 // - reward_info.amount_owed > 0 // Creates burn NFT without ensuring position is cleared let burn_nft = TurbosPositionBurnNFT{ ... }; burn_nft } ``` ### Recommendation Add checks to ensure the position is fully cleared before allowing the burn: ```rust public fun burn_position_nft_with_return_<CoinTypeA, CoinTypeB, FeeType>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, positions: &mut Positions, nft: TurbosPositionNFT, versioned: &Versioned, ctx: &mut TxContext ) -> TurbosPositionBurnNFT { pool::check_version(versioned); let nft_address = object::id_address(&nft); let position = dof::borrow_mut<address, Position>(&mut positions.id, nft_address); // Check no liquidity assert!(position.liquidity == 0, EPositionNotCleared); // Check no uncollected fees assert!(position.tokens_owed_a == 0 && position.tokens_owed_b == 0, EPositionNotCleared); // Check no uncollected rewards let i = 0; let len = vector::length(&position.reward_infos); while (i < len) { let reward_info = vector::borrow(&position.reward_infos, i); assert!(reward_info.amount_owed == 0, EPositionNotCleared); i = i + 1; }; // Rest of the function... } ``` ## [H-06] pool.move: Hardcoded reward index in modify_tick_reward() and modify_tick() ### Code snippet ```rust public(friend) fun modify_tick_reward<CoinTypeA, CoinTypeB, FeeType>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, tick_lower_index: I32, tick_upper_index: I32, ) { // Always uses index 0 let reawrd_info = vector::borrow(reward_infos, 0); modify_tick_reward_outside(pool, tick_lower_index, 0, reawrd_info.growth_global); // ... } ``` ```rust public(friend) fun modify_tick<CoinTypeA, CoinTypeB, FeeType>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, tick_index: I32, ) { if (i32::lte(tick_index, pool.tick_current_index)) { let reward_infos = &pool.reward_infos; let reawrd_info = vector::borrow(reward_infos, 0); // Always uses index 0 modify_tick_reward_outside(pool, tick_index, 0, reawrd_info.growth_global); } else { modify_tick_reward_outside(pool, tick_index, 0, 0); // Always uses index 0 }; } ``` ### Bug description `modify_tick_reward()` and `modify_tick()` in the pool contract incorrectly handle reward indices by hardcoding them to 0, leading to broken reward accounting for all reward tokens beyond the first one. ### PoC - Set up a pool with 2 reward tokens - Add liquidity and generate rewards - Call `modify_tick_reward` or `modify_tick` - Observe that only the first reward token's growth values are updated - Second reward token's accounting remains unchanged ### Recommendation Add another input parameter `reward_index` to specify which reward token to modify. ## [H-07] position_manager.move: Missing tick range validation in multiple functions ### Code snippet 1. `mint_with_return_()` 2. `increase_liquidity_with_return_()` 3. `decrease_liquidity_with_return_()` ### Bug description Several functions in the position_manager.move contract lack proper validation for `tick_lower_index` and `tick_upper_index`. Specifically, they don't verify that: 1. `tick_lower_index` is less than `tick_upper_index` 2. The ticks are within the valid range (-443636 to 443636) 3. The ticks are properly spaced according to the pool's tick spacing ### Recommendation Add proper tick range validations in all position management functions. Example fix for `mint_with_return_()`: ```rust public fun mint_with_return_<CoinTypeA, CoinTypeB, FeeType>( // ... parameters ... ) { let tick_lower_index_i32 = i32::from_u32_neg(tick_lower_index, tick_lower_index_is_neg); let tick_upper_index_i32 = i32::from_u32_neg(tick_upper_index, tick_upper_index_is_neg); // Add validations assert!(i32::lt(tick_lower_index_i32, tick_upper_index_i32), EInvalidTickRange); assert!(i32::ge(tick_lower_index_i32, i32::neg_from(TICK_SIZE)), EInvalidTickRange); assert!(i32::le(tick_upper_index_i32, i32::from(TICK_SIZE)), EInvalidTickRange); let tick_spacing = pool::get_pool_tick_spacing(pool); assert!(i32::to_u32(tick_lower_index_i32) % tick_spacing == 0, EInvalidTickSpacing); assert!(i32::to_u32(tick_upper_index_i32) % tick_spacing == 0, EInvalidTickSpacing); // ... rest of the code ... } ``` Similar validations should be added to `increase_liquidity_with_return_()` and `decrease_liquidity_with_return_()` functions. ## [L-01] Deprecated functions should be removed There are a few deprecated in the codebase, for example: ```rust public(friend) fun collect_reward<CoinTypeA, CoinTypeB, FeeType, RewardCoin>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, vault: &mut PoolRewardVault<RewardCoin>, recipient: address, tick_lower_index: I32, tick_upper_index: I32, amount: u64, reward_index: u64, ctx: &mut TxContext ): u64 { abort(0) } ``` If they are not intended to be used any more in future it is better to remove them. ## [L-02] All users share the `same nft_img_url` The position nft url is set in `position_manager::init_()`: ```rust fun init_(ctx: &mut TxContext) { transfer::share_object(Positions { id: object::new(ctx), nft_minted: 0, user_position: table::new(ctx), nft_name: string::utf8(b"Turbos Position's NFT"), nft_description: string::utf8(b"An NFT created by Turbos CLMM"), nft_img_url: string::utf8(b"https://ipfs.io/ipfs/QmTxRsWbrLG6mkjg375wW77Lfzm38qsUQjRBj3b2K3t8q1?filename=Turbos_nft.png"), }); } ``` All users who mint position nft will share this one single `nft_img_url`. There are a few potential issues: 1. Single Point of Failure: - If the IPFS link becomes unavailable, all NFTs would lose their images simultaneously - If the image is hosted on a centralized server, server downtime would affect all NFTs 2. Centralization Risk: - The protocol owner (pool factory) could change all NFT images at once through `update_nft_img_url()` - This gives too much control to the protocol owner and goes against decentralization principles 3. Lack of Uniqueness: - All NFTs having the same image reduces their uniqueness and value proposition Makes it harder to visually distinguish between different positions 4. Resource Efficiency: - Multiple requests to the same URL could lead to rate limiting or excessive load on the hosting service ## [L-03] pool.move: Zero Liquidity Swaps Allowed ```rust public(friend) fun compute_swap_result<CoinTypeA, CoinTypeB, FeeType>( // ... ) { //assert!(state.liquidity > 0, EInsufficientLiquidity); // ... rest of the function } ``` The code has commented out a critical check for zero liquidity. This means swaps can occur even when there's no liquidity in the pool, which could lead to: - Division by zero errors - Incorrect price calculations - Potential manipulation of pool state ## [L-04] pool.move: Uncontrolled Fee Protocol Initialization The `fee_protocol` parameter can be set to any arbitrary value during pool deployment without proper validation. This allows potential manipulation of protocol fee percentages, which could lead to unexpected economic impacts on the pool's fee collection mechanism. Exploit Method: 1. When calling `deploy_pool`, a malicious deployer can set an extremely high or manipulated `fee_protocol` value. 2. Since there's no upper bound or validation check on the `fee_protocol` parameter, this could lead to: - Disproportionate fee extraction - Potential economic disruption of the pool's fee distribution model Practical Exploit Scenario: ```rust // A malicious deployer could deploy a pool with an extreme fee protocol value let malicious_pool = deploy_pool<CoinA, CoinB, FeeType>( 500, // standard fee 1, // tick spacing sqrt_price, // sqrt price 99999, // extremely high fee protocol percentage &clock, ctx ); ``` The key issue is the lack of a constraint on `fee_protocol`, which should typically have a predefined, limited range (e.g., 0-100%) to prevent economic manipulation. **Recommendation:** ```rust // Add validation during pool deployment if fee_protocol > 100 { abort!(INVALID_FEE_PROTOCOL); } ``` ### [L-05] pool.move: Missing fee tier and tick spacing validation during deployment ```rust public(friend) fun deploy_pool<CoinTypeA, CoinTypeB, FeeType>( fee: u32, tick_spacing: u32, sqrt_price: u128, fee_protocol: u32, clock: &Clock, ctx: &mut TxContext ) : Pool<CoinTypeA, CoinTypeB, FeeType> { // No validation for fee tier and tick spacing ``` The function allows completely unrestricted setting of fee and tick_spacing without any validation against the standard Uniswap V3 protocol specifications. This can lead to creating pools with invalid or extreme configurations that deviate from the expected protocol design: - No check for tick_spacing being non-zero - No validation that tick_spacing is a power of 2 - Could lead to division by zero errors or incorrect tick calculations A malicious deployer can manipulate the pool creation by passing arbitrary values for: - `fee`: Uniswap V3 typically uses predefined fee tiers (0.05%, 0.3%, 1%) - `tick_spacing`: Should align with specific fee tiers For example: ```rust // Create a pool with an extreme, non-standard configuration let malicious_pool = deploy_pool<SUI, USDC, FeeType>( 10000, // Extremely high fee (100%) 1, // Minimal tick spacing sqrt_price, fee_protocol, clock, ctx ) ``` **Recommendation:** Implement strict validation for `fee` and `tick_spacing` to match Uniswap V3 standard configurations: - Restrict `fee` to predefined tiers (0.05%, 0.3%, 1%) - Enforce tick spacing based on fee tier - Add explicit validation checks before pool creation ## [Informational-01] Fix typos in the codebase There are some cases where variable names are misspelled, for example `reawrd_info` should be `reward_info`: ```rust public(friend) fun modify_tick<CoinTypeA, CoinTypeB, FeeType>( pool: &mut Pool<CoinTypeA, CoinTypeB, FeeType>, tick_index: I32, ) { if (i32::lte(tick_index, pool.tick_current_index)) { let reward_infos = &pool.reward_infos; let reawrd_info = vector::borrow(reward_infos, 0); modify_tick_reward_outside(pool, tick_index, 0, reawrd_info.growth_global); } else { modify_tick_reward_outside(pool, tick_index, 0, 0); }; } ```

Syntax	Example	Reference
# Header	Header	基本排版
- Unordered List	Unordered List
1. Ordered List	Ordered List
- [ ] Todo List	Todo List
> Blockquote	Blockquote
Bold font	Bold font
Italics font	Italics font
~~Strikethrough~~	~~Strikethrough~~
19^th^	19^th
H~2~O	H₂O
++Inserted text++	Inserted text
==Marked text==	Marked text
[link text](https:// "title")	Link
![image alt](https:// "title")	Image
`Code`	`Code`	在筆記中貼入程式碼
```javascript var i = 0; ```	`var i = 0;`
:smile:		Emoji list
{%youtube youtube_id %}	Externals
$L^aT_eX$	L^aT_eX
:::info This is a alert area. :::	This is a alert area.