User can sandwich rebalance process by calling `deposit` or `withdraw`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L280 https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L226-L239

Vulnerability details

Impact

When prices move, vault will do rebalance to adjust its token distribution. However, due to the placement of rebalance call inside deposit and withdraw, user can easily sandwich the operations.

Proof of Concept

Consider this scenario :

1. User see that price of ETH go up, and GeVault that hold ETH/USDC not yet rebalance to this price.
2. User call deposit, will still use current vault value when calculating the amount of liquidity received :

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

  function deposit(address token, uint amount) public payable nonReentrant returns (uint liquidity) 
  {
    require(isEnabled, "GEV: Pool Disabled");
    require(poolMatchesOracle(), "GEV: Oracle Error");
    require(token == address(token0) || token == address(token1), "GEV: Invalid Token");
    require(amount > 0 || msg.value > 0, "GEV: Deposit Zero");

    // Wrap if necessary and deposit here
    if (msg.value > 0){
      require(token == address(WETH), "GEV: Invalid Weth");
      // wraps ETH by sending to the wrapper that sends back WETH
      WETH.deposit{value: msg.value}();
      amount = msg.value;
    }
    else { 
      ERC20(token).safeTransferFrom(msg.sender, address(this), amount);
    }

    // Send deposit fee to treasury
    uint fee = amount * getAdjustedBaseFee(token == address(token0)) / 1e4;
    ERC20(token).safeTransfer(treasury, fee);
    uint valueX8 = oracle.getAssetPrice(token) * (amount - fee) / 10**ERC20(token).decimals();
    require(tvlCap > valueX8 + getTVL(), "GEV: Max Cap Reached");
    // @audit - will use TVL from previous price change
    uint vaultValueX8 = getTVL();
    uint tSupply = totalSupply();
    // initial liquidity at 1e18 token ~ $1
    if (tSupply == 0 || vaultValueX8 == 0)
      liquidity = valueX8 * 1e10;
    else {
      // @audit - will use vaultValue before price go up
      liquidity = tSupply * valueX8 / vaultValueX8;
    }

    rebalance();
    require(liquidity > 0, "GEV: No Liquidity Added");
    _mint(msg.sender, liquidity);    
    emit Deposit(msg.sender, token, amount, liquidity);
  }

It can be observed that liquidity calculated using vaultValueX8 from getTVL() operation :

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

  function getTVL() public view returns (uint valueX8){
    for(uint k=0; k<ticks.length; k++){
      TokenisableRange t = ticks[k];
      uint bal = getTickBalance(k);
      valueX8 += bal * t.latestAnswer() / 1e18;
    }
  }

And the valueX8 depend on latestAnswer call that will eventually call returnExpectedBalanceWithoutFees to determined token amounts that will be used to determine the value :

  function returnExpectedBalanceWithoutFees(uint TOKEN0_PRICE, uint TOKEN1_PRICE) internal view returns (uint256 amt0, uint256 amt1) {
    // if 0 get price from oracle
    if (TOKEN0_PRICE == 0) TOKEN0_PRICE = ORACLE.getAssetPrice(address(TOKEN0.token));
    if (TOKEN1_PRICE == 0) TOKEN1_PRICE = ORACLE.getAssetPrice(address(TOKEN1.token));

    (amt0, amt1) = LiquidityAmounts.getAmountsForLiquidity( uint160( sqrt( (2 ** 192 * ((TOKEN0_PRICE * 10 ** TOKEN1.decimals) / TOKEN1_PRICE)) / ( 10 ** TOKEN0.decimals ) ) ), TickMath.getSqrtRatioAtTick(lowerTick), TickMath.getSqrtRatioAtTick(upperTick),  liquidity);
  }

If price go up, but rebalance not yet happened, the amt1 token portion used will be bigger and this cause the vault value will be lower.

So user will mint liquidity based on this value, rebalance operation happened after this calculation. Right after deposit, user can call withdraw to get the profit from price movement.

Tools Used

Manual review

Recommended Mitigation Steps

Consider to move rebalance operation before the deposit and withdraw operation :

  function deposit(address token, uint amount) public payable nonReentrant returns (uint liquidity) 
  {
    require(isEnabled, "GEV: Pool Disabled");
    require(poolMatchesOracle(), "GEV: Oracle Error");
    require(token == address(token0) || token == address(token1), "GEV: Invalid Token");
    require(amount > 0 || msg.value > 0, "GEV: Deposit Zero");
+   rebalance();    
    // Wrap if necessary and deposit here
    if (msg.value > 0){
      require(token == address(WETH), "GEV: Invalid Weth");
      // wraps ETH by sending to the wrapper that sends back WETH
      WETH.deposit{value: msg.value}();
      amount = msg.value;
    }
    else { 
      ERC20(token).safeTransferFrom(msg.sender, address(this), amount);
    }

    // Send deposit fee to treasury
    uint fee = amount * getAdjustedBaseFee(token == address(token0)) / 1e4;
    ERC20(token).safeTransfer(treasury, fee);
    uint valueX8 = oracle.getAssetPrice(token) * (amount - fee) / 10**ERC20(token).decimals();
    require(tvlCap > valueX8 + getTVL(), "GEV: Max Cap Reached");

    uint vaultValueX8 = getTVL();
    uint tSupply = totalSupply();
    // initial liquidity at 1e18 token ~ $1
    if (tSupply == 0 || vaultValueX8 == 0)
      liquidity = valueX8 * 1e10;
    else {
      liquidity = tSupply * valueX8 / vaultValueX8;
    }

-   rebalance();
    require(liquidity > 0, "GEV: No Liquidity Added");
    _mint(msg.sender, liquidity);    
    emit Deposit(msg.sender, token, amount, liquidity);
  }

  function withdraw(uint liquidity, address token) public nonReentrant returns (uint amount) {
    require(poolMatchesOracle(), "GEV: Oracle Error");
+   if (isEnabled) rebalance();
    if (liquidity == 0) liquidity = balanceOf(msg.sender);
    require(liquidity <= balanceOf(msg.sender), "GEV: Insufficient Balance");
    require(liquidity > 0, "GEV: Withdraw Zero");

    uint vaultValueX8 = getTVL();
    uint valueX8 = vaultValueX8 * liquidity / totalSupply();
    amount = valueX8 * 10**ERC20(token).decimals() / oracle.getAssetPrice(token);
    uint fee = amount * getAdjustedBaseFee(token == address(token1)) / 1e4;

    _burn(msg.sender, liquidity);
    removeFromAllTicks();
    ERC20(token).safeTransfer(treasury, fee);
    uint bal = amount - fee;

    if (token == address(WETH)){
      WETH.withdraw(bal);
      payable(msg.sender).transfer(bal);
    }
    else {
      ERC20(token).safeTransfer(msg.sender, bal);
    }

    // if pool enabled, deploy assets in ticks, otherwise just let assets sit here until totally withdrawn
    if (isEnabled) deployAssets();
    emit Withdraw(msg.sender, token, amount, liquidity);
  }

Assessed type

MEV

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

Keref marked the issue as sponsor disputed

[Keref]

Rebalance doesnt do swaps but only moves assets around. Value of underlying assets only depends on assets price and amounts, not how those are distributed among various Uniswap positions.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid