Lines of code

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

Vulnerability details

GeVault offers admins the modifyTick method to replace a ticker in its list. Despite being deployed behind a proxy, TokenisableRange instances don't have an upgradable implementation, so to upgrade the TokenisableRange code backing GeVault, modifyTick is the only option. Once modifyTick completes execution, the TokenisableRange liquidity tokens owned by GeVault can no longer be rescued because GeVault loses its reference to the removed TokenisableRange.

Impact

In the worst case scenario, a vulnerability is discovered in the TokenisableRange contract. The admin is forced to make the tough choice between keeping the vulnerability in the protocol, or locking the funds deployed by GeVault in one or more, if not all, of the referenced TokenisableRange contracts.

Proof of Concept

Start with a GeVault contract with 4 tickers and assets deployed on all of them
Call getTVL() to record the total value locked - this is expected to be invariant
Have modifyTick() replace one of the tickers
Call getTVL() again to verify that part of the value locked in the contract has been lost

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider having GeVault withdraw its assets from the to-be-removed TokenisableRange instance before its address is deleted from storage:

  /// @notice Modify ticker
  /// @param tr New tick address
  /// @param index Tick to modify
  function modifyTick(address tr, uint index) public onlyOwner {
    (ERC20 t0,) = TokenisableRange(tr).TOKEN0();
    (ERC20 t1,) = TokenisableRange(tr).TOKEN1();
    require(t0 == token0 && t1 == token1, "GEV: Invalid TR");
+    removeFromTick(index, true);
    ticks[index] = TokenisableRange(tr);
    emit ModifyTick(tr, index);
  }

Also, it would be important to consider adding an extra parameter for stricter validation, because unlike the other currently-implemented ticker withdrawals where the tokens lended by the Roe pool are not withdrawn from the ticker, the full "aToken" balance must be removed to not be lost:

-  function removeFromTick(uint index) internal {
+  function removeFromTick(uint index, boolean requireFullBalance) 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 > sBal && !requireFullBalance) aBal = sBal;
    if (aBal > 0){
      lendingPool.withdraw(address(tr), aBal, address(this));
      tr.withdraw(aBal, 0, 0);
    }
  }

Assessed type

Uniswap

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

Missing withdrawal in GeVault's modifyTick can cause GeVault to lock assets in discarded TokenisableRange instances #19