Wrong invocation of Whirpools's updateFeesAndRewards will cause it to always revert

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/lockbox-solana/solidity/liquidity_lockbox.sol#L277-L293

Vulnerability details

Impact

Deposits will be unwithdrawable from the lockbox

Proof of Concept

If the entire liquidity of a position has been removed, the withdraw function calls the updateFeesAndRewards function on the Orca pool before attempting to close the position.

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/lockbox-solana/solidity/liquidity_lockbox.sol#L277-L293

    function withdraw(uint64 amount) external {
        address positionAddress = positionAccounts[firstAvailablePositionAccountIndex];

        ......

        uint64 positionLiquidity = mapPositionAccountLiquidity[positionAddress];

        ......

        uint64 remainder = positionLiquidity - amount;

        ......

        if (remainder == 0) {
            // Update fees for the position
            AccountMeta[4] metasUpdateFees = [
                AccountMeta({pubkey: pool, is_writable: true, is_signer: false}),
                AccountMeta({pubkey: positionAddress, is_writable: true, is_signer: false}),
                AccountMeta({pubkey: tx.accounts.tickArrayLower.key, is_writable: false, is_signer: false}),
                AccountMeta({pubkey: tx.accounts.tickArrayUpper.key, is_writable: false, is_signer: false})
            ];
            whirlpool.updateFeesAndRewards{accounts: metasUpdateFees, seeds: [[pdaProgramSeed, pdaBump]]}();

This is faulty as the updateFeesAndRewards function will always revert if the position's liquidity is 0.

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/lockbox-solana/solidity/interfaces/whirlpool.sol#L198

Whirlpool source code:

update_fees_and_rewards -> calculate_fee_and_reward_growths -> _calculate_modify_liquidity

https://github.com/orca-so/whirlpools/blob/3206c9cdfbf27c73c30cbcf5b6df2929cbf87618/programs/whirlpool/src/manager/liquidity_manager.rs#L97-L99

fn _calculate_modify_liquidity(
    whirlpool: &Whirlpool,
    position: &Position,
    tick_lower: &Tick,
    tick_upper: &Tick,
    tick_lower_index: i32,
    tick_upper_index: i32,
    liquidity_delta: i128,
    timestamp: u64,
) -> Result<ModifyLiquidityUpdate> {
    // Disallow only updating position fee and reward growth when position has zero liquidity
    if liquidity_delta == 0 && position.liquidity == 0 {
        return Err(ErrorCode::LiquidityZero.into());
    }

Since the withdrawal positions are chosen sequentially, only a maximum of (first position's liquidity - 1) amount of liquidity can be withdrawn.

POC Test

https://gist.github.com/10xhash/a687ef66de8210444a41360b86ed4bca

Tools Used

Manual review

Recommended Mitigation Steps

Avoid the update_fees_and_rewards call completely since fees and rewards would be updated in the decreaseLiquidity call.

Assessed type

call/delegatecall

[c4-pre-sort]

alex-ppg marked the issue as sufficient quality report

[c4-sponsor]

mariapiamo (sponsor) disputed

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid

[10xhash]

Why is this issue not valid?

The call to updateFeesAndRewards of whirlpool will revert when the liquidity of that position is 0. This implementation calls updateFeesAndRewards after withdrawing (ie. calling decreaseLiquidity) on whirlpool and hence will revert. In the newly released version of lockbox written in Rust, they have updated the call order to only call decreaseLiquidity after the call to updateFeesAndRewards. If it helps to view the issue's validity on the newer codebase, please apply the following diff and run the tests. In the test, the second liquidity withdrawal will fail.

diff --git a/lockbox2/programs/liquidity_lockbox/src/lib.rs b/lockbox2/programs/liquidity_lockbox/src/lib.rs
index c94a678..13c86a4 100644
--- a/lockbox2/programs/liquidity_lockbox/src/lib.rs
+++ b/lockbox2/programs/liquidity_lockbox/src/lib.rs
@@ -376,6 +376,29 @@ pub mod liquidity_lockbox {
     // Get program signer seeds
     let signer_seeds = &[&ctx.accounts.lockbox.seeds()[..]];

+        // CPI to decrease liquidity
+        let cpi_program_modify_liquidity = ctx.accounts.whirlpool_program.to_account_info();
+        let cpi_accounts_modify_liquidity = ModifyLiquidity {
+          whirlpool: ctx.accounts.whirlpool.to_account_info(),
+          position: ctx.accounts.position.to_account_info(),
+          position_authority: ctx.accounts.lockbox.to_account_info(),
+          position_token_account: ctx.accounts.pda_position_account.to_account_info(),
+          tick_array_lower: ctx.accounts.tick_array_lower.to_account_info(),
+          tick_array_upper: ctx.accounts.tick_array_upper.to_account_info(),
+          token_owner_account_a: ctx.accounts.token_owner_account_a.to_account_info(),
+          token_owner_account_b: ctx.accounts.token_owner_account_b.to_account_info(),
+          token_vault_a: ctx.accounts.token_vault_a.to_account_info(),
+          token_vault_b: ctx.accounts.token_vault_b.to_account_info(),
+          token_program: ctx.accounts.token_program.to_account_info()
+        };
+    
+        let cpi_ctx_modify_liquidity = CpiContext::new_with_signer(
+          cpi_program_modify_liquidity,
+          cpi_accounts_modify_liquidity,
+          signer_seeds
+        );
+        whirlpool::cpi::decrease_liquidity(cpi_ctx_modify_liquidity, amount as u128, token_min_a, token_min_b)?;
+
     // Update fees for the position
     let cpi_program_update_fees = ctx.accounts.whirlpool_program.to_account_info();
     let cpi_accounts_update_fees = UpdateFeesAndRewards {
@@ -413,29 +436,6 @@ pub mod liquidity_lockbox {
     );
     whirlpool::cpi::collect_fees(cpi_ctx_collect_fees)?;

-    // CPI to decrease liquidity
-    let cpi_program_modify_liquidity = ctx.accounts.whirlpool_program.to_account_info();
-    let cpi_accounts_modify_liquidity = ModifyLiquidity {
-      whirlpool: ctx.accounts.whirlpool.to_account_info(),
-      position: ctx.accounts.position.to_account_info(),
-      position_authority: ctx.accounts.lockbox.to_account_info(),
-      position_token_account: ctx.accounts.pda_position_account.to_account_info(),
-      tick_array_lower: ctx.accounts.tick_array_lower.to_account_info(),
-      tick_array_upper: ctx.accounts.tick_array_upper.to_account_info(),
-      token_owner_account_a: ctx.accounts.token_owner_account_a.to_account_info(),
-      token_owner_account_b: ctx.accounts.token_owner_account_b.to_account_info(),
-      token_vault_a: ctx.accounts.token_vault_a.to_account_info(),
-      token_vault_b: ctx.accounts.token_vault_b.to_account_info(),
-      token_program: ctx.accounts.token_program.to_account_info()
-    };
-
-    let cpi_ctx_modify_liquidity = CpiContext::new_with_signer(
-      cpi_program_modify_liquidity,
-      cpi_accounts_modify_liquidity,
-      signer_seeds
-    );
-    whirlpool::cpi::decrease_liquidity(cpi_ctx_modify_liquidity, amount as u128, token_min_a, token_min_b)?;
-
     // Update the position liquidity
     ctx.accounts.lockbox.total_liquidity = ctx.accounts.lockbox
       .total_liquidity

[dmvt]

This:

This is faulty as the updateFeesAndRewards function will always revert if the position's liquidity is 0.

is incorrect. liquidity_delta must also be 0.

[10xhash]

But a call to updateFeesAndRewards would be having liquidity_delta 0 since no liquidity is being removed/added

[dmvt]

I've ask the sponsor to comment with their reasoning as well, but this issue as reported is insufficient quality for a high risk. The language is incorrect and the impact is not clearly stated.

[kupermind]

We have changed the order of operations in our rust program indeed, and it works there. Verified the order of instructions provided here and it fails if the remainder is zero.

[dmvt]

Ok. I really hate when this happens, but seems like you have a valid unique high with less than ideal quality. I'm going to reinstate the credit because regardless of quality issues, you've saved the sponsor from a major issue in production. In the future, please spend a bit more time describing your issues and their impact, particularly when high risk. The more detail you can give and the more specific / accurate your statements, the more value you provide.

[c4-judge]

dmvt removed the grade

[c4-judge]

dmvt marked the issue as selected for report

[c4-judge]

dmvt marked the issue as primary issue