Omniscia Steer Protocol Audit

LiquidityManagerVault Manual Review Findings

LiquidityManagerVault Manual Review Findings

LMV-01M: Non-Standard Disable of Initialization

Description:

The referenced LiquidityManagerVault::constructor contains the Initializable::initializer to prevent initialization of the logic implementation contract, however, this practice is non-standard.

Example:

src/LiquidityManagerVault.sol
260constructor() initializer {}

Recommendation:

We advise the Initializable::_disableInitializers function to be invoked instead, ensuring both initializers as well as re-initializations are prohibited at the logic implementation contract.

Alleviation (121dc0b1329a48555f90bb92e58a95e8ff8a7c79):

The code was revised to utilize the proper Initializable::_disableInitializers function, alleviating this exhibit.

LMV-02M: Improper Deviation of Logic

Description:

The referenced code contains special logic for handling native fund settlements in the LiquidityManagerVault::unlockCallback function, however, these adjustments are unnecessary and incorrect as the OpenZeppelin CurrencySettler library supports native fund settlements and the Steer Protocol team code does not sync the currency prior to settling it.

Impact:

Native fund settlements will not sync the pool manager's state (i.e. reset the transient storage value tracker) which may result in misbehaviours under specific multi-contract execution scenarios.

Example:

src/LiquidityManagerVault.sol
824// Transfer tokens to pool manager
825if (zeroForOne && (Currency.unwrap(poolKey.currency0)) == address(0)) {
826    poolManager.settle{value: uint256(int256(-swapDelta.amount0()))}();
827} else {
828    // Use the appropriate token for transfer - token0 for zeroForOne, token1 otherwise
829    Currency token = zeroForOne ? poolKey.currency0 : poolKey.currency1;
830    token.settle(
831        poolManager,
832        address(this),
833        zeroForOne ? uint256(int256(-swapDelta.amount0())) : uint256(int256(-swapDelta.amount1())),
834        false
835    );
836}

Recommendation:

We advise the code to either sync the native currency prior to settling it or to utilize the CurrencySettler regardless of the currency type, the latter of which we advise.

Alleviation (121dc0b1329a48555f90bb92e58a95e8ff8a7c79):

The Steer Protocol team followed the latter of our two recommendations, utilizing the CurrencySettler implementation regardless of the underlying currency type and thus addressing this exhibit.

LMV-03M: Inexistent Validation of Fee Values

Description:

The individual fee values yielded by the IFeeManager must accumulate to FEE_DIVISOR, however, this trait is not validated in the codebase.

Impact:

Although the IFeeManager instance is expected to be competently managed, a single unit deviation in the proportions can result in significant discrepancies of the vault's balance evaluation mechanisms meriting a non-informational rating.

Example:

src/LiquidityManagerVault.sol
739// Distribute fees to fee recipients
740vars.fees = IFeeManager(feeManager).getFees(address(this));
741for (uint256 j; j != vars.fees.length; ++j) {
742    accruedFees0[vars.fees[j].feeIdentifier] += FullMath.mulDiv(vars.cut0, vars.fees[j].feeValue, FEE_DIVISOR);
743    accruedFees1[vars.fees[j].feeIdentifier] += FullMath.mulDiv(vars.cut1, vars.fees[j].feeValue, FEE_DIVISOR);
744}
745

746// Update total fees
747totalFees0 += vars.cut0;
748totalFees1 += vars.cut1;

Recommendation:

We advise this trait to be validated whenever fees are disbursed, preventing overdisbursement of fees and thus erroneous tracking of the vault's balance.

Alleviation (121dc0b1329a48555f90bb92e58a95e8ff8a7c79):

The Steer Protocol team clarified that the fee contract is a previously audited version that ensures the cumulative fee values are at most equal to 100%, rendering the recommended restriction to be redundant and this exhibit be nullified.

LMV-04M: Rounding of Fee Distribution

Description:

The fee distribution mechanism of the LiquidityManagerVault::_executeBurn function will round each proportional disbursement of fee cuts, resulting in funds being locked in the contract as "total fees" which will never be redeemed.

Impact:

The totalFees0 and totalFees1 values meant to track the fees due for various parties will not properly represent the actual values that have been debited as the proportional calculations will round downward resulting in a potential error of vars.fee.length per each burn operation.

Example:

src/LiquidityManagerVault.sol
739// Distribute fees to fee recipients
740vars.fees = IFeeManager(feeManager).getFees(address(this));
741for (uint256 j; j != vars.fees.length; ++j) {
742    accruedFees0[vars.fees[j].feeIdentifier] += FullMath.mulDiv(vars.cut0, vars.fees[j].feeValue, FEE_DIVISOR);
743    accruedFees1[vars.fees[j].feeIdentifier] += FullMath.mulDiv(vars.cut1, vars.fees[j].feeValue, FEE_DIVISOR);
744}

Recommendation:

We advise the code to disburse the remainder of the cuts at the last iteration without calculating a proportion, ensuring that the disbursement of fees is done in full regardless of the underlying cut values.

Alleviation (121dc0b1329a48555f90bb92e58a95e8ff8a7c79):

The code was updated per our recommendation, tracking the total amount of funds distributed in each currency and disbursing the remainder in the last fee recipient iteration.

LMV-05M: Incorrect Evaluations of Vault Totals

Description:

The LiquidityManagerVault::getTotalAmounts and the LiquidityManagerHelper::getTotalAmounts function implementations will utilize a fixed fee percentage for evaluating what portion of the fees will be captured as a cut by the Steer Protocol team, however, the actual burn procedure will utilize a dynamic percentage fetched by the IFeeManager.

Should those two values deviate, the accounting system of the vault will be breached and might result in underflows, unfulfillable withdrawals, and other similar errors.

Impact:

Should the IFeeManager configuration be updated to impose a higher fee than expected, the actual cut of the fees due toward the Steer Protocol protocol will change and will therefore result in incorrect total asset evaluations for the vault.

Example:

src/LiquidityManagerVault.sol
734// Apply fee manager cut
735vars.totalFeesBps = IFeeManager(feeManager).vaultTotalFees(address(this));
736vars.cut0 = FullMath.mulDiv(vars.fees0, vars.totalFeesBps, FEE_DIVISOR);
737vars.cut1 = FullMath.mulDiv(vars.fees1, vars.totalFeesBps, FEE_DIVISOR);

Recommendation:

We advise constant declaration of the contract to be omitted and the dynamic value of the IFeeManager to be utilized instead, ensuring that the vault's total assets are consistently evaluated correctly.

Alleviation (121dc0b1329a48555f90bb92e58a95e8ff8a7c79):

The total fees are now dynamically queried within the LiquidityManagerHelper::getTotalAmounts implementation, rendering this exhibit alleviated.

LMV-06M: Inexistent Prevention of Overlapping Tick Ranges

Description:

The LiquidityManagerVault::migratePositions function does not prohibit overlapping tick positions which will significantly affect the vault's accounting system and result in withdrawals being calculated twice for the same position.

Impact:

A mistake in the vault's configuration to support the same lower and upper tick range twice will result in two distinct opPositions entries resolving to the same underlying LP position and thus breaking the vault's accounting system.

Example:

src/LiquidityManagerVault.sol
937function migratePositions(LiquidityPositions memory _positions) internal {
938    uint256 newPosLength = _positions.lowerTick.length;
939    delete opPositions;
940    IHelper.NewLiquidityPositions memory temp;
941    for (uint256 i; i != newPosLength; ++i) {
942        temp = IHelper.NewLiquidityPositions({
943            lowerTick: _positions.lowerTick[i],
944            upperTick: _positions.upperTick[i],
945            relativeWeight: _positions.relativeWeight[i]
946        });
947        opPositions.push(temp);
948    }
949}

Recommendation:

We advise the code to prevent same lower and upper tick configurations, potentially by marking them as "active" in a mapping and prohibiting them from being re-added in the same loop.

Alternatively, we advise a salt to accompany each position eliminating the overlap error altogether.

Alleviation (2bab393ad0):

The issue has been partially alleviated as the range overlap evaluation performed in the fourth step of the LiquidityManagerVault::unlockCallback does not sufficiently prevent overlaps.

Namely, the lowerTick of the new position should be higher than the upperTick of the previous position to ensure no overlap occurs.

Alleviation (2bab393ad0):

The Steer Protocol team evaluated this exhibit and opted to retain the latest logic in place as they do not wish to impose the stricter validation we advised. As such, we consider this exhibit acknowledged.

LMV-07M: Inexistent Utilizations of Currency Metadata

Description:

All LiquidityManagerVault instances will have the same EIP-20 name and symbol regardless of the underlying pool and currencies utilized.

Impact:

All off-chain LiquidityManagerVault representations will use the same symbol and name which enables phishing attacks and increases the chance of user mistakes.

Example:

src/LiquidityManagerVault.sol
407__ERC20_init("STEER_V4UNI_VAULT","STEERUV4");

Recommendation:

We advise a distinction to be made, potentially by querying the underlying currency names or by utilizing the poolKey, preventing off-chain confusion of various vault positions.

Alleviation (121dc0b1329a48555f90bb92e58a95e8ff8a7c79):

The construction of the EIP-20 asset name and symbol has been updated to incorporate the vault count of the caller instead, ensuring each vault is unique and thus addressing this exhibit.

LMV-08M: Re-Entrancy Fee Duplication Vulnerability

Description:

The LiquidityManagerVault::collectFees function does not have any re-entrancy protections imposed, permitting the recipient of the token0 fees to re-enter the system.

As the code will cache the accruedFees1 value before the native transfer and will update the accruedFees1 value to the result of the cached value subtracted by the amount1 desired, a re-entrancy attack is possible and permits a full withdrawal of the accruedFees1 value to be repeated ad infinitum.

Impact:

A fee recipient can drain all token1 funds in the contract via repetitive re-entrancy attacks due to an outdated cached value of the accruedFees1 storage entry.

Example:

src/LiquidityManagerVault.sol
501/// @notice Used to collect accumulated fees.
502function collectFees(string memory feeIdentifier, uint256 amount0, uint256 amount1) external whenNotPaused {
503    require(msg.sender == feeManager);
504    address to = IFeeManager(feeManager).withdrawalPermissions(address(this), feeIdentifier);
505    uint256 currentFee0 = accruedFees0[feeIdentifier];
506    uint256 currentFee1 = accruedFees1[feeIdentifier];
507

508    if (amount0 > 0) {
509        accruedFees0[feeIdentifier] = currentFee0 - (amount0);
510        totalFees0 -= amount0;
511        if (Currency.unwrap(poolKey.currency0) == address(0)) {
512            (bool success,) = to.call{value: amount0}("");
513            require(success, "Native transfer failed");
514        } else {
515            IERC20(Currency.unwrap(poolKey.currency0)).safeTransfer(to, amount0);
516        }
517    }
518    if (amount1 > 0) {
519        accruedFees1[feeIdentifier] = currentFee1 - (amount1);
520        totalFees1 -= amount1;
521        IERC20(Currency.unwrap(poolKey.currency1)).safeTransfer(to, amount1);
522    }
523}

Recommendation:

We advise the code to avoid caching the accruedFees1 value before the native transfer, ensuring that the code cannot be exploited via re-entrancy attacks due to outdated cached values in memory.

Alleviation (121dc0b132):

The code was updated to instead incorporate the ReentrancyGuard::nonReentrant modifier, preventing a re-entrancy from occurring altogether.

Although this is an adequate alleviation, we continue to advise the currentFee1 value to not be cached as it may result in a re-entrancy error in case the logic is copied in future development versions.

Alleviation (2bab393ad0):

The alleviation of this exhibit has been enhanced per our recommendation, ensuring that the codebase complies to the CEI pattern by performing a subtraction-and-assignment operation on each accruedFeesX entry.

Prev

LiquidityManagerHelper.sol (LMH-M)

Next

LiquidityManagerHelper.sol (LMH-C)

On this page