Morpho: Midnight - BlueBuyCallback [93db7e08]

Description

Let $B_{\mathrm{Blue}}>0$ be the assets withdrawable by BlueBuyCallback. The function returns $B_{\mathrm{Blue}}$ while ignoring id, market, and buyer. It therefore also reports a positive bound for buyer != OWNER, although onBuy always rejects that buyer.

Assume the buyer has no debt and the market entry gate rejects or reverts for canIncreaseCredit(buyer). For every $U>0$:

Consequently, Midnight.take reverts for every positive-unit purchase, but buyerAssetsBound still returns $B_{\mathrm{Blue}}>0$. A router using the advertised bound can therefore select an unexecutable take.

The current API also cannot return the exact gate-aware bound when the buyer has debt. Such a buyer may purchase up to its debt without entering, but converting that unit limit into buyerAssets requires the offer direction and price. For the same reason, the endpoint cannot account for remaining asset/unit caps or maker-side reduceOnly.

Recommendation

At minimum, return

• zero when buyer != OWNER, or
• when the buyer has zero debt and market.enterGate rejects or reverts for canIncreaseCredit(buyer).

For an exact executable bound, change the API to accept the Offer. Bound the result by Blue liquidity, remaining offer capacity, and the assets corresponding to the buyer's debt whenever entry is denied or a maker-buy offer is reduceOnly.

Footnote

buyerAssetsBound does not also consider inconsistencies between IRM accounting when:

• borrowRate
• borrowRateView

Description/Recommendation

If the Midnight market loan $L_M$ token does not match the decoded Blue market's loan token $L_B$, buyerAssetsBound should return 0.

Description

If data does not occupy enough memory so, decoding it into MarketParams can revert. An example test would be:

diff --git a/test/BlueBuyCallbackTest.sol b/test/BlueBuyCallbackTest.solindex ab34dbb1..9fb19fcd 100644--- a/test/BlueBuyCallbackTest.sol+++ b/test/BlueBuyCallbackTest.sol@@ -204,6 +204,11 @@ contract BlueBuyCallbackTest is Test {         assertEq(result, 0);     } +    function testBuyerAssetsBoundRevertsWhenDataIsSmall() public {+        uint256 result = callback.buyerAssetsBound(bytes32(0), market, owner, hex"");+        assertEq(result, 0);+    }+     function testOnBuyRevertsIfCallerIsNotMidnight(address caller) public {         vm.assume(caller != address(midnight));         vm.expectRevert(IBlueBuyCallback.NotMidnight.selector);

Recommendation

If in buyerAssetsBound the length of data is less than the memory required to decode MarketParams (5 words) return 0. One could also explicitly revert depending on the design decision.

Morpho

Further clarification has been added to the NatSpec:

/// @dev Reverts if data is not well formed.

Description

Let $B_N$ be the native-token balance of BlueBuyCallback, and let $B_T$ be its balance of an arbitrary ERC-20 token $T$.

Native tokens can be forced or prefunded to the callback address, and any ERC-20 can be transferred to it. However, the callback's only asset-related flow is onBuy, which withdraws the exact requested loan-token amount from Blue and approves Midnight to pull it. Neither this flow nor the owner-controlled endpoints can transfer an existing balance to OWNER.

Consequently, accidental or forced balances satisfy

These assets remain stranded in the callback.

Recommendation

Add an OWNER-only skim endpoint that transfers the callback's full balance of either the native token or an arbitrary ERC-20 to a specified receiver. Emit the token, receiver, and amount so the recovery is reconstructible from events. Use a safe ERC-20 transfer helper and bubble or explicitly handle native-transfer failure.

Description

Anyone can call the BlueBuyCallbackFactory.createBlueBuyCallback function and deploy a BlueBuyCallback contract on behalf of an arbitrary owner.

The function should track in the CreateBlueBuyCallback event the caller of the function given that it could be different from the owner input parameter.

Recommendation

Morpho should add address indexed caller to the parameters of the CreateBlueBuyCallback event definition.

Description

The BlueBuyCallbackFactory does not perform any sanity checks in the constructor. If the MIDNIGHT and BLUE immutable variables are left uninitialized, any calls to the createBlueBuyCallback would generate "useless" BlueBuyCallback contracts.

Recommendation

Morpho should consider adding basic sanity checks to the BlueBuyCallbackFactory constructor

• require(_midnight != address(0) && _midnight.code.length != 0);
• require(_blue != address(0) && _blue.code.length != 0);

Morpho: see Morpho Protocols Audit Guidelines

Description

Both the setAuthorization and setAuthorizationWithSig are allowing the OWNER to self-unauthorize itself on the BLUE contract as the authorized user to interact on Morpho Blue on behalf of the BlueBuyCallback contract.

The owner should always be authorized to be able to withdraw from the positions that are owned by the callback's contract (in addition to executing other actions that could require the authorization).

Recommendation

Morpho should:

• revert setAuthorization when authorized == OWNER && newIsAuthorized == FALSE
• revert setAuthorizationWithSig when authorization.authorized == OWNER && authorization.isAuthorized FALSE

Description

The current implementation of BlueBuyCallback.onBuy is providing the MIDNIGHT contract an infinite allowance for the market.loanToken token even if the exact amount pulled by MIDNIGHT (after the callback return) is already known.

Recommendation

Morpho should consider replacing forceApproveMax(market.loanToken, MIDNIGHT); with safeApprove(market.loanToken, MIDNIGHT, buyerAssets);

Description

When buyerAssets is zero, the Midnight.take function will pull an empty amount of tokens from the BlueBuyCallback contract (the payer in this case)

if (buyerCallback != address(0)) {            bytes memory buyerCallbackData = offer.buy ? offer.callbackData : takerCallbackData;            require(                IBuyCallback(buyerCallback)                    .onBuy(id, offer.market, buyerAssets, units, buyerPendingFeeIncrease, buyer, buyerCallbackData)                == CALLBACK_SUCCESS,                WrongBuyCallbackReturnValue()            );        }
        SafeTransferLib.safeTransferFrom(offer.market.loanToken, payer, address(this), buyerAssets - sellerAssets);        SafeTransferLib.safeTransferFrom(offer.market.loanToken, payer, receiver, sellerAssets);

This means that no allowance will be consumed and there's no reason for the BlueBuyCallback contract to provide infinite allowance to the Midnight contract.

Recommendation

Morpho should consider moving forceApproveMax(market.loanToken, MIDNIGHT); inside the if (buyerAssets > 0) branch of the BlueBuyCallback.onBuy function.

Description

If the BlueBuyCallback is configured as the fee recipient for the BLUE market, the value returned by IMorpho(BLUE).position(marketParams.id(), address(this)).supplyShares could not yet include the shares minted by Morpho._accrueInterest.

Recommendation

Morpho should document in the buyerAssetsBound natspec if the BlueBuyCallback can be configured as a BLUE fee recipient.

If that's the case the buyerAssetsBound should explicitly disclose that the internal logic could underestimate the supplyAssets value and so the final value returned.

Description

Other Morpho's contracts like the BlueBundlesV1 or Midnight have very extensive and detailed natspec documentation that covers which is the contract's scope, the explicit or implicit assumptions and implementation behaviors like no-op operations, zero checks, and so on.

Recommendation

Morpho should rewrite the BlueBuyCallback natspec to cover the following aspects

• Contract's scope
• Explicit and Implicit assumptions
• Expected requirements
• General implementations adopted standards like no-op operations, zero checks and so on
• "Custom" implementation behaviors relevant only to BlueBuyCallback

Description

The main goal of BlueBuyCallback is to be able to allow OWNER to park tokens in the BLUE markets to yield interest while waiting for possible MIDNIGHT.take actions (where the OWNER is the buyer).

The "main" reason to set OWNER as an authorized user in BLUE for the BlueBuyCallback instance is to allow the OWNER to withdraw the callback's supply position (+ yield).

But because BlueBuyCallback.setAuthorization and BlueBuyCallback.setAuthorizationWithSig allow the OWNER to add/remove new authorized users on MORPHO for the BlueBuyCallback contracts it means that multiple users could manage the callback contract on BLUE which could include executing borrow flows (supplyCollateral can be executed by anyone on behalf of BlueBuyCallback).

Given the specific core scope of BlueBuyCallback Morpho should consider to reduce what can be done with the contract's positions on Morpho Blue enabled by the authorization system.

1. remove IMorpho(BLUE).setAuthorization(OWNER, true); from constructor
2. remove the setAuthorization function
3. remove the setAuthorizationWithSig function
4. Add a withdraw function callable only by the OWNER that will call BLUE.withdraw
5. Add a withdrawCollateral function callable only by the OWNER that will call BLUE.withdrawCollateral
6. (optional) If there's a need for the OWNER to allow delegation, implement a "local" delegation system to allow the owner to delegate the execution of BlueBuyCallback withdraw and withdrawCollateral functions.

Recommendation

Morpho should consider reducing the complexity of BlueBuyCallback flows derived by unrestricted delegation on Blue positions

Morpho: indeed that would have been an other path. we are ok with the fact that someone could indeed borrow / deposit collateral etc through the callback, so we decided to ack the issue.

Description

The BlueBuyCallback.safeApprove does not validate the token's existence on purpose assuming that such validation will be done by the BlueBuyCallback.onBuy caller.

Recommendation

Morpho should explicitly document this assumption like it has been already done in the bundler's TokenLib contract.

The strong assumption is that Midnight.take will perform that validation once SafeTransferLib.safeTransferFrom will be executed after the callback returns the flow to the caller contract.

Description

In BlueBuyCallbackFactory, createBlueBuyCallback() always attempts to deploy a new BlueBuyCallback for the specified owner without checking if the contract already exists.

As such, if BlueBuyCallback is already deployed for an owner, calling createBlueBuyCallback() for the same owner will revert.

If a user batches createBlueBuyCallback() with other operations (eg. setAuthorization(), BLUE.supply()), an attacker can force the entire batch to revert by front-running and calling createBlueBuyCallback() first.

Recommendation

Consider checking if BlueBuyCallback was deployed first and perform a no-op if so:

address callback = callbackOf[owner];if (callback != address(0)) return callback;

Description

In BlueBuyCallback, the onBuy() callback always withdraws buyerAssets from Morpho Blue:

if (buyerAssets > 0) IMorpho(BLUE).withdraw(marketParams, buyerAssets, 0, address(this), address(this));

This introduces a front-running attack vector, where an attacker could force a call to take() to revert by temporarily consuming liquidity in BLUE (eg. by borrowing enough such that totalSupplyAssets - totalBorrowAssets < buyerAssets).

Recommendation

Consider adding a comment that bundles/routers should call buyerAssetsBound() and take() in the same transaction to avoid this.