Incorrect parameters passed to UniV3 may cause funds stuck in the vault

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L306-L313

Vulnerability details

Impact

Note: this issue happened on the deployed version of GoodEntry and was discovered when using https://alpha.goodentry.io

Due to incorrect parameters and validation when working with UniV3 LP the vault may enter a state where rebalancing reverts. This means any deposits and withdrawals from the vault become unavailable.

Code walkthrough

When rebalancing a vault, the existing positions need to be removed from Uni. This is done in removeFromTick function.

    if (aBal > 0){
      lendingPool.withdraw(address(tr), aBal, address(this));
      tr.withdraw(aBal, 0, 0);
    }

Here, zeros are passed as amount0Min and amount1Min arguments. The execution continues in TokenisableRange.withdraw function. decreaseLiquidity is called to remove liquidity from Uni.

    (removed0, removed1) = POS_MGR.decreaseLiquidity(
      INonfungiblePositionManager.DecreaseLiquidityParams({
        tokenId: tokenId,
        liquidity: uint128(removedLiquidity),
        amount0Min: amount0Min,
        amount1Min: amount1Min,
        deadline: block.timestamp
      })
    );

Here there is an edge-case that for really small change in liquidity the returned values removed0 and removed1 can be 0s (will be explained at the end of a section).

Then, collect is called and removed0,removed1 are passed as arguments.

    POS_MGR.collect( 
      INonfungiblePositionManager.CollectParams({
        tokenId: tokenId,
        recipient: msg.sender,
        amount0Max: uint128(removed0),
        amount1Max: uint128(removed1)
      })
    );

However, collect reverts when both of these values are zeros - https://github.com/Uniswap/v3-periphery/blob/main/contracts/NonfungiblePositionManager.sol#L316

As a result, any deposit/withdraw/rebalancing of the vault will revert when it will be attempting to remove existing liquidity.

When can decreaseLiquidity return 0s

This edge-case is possible to achieve as it happened in the currently deployed alpha version of the product. The sponsor confirmed that the code deployed is the same as presented for the audit.

The tick that caused the revert has less than a dollar of liquidity. Additionally, that tick has outstanding debt and so the aBal value was small enough to cause the issue. In the scenario that happened on-chain aBal is only 33446.

    uint aBal = ERC20(aTokenAddress).balanceOf(address(this));
    uint sBal = tr.balanceOf(aTokenAddress);

    // if there are less tokens available than the balance (because of outstanding debt), withdraw what's available
    if (aBal > sBal) aBal = sBal;
    if (aBal > 0){
      lendingPool.withdraw(address(tr), aBal, address(this));
      tr.withdraw(aBal, 0, 0);
    }

Proof of Concept

The issue can be demonstrated on the contracts deployed on the arbitrum mainnet. This is the foundry test

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

interface IGeVault {
    function withdraw(uint liquidity, address token) external returns (uint amount);
}

contract GeVaultTest is Test {
    IGeVault public vault;

    function setUp() public {
        vault = IGeVault(0xdcc16DEfe27cd4c455e5520550123B4054D1b432);
        // 0xdcc16DEfe27cd4c455e5520550123B4054D1b432 btc vault
    }

    function testWithdraw() public {
        // Account that has a position in the vault
        vm.prank(0x461F5f86026961Ee7098810CC7Ec07874077ACE6);

        // Trying to withdraw 1 USDC
        vault.withdraw(1e6, 0xFF970A61A04b1cA14834A43f5dE4533eBDDB5CC8);
    }
}

The test can be executed by forking the arbitrum mainnet

forge test -vvv --fork-url <your_arb_rpc> --fork-block-number 118634741

The result is an error in UniV3 NonfungiblePosition.collect method

   │   │   │   ├─ [1916] 0xC36442b4a4522E871399CD717aBDD847Ab11FE88::collect((697126, 0xdcc16DEfe27cd4c455e5520550123B4054D1b432, 0, 0)) 
    │   │   │   │   └─ ← "EvmError: Revert"

Tools Used

E2E testing, then code review

Recommended Mitigation Steps

It is unclear why this collect call is needed because the fees are already collected a few lines above in claimFees. I suggest removing the second collect call altogether. If it's needed then perhaps only collect if one of removed0/removed1 is non-zero.

Assessed type

Uniswap

[c4-pre-sort]

141345 marked the issue as duplicate of #78

[c4-pre-sort]

141345 marked the issue as duplicate of #260

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[gzeon-c4]

Not dupe of #260, I think the write up is correct and it seems fund can be stuck in certain edge case.

[c4-judge]

gzeon-c4 marked the issue as grade-b

[kiseln]

Adding more context focusing on the most important parts.

Proof that Uni may return (0, 0) for small amounts in decreaseLiquidity

Here's a foundry test that can be run on the forked arbitrum mainnet. When we try to remove 40000 of liquidity the function will return zeros. https://gist.github.com/imherefortech/9c8a84da66714458be610cfd6fc184e3

This will cause a revert in GoodEntry due to subsequent collect call as described in the main report.

Why would liquidity we are attempting to withdraw be so small?

This can happen if the tick has outstanding debt as specified in the comments. Outstanding debt in the context of the protocol means that there are open positions for this liquidity tick. The issue will arise if open positions take the majority of the tick's liquidity.

How likely is this to happen?

Looking at the vault page we can see that there are often ticks with very little liquidity. Meaning that an adversary can easily open a position taking most of the liquidity

Impact

Deposits/Withdrawal from the vaults are not working and user funds are stuck. As far as I see, there is no way to get back the funds other than upgrading the contracts. The issue has been present for about a week on alpha and was fixed by the devs via an upgrade.

[gzeon-c4]

lol edited my previous comment, I was meant to say the writeup looks correct but it seems to be quite an edge case and therefore the risk is low but @Keref can you review?

[Keref]

Hi, sry overlooked this one I guess it didnt appear in the list? Yes it was confirmed, the auditor also contacted me in private and we solved the bug and updated the live contract during the audit, checking that it didn't try to collect (0, 0)

[gzeon-c4]

Cool, I will bump this back to Med as this clearly affected the availability of the protocol. However, I am not fully convinced that this will make the asset stuck permanently without an upgrade. For example, it seems to be possible to borrow everything from the lendingpool so that aBal is set to 0 and the withdraw will be skipped.

[c4-judge]

This previously downgraded issue has been upgraded by gzeon-c4

[c4-judge]

gzeon-c4 marked the issue as not a duplicate

[c4-judge]

gzeon-c4 marked the issue as primary issue

[c4-judge]

gzeon-c4 marked the issue as selected for report

[kiseln]

Yes it might be correct that there are ways to unstuck the funds without an upgrade.