Good Entry - LokiThe5th's results

Lines of code

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

Vulnerability details

Impact

GeVaults have a state variable isEnabled which affords the team the ability to disable a pool if the need arises. The intent from the code is then to allow users to call withdraw in such a scenario to retrieve their assets and burn their liquidity tokens.

If isEnabled is set to false and then removeFromAllTicks() is called (through either withdraw or rebalance()), then all the underlying will be withdrawn from the ticks and not redeposited.

In practice, after isEnabled is set to false, only the first user will be able to withdraw their assets (if a griefer hasn't called rebalance() yet), once that user has completed their withdrawal the assets will not be redeployed into the ticks (due to the isEnabled check here), essentially setting the getTVL() to 0 while the GeVault is paused.

The likelihood of this occurring is high in the case of a disabled pool, as all withdrawals trigger the call to removeFromAllTicks(). The impact is considered high as it may lead to loss of tokens for users.

Also note: should isEnabled be set to true in an attempt to allow users to withdraw again, then the burnt liquidity from other users' failed withdrawals will result in those who still have liquidity tokens trying to withdraw the excess liquidity (as their liquidity tokens are now worth more).

Proof of Concept

Once the team calls setEnabled(bool) and sets isEnabled to false, then calls to the public function rebalance(), or the internal function removeFromAllTicks(), will cause all the assets to be withdrawn back to the contract, but not redeployed, due to this code statement:

  function rebalance() public {
    require(poolMatchesOracle(), "GEV: Oracle Error");
    removeFromAllTicks();
    if (isEnabled) deployAssets();
  }

and in the case of calls to withdraw():

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

This means the getTVL() function will return 0, because the TVL is calculated from the balance of the ticks held by the vault, as given by liquidity = ERC20(aTokenAddress).balanceOf(address(this)) from the getTickBalance() function.

The getTVL() function then uses the amount returned from getTickBalance() and multiplies this by the TokenisableRange's latestAnswer(). In essence, TVL is calculated here by: tickTVL[i] = (amount of LP tokens for tick[i]) * (price per LP token for tick[i])

As the assets have been withdrawn from the TokenisableRange by the earlier calls to rebalance() or withdraw() (which burns the tick LP tokens held by the contract) the call to getTickBalance() will return 0, as the total amount of LP tokens is 0. Thus getTVL() also evaluates to 0.

Once the isEnabled is false, if ALICE tries to call withdraw and another user has called withdraw() before her, or a griefer has called rebalance(), then the ALICE's amount returned will become 0 due to the getTVL() evaluating to 0 in this code:

    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;

Even though no tokens are transferred to ALICE, her liquidity tokens are still burnt.

ALICE no longer has any GeVault liquidity tokens so she cannot recover her funds.

The funds now accrue to the other liquidity token holders who also cannot withdraw it while isEnabled is set to false.

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider reworking the isEnabled check in the withdraw function to set the TVL for the contract to the value of the underlying tokens (not the aTokens from the TokenisableRange) contained in the contract.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The poolMatchesOracle() function logic is flawed for the intended assets on the target deployment chain (Arbitrum). The test files use a mainnet fork and tests the poolMatchesOracle() functionality using the UniswapV3Pool for USDC/WETH. This does not accurately represent the pool configuration on Arbitrum.

The code in question is the poolMatchesOracle function in GeVault.sol:

  /// @notice Checks that the pool price isn't manipulated
  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 / 10**decimals1;
    uint oraclePrice = 1e8 * oracle.getAssetPrice(address(token0)) / oracle.getAssetPrice(address(token1));
    if (oraclePrice < priceX8 * 101 / 100 && oraclePrice > priceX8 * 99 / 100) matches = true;
  }

In the above USDC/WETH pool token0 is USDC, which has 6 decimals and a sqrtPriceX96 of about 1850813559491440442676082183598997. These values allow the check to pass without overflow.

But on Arbitrum mainnet token pairs do not always follow the same configuration as on mainnet Ethereum. For example, the equivalent Abritrum pool for USDC and WETH swaps the token config around, with WETH being token0 and a sqrtPriceX96 of around 3387218208183270454039624 at this time. The pool can be viewed here.

Any GeVault using a pool where the token0 has 18 decimals will function correctly until the sqrtPriceX96 reaches roughly 139379657490816394634598239204. For WETH/USDC this translates into a WETH price of about 3.1 million USD in value. As the all-time high for ETH is $4,891.70, it is unlikely that the sqrtPriceX96 will exceed the threshold in the near future.

However, the edge case must be noted that in times of extreme market volatility (in this case a catastrophic price crash of USDC relative to WETH) the sqrtPriceX96 might also breach the threshold; this would lock users' funds at the most inconvenient time.

In the case of WETH/GMX, a token specified for the contest, the token config once again has 18 decimals for token0 and the current sqrtPriceX96 is 460068177510362345677242588441 - already exceeding the threshold for overflow as calculated here on line 374. See the pool on arbitrum here. In other words a vault linked to this pool cannot accept deposits or withdrawals at this time.

The probability of occurrence for the WETH/USDC on Arbitrum is low, but for GMX/WETH it is a certainty (thus high). The impact on users caught in the low probability scenario is high as they may be exposed to market conditions without the ability to exit the market due to the continued locking of assets. The issue has thus been evaluated to medium.

The danger is that vaults may seem to operate perfectly and continue to accrue deposits from users, until prices move to the degree that the calculation at line 374 overflows. At this point users may suffer losses.

Proof of Concept

The below PoC code shows how the implemented code overflows at certain thresholds of the sqrtPriceX96 for tokens with 18 decimals. It is a simple foundry test file, using the sqrtPriceX96 from V3 Pools (Eth and Arbitrum mainnets). It uses the vulnerable code and demonstrates the control scenario on Eth and Arbitrum mainnets, and then demonstrates the PoC scenarios for both the WETH/USDC and GMX/WETH pools.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

contract PoCTest is Test {
    uint256 private sqrtPriceX96EthMainnetUSDC;
    uint256 private sqrtPriceX96ArbUSDC;
    uint256 private sqrtPriceX96ArbUSDCHighETHPrice;
    uint256 private sqrtPriceX96ArbGMX;

    function setUp() public {
        sqrtPriceX96EthMainnetUSDC = 1850813559491440442676082183598997;
        sqrtPriceX96ArbUSDC = 3387218208183270454039624;
        sqrtPriceX96ArbUSDCHighETHPrice = 139379658490816394634598239204;                                       
        sqrtPriceX96ArbGMX = 461311460202047998706386457553;
    }

    // Control Scenario 1: Mainnet USDC/WETH Pool
    function testMainnetUSDC() public {
        // This is the setup from the tests
        // USDC has 6 decimals
        uint256 token0Decimals = 6;
        // WETH has 18 decimals
        uint256 token1Decimals = 18;

        // Equal to `uint decimals0 = token0.decimals();`
        uint decimals0 = token0Decimals;
        // Equal to `uint decimals0 = token1.decimals();`
        uint decimals1 = token1Decimals;

        uint priceX8 = 10**decimals0;
        // Overflow if dont scale down the sqrtPrice before div 2*192
        priceX8 = priceX8 * uint(sqrtPriceX96EthMainnetUSDC / 2 ** 12) ** 2 * 1e8 / 2**168;
    }

    // PoC Scenario 1: Arbitrum USDC/WETH Pool
    // The pool will function at deployment
    function testArbitrumUSDC() public {
        // This is the config for Arbitrum
        // WETH has 18 decimals
        uint256 token0Decimals = 18;
        // WETH has 18 decimals
        uint256 token1Decimals = 6;

        // Equal to `uint decimals0 = token0.decimals();`
        uint decimals0 = token0Decimals;
        // Equal to `uint decimals0 = token1.decimals();`
        uint decimals1 = token1Decimals;

        uint priceX8 = 10**decimals0;
        // It will not overflow at the current `sqrtPriceX96`
        priceX8 = priceX8 * uint(sqrtPriceX96ArbUSDC / 2 ** 12) ** 2 * 1e8 / 2**168;
    }

    // PoC Scenario 2: Arbitrum USDC/WETH Pool
    // EXTREME ETH price or USDC devaluement, the calculation will overflow.
    // The ETH price would have to be 3.1 million USDC per ETH
    function testArbitrumUSDCHighETHPrice() public {
        // This is the config for Arbitrum
        // WETH has 18 decimals
        uint256 token0Decimals = 18;
        // USDC has 6 decimals
        uint256 token1Decimals = 6;

        // Equal to `uint decimals0 = token0.decimals();`
        uint decimals0 = token0Decimals;
        // Equal to `uint decimals0 = token1.decimals();`
        uint decimals1 = token1Decimals;

        uint priceX8 = 10**decimals0;
        // It will now overflow due to the high `sqrtPriceX96`
        vm.expectRevert();
        priceX8 = priceX8 * uint(sqrtPriceX96ArbUSDCHighETHPrice / 2 ** 12) ** 2 * 1e8 / 2**168;
    }

    // PoC Scenario 3: Arbitrum GMX/WETH Pool
    // `sqrtPriceX96` > 139379658490816394634598239204, i.e. the calculation will overflow.
    function testArbitrumGMX() public {
        // This is the config for Arbitrum
        // GMX has 18 decimals
        uint256 token0Decimals = 18;
        // WETH has 18 decimals
        uint256 token1Decimals = 18;

        // Equal to `uint decimals0 = token0.decimals();`
        uint decimals0 = token0Decimals;
        // Equal to `uint decimals0 = token1.decimals();`
        uint decimals1 = token1Decimals;

        uint priceX8 = 10**decimals0;
        // It will now overflow due to the high `sqrtPriceX96`
        vm.expectRevert();
        priceX8 = priceX8 * uint(sqrtPriceX96ArbGMX / 2 ** 12) ** 2 * 1e8 / 2**168;
    }

}

Tools Used

Manual code review. Block explorers for Arbitrum and Ethereum Mainnets. Foundry.

Recommended Mitigation Steps

Consider using the Uniswap V3 FullMath.sol library in price calculations. This supports math in the 512 bit-space and handles overflows of intermediate values.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

In GeVault.sol, withdraw() allows a user to specify any token with a valid price feed to be withdrawn in exchange for GeVault liquidity. Due to the way withdrawal amounts are calculated, an attacker may be able to drain the non-underlying tokens from the vault in exchange for their liquidity tokens, with minimal loss of their own deposited assets.

The practical effect on the state would be a decrease in totalSupply, a decrease in the non-underlying token and no change in the getTVL() result (the same effect as burning the liquidity token).

The following edge cases must also be considered:

1. If the vault receives tokens as an airdrop or reward, which are not the underlying tokens, but has a valid price feed (for example, active addresses being airdropped ARB tokens), an attacker will be able to withdraw this asset in exchange for their liquidity. Due to the calculation logic, such a user will still get the majority of their underlying funds back on withdraw.

2. If an attacker manages to add a valid price feed for any of the TokenisableRange assets that the vault will hold, then the attacker may be able to drain the vault of the ticker assets. This would require external circumstances which is unlikely.

As the above requires non-underlying tokens to be sent to the GeVault, which it is not designed for, but is theoretically possible, this issue was evaluated to medium risk.

Proof of Concept

The code for the withdraw function is here:

  /// @notice Withdraw assets from the ticker
  /// @param liquidity Amount of GEV tokens to redeem; if 0, redeem all
  /// @param token Address of the token redeemed for
  /// @return amount Total token returned
  /// @dev For simplicity+efficieny, withdrawal is like a rebalancing, but a subset of the tokens are sent back to the user before redeploying
  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);
  }

We can see that if a user were to pass in any token address for the token argument, it will need to have a valid price feed from the Aave Oracle due to the getAssetPrice(token) check here:

    amount = valueX8 * 10**ERC20(token).decimals() / oracle.getAssetPrice(token);

The calculation of amount, as shown above, means that a user can withdraw any token as long as it has a valid price feed from the Oracle. Interestingly, the amount the user receives will still be in line with the actual liquidity token value. The effect of this sort of withdraw is that the user burns an amount of their liquidity tokens in exchange for the asset. But because the getTVL() does not change while the totalSupply decreases, the user can still withdraw the majority of the underlying assets they deposited into the vault due to this calculation:

    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;

This would mean that an attacker can profit from this bug if tokens with a valid price feed were to be transferred to the vault, as they would get the non-underlying token and most of their deposited tokens.

For example: State is:

totalSupply = 1000e18
TVL = 1000e8
BOB liquidity tokens = 500e18 
100e18 DAI has been sent to the vault
token0 is WETH
token1 is USDC.e

BOB calls withdraw with 100e18 liquidity

Assume no fees.

    uint vaultValueX8 = 1000e8;
    uint valueX8 = 1000e8 * 100e18 / 1000e18;
    uint valueX8 = 100e8
    //    amount = valueX8 * 10**ERC20(token).decimals() / oracle.getAssetPrice(token);
    amount = 100e8 * 1e18 / 1e8;
    amount = 100e18

We see that 100 DAI (with 18 decimals) is sent to BOB. The same amount of liquidity token was burnt. TVL stays the same as we did not move the underlying.

The state is now:

totalSupply = 900e18
TVL = 1000e8
BOB liquidity tokens = 400e18 
0 DAI in vault
token0 is WETH
token1 is USDC.e

BOB now withdraws his principal amount in USDC:

    uint vaultValueX8 = 1000e8;
    uint valueX8 = 1000e8 * 400e18 / 900e18;
    uint valueX8 = 44 444 444 444
    //    amount = valueX8 * 10**ERC20(token).decimals() / oracle.getAssetPrice(token);
    amount = 44 444 444 444 * 1e6 / 1e8;
    amount = 444.444444e6

We see that BOB has received 444.44 USDC, and he has withdrawn the 100 DAI.

BOB has made a profit, with a total of 544.44 USD worth of tokens.

Tools Used

Manual code review.

Recommended Mitigation Steps

Validate the token address specified in the withdraw function in the same way as for the deposit function.

Assessed type

Token-Transfer