## Description In *lockbox 1*, the [liquidity_lockbox::withdraw(...)](https://github.com/valory-xyz/lockbox-solana/blob/1736f39671c263311b0cedee290aef7eb9a81ef8/lockbox/programs/liquidity_lockbox/src/lib.rs#L228-L441) function allows the `signer` to be read-only as it has no *mutable* constraint, see [WithdrawLiquidityForTokens](https://github.com/valory-xyz/lockbox-solana/blob/1736f39671c263311b0cedee290aef7eb9a81ef8/lockbox/programs/liquidity_lockbox/src/lib.rs#L530) context: ```rust #[derive(Accounts)] pub struct WithdrawLiquidityForTokens<'info> { #[account(mut)] pub whirlpool: Box>, // @audit missing '#[account(mut)]' here pub signer: Signer<'info>, ... } ``` However, the signer needs to be writable/mutable because its `lamports` balance is modified when a [position can is fully closed](https://github.com/valory-xyz/lockbox-solana/blob/1736f39671c263311b0cedee290aef7eb9a81ef8/lockbox/programs/liquidity_lockbox/src/lib.rs#L379-L423): ```rust pub fn withdraw( ctx: Context, id: u32, amount: u64, token_min_a: u64, token_min_b: u64 ) -> Result<()> { ... // Get the post-withdraw token remainder let remainder: u64 = position_liquidity - amount; // If requested amount can be fully covered by the current position liquidity, close the position if remainder == 0 { ... let dest_starting_lamports = ctx.accounts.signer.lamports(); let account = ctx.accounts.pda_lockbox_position.to_account_info(); **ctx.accounts.signer.lamports.borrow_mut() = dest_starting_lamports // @audit signer account is written here .checked_add(account.lamports()) .unwrap(); **account.lamports.borrow_mut() = 0; ... } else { // Update position liquidity ctx.accounts.pda_lockbox_position.position_liquidity = remainder; } ... } ``` The reason this works in the test is that the `signer` is also the `payer`, i.e. the one who invoked the transaction. In this special case, the signer is automatically writable. However, it's common on Solana that the `signer` and `payer` are different accounts/wallets. In those cases, the execution will fail due to the `signer` being read-only. This could also be the case if the withdraw method is invoked via another program (CPI), which is a common use-case. **Summary:** The withdraw function is subject to DoS on position closure, if the transaction is invoked with different `signer` and `payer`. This is a normal and intended use-case on the Solana network. **Root cause:** The `signer` is declared read-only although it can be written on withdrawal. ## Proof of Concept The following PoC invokes a withdrawal transaction via a second user account as `payer`, while the first user account is still the `signer`. As a consequence, the execution will fail due to the signer not being writable, see error message below. Please apply the *diff* below and run the test via the following command: `solana airdrop 10000 9fit3w7t6FHATDaZWotpWqN7NpqgL3Lm1hqUop4hAy8h --url localhost && solana airdrop 10000 24PNhTaNtomHhoy3fTRaMhAFCRj4uHqhZEEoWrKDbR5p --url localhost && npx ts-node tests/liquidity_lockbox.ts` *Note the second airdrop command for the second user wallet.* ```diff diff --git a/lockbox/tests/liquidity_lockbox.ts b/lockbox/tests/liquidity_lockbox.ts index 82ee866..d6ffacd 100644 --- a/lockbox/tests/liquidity_lockbox.ts +++ b/lockbox/tests/liquidity_lockbox.ts @@ -41,6 +41,19 @@ async function main() { const userWallet = provider.wallet["payer"]; console.log("User wallet:", userWallet.publicKey.toBase58()); + // PoC: Create second user wallet + const userWallet2 = anchor.web3.Keypair.fromSecretKey(new Uint8Array([ + 174, 47, 154, 16, 202, 193, 206, 113, 199, 190, 53, 133, 169, 175, 31, 56, + 222, 53, 138, 189, 224, 216, 117, 173, 10, 149, 53, 45, 73, 251, 237, 246, + 15, 185, 186, 82, 177, 240, 148, 69, 241, 227, 167, 80, 141, 89, 240, 121, + 121, 35, 172, 247, 68, 251, 226, 218, 48, 63, 176, 109, 168, 89, 238, 135, + ])); + console.log("User wallet 2:", userWallet2.publicKey.toBase58()); + + // PoC: Lockbox program to be invoked via second user + const provider2 = new anchor.AnchorProvider(provider.connection, new anchor.Wallet(userWallet2), {}); + const program2 = new Program(idl as anchor.Idl, PROGRAM_ID, provider2) as Program; + const ctx = WhirlpoolContext.withProvider(provider, orca); const client = buildWhirlpoolClient(ctx); const whirlpoolClient = await client.getPool(whirlpool); @@ -603,9 +616,11 @@ async function main() { console.log("Amount of bridged tokens to withdraw:", tBalalnce.toString()); try { - signature = await program.methods.withdraw(numPosition, tBalalnce, zeroAmount, zeroAmount) + // PoC: Invoke withdraw transaction from user 2 (payer) + signature = await program2.methods.withdraw(numPosition, tBalalnce, zeroAmount, zeroAmount) .accounts( { + signer: userWallet.publicKey, // PoC: User 1 is still the signer lockbox: pdaProgram, whirlpoolProgram: orca, whirlpool: whirlpool, ``` Error message: `Error processing Instruction 0: Cross-program invocation with unauthorized signer or writable account` ## Recommendation Declaring the `signer` account to be *mutable* via an Anchor constraint mitigates the present issue and enables the above PoC to proceed without errors. ```diff diff --git a/lockbox/programs/liquidity_lockbox/src/lib.rs b/lockbox/programs/liquidity_lockbox/src/lib.rs index 9d15c0e..8c8f345 100644 --- a/lockbox/programs/liquidity_lockbox/src/lib.rs +++ b/lockbox/programs/liquidity_lockbox/src/lib.rs @@ -527,6 +527,7 @@ pub struct WithdrawLiquidityForTokens<'info> { #[account(mut)] pub whirlpool: Box>, + #[account(mut)] pub signer: Signer<'info>, #[account(mut)] ```

Mario Poneder

Public reviews