UniswapInteractionHelper Manual Review Findings

UIH-01M: Incorrect Tick Validation

Type	Severity	Location
Logical Fault		UniswapInteractionHelper.sol:L278, L282-L283

Description:

The isTicksValid function will evaluate whether the supplied ticks are also wholly divisible by the tickSpacing value via a modulo (%) operation, however, the ticks are "floored" in calculations after the tick validation is performed.

Impact:

The tick validation mechanism may fail for properly defined ticks if the baseTicks value is not wholly divisible by the tickSpacing value.

Example:

Copy
260if (zeroGreaterOne) {
261    uint160 nextSqrt0 = SqrtPriceMath.getNextSqrtPriceFromAmount0RoundingUp(
262        sqrtPriceX96,
263        liquidity,
264        amount0Max,
265        false
266    );
267    ticks.tickUpper = floor(TickMath.getTickAtSqrtRatio(nextSqrt0), tickSpacing);
268} else {
269    uint160 nextSqrt1 = SqrtPriceMath.getNextSqrtPriceFromAmount1RoundingDown(
270        sqrtPriceX96,
271        liquidity,
272        amount1Max,
273        false
274    );
275    ticks.tickLower = floor(TickMath.getTickAtSqrtRatio(nextSqrt1), tickSpacing);
276}
277
278if (!isTicksValid(ticks.tickLower, ticks.tickUpper, tickSpacing)) {
279    revert Errors.TicksOutOfRange();
280}
281
282ticks.tickLower = floor(ticks.tickLower, tickSpacing);
283ticks.tickUpper = floor(ticks.tickUpper, tickSpacing);

Recommendation:

We advise either the isTicksValid check to be relocated after the floor operations or the floor operation of the remaining tick to be included in each if / else clause of the conditional structure that precedes the isTicksValid function execution.

Alleviation:

The tick validation mechanism was relocated after the flooring operations take place, ensuring the validation is performed on finalized data points.

UIH-02M: Unsafe Casting Operation

Type	Severity	Location
Mathematical Operations		UniswapInteractionHelper.sol:L41

Description:

The burnLiquidity function will calculate the proportionate liquidity to remove based on the shares of the totalSupply, however, when the actual liquidityToRemove is passed in to the burn function of IUniswapV3Pool, it is cast to a uint128 value unsafely.

Impact:

Based on the implementation of the IUniswapV3Pool itself as well as the getLiquidityInPosition function, the liquidityToRemove cannot exceed the uint128 bound unless the shares passed in to the burnLiquidity function exceed the totalSupply which is permitted by functions such as MetavisorManagedVault::withdraw. While in the current implementation the MetavisorManagedVault::withdraw function will fail at a later point, arithmetic errors such as the one identified should not be permitted to occur.

Example:

Copy
33uint256 liquidityToRemove = everything
34    ? liquidity
35    : FullMath.mulDiv(liquidity, shares, totalSupply);
36
37if (liquidity > 0) {
38    (amount0, amount1) = pool.burn(
39        ticks.tickLower,
40        ticks.tickUpper,
41        uint128(liquidityToRemove)
42    );

Recommendation:

We advise the casting operation to be performed safely by ensuring that the value of liquidityToRemove does not exceed the upper bound of the uint128 data type (type(uint128).max). We should note that Solidity's built-in safe arithmetic in pragma versions 0.8.X does not accommodate for casting operations and as such they need to be manually protected.

Alleviation:

The MetavisorManagedVault::withdraw function now sanitizes the input shares amount to prevent it exceeding the totalSupply supplied to the burnLiquidity function of UniswapInteractionHelper which is affected by this vulnerability. As a result, a casting overflow is no longer possible rendering this exhibit alleviated.

UIH-03M: Incorrect Calculation of Next Price

Type	Severity	Location
Mathematical Operations		UniswapInteractionHelper.sol:L258, L260-L276

Description:

The referenced calculations for assessing the optimal tick to supply liquidity to are incorrect as they simulate the price movement of a "swap" of the full amount of amount0Max / amount1Max whilst a portion of this amount will be supplied as liquidity instead with a non-zero paired amount1 / amount0 value.

Impact:

The current price-point assessments are ineffectual and thus can cause the system to misallocate liquidity and diminish its potential returns.

Example:

Copy
258bool zeroGreaterOne = swapDirection(amount0Max, amount1Max, amount0, amount1);
259
260if (zeroGreaterOne) {
261    uint160 nextSqrt0 = SqrtPriceMath.getNextSqrtPriceFromAmount0RoundingUp(
262        sqrtPriceX96,
263        liquidity,
264        amount0Max,
265        false
266    );
267    ticks.tickUpper = floor(TickMath.getTickAtSqrtRatio(nextSqrt0), tickSpacing);
268} else {
269    uint160 nextSqrt1 = SqrtPriceMath.getNextSqrtPriceFromAmount1RoundingDown(
270        sqrtPriceX96,
271        liquidity,
272        amount1Max,
273        false
274    );
275    ticks.tickLower = floor(TickMath.getTickAtSqrtRatio(nextSqrt1), tickSpacing);
276}

Recommendation:

We advise the code to instead simulate the price movement of amount0Max - amount0 / amount1Max - amount1, ensuring that the over-abundant token supply is simulated rather than the whole matched supply and increasing the accuracy of the next price calculations. Additionally, we advise the zeroGreaterOne variable to be aptly renamed to zeroForOne similarly to the UniswapV3 paradigm to ensure consistency across the codebases.

Alleviation:

After extensive discussions with the Metavisor team and supplemental tests highlighted from them, we have concluded that our initial understanding of the formula was incorrect. The code (which behaves as expected) attempts to identify the best tick range to supply assets in that will minimize the excess assets we are left with by calculating the "edge" price that the pair would land in if all the liquidity of the non-excess asset is consumed. Based on this approach, the code is sound and we consider this exhibit nullified.

UIH-04M: Incorrect Price Impact Assessment

Type	Severity	Location
Language Specific		UniswapInteractionHelper.sol:L79, L81-L83

Description:

The swapToken function attempts to assess the "maximum" price impact limit to pass in to the swap call of the IUniswapV3Pool, however, the calculation to assess the impact is done entirely on-chain on an arbitrary 0.5% percentage.

Impact:

Currently, any rescale operation is fully susceptible to an on-chain sandwich attack (or any other slippage related attack vector).

Example:

Copy
76function swapToken(IUniswapV3Pool pool, bool zeroForOne, int256 amountSpecified) internal {
77    (uint160 sqrtPriceX96, , , , , , ) = pool.slot0();
78
79    uint160 exactSqrtPriceImpact = (sqrtPriceX96 * (1e5 / 2)) / 1e6;
80
81    uint160 sqrtPriceLimitX96 = zeroForOne
82        ? sqrtPriceX96 - exactSqrtPriceImpact
83        : sqrtPriceX96 + exactSqrtPriceImpact;
84
85    pool.swap(address(this), zeroForOne, amountSpecified, sqrtPriceLimitX96, "");
86}

Recommendation:

We advise the swapToken function's call-chain (MetavisorManagedVault::rescale) to accept the price limit for the swap as an input argument, permitting the registry-authorized msg.sender to specify a price limit for the swap. While this does entrust the caller of the rescale function to supply a valid price limit, the authorized actor will not achieve a net benefit if they act maliciously as they will be removed from the registry, will be identifiable, and will achieve little gain.

Alleviation:

The code was refactored to pass in a maxPriceImpact argument to swapToken via MetavisorManagedVault::rescale, ensuring that the maximum tolerable price impact is passed in using an off-chain assessment rather than on-chain assessment of prices.

UIH-05M: Incorrect Price Utilization

Type	Severity	Location
Mathematical Operations		UniswapInteractionHelper.sol:L146

Description:

The sqrtPriceX96 that is stored within slot0 of a Uniswap V3 pool is not strictly associated with the current tick of the pool as a tick is valid for a range of prices; the square ratio generated at a particular tick, however, is associated in a one-to-one fashion with it.

Impact:

As the code extracts the active sqrtPriceX96 instead of the price associated with the active tick, it is possible to manipulate the price trivially in one direction or the other within the tick which may also bypass secondary protection measures such as TWAPs based on ticks.

Example:

Copy
125function computeShares(
126    IUniswapV3Pool pool,
127    uint256 amount0Max,
128    uint256 amount1Max,
129    uint256 balance0,
130    uint256 balance1,
131    uint256 totalSupply,
132    TicksData memory ticks
133) internal returns (uint256 shares) {
134    (uint256 amount0, uint256 amount1, uint256 fees0, uint256 fees1, ) = getReserves(
135        pool,
136        ticks
137    );
138
139    uint256 reserve0 = amount0 + fees0 + balance0;
140    uint256 reserve1 = amount1 + fees1 + balance1;
141
142    if (totalSupply > 0) {
143        assert(reserve0 != 0 || reserve1 != 0);
144    }
145
146    (uint160 sqrtPriceX96, , , , , , ) = pool.slot0();
147    uint256 price = FullMath.mulDiv(
148        uint256(sqrtPriceX96) ** 2,
149        PRECISION_FACTOR,
150        2 ** (96 * 2)
151    );
152
153    shares = amount1Max + (amount0Max * price) / PRECISION_FACTOR;
154
155    if (totalSupply != 0) {
156        uint256 reserve0PricedInToken1 = (reserve0 * price) / PRECISION_FACTOR;
157        shares = (shares * totalSupply) / (reserve0PricedInToken1 + reserve1);
158    }
159}

Recommendation:

We advise the code to be revamped to utilize the current tick rather than the actual sqrtPriceX96, ensuring that the code is not susceptible to in-tick price manipulation attacks.

Alleviation:

The square ratio at the current tick of the pool is now dynamically calculated instead of being fetched directly from the pool, preventing spot-price manipulation attacks from being possible.

UIH-06M: Flash-Loan Price Manipulation Susceptibility

Type	Severity	Location
Logical Fault		UniswapInteractionHelper.sol:L146

Description:

The computeShares function utilizes the current price of the UniswapV3 pool which is susceptible to price manipulations, permitting its users to artificially inflate the asset prices and acquire disproportionate shares.

Impact:

As the code contains no protection against an artificial price, users are able to skew the price of the UniswapV3 pool towards the least expensive asset of a pair, deposit it, skew it towards the expensive asset of the pair, and ultimately perform a withdrawal at the expense of its users.

Example:

Copy
125function computeShares(
126    IUniswapV3Pool pool,
127    uint256 amount0Max,
128    uint256 amount1Max,
129    uint256 balance0,
130    uint256 balance1,
131    uint256 totalSupply,
132    TicksData memory ticks
133) internal returns (uint256 shares) {
134    (uint256 amount0, uint256 amount1, uint256 fees0, uint256 fees1, ) = getReserves(
135        pool,
136        ticks
137    );
138
139    uint256 reserve0 = amount0 + fees0 + balance0;
140    uint256 reserve1 = amount1 + fees1 + balance1;
141
142    if (totalSupply > 0) {
143        assert(reserve0 != 0 || reserve1 != 0);
144    }
145
146    (uint160 sqrtPriceX96, , , , , , ) = pool.slot0();
147    uint256 price = FullMath.mulDiv(
148        uint256(sqrtPriceX96) ** 2,
149        PRECISION_FACTOR,
150        2 ** (96 * 2)
151    );
152
153    shares = amount1Max + (amount0Max * price) / PRECISION_FACTOR;
154
155    if (totalSupply != 0) {
156        uint256 reserve0PricedInToken1 = (reserve0 * price) / PRECISION_FACTOR;
157        shares = (shares * totalSupply) / (reserve0PricedInToken1 + reserve1);
158    }
159}

Recommendation:

We advise the code's execution to be protected by a TWAP mechanism or similar protection measure as it is presently insecure and can compromise user assets.

Alleviation:

The isTwapWithinThreshold and getSqrtRatioX96AtInterval functions were newly introduced to the UniswapInteractionHelper contract and are now in use by the MetavisorManagedVault::deposit, MetavisorManagedVault::withdraw, MetavisorManagedVault::compound, and MetavisorManagedVault::rescale functions of the vault system. Given that all externally facing functions are protected at the vault level via a TWAP mechanism, we consider all computeShares calculations to be safely performed thus alleviating this exhibit.