fix: addressing sandwich attack fix in the deposit function #14
Conversation
| @@ -266,7 +240,7 @@ pub mod liquidity_lockbox { | |||
| cpi_accounts_modify_liquidity, | |||
| signer_seeds | |||
| ); | |||
| whirlpool::cpi::increase_liquidity(cpi_ctx_modify_liquidity, liquidity_amount, delta_a, delta_b)?; | |||
| whirlpool::cpi::increase_liquidity(cpi_ctx_modify_liquidity, liquidity_amount as u128, token_max_a, token_max_b)?; | |||
There was a problem hiding this comment.
what about the approval?
There was a problem hiding this comment.
It's in the code below :)
There was a problem hiding this comment.
We approve the max provided amounts, then approve back to zero
There was a problem hiding this comment.
Oh sorry! the approval is above for token_max_a token_max_b !!
There was a problem hiding this comment.
Fix for .unwrap_or_else(|| panic!("Liquidity overflow")) in another PR?
There was a problem hiding this comment.
This will be done in a next PR
| @@ -266,7 +240,7 @@ pub mod liquidity_lockbox { | |||
| cpi_accounts_modify_liquidity, | |||
| signer_seeds | |||
| ); | |||
| whirlpool::cpi::increase_liquidity(cpi_ctx_modify_liquidity, liquidity_amount, delta_a, delta_b)?; | |||
| whirlpool::cpi::increase_liquidity(cpi_ctx_modify_liquidity, liquidity_amount as u128, token_max_a, token_max_b)?; | |||
There was a problem hiding this comment.
Oh sorry! the approval is above for token_max_a token_max_b !!
| /// - `token_max_a` - Max amount of SOL token to be added for liquidity. | ||
| /// - `token_max_b` - Max amount of OLAS token to be added for liquidity. | ||
| pub fn deposit(ctx: Context<DepositPositionForLiquidity>, | ||
| liquidity_amount: u64, |
There was a problem hiding this comment.
Adding a liquidity_amount parameter.
| let sqrt_price_current_x64 = ctx.accounts.whirlpool.sqrt_price; | ||
| let sqrt_price_upper_x64 = sqrt_price_from_tick_index(ctx.accounts.position.tick_upper_index); | ||
|
|
||
| // get_liquidity_from_token_a is imported from whirlpools-sdk (getLiquidityFromTokenA) | ||
| let liquidity_amount = get_liquidity_from_token_a(token_max_a as u128, sqrt_price_current_x64, sqrt_price_upper_x64)?; | ||
|
|
||
| let (delta_a, delta_b) = calculate_liquidity_token_deltas( | ||
| tick_index_current, | ||
| sqrt_price_current_x64, | ||
| &ctx.accounts.position, | ||
| liquidity_amount as i128 | ||
| )?; | ||
|
|
||
| // block too much deposit | ||
| if delta_a > token_max_a || delta_b > token_max_b { | ||
| return Err(ErrorCode::DeltaAmountOverflow.into()); | ||
| } | ||
|
|
||
| // Check that the liquidity is within uint64 bounds | ||
| if liquidity_amount > std::u64::MAX as u128 { | ||
| return Err(ErrorCode::LiquidityOverflow.into()); | ||
| } | ||
|
|
||
| let position_liquidity = liquidity_amount as u64; | ||
|
|
There was a problem hiding this comment.
This is obsolete as the liquidity amount is already provided.
| delta_a, | ||
| token_max_a, |
There was a problem hiding this comment.
We now approve for max amount of tokens.
| // Revoke approval for unused SOL tokens | ||
| token::approve( | ||
| CpiContext::new( | ||
| ctx.accounts.token_program.to_account_info(), | ||
| Approve { | ||
| to: ctx.accounts.token_owner_account_a.to_account_info(), | ||
| delegate: ctx.accounts.lockbox.to_account_info(), | ||
| authority: ctx.accounts.signer.to_account_info(), | ||
| }, | ||
| ), | ||
| 0, | ||
| )?; | ||
|
|
||
| // Revoke approval for OLAS tokens | ||
| token::approve( | ||
| CpiContext::new( | ||
| ctx.accounts.token_program.to_account_info(), | ||
| Approve { | ||
| to: ctx.accounts.token_owner_account_b.to_account_info(), | ||
| delegate: ctx.accounts.lockbox.to_account_info(), | ||
| authority: ctx.accounts.signer.to_account_info(), | ||
| }, | ||
| ), | ||
| 0, | ||
| )?; |
There was a problem hiding this comment.
Revoke the approval of unused tokens.
| /// - `amount` - Token A amount. | ||
| /// - `sqrt_price_lower_x64` - Lower sqrt price. | ||
| /// - `sqrt_price_upper_x64` - Upper sqrt price. | ||
| fn get_liquidity_from_token_a(amount: u128, sqrt_price_lower_x64: u128, sqrt_price_upper_x64: u128 ) -> Result<u128> { |
There was a problem hiding this comment.
This becomes obsolete as there's no need to calculate a liquidity here (already provided).
deposit()function as specified in doc: adding vulnerabilities doc for lockbox2 #13.