rebalance process not considering `baseTokenIsToken0` inside GeVault

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L64 https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L124-L127 https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L145-L148 https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L337-L363

Vulnerability details

Impact

When GeVault is initialized, it has an option to set baseTokenIsToken0 to decide how ticks are ordered. However, inside rebalance process it never check the baseTokenIsToken0 and can make rebalance process broken.

Proof of Concept

When first created, GeVault creator need to set baseTokenIsToken0 inside constructor value :

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L67-L94

  constructor(
    address _treasury, 
    address roeRouter, 
    address _uniswapPool, 
    uint poolId, 
    string memory name, 
    string memory symbol,
    address weth,
    bool _baseTokenIsToken0
  ) 
    ERC20(name, symbol)
  {
    require(_treasury != address(0x0), "GEV: Invalid Treasury");
    require(_uniswapPool != address(0x0), "GEV: Invalid Pool");
    require(weth != address(0x0), "GEV: Invalid WETH");

    (address lpap, address _token0, address _token1,, ) = RoeRouter(roeRouter).pools(poolId);
    token0 = ERC20(_token0);
    token1 = ERC20(_token1);

    lendingPool = ILendingPool(ILendingPoolAddressesProvider(lpap).getLendingPool());
    oracle = IPriceOracle(ILendingPoolAddressesProvider(lpap).getPriceOracle());
    treasury = _treasury;
    uniswapPool = IUniswapV3Pool(_uniswapPool);
    WETH = IWETH(weth);
    baseTokenIsToken0 = _baseTokenIsToken0;
  }

This baseTokenIsToken0 will decide how owner order the added ticks when pushTick or shiftTick is called :

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L116-L132

  function pushTick(address tr) public onlyOwner {
    TokenisableRange t = TokenisableRange(tr);
    (ERC20 t0,) = t.TOKEN0();
    (ERC20 t1,) = t.TOKEN1();
    require(t0 == token0 && t1 == token1, "GEV: Invalid TR");
    if (ticks.length == 0) ticks.push(t);
    else {
      // Check that tick is properly ordered
      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");

      ticks.push(TokenisableRange(tr));
    }
    emit PushTick(tr);
  } 

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L137-L161

  function shiftTick(address tr) public onlyOwner {
    TokenisableRange t = TokenisableRange(tr);
    (ERC20 t0,) = t.TOKEN0();
    (ERC20 t1,) = t.TOKEN1();
    require(t0 == token0 && t1 == token1, "GEV: Invalid TR");
    if (ticks.length == 0) ticks.push(t);
    else {
      // Check that tick is properly ordered
      if (!baseTokenIsToken0) 
        require( t.lowerTick() > ticks[0].upperTick(), "GEV: Shift Tick Overlap");
      else 
        require( t.upperTick() < ticks[0].lowerTick(), "GEV: Shift Tick Overlap");

      // extend array by pushing last elt
      ticks.push(ticks[ticks.length-1]);
      // shift each element
      if (ticks.length > 2){
        for (uint k = 0; k < ticks.length - 2; k++) 
          ticks[ticks.length - 2 - k] = ticks[ticks.length - 3 - k];
        }
      // add new tick in first place
      ticks[0] = t;
    }
    emit ShiftTick(tr);
  }

However, when rebalance process is performed and deployAssets is called, the distribution never considered the baseTokenIsToken0.

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L337-L363

  function deployAssets() internal { 
   // @audit - these operation assume `baseTokenIsToken0` is true
    uint newTickIndex = getActiveTickIndex();
    uint availToken0 = token0.balanceOf(address(this));
    uint availToken1 = token1.balanceOf(address(this));

    // Check which is the main token
    (uint amount0ft, uint amount1ft) = ticks[newTickIndex].getTokenAmountsExcludingFees(1e18);
    uint tick0Index = newTickIndex;
    uint tick1Index = newTickIndex + 2;
    if (amount1ft > 0){
      tick0Index = newTickIndex + 2;
      tick1Index = newTickIndex;
    }

    // Deposit into the ticks + into the LP
    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);
      depositAndStash(ticks[tick1Index+1], 0, availToken1 / 2);
    }

    if (newTickIndex != tickIndex) tickIndex = newTickIndex;
    emit Rebalance(tickIndex);
  }

This could cause rebalance process have opposite effect.

Tools Used

Manual review

Recommended Mitigation Steps

Adjust rebalance and deployAssets call based on baseTokenIsToken0 value.

Assessed type

Context

[141345]

seems invalid

RoeRouter(roeRouter).pools(poolId) should already contain the infomation

[c4-sponsor]

Keref marked the issue as sponsor disputed

[Keref]

Ticks are already added in the correct order so that it doesn't need to be checked afterwards

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid