Good Entry - said's results

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L214-L241 https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L313-L317 https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L321-L333

Vulnerability details

Impact

When GeVault is disabled by setting isEnabled via setEnabled(), it will prevent users to deposit to the vault, but still allow withdraw operation to get their underlying tokens. However, only the first caller of withdraw operation can get his/her tokens. Or worse, griefer can completely denied anyone to claim the token.

Proof of Concept

Consider this scenario :

1. When isEnabled is false, withdraw is called :

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

  function withdraw(uint liquidity, address token) public nonReentrant returns (uint amount) {
    require(poolMatchesOracle(), "GEV: Oracle Error");
    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);
  }

It will calculate the amount that will be send based on vaultValueX8 from getTVL() :

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

  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;
    }
  }

the calculated value based on getTickBalance, this value based on aToken balance of this vault :

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

  function getTickBalance(uint index) public view returns (uint liquidity) {
    TokenisableRange t = ticks[index];
    address aTokenAddress = lendingPool.getReserveData(address(t)).aTokenAddress;
    liquidity = ERC20(aTokenAddress).balanceOf(address(this));
  }

After this withdraw operations, all aToken of this vault will be withdrawn from the lending pool because of removeFromAllTicks(), this call will loop trough all ticks and do the removeFromTick operation :

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

  function removeFromTick(uint index) internal {
    TokenisableRange tr = ticks[index];
    address aTokenAddress = lendingPool.getReserveData(address(tr)).aTokenAddress;
    uint aBal = ERC20(aTokenAddress).balanceOf(address(this));
    uint sBal = tr.balanceOf(aTokenAddress);

    // if there are less tokens available than the balance (because of outstanding debt), withdraw what's available
    if (aBal > sBal) aBal = sBal;
    if (aBal > 0){
      lendingPool.withdraw(address(tr), aBal, address(this));
      tr.withdraw(aBal, 0, 0);
    }
  }

But because isEnabled false, it will not deploy the assets again to the ticks.

Now the next withdraw caller will always return 0 amount, due to getTVL will always return 0.

The worse scenario is griefer can trigger rebalance() after isEnabled become false to make getTVL become 0 before anyone call withdraw.

Now user can't withdraw their tokens and the tokens will stuck inside the vault.

Tools Used

Manual review

Recommended Mitigation Steps

Consider to add another way to calculate getTVL when vault is disabled.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

poolMatchesOracle is used to compare price calculated from uniswap v3 pool and chainlink oracle and decide whether rebalance should happened or not. priceX8 will be holding price information calculated using sqrtPriceX96 and when operations is performed, it will try to scale down using 2 ** 12. However, the scale down is not enough and overflow can still happened.

Proof of Concept

Consider this scenario, The GeVault is using WBTC for token0 and WETH for token1.

These are information for the WBTC/WETH from uniswap v3 pool (0x4585FE77225b41b697C938B018E2Ac67Ac5a20c0):

slot0 data (at current time) :

sqrtPriceX96   uint160 :  31520141554881197083247204479961147

token0 (WBTC) decimals is 8 and token1 (WETH) decimals is 18.

Using these information, try to reproduce the priceX8 calculation :

    function testOraclePrice() public {
        uint160 sqrtPriceX96 = 31520141554881197083247204479961147;
        // decimals0 is 8
        uint priceX8 = 10 ** 8;
        // Overflow if dont scale down the sqrtPrice before div 2*192 
        // @audit - the overflow still possible
        priceX8 =
            (priceX8 * uint(sqrtPriceX96 / 2 ** 12) ** 2 * 1e8) /
            2 ** 168;
        // decimals1 is 18
        priceX8 = priceX8 / 10 ** 18;
        assertEq(true, true);
    }

the test result in overflow :

[FAIL. Reason: Arithmetic over/underflow] testOraclePrice() 

This will cause calculation still overflow, even using the widely used WBTC/WETH pair

Tools Used

Manual review

Recommended Mitigation Steps

Consider to change the scale down using the recommended value from uniswap v3 library:

https://github.com/Uniswap/v3-periphery/blob/main/contracts/libraries/OracleLibrary.sol#L49-L69

or change the scale down similar to the one used inside library

  function poolMatchesOracle() public view returns (bool matches){
    (uint160 sqrtPriceX96,,,,,,) = uniswapPool.slot0();
    
    uint decimals0 = token0.decimals();
    uint decimals1 = token1.decimals();
    uint priceX8 = 10**decimals0;
    // Overflow if dont scale down the sqrtPrice before div 2*192
-    priceX8 = priceX8 * uint(sqrtPriceX96 / 2 ** 12) ** 2 * 1e8 / 2**168;
+    priceX8 = priceX8 * (uint(sqrtPriceX96) ** 2 / 2 ** 64) * 1e8 / 2**128;
    priceX8 = priceX8 / 10**decimals1;
    uint oraclePrice = 1e8 * oracle.getAssetPrice(address(token0)) / oracle.getAssetPrice(address(token1));
    if (oraclePrice < priceX8 * 101 / 100 && oraclePrice > priceX8 * 99 / 100) matches = true;
  }

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/RangeManager.sol#L95-L102 https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L134-L163

Vulnerability details

Impact

After the owner of RangeManager create new range via generateRange, they can then call initRange to init the range and providing the initial underlying tokens for initial uniswap v3 mint amounts. However, after operation the refunded underlying tokens is not send back to the owner, this will allow user to steal this token by triggering cleanup().

Proof of Concept

When initRange is called, it will trigger init inside the TokenisableRange :

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/RangeManager.sol#L95-L102

  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)));
  }

Inside init, it will try to mint Uniswap v3 NFT and provide initial liquidity based on desired underlying amount provided :

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L134-L163

  function init(uint n0, uint n1) external {
    require(status == ProxyState.INIT_LP, "!InitLP");
    require(msg.sender == creator, "Unallowed call");
    status = ProxyState.READY;
    TOKEN0.token.safeTransferFrom(msg.sender, address(this), n0);
    TOKEN1.token.safeTransferFrom(msg.sender, address(this), n1);
    TOKEN0.token.safeIncreaseAllowance(address(POS_MGR), n0);
    TOKEN1.token.safeIncreaseAllowance(address(POS_MGR), n1);
    (tokenId, liquidity, , ) = POS_MGR.mint( 
      INonfungiblePositionManager.MintParams({
         token0: address(TOKEN0.token),
         token1: address(TOKEN1.token),
         fee: feeTier * 100,
         tickLower: lowerTick,
         tickUpper: upperTick,
         amount0Desired: n0,
         amount1Desired: n1,
         amount0Min: n0 * 95 / 100,
         amount1Min: n1 * 95 / 100,
         recipient: address(this),
         deadline: block.timestamp
      })
    );
    
    // Transfer remaining assets back to user
    TOKEN0.token.safeTransfer( msg.sender,  TOKEN0.token.balanceOf(address(this)));
    TOKEN1.token.safeTransfer(msg.sender, TOKEN1.token.balanceOf(address(this)));
    _mint(msg.sender, 1e18);
    emit Deposit(msg.sender, 1e18);
  }

Uniswap mint will always refund the unused underlying tokens, in this case, the init will send it back to RangeManager.

However, inside initRange, it will only transfer to msg.sender the minted tr token, the remaining underlying token will stay inside this RangeManager.

Now user that aware of this initRange operation called by admin, can back-run the operation with calling removeAssetsFromStep that will trigger cleanup function to sweep the token :

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/RangeManager.sol#L190-L207

  function cleanup() internal {
    uint256 asset0_amt = ASSET_0.balanceOf(address(this));
    uint256 asset1_amt = ASSET_1.balanceOf(address(this));
    
    if (asset0_amt > 0) {
      ASSET_0.safeIncreaseAllowance(address(LENDING_POOL), asset0_amt);
      LENDING_POOL.deposit(address(ASSET_0), asset0_amt, msg.sender, 0);
    }
    
    if (asset1_amt > 0) {
      ASSET_1.safeIncreaseAllowance(address(LENDING_POOL), asset1_amt);
      LENDING_POOL.deposit(address(ASSET_1), asset1_amt, msg.sender, 0);
    }
    
    // Check that health factor is not put into liquidation / with buffer
    (,,,,,uint256 hf) = LENDING_POOL.getUserAccountData(msg.sender);
    require(hf > 1.01e18, "Health factor is too low");
  }

NOTE : This scenario is not an admin mistake, as the uniswap mint operation is very likely to refund underlying tokens (https://docs.uniswap.org/contracts/v3/guides/providing-liquidity/mint-a-position#updating-the-deposit-mapping-and-refunding-the-calling-address)

Tools Used

Manual Review

Recommended Mitigation Steps

Consider to add cleanup after the initRange call :

  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)));
+   cleanup();
  }

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L333-L339

Vulnerability details

Impact

returnExpectedBalanceWithoutFees is a crucial function that will return the amount of token0 and token1 from the given price, ticks price and liquidity. However, the calculation of sqrt price using oracle price has very minimal underflow protection, could go underflow for certain pairs.

Proof of Concept

This is the calculation inside returnExpectedBalanceWithoutFees :

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L338

  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);
  }

It can be observed that it has inner calculation :

(TOKEN0_PRICE * 10 ** TOKEN1.decimals) / TOKEN1_PRICE)

This calculation have very minimal underflow protection ((TOKEN0_PRICE * 10 ** TOKEN1.decimals) must be greater than TOKEN1_PRICE), especially when TOKEN1.decimals is low and TOKEN0_PRICE value is very low compared to TOKEN1_PRICE.

When this happened (price underflow to 0) and passed to getAmountsForLiquidity, the function will always process and return only amt0. This will result wrong amount is calculated and used when deciding amount to process inside claimFees.

Tools Used

Manual review

Recommended Mitigation Steps

Change the operation when calculating price using this calculation to improve overflow/underflow protection :

sqrt((TOKEN0_PRICE * 10 ** TOKEN1.decimals * 2 ** 96) / (TOKEN1_PRICE * 10 ** TOKEN0.decimals)) * 2 **48

reference : https://github.com/makerdao/univ3-lp-oracle

Assessed type

Math