Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: addressing sandwich attack fix in the deposit function #14

Merged
merged 2 commits into from
Feb 9, 2024
Merged

fix: addressing sandwich attack fix in the deposit function #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
844e8ae
fix: addressing sandwich attack fix in the deposit function
kupermind Feb 9, 2024
a5fdd1d
doc: removing vulnerabilities from lockbox2 as they are resolved
kupermind Feb 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Binary file modified lockbox/doc/Bonding_mechanism_with_liquidity_on_Solana_v1_v2.pdf
View file
Open in desktop
Binary file not shown.
1 change: 0 additions & 1 deletion lockbox2/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ Integration test is located here: [liquidity_lockbox.ts](https://github.com/valo

## Audits
- The audit is provided as development matures. The latest audit report can be found here: [audits](https://github.com/valory-xyz/lockbox-solana/tree/main/lockbox2/audits).
- The list of known vulnerabilities can be found here: [Vulnerabilities list](https://github.com/valory-xyz/lockbox-solana/blob/main/lockbox2/doc/Vulnerabilities_list_solana_lockbox_v2.pdf).



Expand Down
Binary file modified lockbox2/doc/Bonding_mechanism_with_liquidity_on_Solana_v1_v2.pdf
View file
Open in desktop
Binary file not shown.
Binary file removed lockbox2/doc/Vulnerabilities_list_solana_lockbox_v2.pdf
View file
Open in desktop
Binary file not shown.
102 changes: 34 additions & 68 deletions lockbox2/programs/liquidity_lockbox/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@ use anchor_spl::token::{self, Mint, Token, TokenAccount, Approve};
use whirlpool::{
self,
state::{Whirlpool, TickArray, Position},
math::sqrt_price_from_tick_index,
math::{mul_u256, U256Muldiv},
manager::liquidity_manager::calculate_liquidity_token_deltas,
cpi::accounts::ModifyLiquidity,
cpi::accounts::UpdateFeesAndRewards,
cpi::accounts::CollectFees
Expand Down Expand Up @@ -121,9 +118,11 @@ pub mod liquidity_lockbox {
/// Deposits an NFT position under the Lockbox management and gets bridged tokens minted in return.
///
/// ### Parameters
/// - `liquidity_amount` - Requested liquidity amount.
/// - `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,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a liquidity_amount parameter.

token_max_a: u64,
token_max_b: u64,
) -> Result<()> {
Expand Down Expand Up @@ -185,35 +184,10 @@ pub mod liquidity_lockbox {
return Err(ErrorCode::OutOfRange.into());
}

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;

Comment on lines -188 to -212
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is obsolete as the liquidity amount is already provided.

// Total liquidity update with the check
ctx.accounts.lockbox.total_liquidity = ctx.accounts.lockbox
.total_liquidity
.checked_add(position_liquidity)
.checked_add(liquidity_amount)
.unwrap_or_else(|| panic!("Liquidity overflow"));

// Approve SOL tokens for the lockbox
Expand All @@ -226,7 +200,7 @@ pub mod liquidity_lockbox {
authority: ctx.accounts.signer.to_account_info(),
},
),
delta_a,
token_max_a,
Comment on lines -229 to +203
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now approve for max amount of tokens.

)?;

// Approve OLAS tokens for the lockbox
Expand All @@ -239,7 +213,7 @@ pub mod liquidity_lockbox {
authority: ctx.accounts.signer.to_account_info(),
},
),
delta_b,
token_max_b,
)?;

// Get lockbox signer seeds
Expand All @@ -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)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the approval?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in the code below :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We approve the max provided amounts, then approve back to zero

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry! the approval is above for token_max_a token_max_b !!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix for .unwrap_or_else(|| panic!("Liquidity overflow")) in another PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be done in a next PR

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


// Mint bridged tokens in the amount of position liquidity
invoke_signed(
Expand All @@ -276,7 +250,7 @@ pub mod liquidity_lockbox {
ctx.accounts.bridged_token_account.to_account_info().key,
ctx.accounts.lockbox.to_account_info().key,
&[ctx.accounts.lockbox.to_account_info().key],
position_liquidity,
liquidity_amount,
)?,
&[
ctx.accounts.bridged_token_mint.to_account_info(),
Expand All @@ -287,12 +261,36 @@ pub mod liquidity_lockbox {
&[&ctx.accounts.lockbox.seeds()],
)?;

// 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,
)?;
Comment on lines +264 to +288
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revoke the approval of unused tokens.


emit!(DepositEvent {
signer: ctx.accounts.signer.key(),
position: ctx.accounts.position.key(),
amount_a: delta_a,
amount_b: delta_b,
deposit_liquidity: position_liquidity,
deposit_liquidity: liquidity_amount,
total_liquidity: ctx.accounts.lockbox.total_liquidity
});

Expand Down Expand Up @@ -455,34 +453,6 @@ pub mod liquidity_lockbox {
}
}

// https://github.com/orca-so/whirlpools/blob/main/sdk/src/quotes/public/increase-liquidity-quote.ts#L147
// https://github.com/orca-so/whirlpools/blob/537306c096bcbbf9cb8d5cff337c989dcdd999b4/sdk/src/utils/position-util.ts#L69
/// Gets liquidity from token A parameters.
///
/// ### Parameters
/// - `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> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This becomes obsolete as there's no need to calculate a liquidity here (already provided).

// Δa = liquidity/sqrt_price_lower - liquidity/sqrt_price_upper
// liquidity = Δa * ((sqrt_price_lower * sqrt_price_upper) / (sqrt_price_upper - sqrt_price_lower))
assert!(sqrt_price_lower_x64 < sqrt_price_upper_x64);
let sqrt_price_diff = sqrt_price_upper_x64 - sqrt_price_lower_x64;

let numerator = mul_u256(sqrt_price_lower_x64, sqrt_price_upper_x64); // x64 * x64
let denominator = U256Muldiv::new(0, sqrt_price_diff); // x64

let (quotient, _remainder) = numerator.div(denominator, false);

let liquidity = quotient
.mul(U256Muldiv::new(0, amount))
.shift_word_right()
.try_into_u128()
.or(Err(ErrorCode::WhirlpoolNumberDownCastError.into()));

return liquidity;
}

#[derive(Accounts)]
pub struct InitializeLiquidityLockbox<'info> {
#[account(mut)]
Expand Down Expand Up @@ -708,10 +678,6 @@ pub struct DepositEvent {
// Liquidity position
#[index]
pub position: Pubkey,
// Amount of SOL token for the liquidity
pub amount_a: u64,
// Amount of OLAS token for the liquidity
pub amount_b: u64,
// Deposit liquidity amount
pub deposit_liquidity: u64,
// Total position liquidity
Expand Down
9 changes: 5 additions & 4 deletions lockbox2/tests/liquidity_lockbox.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ async function main() {
// Find a PDA account for the program
const [pdaProgram, bump] = await anchor.web3.PublicKey.findProgramAddress([Buffer.from("liquidity_lockbox", "utf-8")], program.programId);
const bumpBytes = Buffer.from(new Uint8Array([bump]));
console.log("Lockbox PDA:", pdaProgram.toBase58());
console.log("Lockbox PDA address:", pdaProgram.toBase58());
console.log("Lockbox PDA bump:", bump);

// Create new bridged token mint with the pda mint authority
const bridgedTokenMint = await createMint(provider.connection, userWallet, pdaProgram, null, 8);
Expand Down Expand Up @@ -215,7 +216,7 @@ async function main() {
// Output the estimation
console.log("SOL max input:", DecimalUtil.fromBN(quote.tokenMaxA, token_a.decimals).toFixed(token_a.decimals));
console.log("OLAS max input:", DecimalUtil.fromBN(quote.tokenMaxB, token_b.decimals).toFixed(token_b.decimals));
console.log("Estimated liquidity:", quote.liquidityAmount.toString());
console.log("Requested liquidity:", quote.liquidityAmount.toString());

//console.log(quote);

Expand Down Expand Up @@ -259,7 +260,7 @@ async function main() {

// Execute the correct deposit tx
try {
signature = await program.methods.deposit(quote.tokenMaxA, quote.tokenMaxB)
signature = await program.methods.deposit(quote.liquidityAmount, quote.tokenMaxA, quote.tokenMaxB)
.accounts(
{
position: position,
Expand Down Expand Up @@ -309,7 +310,7 @@ async function main() {

// Execute the second correct deposit tx
try {
signature = await program.methods.deposit(quote.tokenMaxA, quote.tokenMaxB)
signature = await program.methods.deposit(quote.liquidityAmount, quote.tokenMaxA, quote.tokenMaxB)
.accounts(
{
position: position,
Expand Down
3 changes: 2 additions & 1 deletion lockbox2/tests/lockbox_init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ async function main() {
// Find a PDA account for the program
const [pdaProgram, bump] = await anchor.web3.PublicKey.findProgramAddress([Buffer.from("liquidity_lockbox", "utf-8")], program.programId);
const bumpBytes = Buffer.from(new Uint8Array([bump]));
console.log("Lockbox PDA:", pdaProgram.toBase58());
console.log("Lockbox PDA address:", pdaProgram.toBase58());
console.log("Lockbox PDA bump:", bump);

// Create new bridged token mint with the pda mint authority
const bridgedTokenMint = await createMint(provider.connection, userWallet, pdaProgram, null, 8);
Expand Down