Hann Finance ~ CDP Core

...
function getLossRatios() public view returns (uint256 shortfallBps, uint256 claimLossBps) {
...
        uint256 tv = cooldownVault.totalLockedAssets() + cooldownVault.idleBalance();
...
        uint256 shortfall = cooldownVault.totalShortfall();
        uint256 claimLoss = cooldownVault.totalClaimLoss();
...
function getDynamicHaircutBps() public view returns (uint256 haircutBps) {
...
        //@Dedaub: Imagine that a user has atomically called a Defi
        //         protocol that uses Superearn and explicitly creates some
        //         cooldown vault debt. If liquidity is low, it's possible
        //         that the created debt is enough to cause a price manipulation
        (uint256 shortfallBps, uint256 claimLossBps) = getLossRatios();
...
        uint256 haircutBps = getDynamicHaircutBps();
        if (haircutBps > 0 && price > 0) {
            uint256 discount = (price * haircutBps) / 10_000;
            if (discount > price) {
                discount = price;
            }
            price = price - discount;
        }
...

Both wearnUSDT and HNKAIA are implemented as asset vaults. Therefore their price is being derived from the on-chain redemption rate of each asset:

...
uint256 oneShare = 10 ** uint256(shareDecimals); // 1 earnUSDT
uint256 assets;
//@Dedaub: will query coolDownVault's exchange rate
try router.previewRedeem(earnUSDT, oneShare) returns (uint256 a) {
...

...
uint256 balance = IERC20Metadata(asset).balanceOf(address(hnkaia));
...
uint256 qty18 = _normalizeToDecimals(balance, _getDecimals(asset), 18);

if (hasProvider) {
    uint256 rate = _getRedeemRateView(asset);
    //@Dedaub: Technically calculating the redemption rate denominated in KAIA
    result.navKaia = qty18 * rate / PRECISION;
...

It’s possible for an attacker to flash-loan an amount that is O(shares) (e.g., 100 * shares) and inflate the price of the collateral by directly transferring to the vault contract. In such scenarios, the cost of manipulation is O(shares), so the attacker has to use some additional collateral X, such that X * manipulated_price * (1/1.1) > cost_of_manipulation and directly profit from minting HNUSD if they were to redeem or sell it.

Ultimately, this means that vault-based-assets have to accumulate enough liquidity so that manipulating the price via donations becomes impractical.

There are some implicit and explicit assumptions on the price behavior of earnUSDT and HNKAIA that may have security consequences if they are not accounted for properly:

[wearnUSDT]

• WEarnUSDTPriceFeed returns the price of wearnUDT denominated in USDT. This carries the implicit assumption that USDT will be very close to 1$. In the event of price deviations in the price market of USDT compared to USD, profitable arbitrage opportunities might arise within the protocol.

...
uint256 oneShare = 10 ** uint256(shareDecimals); // 1 earnUSDT
uint256 assets;
//@Dedaub: will query coolDownVault's earnUSDT -> USDT exchange rate
try router.previewRedeem(earnUSDT, oneShare) returns (uint256 a) {
...

• The price technically depends on the available liquidity in SuperEarn’s CooldownVault contract (this is outside the scope of this engagement, but we reviewed the integration for the sake of completeness based on information provided by the protocol team). Since this seems to be intended to facilitate all queued SuperEarn withdrawals, the dynamics of withdrawals have to be considered: If liquidity ever gets low this could lead to a sharp fall in the asset's price within your protocol. While the portfolio of SuperEarn might be in perfect health, such instances of sparse liquidity might give rise to liquidation opportunities (or even a complete branch shutdown if price falls below SCR).

• The code expects a monotonic price increase for wearnUSDT (or a relative stability in the price):

...
if (lastGoodPrice != 0) {
    uint256 tol = monotonicTolerance + (lastGoodPrice * monotonicToleranceBps) / 10_000;
    if (nav + tol < lastGoodPrice) {
        emit OracleFailure("non-monotonic");
        _registerOracleFailure();
        return (lastGoodPrice, true);
    }
...

From an economics perspective, the protocol might end up operating on stale prices that do not reflect the market conditions of earnUSDT, leading to arbitrage opportunities in adverse market conditions.

• On an analogous note, this implicit smoothing out of price falls can give a lot of leeway for parties to avoid liquidations (e.g., a user can notice a sharp fall in price, the last good price will be used and they can exit without risking their liquidity). Although this can benefit UX, it deteriorates the health of a branch when considering its adaptability to market price changes.

[HNKAIA]

• When the redemption rate of HNKAIA decreases, the price feed effectively pauses future deposits in the HNKAIA multi-asset vault for the asset for which the decrease was noted:

...
RateCache storage cache = redeemRateCache[asset];
uint256 last = cache.value;

if (last != 0 && newRate < last) {
    _pauseAssetDeposits(asset, REASON_RATE_DECREASE);
    emit RedeemRateFallbackUsed(asset, last);
...
}
...

function _ensureFreshRedeemRate(address asset) internal view {
    //@Dedaub: will end up reverting when paused
    try priceFeed.getRedeemRate(asset) returns (uint256) {
        return;
    } catch (bytes memory reason) {
        if (reason.length == 0) revert StalePriceDetected();
        assembly {
            revert(add(reason, 0x20), mload(reason))
        }
    }
}

This might create withdrawal pressure on HNKAIA. From a tokenomics perspective, although this might create scarcity in the HNKAIA supply and drive the market price up, this scarcity might affect the ability of users to top up their troves (since the price of their collateral has just fallen). It might make sense to not react to price drops so aggressively and letting the core protocol dynamics encourage minting/burning.

• The HNKAIA price feed holds the implicit assumption of a somewhat constant cardinality of observations in the Uniswap v3 oracle before querying the TWAP price.

function _requiredObservationCount(uint32 period) internal pure returns (uint16) {
    uint32 required = period / CARDINALITY_PER_MINUTE;
...

This is not entirely correct, since in practice the number of observations depends on how many swaps have been actually performed. When a query for a TWAP interval which lies outside the oracle's observations is made, the call reverts. Therefore, external UniswapV3 integrators don’t have to try and approximate the cardinality of observations.

// @dev Reverts if an observation at or before the desired observation timestamp does not exist.
function observeSingle(
...

On the same note, the observationCardinalityNext of the slot0 field of a pool always yields the length of allocated observations — but this does not mean that they have all been initialized just yet. This means that examining the length of oracle observations does not suffice to determine whether a TWAP query will succeed.

• Having multiple TWAP windows and choosing the smallest price has the following consequences:

• Make it easier to defeat the strength of the TWAP if the shorter duration (easier to manipulate) yields the smaller price. Effectively, with this scheme it's easier to manipulate the price downwards with one large trade within the last minute.

• The TWAP’s lag might create profitable arbitrage redemptions, since the protocol might end up valuing the collateral cheaper than the market value. In such a scenario:

• The user buys/mints some USDHN

• The user redeems USDHN

• The protocol undervalues the collateral, so the user ends up receiving more collateral for the redeemed USDHN than what they have gotten with the market price.

for (uint256 i = 0; i < twapWindows.length; ++i) {
...
    uint32 period = twapWindows[i];
...
    (uint256 windowQuote, bool valid) = _consultTwapQuote(pool, baseToken, quoteToken, period, baseAmount);
...
    if (windowQuote < minQuoteRaw) {
        minQuoteRaw = windowQuote;
    }
...

It’s recommended to have a single TWAP duration that’s both hard to
manipulate and does not create a significant deviation from the market price in
periods of volatility.

• On an analogous note, unconditionally choosing the smallest price between NAV and TWAP prices — even when redeeming - can potentially create the same profitable redemption arbitrage as described in the previous point.

...
// use min(NAV, TWAP) for default
if (!v.navAvailable) revert NavUnavailable();
uint256 price = v.navPrice;
if (v.marketAvailable && v.marketPrice > 0) {
    price = Math.min(price, v.marketPrice);
}
...

The smaller price always benefits the user when redeeming — that's why the original LiquityV2 codebase had special code paths for when fetching the redemption price.

[KAIA]

• The KAIA price feed enforces a branch shutdown when a significant price change occurs:

...
if (!_isWithinDeviation(price)) {
    emit OracleFailure("Price deviation too high");
    bool triggerShutdown = _registerOracleFailure();
    if (triggerShutdown) {
        return (_shutDownAndSwitchToLastGoodPrice(), true);
    }
    return (lastGoodPrice, true);
}
...

This is a significant assumption with potentially serious consequences; depending on the deviation threshold that is chosen, can end up prematurely shutting down the branch during times of volatility (where large price upwards/downwards movements might happen).

• The scaling of the Pyth oracle response might end up breaking the protocol’s accounting if the exponent component of the response ever becomes positive:

function _scalePythPrice(int64 priceValue, int32 expo) internal pure returns (uint256) {
    if (priceValue <= 0) return 0;
    int256 priceInt = int256(priceValue);
    int256 exponent = int256(expo) + 18;
...

In such a scenario the scaled price will end up having more than 18 decimals. Although this is fairly untypical of Pyth oracles, it might make sense guarding against such a scenario since the Pyth protocol technically allows it.

In LiquityV2, a price feed failures automatically trigger a branch shutdown:

...
if (ethUsdOracleDown) {
    return (_shutDownAndSwitchToLastGoodPrice(address(ethUsdOracle.aggregator)), true);
}
if (exchangeRateIsDown) {
    return (_shutDownAndSwitchToLastGoodPrice(rateProviderAddress), true);
}
// If the ETH-USD feed is live but the RETH-ETH oracle is down, shutdown and substitute RETH-ETH with the canonical rate
if (rEthEthOracleDown) {
    return (_shutDownAndSwitchToETHUSDxCanonical(address(rEthEthOracle.aggregator), ethUsdPrice), true);
}
...

...
function _fetchPricePrimary() internal returns (uint256, bool) {
    assert(priceSource == PriceSource.primary);
    (uint256 ethUsdPrice, bool ethUsdOracleDown) = _getOracleAnswer(ethUsdOracle);

    // If the ETH-USD Chainlink response was invalid in this transaction, return the last good ETH-USD price calculated
    if (ethUsdOracleDown) return (_shutDownAndSwitchToLastGoodPrice(address(ethUsdOracle.aggregator)), true);
...

...
if (exchangeRateIsDown) {
    return (_shutDownAndSwitchToLastGoodPrice(rateProviderAddress), true);
}
if (ethUsdOracleDown) {
    return (_shutDownAndSwitchToLastGoodPrice(address(ethUsdOracle.aggregator)), true);
}

// If the STETH-USD feed is down, shut down and try to substitute it with the ETH-USD price
if (stEthUsdOracleDown) {
    return (_shutDownAndSwitchToETHUSDxCanonical(address(stEthUsdOracle.aggregator), ethUsdPrice), true);
}
...

However, with the exception of the KAIA price feed, the HNKAIA and EARNUSDT price feeds do not shut down the branches when oracle failures are detected:

...
(uint256 nav, bool ok) = _tryReadNavUsdPerToken();
if (!ok || nav == 0) {
    _registerOracleFailure();
    return (lastGoodPrice, true);
}
...

function _registerOracleFailure() internal {
    if (lastFailureTime == 0 || block.timestamp > lastFailureTime + failureWindow) {
        failureCount = 1;
        lastFailureTime = block.timestamp;
    } else {
        unchecked {
            ++failureCount;
        }
    }
    if (failureCount >= failureThreshold && !oracleHardDown) {
        oracleHardDown = true;
        emit OracleHardDown(failureCount, lastFailureTime, lastFailureTime + failureWindow);
    }
}

...
(uint256 kaiaUsdPrice, bool kaiaFallback, bool kaiaOk) = _refreshKaiaUsdPrice();
if (!kaiaOk) {
    if (lastGoodPrice == 0) revert KaiaPriceUnavailable();
    return (lastGoodPrice, true);
}
if (kaiaFallback) {
    emit KaiaUsdFallbackUsed(kaiaUsdPrice);
}
(PortfolioValuation memory valuation, bool ok) = _pullPortfolioPrices();
if (!ok) {
    return (lastGoodPrice, true);
}
...

Gracefully switching a last valid price can be beneficial in terms of UX, but one has to account for the potential economic consequences: Miss-informed pricing schemes can leave the door to devastating arbitrage attacks against the protocol and it then becomes hard to argue about the economic soundness of the system.

LiquityV2 does not support collateral types that do not have 18-decimal accounting. This can be seen if ones examines the ICR calculations sitting at the heart of the protocol’s accounting:

...
function _computeCR(uint256 _coll, uint256 _debt, uint256 _price) internal pure returns (uint256) {
    if (_debt > 0) {
        uint256 newCollRatio = _coll * _price / _debt;
...

With the typical design choice of price being represented using 18 decimals, the above calculation is correct only when the collateral is also represented using 18 decimals.

Instead of having an immutable set of initially supported collateral types like the original LiquityV2 codebase does, the protocol’s CollateralRegistry aims to enable the dynamic addition of new collateral types. However, there’s no enforcement with respect to the number of decimals for a new collateral:

...
uint8 tokenDecimals = asset.decimals();
uint8 resolvedDecimals = decimals == 0 ? tokenDecimals : decimals;
if (resolvedDecimals != tokenDecimals) revert InvalidDecimals();
...

Since this can have serious security-related consequences, it might make sense adding checks to guard against the addition of unsupported assets.

The protocol’s CollateralRegistry contract introduces functionality to dynamically add/deprecate collateral types:

...
function queueCollateral(IERC20Metadata asset, ITroveManager troveManager, address priceFeed, uint8 decimals)
    public
    onlyGov
    returns (uint256 index)
{
...
    index = _addCollateralRecord(
        asset,
        troveManager,
        priceFeed,
        resolvedDecimals,
        ICollateralRegistry.CollateralStatus.Pending,
        queuedAt,
        activateAt,
        msg.sender
    );

...

...
_collaterals.push(
    CollateralConfig({
        asset: asset,
        troveManager: troveManager,
        priceFeed: priceFeed,
        decimals: decimals,
        status: status,
        queuedAt: queuedAt,
        activateAt: activateAt,
        addedAt: uint48(block.timestamp),
        addedBy: addedBy
    })
);
...

While a collateral type can be in either of the following statuses:

• Pending
• Active
• Deprecated

None of the above-mentioned statuses is enforced within the protocol’s code. Only redemption-routing enforces that the collateral branches from which debt is burnt is active:

...
for (uint256 i; i < collatCount; ++i) {
    if (_collaterals[i].status == ICollateralRegistry.CollateralStatus.Active) {
        ++activeCount;
    }
}
if (activeCount == 0) revert NoActiveCollateral();

uint256[] memory activeIndices = new uint256[](activeCount);
uint256 cursor;
for (uint256 i; i < collatCount; ++i) {
    if (_collaterals[i].status == ICollateralRegistry.CollateralStatus.Active) {
        activeIndices[cursor++] = i;
    }
}
...

Users can open/close/adjust/liquidate troves before a collateral becomes active and even after the collateral gets deprecated. From an economic's perspective, selectively turning off redemptions for a collateral branch does not eliminate all incentives to be minting debt on that branch — and it also puts greater redemption pressure on the rest of the branches when opportunities to restore the peg arise.

This even creates an interesting incentive where users can avoid redemptions altogether by using a deprecated collateral — normal (non-urgent) redemptions will never get routed through their troves and their leverage is somewhat guaranteed.

Currently the earnUSDT branch is going to enforce a 105% minimum collateralization ratio:

...
uint256 constant MCR_EARNUSDT = 105 * _1pct;
...

While convenient for users, it leaves a theoretical possibility for a price drop to make liquidatable troves have ICR < 100% if the price drop is too significant and there are troves near above the MCR.

Reward distribution inside the PILDistributor contract takes place by iterating over all receivers and calculating the rewards to be distributed for each and one of them:

function _processDistribution(uint256 maxReceivers)
...
    while (ctx.index < ctx.receiverCount && processed < ctx.limit && ctx.remainingWeight > 0) {
...
            _transferAndNotify(ctx.index, receiver, amount);
...

Since this potentially takes place within each debt-accruing operation, one has to be considerate about the number of possible receivers; this distribution design cannot reasonably scale to many receivers without incurring high gas costs and raising DOS concerns.

The original Liquity design avoided such a design using index-based reward distribution in order to address the above-mentioned concerns.

The developer team has already confirmed that the number of reward receivers will be bound - with the code being changed to enforce this bound - we leave this consideration for the sake of completeness.

Setting a 0 minimum reward distribution delay delay by calling PILDistributor::setMinDistributionInterval :

function setMinDistributionInterval(uint256 _interval) external onlyOwner {
    uint256 previous = minDistributionInterval;
    minDistributionInterval = _interval;
    emit MinDistributionIntervalUpdated(previous, _interval);
}

function _defaultCanDistribute() internal view returns (bool) {
    return block.timestamp >= lastDistributionTime + minDistributionInterval
        && rewardToken.balanceOf(address(this)) > 0 && totalWeightBps == 10000 && receivers.length > 0;
}

Can give rise to a race condition between PILDistributor::setReceivers and PILDistributor::distribute.

If the owner of the PILDistributor contract decides to change receivers, any current receiver might frontrun a PILDistributtor::distribute() call nonetheless and initiate a distribution on any dust balance in the contract.

Naturally, the impact of this is expected to be non-major so this mostly serves as an informatory note.

Anyone may initiate an InterestRouter::routeInterest operation:

function routeInterest(uint256 _amount, bytes calldata _context) external override nonReentrant {
...

    uint256 sentToPil = _sendToPilDistributor(_amount);
...
}

...
try distributor.distribute() {}
catch (bytes memory reason) {
    emit PilDistributionFailed(reason);
}
...

In the scenario that the interest router will be configured as a “distributor” in the PILDistributor contract, a distribution can be initiated even if the minimum distribution delay has not passed:

...
function _requireAuthorized() internal view {
    require(
        block.timestamp >= lastDistributionTime + minDistributionInterval || msg.sender == owner()
            || hasRole(DISTRIBUTOR_ROLE, msg.sender),
...

A problem may arise if a malicious party calls InterestRouter::routeInterest after donating 1 wei of tokens directly to the PILDistributor contract. In the event this is properly timed to trigger the initiation of a new distribution in the PILDistributor contract, an iteration through all receivers will take place, finalizing the distribution (even if 0 tokens end up being transferred).

This would create a really cheap and effective vector to DOS reward distribution altogether.

Naturally, the same consideration would hold for any misconfigured party having the REWARD_DISTRIBUTOR role but we explicitly focus on the scenario that would allow anyone to permissionlessly trigger the DOS scenario.

The binary search in EarnUSDTFlashSwapper::_usdtForTargetWEarn (and in other places with the zappers module) seems to be a potentially expensive and unnecessary method of quoting token amounts:

...
uint256 wAtHi = _quoteWEarnFromUsdt(hi);
if (wAtHi == 0) {
    revert InsufficientOutput(wEarnTarget, 0);
}

// Hi-doubling phase: ensure we find a hi such that F(hi) >= target, up to 64 iterations
uint8 attempts;
while (wAtHi < wEarnTarget && attempts < 64) {
    if (hi > type(uint256).max / 2) {
        revert InsufficientOutput(wEarnTarget, wAtHi);
    }
    hi = hi * 2;
    wAtHi = _quoteWEarnFromUsdt(hi);
    attempts++;
}
...
// Binary search on [lo, hi] for minimal usdtIn such that F(usdtIn) >= wEarnTarget
uint256 lo = 0;
while (lo < hi) {
    uint256 mid = (lo + hi) / 2;
    uint256 wAtMid = _quoteWEarnFromUsdt(mid);
    if (wAtMid >= wEarnTarget) {
        hi = mid;
    } else {
        lo = mid + 1;
    }
}
...

Apart from the design, some features of the code referenced above are also a bit counterintuitive; even though the snippet finds a lower amount (at mid) that satisfies the required target value, the final loop does not terminate there.

We would highly recommend using analytical calculations when quoting any form of swaps. It's easy to argue about the correctness of the quotes and it's vastly more efficient. Afterall, doing a binary search and treating swap quotes as an incrementally queried black box defeats the entire point of having analytical calculations within smart contracts when a swap is performed.

From an economics perspective, when opening a leveraged trove it's not beneficial to attempt to repay as much debt as possible:

...
IERC20(payToken).safeTransfer(address(pool), owe);
// Apply any remaining USDHN to repay debt (consume base MIN_DEBT first, avoid flushing)
uint256 usdhnAfter = IERC20(address(usdhnToken)).balanceOf(address(this));
if (usdhnAfter != 0) {
    LatestTroveData memory t = troveManager.getLatestTroveData(op.troveId);
    uint256 repayable;
    if (t.entireDebt > MIN_DEBT) {
        uint256 maxRepay = t.entireDebt - MIN_DEBT;
        repayable = usdhnAfter > maxRepay ? maxRepay : usdhnAfter;
    }
    if (repayable != 0) {
        borrowerOperations.repayUSDHN(op.troveId, repayable);
        usdhnAfter -= repayable;
    }
...

Repaying bold and burning the tokens reduces the user's exposure on the leverage token and potentially restricts the user's options.

Depending on intended user flows, this might make things easier for developing around zappers, but this is again a semantic deviation from what the original LiquityV2 codebase does.

For LeveragedGasCompZapper::leverDOwnTrove operations, the flow repays as much debt as the value of the desired amount of collateral to be de-leveraged:

...
(address flashToken, uint8 flashTokenDec) = _flashTokenFor(LeverageTypes.Kind.LeverDown);
uint256 flashAmount =
    _flashAmountForDecrease(flashToken, flashTokenDec, uint256(-params.collDelta));
...

...
// Use the full flash USDT as exact-in to repay as much debt as possible.
uint256 usdhnOut = _runStableSwap(
    sc,
    StableOp({usdtToUsdhn: true, exactOut: false, amountIn: borrowed, amountOut: 0, maxIn: borrowed})
);
if (usdhnOut != 0) borrowerOperations.repayUSDHN(op.troveId, usdhnOut);
...

This means it’s possible to remove the requested collateral. However, because debt is overcollateralized, in order to remove the desired collateral X one is required to repay less than X in USD value.

Ultimately, the current implementation is de-leveraging a bit more than desired.

The original LiquityV2 codebase assumes that an external source has calculated the required bold required just for the collateral removal needed:

...
// Swap Coll from flash loan to Bold, so we can repay and downsize trove
// We swap the flash loan minus the flash loan fee
// The frontend should calculate in advance the `_params.minBoldAmount` to achieve the desired leverage ratio
// (with some slippage tolerance)
uint256 receivedBoldAmount = exchange.swapToBold(_effectiveFlashLoanAmount, _params.minBoldAmount);

// Adjust trove
borrowerOperations.adjustTrove(
    _params.troveId,
    _params.flashLoanAmount,
    false, // _isCollIncrease
    receivedBoldAmount,
    false, // _isDebtIncrease
    0
);
...

if you look at the receiveFlashLoanOnLeverDownTrove function of LeverageLSTZapper — they don't remove more bold than what's required

Strictly following Uniswap accounting’s math, swap-based quoting is asymmetric.

...
IQuoterV2.QuoteExactInputSingleParams memory qp = IQuoterV2.QuoteExactInputSingleParams({
    tokenIn: collateralToken,
    tokenOut: flashToken,
    fee: fee,
    amountIn: collToRelease,
    sqrtPriceLimitX96: 0
});

(uint256 usdtOut,,,) = leverageQuoter.quoteExactInputSingle(qp);
if (usdtOut == 0) revert QuoteFailed();
return usdtOut;
...

The snippet is querying the amount of USDT that will be received as output if collToRelease is provided as input — which is supposed to serve as a quote of how much USD is the collateral worth.

However, it's not guaranteed that if one provides the USDT returned by this snippet they will get the exact same collateral used by the quote because of the execution slippage caused by Uniswap’s accounting

It would perhaps make more sense to use "exact out" quoting for this, since in reality the true query needed for LeveragedGasCompZapper::leverDownTrove is “the amount of USDT needed in order to receive collToRelease”:

...
uint256 flashAmount =
    _flashAmountForDecrease(flashToken, flashTokenDec, uint256(-params.collDelta));
...

Identifying a high severity issue in an area of the codebase that technically lies outside the scope highlights the vast attack surface of the protocol. Despite the engagement’s scope, the auditors made a best effort to provide as much feedback as possible for as large a part of the codebase as possible (even going above the predicted timeline).

The following assumption in the HNKAIA contract is not true for all code paths:

/// @dev Basic guard for deposit flows. Caller/payer/receiver may all differ; authorization is enforced via
/// ERC20 allowance or Permit2. `_vaults[asset]` is retained for introspection but ///not used for gating.
function _requireDepositAuth(address sender, address from, address receiver, address asset) internal view {
    sender; // reserved for future use; prevents unused var warning
    from;
    if (!isSupported[asset]) revert InvalidParameters();
    if (receiver == address(0)) revert InvalidParameters();
}

When initiating HNKAIA::depositFrom, users have to approve the HNKAIA contract and not the msg.sender of the deposit operation, as transferFrom is called by the HNKAIA contract itself:

function depositFrom(address from, address assetIn, uint256 amount, address receiver, uint256 minSharesOut)
    external
    nonReentrant
    whenNotPaused
    returns (uint256 shares)
{
...
    _requireDepositAuth(msg.sender, from, receiver, assetIn);
...
    IERC20(assetIn).safeTransferFrom(from, address(this), amount);
...
    (shares, actualPer) = _finalizeDeposit(receiver, qtyBefore, minSharesOut);
...

Ultimately, because of the way HNKAIA::_requireDepositAuth is implemented an attacker could exploit a previous approval of user A to the HNKAIA contract and force a deposit from A's account which benefits the attacker (this is exacerbated by the fact that they can even set themselves as the receiver).

USDHN implements LayerZero’s OFT standard and the token’s liquidity can be therefore distributed across supported chains. Because tokens are burnt and minted during bridging operations. This means that totalSupply is not a reliable metric to get the total BOLD supply when calculating the redemption base rate:

...
totals.usdhnSupplyAtStart = usdhnToken.totalSupply();
uint256 redemptionRate =
    _calcRedemptionRate(_getUpdatedBaseRateFromRedemption(_usdhnAmount, totals.usdhnSupplyAtStart));
require(redemptionRate <= _maxFeePercentage, "CR: Fee exceeded provided maximum");
...

In the typical scenario, the impact of this is that as more USDHN tokens are bridged, users will experience heavier redemptions fees:

function _getUpdatedBaseRateFromRedemption(uint256 _redeemAmount, uint256 _totalUSDHNSupply)
    internal
    view
    returns (uint256)
{
    uint256 decayedBaseRate = _calcDecayedBaseRate();

    uint256 redeemedUSDHNFraction = _redeemAmount * DECIMAL_PRECISION / _totalUSDHNSupply; //@Dedaub: after tokens are bridged, the same redemption represents a larger fraction

...

Coordinated bridging operations can even drive the base rate above 1.0, potentially causing the fees to be hitting 100% of redemptions:

function _calcRedemptionRate(uint256 _baseRate) internal pure returns (uint256) {
    return HannMath._min(
        REDEMPTION_FEE_FLOOR + _baseRate,
        DECIMAL_PRECISION // cap at a maximum of 100%
    );
}

In practice, even if the 100% fee rate is not hit, it's very likely that a lot of slippage guards will be hit – effectively DOSing redemptions:

require(redemptionRate <= _maxFeePercentage, "CR: Fee exceeded provided maximum");

When a distribution is triggered via PILDistributor::distribute , aside from reward tokens, distribution receivers get a callback by having notifyRewardAmount be called on their staking address:

...
if (amount != 0) {
    _transferAndNotify(ctx.index, receiver, amount);
    unchecked {
        ctx.remainingBalance -= amount;
    }
    sent += amount;
}
...

...
function _transferAndNotify(uint256 index, Receiver storage receiver, uint256 amount) internal {
...
    rewardToken.safeTransfer(receiver.staking, amount);

    try IPILReceiver(receiver.staking).notifyRewardAmount(address(rewardToken), amount) {
...

This allows a potentially malicious receiver to exploit the following inconsistency in the ActivePool contract:

...
    if (remainder > 0) {
        usdhnToken.mint(address(interestRouter), remainder);
        //@Dedaub: execution will be hijacked here
        interestRouter.routeInterest(remainder, bytes(""));
    }
}

lastAggUpdateTime = block.timestamp;
...

Since execution is hijacked before lastAggUpdateTime gets updated, since the current receiver in PILDistributor is not yet updated, the malicious receiver can potentially trigger multiple interest aggregations by calling mintAggInterest() directly.

While the distribute call will revert because of the reentrancy guard that is going to be hit, the following try-catch in InterestRouter will not revert the whole transaction:

...
try distributor.distribute() {}
catch (bytes memory reason) {
    emit PilDistributionFailed(reason);
}
...

Effectively, the reentrant call successfully re-mints USDHN and sends them to the PILDistributor contract to be later distributed. The exacerbation of this attack is for the malicious party to use the above-mentioned mechanism to call distribute multiple times.

Whenever a trove is closed via BorrowerOperations::closeTrove , BorrowerOperations::_prepareBatchContextAndApplyWeights is responsible for preparing the system’s debt accounting information that corresponds to the state after the trove - the payment of which is repaid - is closed down:

...
LatestBatchData memory batch;
address batchManager;
int256 batchDebtDelta = -int256(trove.entireDebt - trove.redistUSDHNDebtGain);
(batch, batchManager) =
    _prepareBatchContextAndApplyWeights(troveManagerCached, _troveId, troveChange, trove, 0, batchDebtDelta);
...

An accounting deviation is created when troves belonging to a batch are closed:

...
batch = _troveManager.getLatestBatchData(batchManager);
//@Dedaub: redistributed debt should not be added when a trove closes - its debt
//         gets eliminated from the system
uint256 base = batch.entireDebtWithoutRedistribution + _trove.redistUSDHNDebtGain;
uint256 batchDebtBase;

if (_batchDebtDelta >= 0) {
    batchDebtBase = base + uint256(_batchDebtDelta);
} else {
    batchDebtBase = base - uint256(-_batchDebtDelta);
}

_initialiseBatchChange(_change, batch, 0);
...
_setWeights(_change, batchDebtBase, batch.annualInterestRate, batch.annualManagementFee);
...

Even though a closing trove’s debt is eliminated from the system, the code always adds any redistributed debt to the batch in which the trove belongs to. Other than a general accounting error, this can have security consequences when the Stability Pool liquidity is low (or can be manipulated to be nearly empty ) and redistributions can be guaranteed.

In such scenarios, the extra debt redistribution in batches could lead to troves inside batches becoming prematurely liquidatable.

In the StableSwapZapper contract, anyone may initiate a liquidity removal operation by calling StableSwapZapper::unstakeAndRemoveLiquidity on behalf of a staker:

function unstakeAndRemoveLiquidity(UnstakeAndRemoveParams calldata params)
    external
    nonReentrant
    returns (uint256[] memory amounts)
{
    PoolAssets memory assets = _validatePool(params.pool);
    BalanceSnapshot memory snapshot = _snapshot(assets);
    _requireValidUnstakeParams(params);

    address staker = params.receiver; // receiver is the staker in this flow
    _requireOperatorApproval(staker);
    // Pull LP directly to the zapper to avoid extra approvals (W-10 fix)
    rewardStaker.unstakeFor(staker, params.liquidity, address(this), staker);

    amounts = _removeLiquidityAndReturn(staker, params, assets, snapshot);
}

It’s expected that the following check will succeed when the specified receiver is the staker themselves:

function _requireOperatorApproval(address staker) private view {
    if (!rewardStaker.operatorApprovals(staker, address(this))) {
        revert OperatorNotApproved();
    }
}

While this limits the severity of the issue - since the receiver of the forced withdrawal will be the staker themselves - forced withdrawals can create other economic incentives to be exploited, as the liquidity of a pool can be accordingly manipulated.

The same consideration holds for other liquidity removal operations in the StableSwapZapper contract

It's possible for non-pool contracts to call EarnUSDTFlashSwapper::onStableSwapExactOut by providing the appropriate sender and op.stablePool parameters:

function onStableSwapExactOut(
    address sender,
    address token0,
    address token1,
    uint256 amount0Owed,
    uint256 amount1Owed,
    bytes calldata data
) external override {
    LeverageTypes.LeverageOp memory op = abi.decode(data, (LeverageTypes.LeverageOp));
    address pool = op.stablePool == address(0) ? earnStablePool : op.stablePool;
    if (msg.sender != pool) revert InvalidFlashPool(msg.sender);
    if (sender != address(this)) revert InvalidFlashPool(sender);
    _requireManagedForFlash(op.troveId);
...

The direct consequence of this is that if any trove is managed by the EarnUSDTFlashSwapper contract (which shouldn't be the general case) one can directly steal USDHN out of it.

Although this has apparent consequences for an atypical user route, we want to emphasize on its resolution since it can serve as an entrypoint gadget for future exploits.

LeveragedGasCompZapper::_scaleDecimals will end up rounding down the result value to 0 when a token value is scaled down (i.e., fromDec decimals is larger than toDec decimals) .

function _scaleDecimals(uint256 amount, uint8 fromDec, uint8 toDec) internal pure returns (uint256) {
    if (fromDec == toDec) return amount;
    uint256 fromFactor = 10 ** fromDec;
    uint256 toFactor = 10 ** toDec;
    return Math.mulDiv(amount, toFactor, fromFactor, Math.Rounding.Up);
}

In such a scenario since 10**fromDec might end up being larger than amount * 10**toDec.

This is especially problematic when dealing with wearn/earn accounting in zappers:

...
// Unwrapping |collDelta| wEarn yields roughly this much earn
uint256 earnBudget = collToRelease / scale;
if (earnBudget == 0) {
    // If |collDelta| < scaleFactor, it cannot produce even 1 earn; fall back to scaling
    return _scaleDecimals(collToRelease, 18, flashTokenDec);
}
...

Leverage operations typically involve involve multiple swaps:

• USDT <> Collateral (through a Uniswapv3-like pool)
• BOLD <> USDT (through the protocol’s stable-swap primitive)

Provided sufficient lack of deep liquidity, any of those paths is potentially vulnerable to sandwich attacks if attackers frontrun operations with large tilting swaps.

While such sandwich attacks cannot happen atomically using flash-loans, this is a novel attack surface that does not exist in the original LiquityV2 codebase:

• Liquity pre-calculates the amount of BOLD tokens to be withdrawn from the UI, if the swap is tilted then there won't be enough tokens to repay the flash swap and the transaction will revert: (see receiveFlashLoanOnLeverUpTrove on LeverageLSDTZapper )

...
// Swap Bold to Coll
// No need to use a min: if the obtained amount is not enough, the flash loan return below won't be enough
// And the flash loan provider will revert after this function exits
// The frontend should calculate in advance the `_params.boldAmount` needed for this to work
exchange.swapFromBold(_params.boldAmount, _params.flashLoanAmount);

// Send coll back to return flash loan
collToken.safeTransfer(address(flashLoanProvider), _params.flashLoanAmount);
...

• The protocol on the other hand directly pulls additional tokens from the trove to cover any swap shortfalls, which creates the attack surface for sandwich attacks:

...
// Exact-out quote for the USDHN required to repay USDT.
uint256 usdhnRequired = _safeQuoteExactOut(sc.pool, sc.usdt, owe);
if (usdhnRequired > usdhnBalance) {
    uint256 shortfall = usdhnRequired - usdhnBalance;
    borrowerOperations.withdrawUSDHN(op.troveId, shortfall, type(uint256).max);
    usdhnBalance += shortfall;
}
...

Typically, it’s possible to specify a non-zero remove-manager for a trove but have the receiver field be address(0). The appointed remove-manager can initiate operations that worsen the trove’s ICR and all removed collateral and minted debt are received by the current owner of the trove’s NFT:

...
if (receiver == address(0) || msg.sender != manager) {
    return _owner;
}
...

While the logic enforcing the dynamic resolution of the current owner is present in AddRemoveManagers::_requireSenderIsOwnerOrRemoveManagerAndGetReceiver , the normalization layer introduced by the protocol enforces different semantics compared to Liquity if the trove NFT ever gets transferred:

...
function _setRemoveManagerAndReceiver(
    uint256 _troveId,
    address _manager,
    address _receiver,
    address _owner,
    address _addManager
) internal {
...
    address normalizedReceiver = _normalizeReceiver(_manager, _receiver, _owner, _addManager);
removeManagerReceiverOf[_troveId] = RemoveManagerReceiver({manager: _manager, receiver: normalizedReceiver});
...

...
if (receiver == address(0)) {
    receiver = _owner;
}
...

When a non-zero remove-manager is specified in AddRemoveManagers::_setRemoveManagerAndReceiver, the code path above will pinpoint the receiver of all removal operations to be the owner at time.

However, this implies that even if the NFT owner changes after a transfer, the receiver of all removal operations will continue being the old owner. The issue is best presented in the following scenario:
The problematic scenario is the following:

• User X opens a trove with remove-manager being R and ( so receiver and manager are 0). The protocol’s code silently sets X as the receiver of all ICR-worsening operations
• User X transfers the trove NFT to a different address (user Y)
• The current owner performs any operation that requires the receiver field; while the original LiquityV2 behavior matches the expected behavior for the same scenario (since the receiver would have been 0, and the current owner would be returned), the current code returns the old owner (X). Ultimately, funds are potentially sent to an unexpected, or even the wrong address in the above scenario.

Troves can be opened on behalf of other users, which is something not possible in the original LiquityV2 codebase:

function _openBorrowerTrove(IZapper.OpenTroveParams memory params, uint256 collAmount)
    internal
    returns (uint256 troveId)
{
    IZapper.OpenTroveParams memory p = params;
    if (p.owner == address(0)) {
        p.owner = msg.sender;
    }
    p.collAmount = collAmount;

    troveId = borrowerOperations.openTrove(
        p.owner,
        _getTroveIndex(p.owner, p.ownerIndex),
        p.collAmount,
        p.usdhnAmount,
        p.upperHint,
        p.lowerHint,
        p.annualInterestRate,
        p.maxUpfrontFee,
        p.addManager,
        p.removeManager,
        p.receiver
    );
}