AlgebraBaseLiquidityManager Manual Review Findings

ABL-01M: Incorrect Gap Size Specification

Type	Severity	Location
Standard Conformity		AlgebraBaseLiquidityManager.sol:L642

Description:

The gap member of the AlgebraBaseLiquidityManager is improperly specified as it does not adhere to the OpenZeppelin convention in contrast to what its comment denotes.

Example:

Copy
69/// @dev Address of vault registry
70///      Address strategist can collect strategist fees, but is not stored here.
71address internal vaultRegistry;
72
73/// @notice Addresses of Token0 and Token1
74IERC20 public token0;
75IERC20 public token1;
76
77/// @dev Fees currently owed to Steer
78uint256 public accruedSteerFees0;
79uint256 public accruedSteerFees1;
80
81/// @dev Fees currently owed to strategist
82uint256 public accruedStrategistFees0;
83uint256 public accruedStrategistFees1;
84
85/// @notice Address of Uniswap V3 pool
86IAlgebraPool public pool;
87
88///@notice Address of DataStorage Contract of Algebra pool
89address public algebraDataStorage;
90
91/// @dev For depositing
92/// Roughly corresponds to a 5% diff between current price and twap price
93int24 public maxTickChange;
94
95/// @dev Number of seconds to get the time-weighted average over
96int56 public twapInterval;

Recommendation:

The total storage slots used by the AlgebraBaseLiquidityManager are 11, meaning that the contract's gap should be set to 39 in order for it to properly comply with the OpenZeppelin convention.

We advise this to be done so, standardizing the storage space of the contract.

Alleviation (0c3f85c7c11805ac412fe291f5681bef26da7244):

The total storage slot padding space has been corrected to 39 from 40, addressing this exhibit.

ABL-02M: Inexistent Sanitization of Liquidity Provision

Type	Severity	Location
Input Sanitization		AlgebraBaseLiquidityManager.sol:L383, L384

Description:

A AlgebraBaseLiquidityManager::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 AlgebraBaseLiquidityManager::deposit and AlgebraBaseLiquidityManager::_calcSharesAndAmounts respectively, it is still advisable to introduce these checks as a matter of consistency.

Example:

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

Recommendation:

We advise the code to ensure either of those arguments is non-zero, ensuring consistency in the way the AlgebraBaseLiquidityManager::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.

ABL-03M: First-Deposit Share Manipulation

Type	Severity	Location
Logical Fault		AlgebraBaseLiquidityManager.sol:L474-L479

Description:

The AlgebraBaseLiquidityManager 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 AlgebraBaseLiquidityManager. Any consequent AlgebraBaseLiquidityManager::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 AlgebraBaseLiquidityManager::deposit function implementation, asset compromise is not possible and this particular vulnerability can solely lead to an unusable AlgebraBaseLiquidityManager.

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

Example:

Copy
474if (_totalSupply == 0) {
475    // For first deposit, just use the amounts desired
476    amount0Used = amount0Desired;
477    amount1Used = amount1Desired;
478    shares = Math.max(amount0Used, amount1Used);
479} else if (total0 == 0) {

Recommendation:

We advise the first AlgebraBaseLiquidityManager::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 AlgebraBaseLiquidityManager::_calcSharesAndAmounts function thus alleviating this exhibit.

ABL-04M: Insufficient Protection of Algebra Callbacks

Type	Severity	Location
Logical Fault		AlgebraBaseLiquidityManager.sol:L200-L209, L211-L225

Description:

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

Impact:

This particular vulnerability should only manifest if the AlgebraBaseLiquidityManager 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
200/// @dev Callback for Uniswap V3 pool.
201function algebraMintCallback(
202    uint256 amount0,
203    uint256 amount1,
204    bytes calldata /* data */
205) external override {
206    require(msg.sender == address(pool));
207
208    _transferTokens(msg.sender, amount0, amount1);
209}

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 AlgebraBaseLiquidityManager::algebraMintCallback or AlgebraBaseLiquidityManager::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.

ABL-05M: Unsafe Casting Operation

Type	Severity	Location
Mathematical Operations		AlgebraBaseLiquidityManager.sol:L613

Description:

The Algebra system that the AlgebraBaseLiquidityManager integrates with supports liquidity value representations up to 256-bits in accuracy, meaning that treating them as uint128 is incorrect. The AlgebraBaseLiquidityManager::_getPositionAmounts function will cast the fetched liquidity value to a uint128 value unsafely as a result.

Impact:

While the likelihood of a single position achieving a liquidity greater than type(uint128).max is low, the impact of the misbehaviour that arises is significant rendering this exhibit of "minor" severity.

Example:

Copy
592function _getPositionAmounts(
593    uint160 sqrtPriceX96,
594    int24 tickLower,
595    int24 tickUpper
596)
597    internal
598    view
599    returns (
600        uint256 amount0,
601        uint256 amount1,
602        uint256 fees0,
603        uint256 fees1
604    )
605{
606    uint256 liquidity;
607    (liquidity, , , fees0, fees1) = _position(tickLower, tickUpper);
608
609    (amount0, amount1) = LiquidityAmounts.getAmountsForLiquidity(
610        sqrtPriceX96,
611        TickMath.getSqrtRatioAtTick(tickLower),
612        TickMath.getSqrtRatioAtTick(tickUpper),
613        uint128(liquidity)
614    );
615}

Recommendation:

We advise the referenced type cast to be performed safely by ensuring that the liquidity value is at most equal to the value of type(uint128).max, preventing casting overflows from occurring.

Alleviation (0c3f85c7c11805ac412fe291f5681bef26da7244):

The return function signature of the AlgebraBaseLiquidityManager::_position function was updated to instead yield a uint128 variable that requires no cast. As such, we consider this exhibit indirectly alleviated as a cast is no longer necessary and thus is not unsafely performed.

ABL-06M: Insufficient TWAP Sanitization

Type	Severity	Location
Input Sanitization		AlgebraBaseLiquidityManager.sol:L329

Description:

The AlgebraBaseLiquidityManager::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 AlgebraBaseLiquidityManager 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 AlgebraBaseLiquidityManager to be manipulated, leading to incorrect volatility measurements.

Example:

Copy
329require(_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.

ABL-07M: Incorrect Integration of Algebra V1.9

Type	Severity	Location
Logical Fault		AlgebraBaseLiquidityManager.sol:L553

Description:

The IDataStorageOperator interface that is utilized by the AlgebraBaseLiquidityManager is outdated and does not represent the 1.9 version which contains an entirely different function signature as well as different return values, as exhibited in the official Algebra repository.

Impact:

All IDataStorageOperator::getTimepoints calls performed by AlgebraBaseLiquidityManager::_checkVolatility will fail as they do not properly integrate with the 1.9 version of Algebra and represent an outdated interface.

Example:

Copy
552// tickCumulatives is basically where the tick was as of twapInterval seconds ago
553(int56[] memory tickCumulatives, ) = IDataStorageOperator(algebraDataStorage).getTimepoints(secondsAgos);

Recommendation:

We advise the code to import the correct interface and utilize it as the system is currently incompatible with the v1.9 version it is meant to integrate with.

Alleviation (0c3f85c7c11805ac412fe291f5681bef26da7244):

The interface utilized by the system has been updated and its return data points have been properly discarded in the place where the original call was being made, alleviating this exhibit in full.