GeVault's depositAndStash always reverts and ignores deposits to the active (and most important) ticker

[code423n4]

Lines of code

https://github.com/GoodEntry-io/ge/blob/c7c7de57902e11e66c8186d93c5bb511b53a45b8/contracts/GeVault.sol#L359 https://github.com/GoodEntry-io/ge/blob/c7c7de57902e11e66c8186d93c5bb511b53a45b8/contracts/GeVault.sol#L360

Vulnerability details

In the new implementation, when deploying assets to the TokenisableRange instances, GeVault always sends a fixed amount for one asset and zero for the other asset:

    if (newTickIndex > 1) 
      depositAndStash(
        ticks[newTickIndex-2], 
        baseTokenIsToken0 ? 0 : availToken0 / liquidityPerTick,
        baseTokenIsToken0 ? availToken1 / liquidityPerTick : 0
      );
    if (newTickIndex > 0) 
      depositAndStash(
        ticks[newTickIndex-1], 
        baseTokenIsToken0 ? 0 : availToken0 / liquidityPerTick,
        baseTokenIsToken0 ? availToken1 / liquidityPerTick : 0
      );
    if (newTickIndex < ticks.length) 
      depositAndStash(
        ticks[newTickIndex], 
        baseTokenIsToken0 ? availToken0 / liquidityPerTick : 0,
        baseTokenIsToken0 ? 0 : availToken1 / liquidityPerTick
      );
    if (newTickIndex+1 < ticks.length) 
      depositAndStash(
        ticks[newTickIndex+1], 
        baseTokenIsToken0 ? availToken0 / liquidityPerTick : 0,
        baseTokenIsToken0 ? 0 : availToken1 / liquidityPerTick
      );

However, the active ticker (newTickIndex), by definition of active ticker, requires depositing non-zero amounts for both assets. If one of the two is zero, only a little amount of liquidity will generated, and the provided asset will go mostly unused, causing the TokenisableRange's deposit slippage check to fail.

Impact

GeVault will consistently fail to deploy assets to the active ticker. Since the active ticker is the one that's traded and generates fees, always depositing outside of that ticker makes the GeVault contract completely lose its intended functionality of concentrating liquidity where it produces value.

Proof of Concept

• Set up a GeVault with the following settings:
  • Token0: USDC (base token)
  • Token1: WETH
  • Range 0: 1752e10, 2000e10
• Have market sit at 1875 (0 will be active)
• deposit to GeVault some USDC and WETH
• check the TokenisableRange balance of GeVault

Tools Used

Code review, Foundry

Recommended Mitigation Steps

When deploying assets to the active ticker (not needed for the others), cap the sent amounts to what's needed to prevent the slippage check from failing:

    if (newTickIndex < ticks.length){
      (uint256 token0Amt, uint256 token1Amt) = ticks[newTickIndex].getExactTokenAmounts(
        availToken0 / liquidityPerTick, 
        availToken1 / liquidityPerTick
      );
      depositAndStash(
        ticks[newTickIndex], 
        token0Amt,
        token1Amt
      );
    }

with the exactTokenAmounts utility function being added to TokenisableRange:

  function getExactTokenAmounts(uint256 amount0, uint256 amount1) public view returns(uint256 exactAmount0, uint256 exactAmount1) {
    address pool = V3_FACTORY.getPool(address(TOKEN0.token), address(TOKEN1.token), feeTier * 100);
    (uint160 sqrtPriceX96,,,,,,)  = IUniswapV3Pool(pool).slot0();
    uint160 lowerX96 = TickMath.getSqrtRatioAtTick(lowerTick);
    uint160 higherX96 = TickMath.getSqrtRatioAtTick(upperTick);

    uint128 inputLiquidity = LiquidityAmounts.getLiquidityForAmounts(sqrtPriceX96, lowerX96, higherX96, amount0, amount1);

    return LiquidityAmounts.getAmountsForLiquidity(sqrtPriceX96, lowerX96, higherX96, inputLiquidity);
  }

The change to GeVault should be safe to apply because the poolMatchesOracle() check removes the need for a further slippage check.

Assessed type

Uniswap

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report

[Keref]

It's been mentioned in the review comments. Basically we dont add liquidity there for various reasons (likely already overallocated, unclear deposit strategy) Additionally the activeTickeIndex doent mean the tick is active (maybe misnomer) but points to the first tick above. Actually the probability that the price is in the tick is low and we just skip depositing in that case

[xuwinnie]

@gzeon-c4 hey, actually this issue is already raised at the original contest, see #414

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as not selected for report

[peppelan]

Hi @gzeon-c4, I would ask to reconsider the Med removal, considering the following:

1. the impact of this issue is no less than #43 - in fact, it's even a little more impactful, because while with #43 alone the GeVault can (by pure chance) end up deploying to the active ticker, this issue prevents correct deployments 100% of times
2. a similar finding was indeed reported in the original contest's #414, but the team acted to mitigate the issue, so I don't see any reason why reporting that the fix does not mitigate the impact (as per previous point) should not be in scope

[Keref]

This isnt a medium risk because there are no funds at risk and it follows our allocation strategy. Mitigation of the issue is to avoid reverting, not picking a different allocation strategy