MultiPositionLiquidityManager Manual Review Findings

MPL-01M: Inverse Check Application

Type	Severity	Location
Mathematical Operations		MultiPositionLiquidityManager.sol:L332, L336

Description:

The linked check is meant to validate that at least 95% of the token has been deposited, however, it presently validates that at least 5% of the token has been deposited as it evaluates whether the current post-deposit funds of the contract are less than 95% of the expected to deposit amount.

Impact:

The arbitrage protection imposed by the system is currently insufficient and possible to circumvent.

Example:

Copy
329if (swapAmount > 0) {
330    // Require that at least 95% of t1 has been deposited
331    // (ensuring that swap amount wasn't too great)
332    require(_getBalance1() < (t1ToDeposit * 95) / 100, "swap");
333} else if (swapAmount < 0) {
334    // Require that at least 95% of t0 has been deposited
335    // (ensuring that swap amount wasn't too great)
336    require(_getBalance0() < (t0ToDeposit * 95) / 100, "swap");
337}

Recommendation:

We advise the checks to be corrected by adjusting the 95 literal to 5, adequately representing 5% instead of 95%.

Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):

Both checks have been corrected to evaluate that the remaining balance is less than 5% of the original to-deposit funds, greatly enhancing the function's resilience against arbitrage attacks.

MPL-02M: Weak Relative Weight Evaluation

Type	Severity	Location
Logical Fault		MultiPositionLiquidityManager.sol:L371

Description:

The way liquidity is spread across multiple tick ranges is done via a relative weight system which denotes the "value" of a liquidity unit in relation to the current price of the pair and the tick range desired. This mechanism is weak as the assets per liquidity unit fluctuate non-linearly.

Impact:

The current relative weight-based system is flawed and does not actually distribute funds according to their weights.

Example:

Copy
366(uint256 amount0Wanted, uint256 amount1Wanted) = LiquidityAmounts
367    .getAmountsForLiquidity(
368        sqrtPriceX96,
369        TickMath.getSqrtRatioAtTick(_positions.lowerTick[i]),
370        TickMath.getSqrtRatioAtTick(_positions.upperTick[i]),
371        uint128(PRECISION * _positions.relativeWeight[i])
372        // No safecast here--an overflow will lead to an incorrect number,
373        // which will either (usually) revert, or cause a harmless liquidity miscalculation.
374    );

Recommendation:

We advise an alternative deposit mechanism to be utilized instead, potentially relying on a weight-based distribution of total available funds rather than liquidity units.

Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):

The Steer Protocol evaluated this exhibit and stated that this is the intended behaviour of the system as relative weight is not meant to be proportionate. As a result, we consider this exhibit nullified as it presents desirable behaviour as per the Steer Protocol team.

MPL-03M: Inexistent Sanitization of Position Validity

Type	Severity	Location
Logical Fault		MultiPositionLiquidityManager.sol:L115-L117

Description:

The new positions defined in the tend call are unsanitized which could render multiple vulnerabilities to arise from incorrect accounting such as exorbitant fees.

Impact:

Maliciously defined positions can compromise any funds that are deposited into the vault.

Example:

Copy
91function tend(
92    uint256 totalWeight,
93    LiquidityPositions memory newPositions,
94    bytes calldata timeSensitiveData
95) external onlyRole(ORCHESTRATOR_ROLE) whenNotPaused {
96    (int256 swapAmount, uint160 sqrtPriceLimitX96) = abi.decode(
97        timeSensitiveData,
98        (int256, uint160)
99    );
100
101    // Get current pool state
102    (uint160 sqrtPriceX96, int24 currentTick, , , , , ) = pool.slot0();
103
104    // currentTick must be close enough to TWAP tick to avoid MEV exploit
105    // This is essentially a way to prevent a flashloan attack
106    // even if sqrtPriceLimit is set incorrectly.
107    _checkVolatility(currentTick);
108
109    // Withdraw liquidity from Uniswap pool by passing in 1 and 1
110    // (indicating we're withdrawing 100% of liquidity)
111    _burnAndCollect(1, 1);
112
113    // Update positions if desired. If newPositions is empty,
114    // we'll just continue with the old positions instead.
115    if (newPositions.lowerTick.length > 0) {
116        positions = newPositions;
117    }

Recommendation:

We advise the tend function to disallow overlapping newPositions to be defined and to also ensure that the ticks represent a proper, incremental array of validly defined ranges. Overlapping liquidity ranges (i.e. the same form- and to- ticks defined multiple times in the newPositions array) can cause a wide array of misbehaviours in the system.

Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):

The code of _computePositionAmounts was updated to evaluate that the position tick ranges are in ascending order, however, overlapping is permitted. The Steer Protocol team stated that the burden of avoiding overlapping ranges will be placed on to the user as the protocol will behave correctly in such a case. As a result, we consider this exhibit adequately alleviated provided that overlapping ranges behave as the Steer Protocol team expects based on their dedicated issue responses.