Hanji

Vault

6.1 The minUsdValue check in removeLiquidity is misleading as it does not account for fees

The removeLiquidity(...) function is intended to protect users from slippage by using the minUsdValue parameter. The NatSpec comment explicitly states this parameter is "The minimum USD value the user expects to receive". However, the implementation checks this condition *before* protocol and admin fees are deducted. This means a user can receive a net value in tokens that is less than the minUsdValue they specified.

The check is performed on the gross lpUSDValue, but the final tokenAmount sent to the user is reduced by both protocolFee and adminTokenAmount.

// ...
uint256 lpUSDValue = (totalUSDValue * burnLP) / lpTotalSupply;
// @audit This check is performed against the pre-fee value.
require(
    lpUSDValue >= minUsdValue,
    ErrorReporter.InsufficientUSDValue()
);

// ...
// Multiple fees are calculated based on the initial tokenAmount.
uint256 protocolFee = tokenAmount.mulDivUp(
    protocolFeeBps,
    BASIS_POINTS_DIVISOR
);
// ...
uint256 adminTokenAmount = tokenAmount.mulDivUp(
    adminBurnLPFeeBps,
    BASIS_POINTS_DIVISOR
);
// ...

// @audit The user receives an amount reduced by fees, potentially worth less than minUsdValue.
tokenAmount -= (adminTokenAmount + protocolFee);
token.safeTransfer(msg.sender, tokenAmount);
// ...

This behavior is counterintuitive and makes the slippage protection less effective than implied. A user might set a minimum acceptable value but still have their transaction processed for a lower net value due to fees, which contradicts the purpose of a minimum output parameter.

Recommendation

Consider performing the minUsdValue check on the final net amount of tokens the user will receive after all fees are accounted for. Alternatively, update the NatSpec to reflect the current code.

Resolution

We have updated the documentation for the removeLiquidity function to provide clearer guidance to end users regarding the purpose and behavior of the minUsdValue parameter.

The primary focus of minUsdValue is to protect users against significant price movements during liquidity removal operations. As such, this parameter validates the USD value of the tokens before any fees are applied, ensuring the gross value meets the user's expectations relative to market conditions.

For comprehensive protection that includes fee considerations, we direct users to the minTokenGet parameter, which specifically enforces the minimum net token amount the user will receive after all protocol and admin fees have been deducted.

This dual-parameter approach provides users with flexibility:

• minUsdValue for price movement protection
• minTokenGet for fee-inclusive amount protection

The updated documentation now accurately reflects this intended behavior and helps users make informed decisions when configuring their liquidity removal parameters.

6.2 Deactivating a LOB prevents claiming of outstanding orders

The changeLob(...) function allows the owner to deactivate a Limit Order Book (LOB) by setting its isActive status to false. However, the claimOrder(...) function, which is used to claim or cancel orders, can only be called on active LOBs due to the check in _checkLOB(...).

If an owner deactivates a LOB that still has open orders placed by the LPManager, the assets backing those orders will become temporarily inaccessible. The claimOrder(...) function will revert for that lobId, preventing the recovery of the funds.

function changeLob(
    address lobAddress,
    bool isActive,
    // ...
) external onlyOwner {
    // ...
    } else {
        // @audit When a LOB is deactivated (!isActive), there is no process
        // @audit to claim or cancel any existing open orders.
        if (!isActive) {
            // When deactivating the pair, reset LOB contract allowances to zero
            _callApprove(
                tokens[lobs[i].tokenIdX].tokenAddress,
                lobAddress,
                0
            );
            _callApprove(
                tokens[lobs[i].tokenIdY].tokenAddress,
                lobAddress,
                0
            );
        }
    }
    // ...
}

function _checkLOB(uint8 lobId) internal view override {
    require(lobId < lobs.length, ErrorReporter.UnknownLob());
    // @audit This check prevents `claimOrder` from being called on a deactivated LOB.
    require(lobs[lobId].isActive, ErrorReporter.LobDisabled());
}

While the owner can resolve this by reactivating the LOB, claiming the orders, and then deactivating it again, this process is cumbersome and introduces operational risk. An owner might forget to clear outstanding orders before deactivation, leading to temporarily locked funds.

Recommendation

Consider adding logic to the changeLob(...) function to automatically cancel all outstanding LPManager orders on a LOB when it is being deactivated. Alternatively, add a strong warning in the NatSpec documentation for the function, advising the owner to manually claim all orders before setting a LOB's status to inactive.

Resolution

The @param isActive description now clearly warns about the consequences of deactivation and the recovery procedure.

6.3 Missing input validation for priceConfig parameters in setConfig function

The setConfig(...) function in the LPManager contract allows the owner to set critical system parameters, including baseMultiplier and maxAllowedPriceDeviation from the PriceConfig struct. These parameters are used in the function ProxyPyth.validateAndResetPriceIfNeeded(...) to validate LP token price movements.

Currently, the setConfig(...) function does not perform any validation on these two values.

// ...
// slot 12
address _adminFeeRecipient = feeConfig.adminFeeRecipient;
_checkForZeroAddress(_adminFeeRecipient);

uint24 _priceValidityPeriod = priceConfig.priceValidityPeriod;
// @audit The baseMultiplier parameter is not validated.
uint64 _baseMultiplier = priceConfig.baseMultiplier;

// ...
adminFeeRecipient = _adminFeeRecipient;
priceValidityPeriod = _priceValidityPeriod;
baseMultiplier = _baseMultiplier;

// slot 13
// @audit The maxAllowedPriceDeviation parameter is not validated.
uint64 _maxAllowedPriceDeviation = priceConfig.maxAllowedPriceDeviation;
// ...

While this function is owner-restricted, a misconfiguration—such as setting baseMultiplier to a value greater than the WAD value of 1e18 or setting either parameter to zero—could compromise the price validation logic.

Recommendation

Consider adding input validation for the baseMultiplier and maxAllowedPriceDeviation parameters within the setConfig(...) function.

Resolution

Fixed in commit 651366ac. Added validation ensuring:

• baseMultiplier is restricted between 50% and 100%
• maxAllowedPriceDeviation cannot exceed maximum daily volatility (100% / √86400)

6.4 Missing validation for liquidity thresholds in setConfig

The setConfig(...) function in the LPManager contract is responsible for setting various configuration parameters, including minLiquidityValueUsd and maxLiquidityValueUsd. These variables define the lower and upper bounds for the total USD value of liquidity in the pool.

The function currently lacks a validation check to ensure that minLiquidityValueUsd is less than or equal to maxLiquidityValueUsd.

// ...
// @audit The minimum liquidity threshold is set here.
minLiquidityValueUsd = _minLiquidityValueUsd;

// slot 11
// @audit The maximum liquidity threshold is set here.
uint128 _maxLiquidityValueUsd = liquidityConfig.maxLiquidityValueUsd;
// @audit A check is missing to ensure _minLiquidityValueUsd <= _maxLiquidityValueUsd.

maxLiquidityValueUsd = _maxLiquidityValueUsd;
// ...

Recommendation

Consider adding a require statement in the setConfig(...) function to validate that minLiquidityValueUsd is less than or equal to maxLiquidityValueUsd.

6.5 cooldownDuration can be cached to save an SLOAD

The checkCooldown(...) function in the LPManager contract reads the cooldownDuration state variable from storage twice if its value is not zero. Each read from storage consumes gas for an SLOAD operation.

function checkCooldown(address account) public view {
    // @audit First SLOAD of `cooldownDuration`.
    if (cooldownDuration == 0) {
        return;
    }
    // @audit Second SLOAD of `cooldownDuration` occurs here.
    require(
        block.timestamp - lastAddedAt[account] >= cooldownDuration,
        ErrorReporter.CooldownDurationNotYetPassed()
    );
}

This double read is inefficient. The value of cooldownDuration can be read from storage once and stored in a local memory variable.

Recommendation

Consider caching the cooldownDuration state variable in a local memory variable at the beginning of the checkCooldown(...) function.

6.6 Functions mint and burn can be declared external

The mint(...) and burn(...) functions in the LPToken contract are declared with public visibility. However, they are only designed to be called externally by the owner of the contract (the LPManager). These functions are never called internally from within the LPToken contract itself.

// ...

    function mint(address account, uint256 value) onlyOwner public {
        _mint(account, value);
        emit TokensMinted(account, value);
    }

    function burn(address account, uint256 value) onlyOwner public {
        _burn(account, value);
        emit TokensBurned(account, value);
    }
// ...

For functions that are only ever called from outside the contract, using external visibility is a best practice.

Recommendation

Consider changing the visibility of the mint(...) and burn(...) functions from public to external.

6.7 Inconsistent early return pattern in state-setting functions

There is an inconsistent pattern in how permissioned, state-setting functions handle redundant calls. Some functions, like changePendingPrimaryMarketMaker(...) and acceptPrimaryMarketMaker(...), check if the new state is the same as the current state and return early to avoid unnecessary operations. However, other functions, namely changeMarketMaker(...) and changeDepositor(...), do not have this check.

These functions will perform a state write and emit an event even if the new value is identical to the one already in storage.

// @audit This function writes to storage even if marketMakers[marketMaker] already equals isActive.
function changeMarketMaker(address marketMaker, bool isActive) external onlyOwner {
    marketMakers[marketMaker] = isActive;

    emit MarketMakerChanged(marketMaker, isActive);
}

// @audit This function writes to storage even if depositors[depositor] already equals isActive.
function changeDepositor(address depositor, bool isActive) external onlyOwner {
    depositors[depositor] = isActive;

    emit DepositorChanged(depositor, isActive);
}

In contrast, other functions in the same contract do implement this check:

function changePendingPrimaryMarketMaker(
    address _primaryMarketMaker
) external onlyOwner {
    address currentPendingPrimaryMarketMaker = pendingPrimaryMarketMaker;
    // @audit This function correctly returns early if there is no state change.
    if (currentPendingPrimaryMarketMaker == _primaryMarketMaker) {
        return;
    }
// ...
}

This inconsistency makes the codebase harder to maintain and can lead to unnecessary gas costs from redundant SSTORE operations and event emissions.

Recommendation

Consider applying a consistent pattern across all state-setting functions.

6.8 Inconsistent type casting for LPManager contract

In various parts of the codebase, instances of the LPManager contract are inconsistently cast. Sometimes they are treated as the LPManager type, while other times they are cast to the parent ProxyLOB type. While this pattern does not introduce a direct security vulnerability, it leads to an inconsistent and confusing coding style.

Recommendation

Consider consistently using the LPManager type for all variables and interactions involving instances of the LPManager contract. Since LPManager inherits from ProxyLOB, it already provides access to all of its parent's public and external functions.

Resolution

Fixed in commit 045ea2f3. We've resolved the inheritance by making ILPManager inherit from IProxyLOB. This eliminates the redundant interface and removes all IProxyLOB casts across integrations, ensuring consistent typing with ILPManager throughout the codebase.

6.9 Misleading NatSpec for withdrawFromYieldVault function

The NatSpec documentation for the withdrawFromYieldVault(...) function in the LPManager contract is inaccurate. Specifically, the comment for the assets parameter states that it is an "Amount of assets to withdraw directly," which does not reflect the function's actual implementation.

The function's logic takes the assets amount, converts it into a corresponding number of vault shares using yieldVault.previewWithdraw(assets), and then adds these shares to the shares parameter. The final withdrawal from the vault is based on this combined total of shares.

/// @notice Withdraws assets from a yield vault.
/// @param yieldVaultId Identifier of the target vault.
/// @param assets Amount of assets to withdraw directly.
/// @param shares Amount of shares to redeem.
/// @param expires Timestamp when operation expires.
function withdrawFromYieldVault(
    uint8 yieldVaultId,
    uint256 assets,
    uint256 shares,
    uint256 expires
) external /* ... */ {
    // ...
    IERC4626 yieldVault = IERC4626(yieldVaultAddress);

    uint256 totalShares = shares;
    if (assets > 0) {
        // @audit The 'assets' amount is converted to shares before withdrawal.
        totalShares += yieldVault.previewWithdraw(assets);
    }

    uint256 redeemedAssets = _callRedeem(yieldVault, totalShares);
    // ...
}

Recommendation

Consider updating the NatSpec comment for the assets parameter in the withdrawFromYieldVault(...) function.

6.10 Missing NatSpec documentation for updateAndGetEpochPrice function

The updateAndGetEpochPrice(...) function has @notice, @dev, and @return comments but it is missing descriptions for its input parameters.

Recommendation

Consider adding comprehensive NatSpec @param documentation.

Resolution

Fixed in commit ff06e0ee. Added documentation for maxOracleAge and oracleConfRel input parameters.