Morpho: Midnight

Description

If for some reason an oracle would return 0 for a collateral price. A non-healthy or matured position (with debt) can be liquidated and all its collateral corresponding to this oracle can be seized without re-paying any loan tokens:

if (seizedAssets > 0) {    repaidUnits = seizedAssets.mulDivUp(liquidatedCollatPrice, ORACLE_PRICE_SCALE).mulDivUp(WAD, lif);} else { ... }

Recommendation

The above case perhaps should not be allowed and to be carefully documented. The NatSpec currently has this clause:

/// @dev If an activated collateral oracle returns 0 on `price`, `isHealthy`, `withdrawCollateral` when the borrower has/// debt, `take` whenever the seller still has debt, and `liquidate` with repaid input all revert.

which does not cover the above scenario.

It would be best to check that if a provided input to the liquidation has seizedAssets > 0 then repaidUnits should also be non-zero.

Morpho: We acknowledge this issue, as we don't think that there is something misleading.

Spearbit: Acknowledged.

Description

In touchObligation(), fees are taken from the default values and stored in obligationState whenever an obligation is created:

uint16[7] memory _defaultTradingFees = defaultTradingFees[obligation.loanToken];_obligationState.fee0 = _defaultTradingFees[0];_obligationState.fee1 = _defaultTradingFees[1];_obligationState.fee2 = _defaultTradingFees[2];_obligationState.fee3 = _defaultTradingFees[3];_obligationState.fee4 = _defaultTradingFees[4];_obligationState.fee5 = _defaultTradingFees[5];_obligationState.fee6 = _defaultTradingFees[6];_obligationState.continuousFee = defaultContinuousFee[obligation.loanToken];

However, a user could call touchObligation() even before the default fees are set to effectively have no fees.

This is applicable for assets which could be used as loanToken at a later time, since it's not possible to set the default fee for them immediately upon deployment (eg. a token deployed in the future).

The feeSetter can correct this by calling setObligationTradingFee()/setObligationContinuousFee() for the specific obligation, but there is still a window to take advantage of this. For example:

1. A new token is deployed.
2. A taker matches a maker and calls take() (and touchObligation() indirectly) immediately after.
3. The fee setter sets the default fees and corrects the obligation created in (2).

However, the taker/maker in (2) did not incur any trading fee, and will not have any continuous fee as the pending fee is zero at time of calling.

Recommendation

A potential mitigation would be to introduce a global default trading fee, instead of a default per loan token.

Morpho: We acknowledge this behavior (was known). And we don't think that it should be documented, as it's not misleading nor hidden.

Spearbit: Acknowledged.

Description

• take: the contract computes the buyer's newly inherited pending fee with mulDivDown and the seller's released pending fee with mulDivUp:

  uint128 buyerPendingFeeIncrease =    UtilsLib.toUint128(buyerCreditIncrease.mulDivDown(_obligationState.continuousFee * timeToMaturity, WAD));uint128 sellerPendingFeeDecrease = sellerPos.credit > 0    ? UtilsLib.toUint128(sellerPos.pendingFee.mulDivUp(sellerCreditDecrease, sellerPos.credit))    : 0;

• updatePositionView: post slash pending fee uses mulDivUp

  uint256 postSlashPending = credit > 0 ? _pendingFee - _pendingFee.mulDivUp(credit - postSlashCredit, credit) : 0;

• withdraw: pending fee decrease uses mulDivUp:

  pendingFeeDecrease = UtilsLib.toUint128(_position.pendingFee.mulDivUp(units, _position.credit));

These rounding directions are reversed.

Recommendation

Reverse the rounding directions:

• round buyerPendingFeeIncrease up, and
• round sellerPendingFeeDecrease down.
• round postSlashPending up.
• round pendingFeeDecrease down.

That is:

uint128 buyerPendingFeeIncrease =    UtilsLib.toUint128(buyerCreditIncrease.mulDivUp(_obligationState.continuousFee * timeToMaturity, WAD));uint128 sellerPendingFeeDecrease = sellerPos.credit > 0    ? UtilsLib.toUint128(sellerPos.pendingFee.mulDivDown(sellerCreditDecrease, sellerPos.credit))    : 0;

and:

uint256 postSlashPending = credit > 0 ? _pendingFee - _pendingFee.mulDivDown(credit - postSlashCredit, credit) : 0;

and

pendingFeeDecrease = UtilsLib.toUint128(_position.pendingFee.mulDivDown(units, _position.credit));

This makes fee accounting conservative in the protocol's favor and also follows the same rounding direction pattern in

Morpho: We acknowledged it.

Spearbit Acknowledged.

Description

Upon touching an offer's obligation the 1st time, no input validation gets performed for the maturity field which has a type uint256. Given a very high value for this parameter the delta pending fee calculated for the buyer can be greater than the corresponding delta credit for the buyer. This would break an invariant which one might assume:

Moreover, expired obligations can still be instantiated as dead markets (Midnight.sol#L658, Midnight.sol#L447, Midnight.sol#L403, Midnight.sol#L835). touchObligation accepts a maturity already in the past, so the first interaction can create an obligation that is already expired and can even accept collateral. There doesn't seem to be a solvency break from this, because any fresh take that would create seller debt reverts at the final liquidatability check, and liquidate still requires pre-existing debt. The issue is that ObligationCreated no longer implies a live borrowable market, which is a footgun for integrators and users. If this state is not intentional, reject maturity < block.timestamp on creation; otherwise document that created may still mean exit-only/dead on arrival.

Recommendation

1. It might make sense to limit the given maturity by perhaps a value like $100$ years from the timestamp the obligation gets touched/created (one can see this is safe when combined with the MAX_CONTINUOUS_FEE value).

   Otherwise, it would be best to document this to let buyer know so they can avoid obligations with very long maturity dates. feeSetter could set _obligationState.continuousFee to 0 for these types of obligations.

2. Disallow creation of dead markets or document that creation of markets with past maturity is allowed.

Overall, one is hoping for checks of the form:

Also make sure the test suite covers the above scenarios.

Morpho: Fixed partially in PR 742

Spearbit: The first recommended fix was implemented in PR 742. The second point was acknowledged.

Description

One can show that the value of buyerAssets - sellerAssets lies in the set:

ie,

_tradingFee.mulDivDown(units, WAD) // or_tradingFee.mulDivUp  (units, WAD) // or

so there could be scenarios where where for a provided unit $a$ one would end up paying no trading fees (buyerAssets - sellerAssets == 0) even though _tradingFee could be non-zero.

Recommendation

Recommendation 1

To enforce that trading fees are always paid to Midnight whenunitand_tradingFee.` are non-zero, it might make sense to apply the following change:

diff --git a/src/Midnight.sol b/src/Midnight.solindex 018f8904..bc12252c 100644--- a/src/Midnight.sol+++ b/src/Midnight.sol@@ -302,9 +302,8 @@ contract Midnight is IMidnight {         uint256 timeToMaturity = UtilsLib.zeroFloorSub(offer.obligation.maturity, block.timestamp);         uint256 _tradingFee = tradingFee(id, timeToMaturity);         uint256 sellerPrice = offer.buy ? offerPrice - _tradingFee : offerPrice;-        uint256 buyerPrice = sellerPrice + _tradingFee;-        uint256 buyerAssets = offer.buy ? units.mulDivDown(buyerPrice, WAD) : units.mulDivUp(buyerPrice, WAD);         uint256 sellerAssets = offer.buy ? units.mulDivDown(sellerPrice, WAD) : units.mulDivUp(sellerPrice, WAD);+        uint256 buyerAssets = sellerAssets + units.mulDivUp(_tradingFee, WAD);          uint256 newConsumed;         if (offer.maxSellerAssets > 0) {

This would make sure that:

There is a slight caveat where when the maker is the buyer it might spend slightly more than expected (at most by one wei of the loan token):

since

Recommendation 2

To fix the above caveat, one can instead apply the following changes:

diff --git a/src/Midnight.sol b/src/Midnight.solindex 018f8904..6de58c20 100644--- a/src/Midnight.sol+++ b/src/Midnight.sol@@ -301,10 +301,11 @@ contract Midnight is IMidnight {         uint256 offerPrice = TickLib.tickToPrice(offer.tick);         uint256 timeToMaturity = UtilsLib.zeroFloorSub(offer.obligation.maturity, block.timestamp);         uint256 _tradingFee = tradingFee(id, timeToMaturity);-        uint256 sellerPrice = offer.buy ? offerPrice - _tradingFee : offerPrice;-        uint256 buyerPrice = sellerPrice + _tradingFee;-        uint256 buyerAssets = offer.buy ? units.mulDivDown(buyerPrice, WAD) : units.mulDivUp(buyerPrice, WAD);-        uint256 sellerAssets = offer.buy ? units.mulDivDown(sellerPrice, WAD) : units.mulDivUp(sellerPrice, WAD);++        uint256 feeAssets = units.mulDivUp(_tradingFee, WAD);+        uint256 offerAssets = offer.buy ? units.mulDivDown(offerPrice, WAD) : units.mulDivUp(offerPrice, WAD);+        uint256 sellerAssets = offer.buy ? offerAssets - feeAssets: offerAssets;+        uint256 buyerAssets = sellerAssets + feeAssets;          uint256 newConsumed;         if (offer.maxSellerAssets > 0) {@@ -388,8 +389,8 @@ contract Midnight is IMidnight {         }          address payer = buyerCallback != address(0) ? buyerCallback : (offer.buy ? buyer : msg.sender);-        SafeTransferLib.safeTransferFrom(offer.obligation.loanToken, payer, address(this), buyerAssets - sellerAssets);-        claimableTradingFee[offer.obligation.loanToken] += buyerAssets - sellerAssets;+        SafeTransferLib.safeTransferFrom(offer.obligation.loanToken, payer, address(this), feeAssets);+        claimableTradingFee[offer.obligation.loanToken] += feeAssets;         SafeTransferLib.safeTransferFrom(offer.obligation.loanToken, payer, receiver, sellerAssets);          if (sellerCallback != address(0)) {

then the prices and limits asset bounds would be guaranteed for the maker.

• maker is the buyer (for this case there is a possibility that offerAssets - feeAssets might cause a revert. But this is also related to the current implementation where potentially offerPrice - _tradingFee could revert):

• maker is the seller:

This design has the desired property that splitting a take call would incur more fees:

Morpho: We decide to acknowledge this.

Spearbit: Acknowledged.

Description

During liquidation explicit check is missing to ensure collateralIndex is one the activated collaterals. There are 3 possible cases:

Case 1. Almost no-op (only bad debt accounting)

In this case repaidUnits == seizedAssets == 0 and potentially bad debt accounting gets performed. The provided collateralIndex can be outside of the activated collateral sets of the borrower in the obligation. This value gets consumed by

• the event EventsLib.Liquidate
• the zero collateral token transfer
• the potential callback to the liquidator

Case 2. repaidUnits > 0

In this case the call would revert if collateralIndex is not in the activated collateral set of the borrower since liquidatedCollatPrice ends up being 0 and gets used in the denominator to calculate seizedAssets.

Case 3. seizedAssets > 0

This case should also revert due to how newCollateral (if one assumes the invariants of the _position.activatedCollaterals are not broken):

uint128 newCollateral = _position.collateral[collateralIndex] - UtilsLib.toUint128(seizedAssets);

Since in this case _position.collateral[collateralIndex] should be 0 and one cannot subtract a positive value of seizedAssets from it.

Recommendation

One can indirectly see that the missing validation does not affect the last cases 2. and 3. as the call would be reverted. But it does affect case 1.. It is recommended to perform a more explicit check of the following form:

require(bitmap.isBitSet(collateralIndex), "collateral index is not activated");while (bitmap != 0) { ... }

(define the isBitSet utility function accordingly.)

Morpho: We decided that it wasn't worth fixing. You would rather want to see that realizing the bad debt is always possible. And the "weird" events are manageable. We added a comment in PR 785.

Spearbit: Fixed by commenting it.

Description

When lossIndex == type(uint128).max, one would not be able to withdraw any of its credits anymore since the call to _updatePosition(...) sets the position's credit to 0 (if not already being at 0). Thus any old or new credits would be perceived as 0. So the call to withdraw with any portions units amount would revert`. For the same obligation at this state, one can still call:

• take to buy credits (which will later be valued at 0) and accrue debts (for the seller)
• repay to reduce its debt. This might make sense since one would might want to withdraw its collateral and to due see it still needs to keep a healthy position. But the repaid units would not be able to be used by anyone and forever locked in the contract.
• supplyCollateral and withdrawCollateral (would make sense to allow)
• liquidation. The effect on lossIndex would be idempotent. The transferred repaidUnits loan tokens would not be able to be used by anyone (forever locked in the contract)

Recommendation

In this potentially rare scenario:

• calling take should be blocked
• calling repay and liquidate to repay loan tokens might not make sense and a different mechanism like unrolling credits for the lenders might be beneficial.

Description

Midnight.take only checks that the maker authorized offer.ratifier in the current Midnight instance before delegating trust to the external ratifier. EcrecoverRatifier.onRatify, however, validates the recovered signer against its own immutable MIDNIGHT address instead of the caller.

As a result, if a maker authorizes the same ratifier $R$ on multiple Midnight deployments $M_A, M_B$, a signer who is authorized for that maker on $M_A$ can ratify an order that executes on $M_B$. This crosses an otherwise natural trust boundary: delegated signer permissions are expected to be local to each Midnight deployment, but the ratifier imports them from a different instance.

This is made easier by the fact that Offer and the signed Root do not bind the current Midnight address. In practice this is primarily a same-chain multi-deployment issue. Cross-chain replay is normally blocked by the EIP-712 chainId, but the boundary still fails if environments intentionally reuse the same chainid and ratifier address layout.

Recommendation

Ratifiers should be bound to the Midnight instance that invokes them. At minimum, EcrecoverRatifier should reject foreign callers with require(msg.sender == MIDNIGHT) before performing any authorization checks.

It is safer to scope the signed material when calculating the leafs as well. Including the current Midnight address in the signed payload or domain separator would make orders instance-specific even if a ratifier is mistakenly authorized elsewhere.

This should be covered with a regression test that deploys two Midnight contracts and verifies that a signer authorized only on one instance cannot ratify orders on the other.

Morpho: Fixed in PR 744

Spearbit: Fix verified.

Description

ApprovalRatifier stores approvals only as approved[maker][root]. There is no scoping by Midnight instance, and Midnight.take accepts any ratifier that the maker authorized in the current deployment.

Because Offer does not include the current Midnight address, the same approved root can be replayed across multiple Midnight contracts on the same chain when they share the same ApprovalRatifier. This means a maker may believe they approved an order set for one deployment while that approval remains valid anywhere else that reuses the ratifier and root.

Recommendation

Approvals should be scoped to the Midnight instance that consumes them. The simplest fix is to require msg.sender to be the intended Midnight and to key approvals by (msg.sender, maker, root) instead of only (maker, root).

If deployment simplicity matters more than ratifier reuse, an acceptable alternative is to deploy one ApprovalRatifier per Midnight instance and treat ratifiers as strictly single-instance components.

This should also be covered with a regression test that deploys two Midnight contracts sharing one ApprovalRatifier and verifies that an approval created for one deployment cannot be consumed by the other.

Morpho: Fixed in PR 744

Spearbit: The PR 744 fixes this finding. The contract has been renamed to SetterRatifier. Moreover additional authority has been added to authorised users of a maker on the pinned Midnight instance for this ratifier to approve roots.

Description

In take(), offers can only be filled to their specified maximum limits:

uint256 newConsumed;if (offer.maxSellerAssets > 0) {    newConsumed = consumed[offer.maker][offer.group] += sellerAssets;    require(newConsumed <= offer.maxSellerAssets, "consumed seller assets");} else if (offer.maxBuyerAssets > 0) {    newConsumed = consumed[offer.maker][offer.group] += buyerAssets;    require(newConsumed <= offer.maxBuyerAssets, "consumed buyer assets");} else {    newConsumed = consumed[offer.maker][offer.group] += units;    require(newConsumed <= offer.maxUnits, "consumed units");}

An attacker can force a call to take() which fills an offer entirely (ie. consumes the maker's entire max limit) to revert by front-running it and calling take() with dust amounts, causing the newConsumed <= maxUnits check to revert in the subsequent take() call.

Note that in general, even not considering these intentional dust attacks, there could be to in-flight (mempool) transactions that could cause the other one to revert.

Recommendation

A possible mitigation would be to add logic which dynamically calculates the amount of units needed to fill an offer entirely based on maxUnits - consumed. This can be done in a separate periphery contract.

Alternatively, to make it less economically feasible for the attacker one can introduce dynamic lower bounds for units. The parameters for this dynamically calculated units lower bound can be included in the obligation structure. This should also make it less enticing for attackers to lend loans in dust amounts.

Morpho: We acknowledge this. This is a known behavior and is handled by the bundler: MidnightBundles.sol

Spearbit: Acknowledged.

Description

In updatePositionView(), the continuous fee is calcualted based on the pending fee multiplied by a percentage of the time passed relative to the time to maturity:

uint128 fee = _lastAccrual < obligation.maturity    ? uint128(postSlashPending.mulDivDown(accrualEnd - _lastAccrual, obligation.maturity - _lastAccrual))    : 0;

For extremely small decimal tokens (eg. 4 decimals or less), this could round down to zero if the time passed since last accrual (ie. accrualEnd - _lastAccrual) is small relative to the time remaining to maturity (ie. obligation.maturity - _lastAccrual).

Fortunately, since both the numerator and denominator become smaller when _lastAccrual is updated, the rounding error becomes less and the full pending fee will be paid out eventually. Therefore, this is only a concern if the lender withdraws his entire credit way ahead of time before an obligation's maturity.

Recommendation

Since this isn't an issue for tokens with 6 decimals or more, consider adding under the token requirements that tokens with decimals less than 6 should not be used.

Morpho: This was commented already.

/// @dev Because of roundings, trading and continuous fees might charge less than expected, which can become problematic/// for chains where the gas is cheaper than 1 asset of the loan token.

(thus it's not really an issue)

We don't think that it should be added to the token requirements, as only the fees break.

Spearbit: Acknowledged.

Description

The ID of an obligation includes the current chain ID:

function toId(Obligation memory obligation) public view returns (bytes32) {    return IdLib.toId(obligation, block.chainid, address(this));}

However, if a hard fork occurs and block.chainid changes, the ID of all obligations will also change which breaks all accounting.

Recommendation

Since the likelihood of this occurring is quite low, consider documenting this risk.

Morpho: Fixed in PR 738

Spearbit: Fix verified.

Description/Recommendation

1. Midnight.sol#L469:

   -     ... if all the collateral is withdrawn and the borrower has no debt.+     ... if the borrower has no debt.

2. Midnight.sol#L643-L648: setIsAuthorized being transitive might not be desired from the user perspective (ie an authorised msg.sender on behalf of a user being able to authorise another account).

3. Midnight.sol#L455-L462, Midnight.sol#L482-L487, Midnight.sol#L595-L599: Refactor position collateral amount and activatedCollaterals updates into utility functions to make sure the required updates happen atomically to avoid potential future mistakes.

4. UtilsLib.sol#L9: Cheaper to perform below, but it's also ok to keep the current implementation to align with the overloaded atMostOneNonZero with three inputs:

   diff --git a/src/libraries/UtilsLib.sol b/src/libraries/UtilsLib.solindex 7e3826ea..2282ecd2 100644--- a/src/libraries/UtilsLib.sol+++ b/src/libraries/UtilsLib.sol@@ -6,7 +6,7 @@ library UtilsLib {     /// @dev Returns true if at most one of `x` and `y` is nonzero.     function atMostOneNonZero(uint256 x, uint256 y) internal pure returns (bool z) {         assembly {-            z := gt(add(iszero(x), iszero(y)), 0)+            z := iszero(mul(x, y))         }     }

5. TickLib.sol#L5: improve comment to floor(ln(1 + 0.025) * 1e18).

6. TickLib.sol#L24: add comment to show the explicit formula used to derive this constant floor(ln(2) * 1e18).

7. TickLib.sol#L6: Make sure all relevant documents (Notion, ...) are updated to indicate this value for max tick 1024.

8. Midnight.sol#L271, Midnight.sol#L384, Midnight.sol#L397, Midnight.sol#L440, Midnight.sol#L619, Midnight.sol#L653, ConstantsLib.sol#L15: Make sure the onCallbackFuncs (onRatify, onBuy, onSell, onRepay, onLiquidate, onFlashLoan) return name spaced scoped per endpoint magic bytes instead of either no check or one magic sequence shared among all callbacks. One can use a following pattern:

   keccak256("morpho-labs/midnight/{{onCallbackFuncs}}/success")

9. Midnight.sol#L270: Define a separate access control storage parameter and logic for allow ratifiers (separate from authorised users which are allowed as callers to specific endpoints).

10. EcrecoverRatifier.sol#L24: Define a separate access control for users that can sign root digests on behalf of someone else this storage parameter can be added in EcrecoverRatifier.

11. Midnight.sol#L261-L264, Midnight.sol#L410-L412, Midnight.sol#L431-L432, Midnight.sol#L450-L452, Midnight.sol#L477-L479: Inconsistent order of the following checks:

    require(onBehalf == msg.sender || isAuthorized[onBehalf][msg.sender], "unauthorized");bytes32 id = touchObligation(obligation);

    make sure all required endpoints first check authorisation of the msg.sender then touch the obligation. Finally perform other required operations.

Spearbit:

Description

Assume an obligation of a loan token can be backed by many collaterals. All except one are real trusted sources of collateral. The one collateral is only there to be used in the future to block collateral.

A user can back its loan by mix of all collateral and maybe only one small unit of the phantom collateral. Then if the position had debt and matured or if it is unhealthy (which could possibly be blocked from checking) the phantom's collateral's oracle could revert on price() fetches and thus block the whole liquidation flow. It can also exaggerate the price of the phantom oracle to let the user withdraw the other supplied collaterals to back the position.

Recommendation

It would be the responsibility of the lenders to make sure they check the legitimacy of all the provided collaterals in an obligation since a barely used phantom collateral can later lock the funds to be returned.

There is already a NatSpec clause regarding the liveliness of the price oracle:

/// @dev If an activated collateral oracle reverts on `price`, `liquidate`, `isHealthy`, `withdrawCollateral`  when the/// borrower has debt, and `take` whenever the seller still has debt all revert.

But perhaps the above scenario can be highlighted further.

Morpho: This scenario is now documented in PR 767.

/// A single reverting oracle blocks liquidation for every borrower with that collateral activated, and a borrower can/// activate such a collateral post-incident to block their own liquidation.

Spearbit: Fix verified.

Description

If an obligation had enough loan token liquidity that can be withdrawn users and feeClaimer can frontrun calls to liquidation to avoid their credits getting slashed. This can create a race condition for these entities to try to capture their credits while supply of loan tokens last. An extreme case would be when slashing is about to happen and obligationState[id].withdrawable is 0.

Recommendation

The above can be documented and analysed.

Description

In a perfect world, one would expect that:

But due to lazy slashing and rounding errors above is not true in general (need to be further analysed). Instead if all positions were updated so the position credits would not hold stale values we would get:

Credit/Debt

If one would instead of two parameters of credit and debt were to work with signed integers, we could define:

and then one could show that:

and that:

Loss Index

Let $i_{\mathcal{O}}$ be the loss index for the obligation $\mathcal{O}$. Define:

Then we have:

in an ideal world one would just keep track of the exact value:

Some invariants

• past maturity the amount of total debt per obligation cannot grow $\sum_{u} D_{\mathcal{O},u}$.

Notation

Description

In the liquidation flow bad debt of a position is calculated as:

(in the above $[x]^{+} = \max(0, x)$)

With the current choice of $\texttt{lif}_{\mathcal{O}, T}^{max}$ and $\texttt{lltv}_{\mathcal{O}, T}$ we have:

LLTV: 0.385 	 | LIF: 1.181683899556868537 	 | LIF x LLTV: 0.454948301329394387LLTV: 0.385 	 | LIF: 1.444043321299638989 	 | LIF x LLTV: 0.555956678700361011LLTV: 0.625 	 | LIF: 1.103448275862068965 	 | LIF x LLTV: 0.689655172413793104LLTV: 0.625 	 | LIF: 1.230769230769230769 	 | LIF x LLTV: 0.769230769230769231LLTV: 0.77 	 | LIF: 1.061007957559681697 	 | LIF x LLTV: 0.816976127320954907LLTV: 0.77 	 | LIF: 1.129943502824858757 	 | LIF x LLTV: 0.870056497175141243LLTV: 0.86 	 | LIF: 1.036269430051813471 	 | LIF x LLTV: 0.891191709844559586LLTV: 0.86 	 | LIF: 1.075268817204301075 	 | LIF x LLTV: 0.924731182795698925LLTV: 0.915 	 | LIF: 1.021711366538952745 	 | LIF x LLTV: 0.934865900383141762LLTV: 0.915 	 | LIF: 1.044386422976501305 	 | LIF x LLTV: 0.955613577023498695LLTV: 0.945 	 | LIF: 1.01394169835234474 	 | LIF x LLTV: 0.95817490494296578LLTV: 0.945 	 | LIF: 1.028277634961439588 	 | LIF x LLTV: 0.971722365038560411LLTV: 0.965 	 | LIF: 1.008827238335435056 	 | LIF x LLTV: 0.97351828499369483LLTV: 0.965 	 | LIF: 1.017811704834605597 	 | LIF x LLTV: 0.982188295165394402LLTV: 0.98 	 | LIF: 1.005025125628140703 	 | LIF x LLTV: 0.984924623115577889LLTV: 0.98 	 | LIF: 1.010101010101010101 	 | LIF x LLTV: 0.989898989898989899LLTV: 1 	 | LIF: 1 	                 | LIF x LLTV: 1LLTV: 1 	 | LIF: 1 	                 | LIF x LLTV: 1

or

So for $\texttt{lif}_T \in [10^{18}, \texttt{lif}_{\mathcal{O}, T}^{max}]$ we have:

so

in general one can prove:

we have:

where

The recovery close factor condition below is not quite using the true upperbound for $a_{repaid}$ ($\tau_{\mathcal{O}}$ is the rcfThreshold):

One can rewrite the 2nd condition as:

where:

ie:

Risky collaterals

Very risky obligations when all collaterals have risky liquidation parameters.

...

Notations

Description

List of supported chain for Morpho v2 products (vaults, ...) are listed on this page. Based this list:

Notes:

• Pharos was checked via its publicly documented Atlantic Testnet RPC, not a listed mainnet RPC.
• Tempo was checked via its publicly listed Moderato testnet RPC, since that is what is publicly available.
• The positive cases were sanity-checked with both CLZ(1)=255 and CLZ(0)=256, not just a single opcode acceptance test.

The chains that don't have a support currently:

Recommendation

If Morpho would want to deploy Midnight to a wider set of chains sooner, it would be best to use a different implementation of msb that avoids the usage of the newly introduced CLZ opcode.

List of other opcodes to potentially check (although most of the above seem to support):

• TLOAD
• TSTORE
• MCOPY
• PUSH0

Morpho: Fixed in PR 737

Spearbit: Documentation has been added in PR 737.

Summary

Finding Description

The Midnight only calls the buyerCallback with the computed buyer / seller, and then uses the buyer callback as the payer in the transfer path. It is therefore the callback contract’s responsibility to verify that the buyer/seller is the expected one, and that the ID, obligation, buyerAssets / sellerAssets, units, and callback data match the flow it intends to support. Otherwise, a maker-funded callback can end up paying a trade the maker did not intend.

Example:

• A maker deploys a funded callback contract to support buy offers.
• Its onBuy() only checks msg.sender == Midnight and blindly approves buyerAssets.
• Later, the maker posts a sell offer.
• A malicious taker takes that sell offer and sets takerCallback to the maker’s funded callback.
• In the sell path, Midnight treats takerCallback as the buyerCallback, calls onBuy(..., buyer = malicious taker, ...), and then pulls funds from that callback as the payer.
• If the callback does not verify buyer == maker and does not distinguish trusted maker-side data from taker-controlled data, it can end up paying for the attacker’s purchase

Recommendation

Document explicitly that callback contracts are responsible for authenticating the full trade context before approving any transfer.

Morpho: We acknowledge this issue. We think that the behavior is pretty clear and safe already.

Spearbit: Acknowledged.

Description

Under certain edge-case market conditions, inflating the obligation loss index $i_{\mathcal{O}}$ may be materially easier than in typical states.

In particular, if totalUnits is small and the minimum viable loan and collateral amounts are both low, an attacker may be able to repeatedly open dust positions, avoid repayment, and later self-liquidate those positions to realize bad debt. Because each bad-debt event increases $i_{\mathcal{O}}$, this allows the attacker to inflate the loss index over time at relatively low cost.

This attack becomes easier, for example, when:

• the collateral-to-loan price remains stable; and
• the collateral lltv is equal, or very close, to WAD.

To end up in a state where lossIndex $i_{\mathcal{O}}$ maxes out, there could be $843$ bad debt liquidations at 10% of the total units.

An attacker could then execute the following sequence:

• inflate $i_{\mathcal{O}}$ to near its maximum value;
• allow honest users to open positions while the loss index is already heavily inflated; and
• perform additional bad-debt liquidations to fully saturate $i_{\mathcal{O}}$ at $2^{128} - 1$.

In other words, honest users' position creation can be sandwiched between two loss-index inflation phases, causing them to enter after most of the inflation has already occurred but before final saturation.

Recommendation

These edge-case scenarios should be investigated further, especially under low-totalUnits conditions and near-WAD lltv configurations. If this behavior is confirmed, the protocol should consider protocol-level mitigations rather than relying on users to avoid opening positions when $i_{\mathcal{O}}$ is already highly inflated, since the inflation itself may be front-runnable.

Morpho: We acknowledge this issue. Note that the core issue (rounding on lossIndex) was documented already.

Spearbit: Acknowledged.

Description

function countBits(uint128 x /* x0 */) internal pure returns (uint256) {    unchecked {        // x1        x = x - ((x >> 1) & 0x55555555555555555555555555555555);
        // x2        x = (x & 0x33333333333333333333333333333333) + ((x >> 2) & 0x33333333333333333333333333333333);
        // x3        x = (x + (x >> 4)) & 0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f;
        return (                x * 0x01010101010101010101010101010101 // x4 and x5 (implicit trick due to `uint128` type and `unchecked`)        ) >> 120 // x6;    }}

Assume $x$ has the following binary representation:

Let

// wrapped multiplication which actually truncates/masks the resultx * 0x01010101010101010101010101010101

The first 3 lines of countBits transform $x$ as follows: