Good Entry - HChang26's results

Lines of code

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

Vulnerability details

Impact

Remaining assets returned from deposit() is not returned to caller and remain unused in GeVault.

Proof of Concept

GeVault.sol manages TokenisableRange.sol tickers. Users can deposit() for a token with specific amount into TokenisableRange.sol.

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

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

After sanity checks and fee calculations, rebalance() will trigger which will then call removeAllTicks(). All assets are removed are from previous ticks.

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

All the removed asset will be deployed by deployAssets(), it checks the balance of token0 and token1 in GeVault. Followed by depositAndStash(), assets are deposit() on TokenisableRange.sol tickers. Not all tokens are guaranteed to be used. However, unused tokens are sent back to msg.sender on deposit(), which is GeVault in this case.

function deposit(uint256 n0, uint256 n1) external nonReentrant returns (uint256 lpAmt) {
     ...
 // New liquidity is indeed the amount of liquidity added, not the total, despite being unclear in Uniswap doc
    (uint128 newLiquidity, uint256 added0, uint256 added1) = POS_MGR.increaseLiquidity(
      INonfungiblePositionManager.IncreaseLiquidityParams({
        tokenId: tokenId,
        amount0Desired: n0,
        amount1Desired: n1,
        amount0Min: n0 * 95 / 100,
        amount1Min: n1 * 95 / 100,
        deadline: block.timestamp
      })
    );
    
    uint256 feeLiquidity;

    // Stack too deep, so localising some variables for feeLiquidity calculations 
    // If we already clawed back fees earlier, do nothing, else we need to adjust returned liquidity
    if ( newFee0 == 0 && newFee1 == 0 ){
      uint256 TOKEN0_PRICE = ORACLE.getAssetPrice(address(TOKEN0.token));
      uint256 TOKEN1_PRICE = ORACLE.getAssetPrice(address(TOKEN1.token));
      require (TOKEN0_PRICE > 0 && TOKEN1_PRICE > 0, "Invalid Oracle Price");
      // Calculate the equivalent liquidity amount of the non-yet compounded fees
      // Assume linearity for liquidity in same tick range; calculate feeLiquidity equivalent and consider it part of base liquidity 
      feeLiquidity = newLiquidity * ( (fee0 * TOKEN0_PRICE / 10 ** TOKEN0.decimals) + (fee1 * TOKEN1_PRICE / 10 ** TOKEN1.decimals) )   
                                    / ( (added0   * TOKEN0_PRICE / 10 ** TOKEN0.decimals) + (added1   * TOKEN1_PRICE / 10 ** TOKEN1.decimals) ); 
    }
                                     
    lpAmt = totalSupply() * newLiquidity / (liquidity + feeLiquidity); 
    liquidity = liquidity + newLiquidity;
    
    _mint(msg.sender, lpAmt);
    TOKEN0.token.safeTransfer( msg.sender, n0 - added0);
    TOKEN1.token.safeTransfer( msg.sender, n1 - added1);
    emit Deposit(msg.sender, lpAmt);
  }

GeVault does not have the mechanic to handle unused tokens sent back from tickers. A portion of the returned amount belongs to the user who called deposit() (rest belongs to others who contributed previously), and is essentially "lost". Until the next user calls deposit(), the tokens are just sitting in GeVault. Even then, there is still a good chance unused tokens are sent back which breaks the design of this contract(only meant to hold balances of tickers. It become an endless cycle.

Tools Used

Manual Review

Recommended Mitigation Steps

I am not sure what the best course of action here. Due to the fact that not all returned tokens belong to the user who called deposit(). There does not seem to be a fair way to distribute the tokens.

Assessed type

Context

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

Remaining assets after minting UniswapV3 position NFT is lost in RangeManager.sol instead of returning to the protocol.

Proof of Concept

TokenisableRange.sol is deployed and initiated by the protocol. In order to initiate a range/tick, generateRange() is called and immediately followed by initRange() by the protocol(owner in this case)

function generateRange(uint128 startX10, uint128 endX10, string memory startName, string memory endName, address beacon) external onlyOwner {
    require(beacon != address(0x0), "Invalid beacon");
    checkNewRange(startX10, endX10);
    stepList.push( Step(startX10, endX10) );
    BeaconProxy trbp = new BeaconProxy(beacon, "");
    tokenisedRanges.push( TokenisableRange(address(trbp)) );
    trbp = new BeaconProxy(beacon, "");
    tokenisedTicker.push( TokenisableRange(address(trbp)) );
    IAaveOracle oracle = IAaveOracle(ILendingPoolAddressesProvider( LENDING_POOL.getAddressesProvider() ).getPriceOracle());
    
    tokenisedRanges[ tokenisedRanges.length - 1 ].initProxy(oracle, ASSET_0, ASSET_1, startX10, endX10, startName, endName, false);
    tokenisedTicker[ tokenisedTicker.length - 1 ].initProxy(oracle, ASSET_0, ASSET_1, startX10, endX10, startName, endName, true); 
    emit AddRange(startX10, endX10, tokenisedRanges.length - 1);
  }

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

initRange() will then call init() on TokenisableRange.sol to mint a position on Uniswap.

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

Unused token0 and token1 are transferred back to the msg.sender which is RangeManager.sol in this case. However, remaining assets are not sent back to protocol. First user to call transferAssetsIntoRangerStep() will receive the remaining assets, as it will be deposited in cleanUp() on behalf of the first user. Although remaining assets may be insignificant, but it does add up with multiple ranges across different pairs.

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

Tools Used

Manual Review

Recommended Mitigation Steps

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)));
+   uint256 bal0 = ASSET_0.balanceOf(address(this));
+   uint256 bal1 = ASSET_1.balanceOf(address(this));
+   If( bal0 > 0){
+   ASSET_0.safeTransfer(msg.sender, bal0);
+   }
+   If( bal1 > 0){
+   ASSET_1.safeTransfer(msg.sender, bal0);
+   }
  }

Assessed type

Token-Transfer