QuickSwapBaseLiquidityManager Manual Review Findings

QSB-01M: Inexistent Sanitization of Liquidity Provision

Type	Severity	Location
Input Sanitization		QuickSwapBaseLiquidityManager.sol:L385, L386

Description:

A QuickSwapBaseLiquidityManager::deposit operation is expected to have a non-zero value for either the amount0Desired or the amount1Desired variables, however, no such sanitization is performed.

Impact:

While such errors would be indirectly caught by the non-zero shares and cross restrictions in QuickSwapBaseLiquidityManager::deposit and QuickSwapBaseLiquidityManager::_calcSharesAndAmounts respectively, it is still advisable to introduce these checks as a matter of consistency.

Example:

Copy
384function deposit(
385    uint256 amount0Desired,
386    uint256 amount1Desired,
387    uint256 amount0Min,
388    uint256 amount1Min,
389    address to
390)
391    public
392    virtual
393    whenNotPaused
394    returns (uint256 shares, uint256 amount0Used, uint256 amount1Used)
395{

Recommendation:

We advise the code to ensure either of those arguments is non-zero, ensuring consistency in the way the QuickSwapBaseLiquidityManager::deposit function is potentially invoked.

Alleviation (0c3f85c7c11805ac412fe291f5681bef26da7244):

The Steer Finance team evaluated this exhibit and opted not to apply it given its informational nature and the contract's close proximity to the bytecode size limit which would render the contract un-deployable.

Given that we have specified the core error of the exhibit would be indirectly caught and thus prevented, we consider it safely acknowledged.

QSB-02M: First-Deposit Share Manipulation

Type	Severity	Location
Logical Fault		QuickSwapBaseLiquidityManager.sol:L480-L484

Description:

The QuickSwapBaseLiquidityManager system's first deposit acts as a "configurational" deposit which will affect the vault's token0 to token1 asset ratio, however, a significant vulnerability can arise from this.

Specifically, a malicious user can perform a first deposit of an abysmal amount (i.e. 1 wei of amount0Used) which would in turn cause 1 wei of shares to be minted. Afterwards, the user can directly transfer a significant amount of assets to the QuickSwapBaseLiquidityManager. Any consequent QuickSwapBaseLiquidityManager::deposit invocations with an amount less than the actual amount held by the contract would cause 0 shares to be minted, causing these operations to fail.

Impact:

As a non-zero shares check is present in the QuickSwapBaseLiquidityManager::deposit function implementation, asset compromise is not possible and this particular vulnerability can solely lead to an unusable QuickSwapBaseLiquidityManager.

As such, we consider it to be a "minor" severity exhibit.

Example:

Copy
466function _calcSharesAndAmounts(
467    uint256 amount0Desired,
468    uint256 amount1Desired
469)
470    internal
471    view
472    returns (uint256 shares, uint256 amount0Used, uint256 amount1Used)
473{
474    uint256 _totalSupply = totalSupply();
475    (uint256 total0, uint256 total1) = getTotalAmounts();
476
477    // If total supply > 0, vault can't be empty.
478    assert(_totalSupply == 0 || total0 > 0 || total1 > 0);
479
480    if (_totalSupply == 0) {
481        // For first deposit, just use the amounts desired
482        amount0Used = amount0Desired;
483        amount1Used = amount1Desired;
484        shares = Math.max(amount0Used, amount1Used);
485    } else if (total0 == 0) {

Recommendation:

We advise the first QuickSwapBaseLiquidityManager::deposit operation to mandate a minimum non-zero shares amount that is significant, similarly to how Uniswap V2 trading pairs pre-mint a set LP supply to the zero-address. For more information on this particular vulnerability, consult TOB-YEARN-003.

Alleviation (0c3f85c7c11805ac412fe291f5681bef26da7244):

A minimum of 1_000_000 shares is imposed (improperly set as 100_00_00) in the updated QuickSwapBaseLiquidityManager::_calcSharesAndAmounts function thus alleviating this exhibit.

QSB-03M: Insufficient Protection of Algebra Callbacks

Type	Severity	Location
Logical Fault		QuickSwapBaseLiquidityManager.sol:L209, L220

Description:

The QuickSwapBaseLiquidityManager::algebraMintCallback and QuickSwapBaseLiquidityManager::algebraSwapCallback functions do not validate the state of the liquidity manager in relation to the action being performed. In detail, a QuickSwapBaseLiquidityManager::algebraMintCallback function call should be permitted solely when the QuickSwapBaseLiquidityManager mints new liquidity units on the Algebra ecosystem and the QuickSwapBaseLiquidityManager::algebraSwapCallback function call should be permitted solely when the QuickSwapBaseLiquidityManager performs swaps.

Impact:

This particular vulnerability should only manifest if the QuickSwapBaseLiquidityManager is upgraded to contain arbitrary calls or malicious token implementations are used, both of which we consider unlikely scenarios. As such, we consider this exhibit to be of "minor" severity.

Example:

Copy
203/// @dev Callback for Uniswap V3 pool.
204function algebraMintCallback(
205    uint256 amount0,
206    uint256 amount1,
207    bytes calldata /* data */
208) external override {
209    require(msg.sender == address(pool));
210
211    _transferTokens(msg.sender, amount0, amount1);
212}

Recommendation:

We advise two contract-level bool variables to be introduced that are being validated via require checks in the referenced callback functions and are "consumed" directly within them, preventing unauthorized invocations of these functions due to f.e. malicious token implementations.

Alleviation (0c3f85c7c1):

A callBackProtection variable has been introduced to the codebase that is expected to be true whenever either QuickSwapBaseLiquidityManager::algebraMintCallback or QuickSwapBaseLiquidityManager::algebraSwapCallback is invoked and is consequently set to false within each.

We consider this exhibit partially alleviated as a bool flag has been introduced yet is not unique per callback function signature. As an additional point, we would advise the Steer Protocol team to introduce a visibility specifier for the variable as well.

Alleviation (b1b5eabd4d):

The Uniswap V3 callback protection bool has been split into two dedicated flags, mintCallBackProtection and swapCallBackProtection, ensuring that the callback restrictions are applied per-call. As such, we consider this exhibit fully alleviated.

QSB-04M: Insufficient TWAP Sanitization

Type	Severity	Location
Input Sanitization		QuickSwapBaseLiquidityManager.sol:L332

Description:

The QuickSwapBaseLiquidityManager::initialize function will ensure that the _twapInterval configuration of the contract is between 5 seconds and 600 seconds in the past, the former of which is a weak restriction.

While the QuickSwapBaseLiquidityManager is expected to be deployed on a non-ETH network, the main criteria for choosing an appropriate TWAP period are the number of blocks in the period as well as the activity of the trading pair in the said period.

Alternative EVM networks will contain more than 2 blocks within 5 seconds which is adequate, however, they may not have sufficient trading activity thus rendering manipulations in such short time windows plausible and profitable.

Impact:

An insecure TWAP window can cause the price measurements of the QuickSwapBaseLiquidityManager to be manipulated, leading to incorrect volatility measurements.

Example:

Copy
332require(_twapInterval > 5 && _twapInterval < 600);

Recommendation:

We advise a sensible limitation to be imposed on the trading pairs, preferably in the 30 second range, to ensure that the TWAP periods over which price estimations are performed are secure.

Alleviation (0c3f85c7c11805ac412fe291f5681bef26da7244):

The minimum _twapInterval permitted by the contract has been increased to 30, ensuring that TWAP window specified contains sufficient trading activity to justify its measurement.