Good Entry - R-Nemes's results

Lines of code

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

Vulnerability details

Impact

This vulnerability could lead to unexpected results or behaviors due to the overflow incident. In the worst-case scenario, this could potentially result in losses of funds, due to inaccurate fee calculation when claiming fees.

When converting to uint160 if the inner calculation is larger than type(uint160).max the value will be truncated returning an underestimated value.

Proof of Concept

In TokenisableRange.sol:337, when the balance of underlying assets based on the assets price is calculated, there is a potential for silent overflow.

uint160( sqrt( (2 ** 192 * ((TOKEN0_PRICE * 10 ** TOKEN1.decimals) / TOKEN1_PRICE)) / ( 10 ** TOKEN0.decimals ) )

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

  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));
    // @audit-issue silent overflow
    (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);
  }

function claimFee() public {
  (uint256 newFee0, uint256 newFee1) = POS_MGR.collect( 
    INonfungiblePositionManager.CollectParams({
      tokenId: tokenId,
      recipient: address(this),
      amount0Max: type(uint128).max,
      amount1Max: type(uint128).max
    })
  );
  // If there's no new fees generated, skip compounding logic;
  if ((newFee0 == 0) && (newFee1 == 0)) return;  
  uint tf0 = newFee0 * treasuryFee / 100;
  uint tf1 = newFee1 * treasuryFee / 100;
  if (tf0 > 0) TOKEN0.token.safeTransfer(treasury, tf0);
  if (tf1 > 0) TOKEN1.token.safeTransfer(treasury, tf1);
  
  fee0 = fee0 + newFee0 - tf0;
  fee1 = fee1 + newFee1 - tf1;
  
  // Calculate expected balance,  
---> (uint256 bal0, uint256 bal1) = returnExpectedBalanceWithoutFees(0, 0);
  
  // If accumulated more than 1% worth of fees, compound by adding fees to Uniswap position
  if ((fee0 * 100 > bal0 ) && (fee1 * 100 > bal1)) {
    TOKEN0.token.safeIncreaseAllowance(address(POS_MGR), fee0);
    TOKEN1.token.safeIncreaseAllowance(address(POS_MGR), fee1);
    (uint128 newLiquidity, uint256 added0, uint256 added1) = POS_MGR.increaseLiquidity(
      INonfungiblePositionManager.IncreaseLiquidityParams({
        tokenId: tokenId,
        amount0Desired: fee0,
        amount1Desired: fee1,
        amount0Min: 0,
        amount1Min: 0,
        deadline: block.timestamp
      })
    );
    // check slippage: validate against value since token amounts can move widely
    uint token0Price = ORACLE.getAssetPrice(address(TOKEN0.token));
    uint token1Price = ORACLE.getAssetPrice(address(TOKEN1.token));
    uint addedValue = added0 * token0Price / 10**TOKEN0.decimals + added1 * token1Price / 10**TOKEN1.decimals;
    uint totalValue =   bal0 * token0Price / 10**TOKEN0.decimals +   bal1 * token1Price / 10**TOKEN1.decimals;
    uint liquidityValue = totalValue * newLiquidity / liquidity;
    require(addedValue > liquidityValue * 95 / 100 && liquidityValue > addedValue * 95 / 100, "TR: Claim Fee Slippage"); // @audit-issue loss of precision
    fee0 -= added0;
    fee1 -= added1;
    liquidity = liquidity + newLiquidity; 
  }
  emit ClaimFees(newFee0, newFee1);
}

Tools Used

Manual Review Foundry Fuzz testing

Recommended Mitigation Steps

Adding safemath checks (or a more recent version of solidity's built-in overflow checks) for all arithmetic operations or use libraries known for handling such situations like OpenZeppelin's SafeMath library can help prevent these overflow issues.

Also use a larger value type such as uint256.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

The calculation of sqrt( (2 ** 192 * ((TOKEN0_PRICE * 10 ** TOKEN1.decimals) / TOKEN1_PRICE)) / ( 10 ** TOKEN0.decimals ) ) in the function returnExpectedBalanceWithoutFees can easily overflow cuasing the function to revert and thereby preventing users from claiming fees. This is most likely with low market cap tokens. The sponser has indicated that they would like to include low market cap tokens in the future.

Proof of Concept

In TokenisableRange.sol:337, when the balance of underlying assets based on the assets price is calculated, there is a potential for overflow, causing a revert.

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

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

  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));
    // @audit-issue silent overflow
    (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);
  }

https://github.com/code-423n4/2023-08-goodentry/blob/4b785d455fff04629d8675f21ef1d1632749b252/contracts/TokenisableRange.sol#L167-L215

function claimFee() public {
    (uint256 newFee0, uint256 newFee1) = POS_MGR.collect( 
      INonfungiblePositionManager.CollectParams({
        tokenId: tokenId,
        recipient: address(this),
        amount0Max: type(uint128).max,
        amount1Max: type(uint128).max
      })
    );
    // If there's no new fees generated, skip compounding logic;
    if ((newFee0 == 0) && (newFee1 == 0)) return;  
    uint tf0 = newFee0 * treasuryFee / 100;
    uint tf1 = newFee1 * treasuryFee / 100;
    if (tf0 > 0) TOKEN0.token.safeTransfer(treasury, tf0);
    if (tf1 > 0) TOKEN1.token.safeTransfer(treasury, tf1);
    
    fee0 = fee0 + newFee0 - tf0;
    fee1 = fee1 + newFee1 - tf1;
    
    // Calculate expected balance,  
    (uint256 bal0, uint256 bal1) = returnExpectedBalanceWithoutFees(0, 0);
    
    // If accumulated more than 1% worth of fees, compound by adding fees to Uniswap position
    if ((fee0 * 100 > bal0 ) && (fee1 * 100 > bal1)) { // @audit-info this checks for at least one percent on each token
      TOKEN0.token.safeIncreaseAllowance(address(POS_MGR), fee0);
      TOKEN1.token.safeIncreaseAllowance(address(POS_MGR), fee1);
      (uint128 newLiquidity, uint256 added0, uint256 added1) = POS_MGR.increaseLiquidity(
        INonfungiblePositionManager.IncreaseLiquidityParams({
          tokenId: tokenId,
          amount0Desired: fee0,
          amount1Desired: fee1,
          amount0Min: 0,
          amount1Min: 0,
          deadline: block.timestamp
        })
      );
      // check slippage: validate against value since token amounts can move widely
      uint token0Price = ORACLE.getAssetPrice(address(TOKEN0.token));
      uint token1Price = ORACLE.getAssetPrice(address(TOKEN1.token));
      uint addedValue = added0 * token0Price / 10**TOKEN0.decimals + added1 * token1Price / 10**TOKEN1.decimals;
      uint totalValue =   bal0 * token0Price / 10**TOKEN0.decimals +   bal1 * token1Price / 10**TOKEN1.decimals;
      uint liquidityValue = totalValue * newLiquidity / liquidity; // @audit-issue precision error
      require(addedValue > liquidityValue * 95 / 100 && liquidityValue > addedValue * 95 / 100, "TR: Claim Fee Slippage");
      fee0 -= added0;
      fee1 -= added1;
      liquidity = liquidity + newLiquidity;
    }
    emit ClaimFees(newFee0, newFee1);
  }

Tools Used

Manual Review Foundry Fuzz testing

Recommended Mitigation Steps

re formalate the equation to ensure no overflow.

Assessed type

Under/Overflow