BaseInterface.sol

The BaseInterface is an abstract contract that defines the required functions that must be implemented by a YieldInterface. The required functions of the YieldManager are all referenced and supported.

There are a number of issues that present themselves across all inheriting YieldInterfaces, so to prevent repetition they have been included in this BaseInterface report. These issues have been highlighted with an additional [INHERIT] tag.

[MEDIUM] [INHERIT] Missing Slippage Protection in Swaps

During the claim call, the YieldInterface converts the non-native token into the native token by swapping against the pool of which the liquidity position is tied. The calls made on each YieldInterface use a rudimentary approach to just detect a non-zero value is received in the native token from the swap.

This offers virtually no slippage protection which could lead to significant value loss, making the protocol vulnerable to MEV attacks and sandwich attacks. This issue is offset somewhat by the ability for only specified claimants to be able to claim, but this still leaves room for attacks.

There are two approaches that we could use to remediate this:

1. Allow for a amountOutMinimum to be passed as a parameter. This will put the onus on the frontend integration to handle this call, but will provide a simple implementation.
2. Add onchain calculations to determine a TWAP value in the _swapNonNativeToken itself. This value can then be reduced by 10% to facilitate an acceptable slippage amount and used as the amountOutMinimum.

In our opinion, integrations would be much simpler for end users if the value was calculated on chain. Uniswap V3 implementations should be much easier to calculate due to built-in pool observe functions. Uniswap V4 does not have built-in oracle functionality, so it would require additional logic which we have explored on its individual report page.

Note: This issue may be self mitigating through arbitrage opportunities if the fees are left open to be claimed by anyone as small gains will be made without impacting too highly.

Affected code:

This code example is taken from the UniswapV3YieldInterface.

function _swapNonNativeToken(uint _tokenId, address _nonNativeCurrency, uint _balance) internal virtual returns (uint ethReceived_) {
  // ...

  ethReceived_ = swapRouter.exactInputSingle(
    ISwapRouter.ExactInputSingleParams({
      tokenIn: _nonNativeCurrency,
      tokenOut: zeroForOne ? pool.token1() : pool.token0(),
      fee: pool.fee(),
      recipient: address(this),
      amountIn: _balance,
      amountOutMinimum: 1,  // @audit Too low!
      sqrtPriceLimitX96: zeroForOne ? TickMath.MIN_SQRT_PRICE + 1 : TickMath.MAX_SQRT_PRICE - 1
    })
  );
}

Recommended Uniswap V3 solution:

For Uniswap V3 (and through inheritance Aerodrome and PancakeSwap V3) we can implement a slippage calculation through using the OracleLibrary. The uniswap/v3-periphery library is not currently installed in the project, but it can be added as an isolated contract. From experience, there may be some integer casting issues introduced from the project using a more recent solc version. Some small updates may be required.

function _swapNonNativeToken(uint _tokenId, address _nonNativeCurrency, uint _balance) internal virtual returns (uint ethReceived_) {
  // ...

  ethReceived_ = swapRouter.exactInputSingle(
    ISwapRouter.ExactInputSingleParams({
      tokenIn: _nonNativeCurrency,
      tokenOut: zeroForOne ? pool.token1() : pool.token0(),
      fee: pool.fee(),
      recipient: address(this),
      amountIn: _balance,
      amountOutMinimum: _calculateMinimumAmountOut(pool, zeroForOne, _balance),  // @audit This now handles a 10% slippage
      sqrtPriceLimitX96: zeroForOne ? TickMath.MIN_SQRT_PRICE + 1 : TickMath.MAX_SQRT_PRICE - 1
    })
  );
}

/// @audit Introduces a slippage safe expected amount to receive from the swap using TWAP
function _calculateMinimumAmountOut(IUniswapV3Pool _pool, bool _zeroForOne, uint _amountIn) internal view returns (uint amountOutMinimum_) {
  // Get the tick and seconds per liquidity cumulative over the last 180 seconds
  (int24 arithmeticMeanTick,) = OracleLibrary.consult(address(_pool), 180);

  // Calculate the expected amount out using the current price
  uint expectedAmountOut = OracleLibrary.getQuoteAtTick({
    tick: arithmeticMeanTick,
    baseAmount: uint128(_amountIn),
    baseToken: _zeroForOne ? _pool.token0() : _pool.token1(),
    quoteToken: _zeroForOne ? _pool.token1() : _pool.token0()
  });

  // Apply 10% slippage tolerance (90% of expected amount)
  amountOutMinimum_ = (expectedAmountOut * 90) / 100;

  // Ensure we have a minimum of 1 to prevent zero amounts
  if (amountOutMinimum_ == 0) {
    amountOutMinimum_ = 1;
  }
}

[LOW] Missing Input Validation

The constructor function does not validate the input parameters. Although these would be defined in a structured deployment script, it would be beneficial to add in checks.

Affected Code:

constructor (address _yieldManager, address[] memory _nativeTokens) {
    nativeTokens = _nativeTokens;
    yieldManager = _yieldManager;
}

Recommended solution:

constructor (address _yieldManager, address[] memory _nativeTokens) {
    require(_yieldManager != address(0), "Invalid yield manager");
    require(_nativeTokens.length > 0, "Native tokens array cannot be empty");
    
    nativeTokens = _nativeTokens;
    yieldManager = _yieldManager;
}

[INFO] [INHERIT] Third Party contracts address references are immutable

All of the YieldInterface contracts specify third-party contracts as immutable in the constructor. It is unlikely that any of the current PositionManager implementations will change, but we would recommend normalising an update path for the swap routers, especially for the Uniswap V4 PoolSwap, as this is not an official UniswapV4 router.

[INFO] [INHERIT] YieldInterface can only support a single YieldManager

Not an issue itself, but if future YieldManager contracts were deployed, then additional interfaces would be required to be deployed. If you imagine that additional YieldManager contracts will be deployed, whilst using the same YieldInterface then either a YieldManagerRegistry or per-interface EnumerableSet of valid YieldManager addresses should be introduced.

Affected code:

/**
 * Modifier to ensure that the function is only called by the {YieldManager}.
 */
modifier onlyYieldManager() {
  if (msg.sender != address(yieldManager)) {
    revert NotYieldManager();
  }
  _;
}

Recommended solution:

/**
 * Modifier to ensure that the function is only called by an approved {YieldManager}.
 */
modifier onlyYieldManager() {
  if (!validYieldManagers.contains(msg.sender)) {
    revert NotYieldManager();
  }
  _;
}

[INFO] [INHERIT] Code commenting in isValid is not accurate

The Position struct returns an exists boolean value, rather than any information regarding tickSpacing or tickLower. This looks like commenting that has not been updated following a change and should be updated for readability of the codebase functionality.

Affected code:

// Check that one of the currencies is a native token and that there is a valid tickLower, otherwise
// the PoolKey could be unknown. If both tokens are native tokens, then the position is not valid. We
// don't have easy access to the `tickSpacing`, so `tickLower` is more gas efficient.

Recommended solution:

// Check that one of the currencies is a native token and that the position exists