GeVault deploys assets outside of the market range when the actively-traded ranges are at index 0 or 1

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L432

Vulnerability details

GeVault is designed to deploy its assets on up to 4 TokenisableRange instances. These instances are validated to have ordered, non-overlapping ranges, and the getActiveTickIndex() function identifies the first of the 4 consecutive ranges where assets will be later deployed.

      for (activeTickIndex = 0; activeTickIndex < ticks.length - 3; activeTickIndex++){
        (uint amt0, uint amt1) = ticks[activeTickIndex+1].getTokenAmountsExcludingFees(1e18);
        (uint amt0n, uint amt1n) = ticks[activeTickIndex+2].getTokenAmountsExcludingFees(1e18);
        if ( (amt0 == 0 && amt0n > 0) || (amt1 == 0 && amt1n > 0) )
          break;
      }

This function identifies the "market" range by finding the first three consecutive tickers such as:

• ticks[activeTickIndex] has one asset at zero (this check not done as it's implicit)
• ticks[activeTickIndex+1] has one asset at zero
• ticks[activeTickIndex+2] has one asset at non-zero (meaning market is most likely trading here)

However, if the market is trading at ticks[0] or ticks[1], the above condition will never be met, because the loop will find zeros in all the inspected, and getActiveTickIndex() will return the last possible index.

Impact

Whenever the above condition is met, assets will be deployed as far as the GeVault can from where they should be to produce value. Protocol users would therefore have their assets not produce value without however losing any of them. The protocol owners can intervene and fix the situation by adding extra ticks at the beginning of the array and triggering a rebalance.

Proof of Concept

I have a running PoC in my environment, which I will keep aside and will be happy to provide if requested, but I would rather not share it because it's a monstrous Foundry setup with a couple of workarounds to not make it even more monstrous.

High level my setup is:

• set up a GeVault with 7 ranges, most of which are at lower prices:
• range at index 0 at 2001-2250 (implicit, e10)
• 1 at 1751-2000
• 2 at 1501-1750
• 3 at 1251-1500
• 4 at 1001-1240
• 5 at 751-1000
• 6 at 500-750
• with USDC/WETH at 1870 (I forked mainnet at block 17811921)
• call getActiveTickIndex() which will return 4 instead of 1

Tools Used

Code review, Foundry

Recommended Mitigation Steps

It is recommended to not only identify the "out-of-market -> in-market" transition, but also the "in-market -> out-of-market" one, as well as including ticks[activeTickIndex] in the checks, to make sure that all liquidity pools can actually be identified as "activeTickIndex" when their glory time on Uniswap comes.

Assessed type

Loop

[Keref]

It is expected that if valid ticks are missing below, there is no valid activeTickIndex. Indeed if price trades between tick 0 and 1, there should be 2 ticks below and not one, and activeTickIndex should (intuitively) be at -1 (makes no sense)

We could update the function to make it fail explicitly if no valid tick was found. This is QA

[c4-sponsor]

Keref marked the issue as sponsor acknowledged

[c4-sponsor]

Keref marked the issue as disagree with severity

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[gzeon-c4]

asset not at risk, function of the protocol is arguably working as expected

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as grade-a

[Keref]

Code has been updated, see PR#11 ActiveTickIndex should now always return a valid middle price (actually the index of the tick), unless price is above last tick, then it returns ticks.length