From 6689a8e4cc47687e5690fc567208dd5c0f800d31 Mon Sep 17 00:00:00 2001 From: Maxence Raballand Date: Thu, 6 Aug 2026 15:58:54 +0200 Subject: [PATCH 1/3] fix: harden the borrow-offers fill accounting and offer lifecycle - consume walk: evaluate the profitability/bonus floor once per offer on its whole remaining amounts (scale-invariant), not on the rounded chunk, so partitioning a target cannot skip an exactly-at-floor offer and the isConsumable view agrees with the walk - consume write-back: release a partially-filled slot whose leftover is no longer strictly profitable (unusable dust must not hold slab slots until expiry); below-floor-but-profitable leftovers stay, like any below-floor offer, so a floor change can re-admit them - band pre-liquidation: after the Morpho repay settles, re-quote the position at the entry price and revert (LtvNotReduced) unless the LTV strictly decreased; kills the virtual-share-boundary dust fills whose settled totals leave the LTV flat or higher - revokeOffers: the recorded proposer may revoke its own offer at any time (before and after activeAt); guardian and registry-owner revocation unchanged, authorization now checked per offer --- src/borrow/MorphoBorrowPosition.sol | 93 +++++++-- src/interfaces/borrow/IBorrowOffers.sol | 17 +- src/libs/borrow/LibBorrowErrors.sol | 15 +- src/libs/borrow/LibBorrowOffers.sol | 84 +++++--- test/borrow/LibBorrowOffers.t.sol | 106 +++++++++- test/borrow/MorphoBorrowPositionOffers.t.sol | 199 ++++++++++++++++++- 6 files changed, 449 insertions(+), 65 deletions(-) diff --git a/src/borrow/MorphoBorrowPosition.sol b/src/borrow/MorphoBorrowPosition.sol index 43fb6599..213ddc35 100644 --- a/src/borrow/MorphoBorrowPosition.sol +++ b/src/borrow/MorphoBorrowPosition.sol @@ -416,29 +416,80 @@ contract MorphoBorrowPosition is IBorrowPosition, IBorrowOffers, Initializable, /// `_isHealthy(LLTV)` holds; every fill strictly lowers LTV, so the post-withdraw state that /// Morpho re-checks inside `withdrawCollateral` retains margin. The conservative rounding in /// the fill math is load-bearing at the knife edge. + /// + /// Settlement guard: the walk's per-fill checks run against a snapshot of Morpho's totals, + /// and on a virtual-share boundary the settled totals can round the remaining debt such + /// that an accepted dust-sized fill leaves the LTV flat or higher (and repeated fills could + /// walk it past `liquidationLtv` into the proportional path). After the repay settles, the + /// position is re-quoted at the entry price and the call reverts unless the LTV strictly + /// decreased ({_checkLtvReduced}); with the entry LTV at or below `liquidationLtv`, a + /// strict decrease also keeps the settled LTV within the band's bound. function _offerPreLiquidate(address borrower, uint256 seizedAssets, uint256 repaidShares, bytes calldata data) internal returns (uint256, uint256) { + // One oracle read for the whole call: the walk, the entry quote and the settlement guard all + // price collateral identically, so a mid-call price move (e.g. from the liquidator callback) + // cannot skew the entry/exit comparison. + uint256 price = IOracle(_borrowPositionStorage().marketParams.oracle).price(); + (uint256 entryDebtValue, uint256 entryCollateralValue) = _positionLtvFrame(price); + // The walk reads market/position once (post-accrual, so exact) and applies all offer mutations // as effects before the repay below. Scoped in a helper to keep this function's stack shallow. - (uint256 totalSeized, uint256 totalDebtShares) = _consumeOffers(borrower, seizedAssets, repaidShares); + (uint256 totalSeized, uint256 totalDebtShares) = _consumeOffers(borrower, seizedAssets, repaidShares, price); // Mandatory: a zero-amount repay would hit Morpho's exactlyOneZero (INCONSISTENT_INPUT); this // gives liquidators a clear signal when the band is entered but nothing is fillable. if (totalSeized == 0 || totalDebtShares == 0) revert LibBorrowErrors.NoConsumableOffer(); - bytes memory callbackData = abi.encode(totalSeized, borrower, msg.sender, data); - (uint256 repaidAssets,) = - MORPHO.repay(_borrowPositionStorage().marketParams, 0, totalDebtShares, borrower, callbackData); + uint256 repaidAssets = _settleOfferRepay(borrower, totalSeized, totalDebtShares, data); + + _checkLtvReduced(price, entryDebtValue, entryCollateralValue); return (totalSeized, repaidAssets); } - /// @dev Reads the post-accrual market/position snapshot and runs the offer consume walk. Returns + /// @dev Performs the single shares-mode Morpho repay that settles a band pre-liquidation. + /// Scoped out of {_offerPreLiquidate} so the callback bytes do not coexist on its stack + /// with the entry-frame values (this codebase compiles without the via-IR pipeline). + function _settleOfferRepay(address borrower, uint256 totalSeized, uint256 totalDebtShares, bytes calldata data) + internal + returns (uint256 repaidAssets) + { + bytes memory callbackData = abi.encode(totalSeized, borrower, msg.sender, data); + (repaidAssets,) = MORPHO.repay(_borrowPositionStorage().marketParams, 0, totalDebtShares, borrower, callbackData); + } + + /// @dev Quotes the position's debt and collateral values at `price` from the current Morpho + /// state (post-accrual totals; debt rounds up, collateral rounds down, matching + /// {_isHealthy}). Called by {_offerPreLiquidate} before the consume and after the repay + /// settles, so the two frames differ only by the fill's real effect on Morpho's totals. + function _positionLtvFrame(uint256 price) internal view returns (uint256 debtValue, uint256 collateralValue) { + BorrowPositionStorage storage _storage = _borrowPositionStorage(); + Position memory position = MORPHO.position(_storage.marketId, address(this)); + Market memory market = MORPHO.market(_storage.marketId); + debtValue = + uint256(position.borrowShares).toAssetsUp(uint256(market.totalBorrowAssets), uint256(market.totalBorrowShares)); + collateralValue = uint256(position.collateral).mulDiv(price, ORACLE_PRICE_SCALE); + } + + /// @dev Reverts {LibBorrowErrors.LtvNotReduced} unless the settled position's LTV is strictly + /// below the entry frame's: `exitDebt / exitCollateral < entryDebt / entryCollateral`, + /// compared exactly by cross-multiplication (`fullMulDiv` floors, so + /// `floor(a / d) < y <=> a < y * d` makes the comparison exact). `entryDebtValue > 0` in + /// the band (the position has debt, rounded up). + function _checkLtvReduced(uint256 price, uint256 entryDebtValue, uint256 entryCollateralValue) internal view { + (uint256 exitDebtValue, uint256 exitCollateralValue) = _positionLtvFrame(price); + if (FixedPointMathLib.fullMulDiv(exitDebtValue, entryCollateralValue, entryDebtValue) >= exitCollateralValue) { + revert LibBorrowErrors.LtvNotReduced(); + } + } + + /// @dev Reads the post-accrual market/position snapshot and runs the offer consume walk at the + /// caller-provided oracle `price` (read once per call in {_offerPreLiquidate}). Returns /// the aggregate `(totalSeized, totalDebtShares)`. Split out of {_offerPreLiquidate} so the /// large `Market`/`Position` memory structs do not coexist on the stack with the repay. - function _consumeOffers(address borrower, uint256 seizedAssets, uint256 repaidShares) + function _consumeOffers(address borrower, uint256 seizedAssets, uint256 repaidShares, uint256 price) internal returns (uint256 totalSeized, uint256 totalDebtShares) { @@ -450,7 +501,7 @@ contract MorphoBorrowPosition is IBorrowPosition, IBorrowOffers, Initializable, LibBorrowOffers.ConsumeInput memory input = LibBorrowOffers.ConsumeInput({ seizedTarget: seizedAssets, repaidSharesTarget: repaidShares, - price: IOracle(_storage.marketParams.oracle).price(), + price: price, totalBorrowAssets: uint256(market.totalBorrowAssets), totalBorrowShares: uint256(market.totalBorrowShares), positionCollateral: uint256(position.collateral), @@ -890,11 +941,10 @@ contract MorphoBorrowPosition is IBorrowPosition, IBorrowOffers, Initializable, /// Both gates evaluate {LibBorrowOffers.isProfitableAboveBonusFloor} (the single /// implementation of the floor's inequality and rounding); the strict profitability /// comparison is split out first only to pick the right error. They are proposal-time - /// filters against the state now; the same - /// checks are re-evaluated per fill at consume time (see {LibBorrowOffers._priceAction}), so - /// a live offer whose bonus later drifts below the floor via price or accrued-interest - /// movement is skipped by the consume walk rather than dragging the band's profitability - /// down (a guardian can also revoke it). + /// filters against the state now; the same checks are re-evaluated per offer at consume + /// time on its whole remaining amounts, so a live offer whose bonus later drifts below the + /// floor via price or accrued-interest movement is skipped by the consume walk rather than + /// dragging the band's profitability down (a guardian can also revoke it). function _checkOfferProfitable(uint128 collateral, uint128 debtShares, uint16 minBonusBps) internal view { BorrowPositionStorage storage _storage = _borrowPositionStorage(); (,, uint256 totalBorrowAssets, uint256 totalBorrowShares) = @@ -908,19 +958,26 @@ contract MorphoBorrowPosition is IBorrowPosition, IBorrowOffers, Initializable, } /// @inheritdoc IBorrowOffers - /// @dev Gated by {IBorrowOffersRegistry.checkCanRevokeOffer} (a guardian, or the registry - /// owner). Removes each id from the offer book (the guardian veto inside the timelock - /// window, and the kill switch for a bad standing offer at any later stage). Self-revoke - /// by a plain proposer is deliberately not offered: revoke stays with the guardians and - /// the registry owner, who can already remove any offer. + /// @dev Authorization is per offer: the recorded proposer may revoke its own offer at any time + /// (a mistaken offer must not stand just because no guardian reacted before `activeAt`); + /// any other caller must pass {IBorrowOffersRegistry.checkCanRevokeOffer} (a guardian, or + /// the registry owner), the veto inside the timelock window and the kill switch for a bad + /// standing offer at any later stage. Liveness is checked before the proposer read so an + /// unknown id reverts {LibBorrowErrors.OfferNotFound} for every caller; an empty batch + /// performs no authorization check and is a no-op. function revokeOffers(uint8[] calldata ids) external override { - OFFERS_REGISTRY.checkCanRevokeOffer(msg.sender); LibBorrowOffers.BorrowOffersStorage storage o = LibBorrowOffers.borrowOffersStorage(); + bool checkedCanRevokeAny; // the registry guardian/owner check passes at most once // Cache the calldata length once, and each id into a stack variable, so the loop performs a // single CALLDATALOAD per iteration instead of re-reading `ids.length` and `ids[i]` twice each. uint256 length = ids.length; for (uint256 i; i < length; ++i) { uint8 id = ids[i]; + if (!o.isLive(id)) revert LibBorrowErrors.OfferNotFound(); + if (o.slab[id].proposer != msg.sender && !checkedCanRevokeAny) { + OFFERS_REGISTRY.checkCanRevokeOffer(msg.sender); + checkedCanRevokeAny = true; + } o.removeOffer(id); emit OfferRevoked(id, msg.sender); } diff --git a/src/interfaces/borrow/IBorrowOffers.sol b/src/interfaces/borrow/IBorrowOffers.sol index 3a8dce68..a0f8a2b0 100644 --- a/src/interfaces/borrow/IBorrowOffers.sol +++ b/src/interfaces/borrow/IBorrowOffers.sol @@ -24,7 +24,8 @@ pragma solidity ^0.8.22; /// set <=> the slab slot holds a live offer), not by any field of this struct; a freed slot is /// fully zeroed. A live offer always has a non-zero proposer and both remaining amounts > 0. struct Offer { - /// @dev The account that posted the offer (accountability; also used by views). + /// @dev The account that posted the offer. May revoke its own offer at any time (also used by + /// views and for accountability). address proposer; /// @dev Absolute timestamp at which the offer becomes consumable. Fixed at proposal time as /// `proposalTime + effectiveTimelockAtProposal`, so a later timelock change cannot re-time @@ -63,7 +64,8 @@ interface IBorrowOffers { uint40 expiresAt ); - /// @notice Emitted when an offer is revoked (guardian veto / admin / owner kill switch). + /// @notice Emitted when an offer is revoked (proposer self-revoke, guardian veto, or the + /// registry owner's kill switch). /// @param id The slab id of the revoked offer. /// @param by The caller that revoked it. event OfferRevoked(uint8 indexed id, address indexed by); @@ -74,7 +76,8 @@ interface IBorrowOffers { /// @param id The slab id of the consumed offer. /// @param collateralFilled The collateral tokens seized from this offer in this chunk. /// @param debtSharesFilled The borrow shares repaid against this offer in this chunk. - /// @param exhausted True if the offer was fully consumed (and removed) by this chunk. + /// @param exhausted True if this chunk removed the offer: a side was fully consumed, or the + /// dust remainder was no longer strictly profitable (unusable at any bonus floor). event OfferConsumed(uint8 indexed id, uint128 collateralFilled, uint128 debtSharesFilled, bool exhausted); /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ @@ -92,7 +95,8 @@ interface IBorrowOffers { /// @return id The slab id assigned to the new offer. function proposeOffer(uint128 collateral, uint128 debtShares, uint40 expiresAt) external returns (uint8 id); - /// @notice Revokes one or more offers (batch). Gated by + /// @notice Revokes one or more offers (batch). Authorization is per offer: the recorded + /// proposer may revoke its own offer at any time; any other caller must pass /// {IBorrowOffersRegistry.checkCanRevokeOffer} (a guardian, or the registry owner). /// @dev Reverts {LibBorrowErrors.OfferNotFound} if any id is not a currently-live offer. /// @param ids The slab ids to revoke. @@ -116,11 +120,14 @@ interface IBorrowOffers { /// profitable and within the max price at the current position LTV). /// @dev Approximate helper: it evaluates the offer against the current position state in /// isolation and does not account for ordering or the cumulative LTV change of consuming - /// earlier offers. The binding checks are the per-fill checks inside the consume walk. + /// earlier offers. The binding checks run inside the consume walk (profitability and the + /// bonus floor per offer, strict de-risking per fill). function isConsumable(uint8 id) external view returns (bool); /// @notice Off-chain helper that simulates a band consume for the given target without mutating /// state. + /// @dev Simulates the walk only: the post-settlement LTV guard runs solely on the real + /// `preLiquidate` call, so a dust-sized quote from this view can still revert there. /// @param seizedAssets The collateral-seize target (pass 0 to use `repaidShares`). /// @param repaidShares The debt-share-repay target (pass 0 to use `seizedAssets`). /// @return seized The total collateral that would be seized. diff --git a/src/libs/borrow/LibBorrowErrors.sol b/src/libs/borrow/LibBorrowErrors.sol index 50770b31..c2780d57 100644 --- a/src/libs/borrow/LibBorrowErrors.sol +++ b/src/libs/borrow/LibBorrowErrors.sol @@ -40,6 +40,13 @@ library LibBorrowErrors { /// @notice Thrown when attempting to liquidate a healthy position. error PositionHealthy(); + /// @notice Thrown when a band (offer) pre-liquidation settles without strictly lowering the + /// position's LTV. The walk's pre-checks run on a snapshot of Morpho's totals; on a + /// virtual-share boundary the settled totals can round a dust-sized fill into a flat or + /// higher LTV, which repeated fills could walk past `liquidationLtv`, so the settled + /// state is re-checked and such fills revert. + error LtvNotReduced(); + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ /* LTV */ /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ @@ -82,16 +89,16 @@ library LibBorrowErrors { error OfferAmountZero(); /// @notice Thrown at proposal time when the offer is not profitable at the current price - /// (`collateral value <= debt value`). This is a sanity filter only; the binding - /// profitability/de-risking checks are re-evaluated per fill at consume time. + /// (`collateral value <= debt value`). This is a sanity filter only; the binding checks + /// are re-evaluated at consume time (profitability per offer, de-risking per fill). error OfferNotProfitable(); /// @notice Thrown at proposal time when the offer is profitable but its bonus (the excess of the /// collateral value over the debt value, as a fraction of the debt value) is below the /// admin-set minimum. Anti-griefing floor: keeps barely-profitable offers out of the /// book, where they would sort to the head and make band liquidations unattractive. The - /// same floor is re-checked at consume time, where a below-floor fill is skipped (not - /// reverted with this error). + /// same floor is re-checked at consume time on the offer's whole remaining amounts, + /// where a below-floor offer is skipped (not reverted with this error). error OfferBonusTooLow(); /// @notice Thrown when an offer's `expiresAt` is not strictly after its computed `activeAt`, diff --git a/src/libs/borrow/LibBorrowOffers.sol b/src/libs/borrow/LibBorrowOffers.sol index 856c5b8c..506eb405 100644 --- a/src/libs/borrow/LibBorrowOffers.sol +++ b/src/libs/borrow/LibBorrowOffers.sol @@ -24,11 +24,13 @@ import {BPS} from "../Constants.sol"; /// sorts them by effective price at that instant (exact, unlike the storage-order-drift of a /// sorted-at-insert list), and fills cheapest-first. /// -/// Core invariants (binding, re-checked per consumed chunk): -/// - Profitability ({isProfitableAboveBonusFloor}): each fill seizes collateral worth -/// strictly more than the debt value it repays, with at least the minimum bonus on top. -/// - Strict de-risking ({strictlyLowersLtv}): each fill strictly lowers the position LTV, -/// comparing against the running debt/collateral values *before* the fill. +/// Core invariants (binding at consume time): +/// - Profitability with the bonus floor ({isProfitableAboveBonusFloor}): evaluated once per +/// offer on its whole remaining amounts ({_offerClearsFloor}). The floor is scale-invariant +/// (a property of the offer's price, not of a chunk), so an offer exactly at the floor +/// qualifies at any fill size and partitioning a target cannot demote the best offer. +/// - Strict de-risking ({strictlyLowersLtv}): re-checked per consumed chunk, comparing +/// against the running debt/collateral values *before* the fill. /// Conservative rounding (debt values up, collateral values down) makes both checks strict in /// the protocol's favour and is load-bearing for Morpho's own health check at the knife edge. library LibBorrowOffers { @@ -202,7 +204,16 @@ library LibBorrowOffers { for (uint256 i; i < walkLength; ++i) { WalkOffer memory walkOffer = offers[i]; if (walkOffer.filledCollateral == 0) continue; // untouched by the walk - bool exhausted = (walkOffer.remainingCollateral == 0 || walkOffer.remainingDebtShares == 0); + // A slot is also released when the leftover of a partial fill is no longer strictly + // profitable (floor of 0): such a remainder cannot be consumed at any configured bonus + // floor, and keeping it live would let dust hold the book's slots until expiry. The walk + // admitted this offer's pre-fill amounts and a fill never worsens the remainder's ratio + // (fill shares round up), so only value rounding on an atom-scale remainder fails here. A + // below-floor-but-profitable leftover is deliberately kept: like any below-floor offer it + // is re-admitted by a floor decrease or a price move, and must not be destroyed over a + // one-atom rounding of the mutable floor. + bool exhausted = (walkOffer.remainingCollateral == 0 || walkOffer.remainingDebtShares == 0) + || !_offerClearsFloor(inp, walkOffer.remainingCollateral, walkOffer.remainingDebtShares, 0); if (exhausted) { clearBits |= uint256(1) << walkOffer.id; delete s.slab[walkOffer.id]; @@ -351,8 +362,9 @@ library LibBorrowOffers { /// offer's fixed ratio, so a nearly-repaid position can never yield the liquidator more /// collateral per share than the proposer authorized. /// - /// The binding checks (profitability, the consume-time bonus floor, strict de-risking) and - /// their conservative (protocol-favorable) rounding are evaluated in {_priceAction}. + /// The profitability and consume-time bonus-floor gate is evaluated here on the offer's + /// whole remaining amounts ({_offerClearsFloor}); the strict de-risking check on the final + /// fill amounts is evaluated in {_priceAction}. Both round conservatively (protocol-favorable). /// /// The position's remaining collateral/shares before this fill (`remainingPositionCollateral` /// / `remainingPositionShares`) are derived from `inp` and the running totals here rather than @@ -369,6 +381,14 @@ library LibBorrowOffers { // requires both amounts > 0 and the walk deletes an offer the moment a side hits zero. if (remainingCollateral == 0 || remainingDebtShares == 0) return (FillAction.Skip, 0, 0); + // Profitability + bonus floor, evaluated on the offer's whole remaining amounts (the floor is + // a property of the offer's price, so it must not depend on how the target is partitioned; see + // {_offerClearsFloor}). Skip, not stop: the walk is sorted ascending in price and a + // higher-bonus offer later can still qualify. + if (!_offerClearsFloor(inp, remainingCollateral, remainingDebtShares, inp.minOfferBonusBps)) { + return (FillAction.Skip, 0, 0); + } + uint256 remainingPositionCollateral = inp.positionCollateral - totalSeized; uint256 remainingPositionShares = inp.positionBorrowShares - totalDebtShares; @@ -406,25 +426,22 @@ library LibBorrowOffers { } if (fillShares == 0) return (FillAction.Skip, 0, 0); // cannot charge any shares for this collateral - // The profitability, bonus-floor and de-risking checks are evaluated in a separate frame - // ({_priceAction}) to keep this function's stack within the limits of the non-via-IR - // pipeline. `inp` is passed by reference - // (one stack slot) rather than unpacking its price/totals/floor fields, which keeps the callee - // shallow enough to add the bonus-floor check without spilling. + // The strict de-risking check is evaluated in a separate frame ({_priceAction}) to keep this + // function's stack within the limits of the non-via-IR pipeline. `inp` is passed by reference + // (one stack slot) rather than unpacking its price/totals fields, which keeps the callee + // shallow. FillAction action = _priceAction(inp, fillCollateral, fillShares, remainingPositionCollateral, remainingPositionShares); if (action == FillAction.Consume) return (FillAction.Consume, fillCollateral, fillShares); return (action, 0, 0); } - /// @dev Evaluates the binding per-fill checks on the final fill amounts (pure). - /// - Profitability + bonus floor ({isProfitableAboveBonusFloor}): not met => - /// {FillAction.Skip}. The offer's price is fixed by its terms, so an unprofitable or - /// below-floor offer is skipped (not stopped): the walk is sorted ascending in price, and - /// a below-floor offer can be followed by higher-bonus offers that still qualify. - /// - Strict de-risking ({strictlyLowersLtv}): not met => {FillAction.Stop} (over max price). + /// @dev Evaluates the strict de-risking check ({strictlyLowersLtv}) on the final fill amounts + /// (pure): not met => {FillAction.Stop} (over max price; the walk is sorted ascending in + /// price and not consuming freezes the LTV, so nothing later can qualify). The + /// profitability/bonus-floor gate is per offer, not per fill: see {_offerClearsFloor}. /// Collateral values (`seizedValue`/`remainingCollateralValue`) round down and debt values - /// (`repaidDebtValue`/`remainingDebtValue`) round up, so the checks are strict in the + /// (`repaidDebtValue`/`remainingDebtValue`) round up, so the check is strict in the /// protocol's favour. `repaidDebtValue > 0` since `fillShares >= 1`. function _priceAction( ConsumeInput memory inp, @@ -435,7 +452,6 @@ library LibBorrowOffers { ) private pure returns (FillAction) { uint256 seizedValue = fillCollateral.mulDiv(inp.price, ORACLE_PRICE_SCALE); uint256 repaidDebtValue = fillShares.toAssetsUp(inp.totalBorrowAssets, inp.totalBorrowShares); - if (!isProfitableAboveBonusFloor(seizedValue, repaidDebtValue, inp.minOfferBonusBps)) return FillAction.Skip; uint256 remainingDebtValue = remainingPositionShares.toAssetsUp(inp.totalBorrowAssets, inp.totalBorrowShares); uint256 remainingCollateralValue = remainingPositionCollateral.mulDiv(inp.price, ORACLE_PRICE_SCALE); @@ -446,6 +462,27 @@ library LibBorrowOffers { return FillAction.Consume; } + /// @dev Evaluates the profitability gate with a `minBonusBps` floor on an offer's whole + /// remaining amounts (pure). The gate is deliberately not evaluated on the rounded + /// per-fill amounts: the floor is scale-invariant on exact values, but a chunk's rounded-up + /// shares price the chunk a hair below the offer's nominal bonus, so a partial fill against + /// an exactly-at-floor offer (the most owner-favorable admissible price) would miss the + /// floor by one raw share and hand the fill to a worse offer. Whole-remaining evaluation + /// makes the decision independent of how the liquidator partitions the target, and is the + /// same expression as {consumableAtPrice}, so the `isConsumable` view agrees with the walk. + /// The walk passes the configured `inp.minOfferBonusBps`; the release check in {consume} + /// passes 0 (strict profitability only). + function _offerClearsFloor( + ConsumeInput memory inp, + uint256 remainingCollateral, + uint256 remainingDebtShares, + uint256 minBonusBps + ) private pure returns (bool) { + uint256 offerValue = remainingCollateral.mulDiv(inp.price, ORACLE_PRICE_SCALE); + uint256 offerDebtValue = remainingDebtShares.toAssetsUp(inp.totalBorrowAssets, inp.totalBorrowShares); + return isProfitableAboveBonusFloor(offerValue, offerDebtValue, minBonusBps); + } + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ /* CANONICAL CHECKS */ /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ @@ -454,7 +491,7 @@ library LibBorrowOffers { /// value must strictly exceed the repaid debt value, by at least `minBonusBps` basis /// points of that debt value. /// @dev Single source of truth for the floor's inequality and rounding (`mulDivUp`, so the floor - /// is conservative), shared by the consume walk ({_priceAction}), the `isConsumable` view + /// is conservative), shared by the consume walk ({_offerClearsFloor}), the `isConsumable` view /// ({consumableAtPrice}) and the proposal-time admission filter /// ({MorphoBorrowPosition._checkOfferProfitable}). `minBonusBps == 0` disables the floor /// (the required excess is 0) and only strict profitability governs. @@ -520,7 +557,8 @@ library LibBorrowOffers { /// offer) would pass the profitability, bonus-floor and de-risking gates against the /// current whole-position state. /// @dev Pure helper for the `isConsumable` view, built from the same {isProfitableAboveBonusFloor} - /// / {strictlyLowersLtv} checks as the consume walk so the view agrees with a real consume. It + /// / {strictlyLowersLtv} checks as the consume walk (whose floor gate, {_offerClearsFloor}, + /// also evaluates the whole remaining amounts) so the view agrees with a real consume. It /// evaluates the offer's price vs the current LTV in isolation: it does not account for the /// cumulative LTV change of consuming cheaper offers first. Returns false if the position /// has no debt (nothing to liquidate) or the offer is degenerate; pass `minBonusBps == 0` to diff --git a/test/borrow/LibBorrowOffers.t.sol b/test/borrow/LibBorrowOffers.t.sol index 79c5c537..977579df 100644 --- a/test/borrow/LibBorrowOffers.t.sol +++ b/test/borrow/LibBorrowOffers.t.sol @@ -5,6 +5,7 @@ import {Test} from "forge-std/Test.sol"; import {Vm} from "forge-std/Vm.sol"; import {BorrowOffersHarness} from "test/mock/borrow/BorrowOffersHarness.sol"; import {LibBorrowOffers} from "src/libs/borrow/LibBorrowOffers.sol"; +import {SharesMathLib} from "src/libs/borrow/SharesMathLib.sol"; import {LibBorrowErrors} from "src/libs/borrow/LibBorrowErrors.sol"; import {IBorrowOffers, Offer} from "src/interfaces/borrow/IBorrowOffers.sol"; import {MAX_OFFERS} from "src/libs/borrow/LibBorrowOffersConstants.sol"; @@ -24,6 +25,8 @@ import {ORACLE_PRICE_SCALE} from "lib/morpho-blue/src/libraries/ConstantsLib.sol /// scenarios that a bounded invariant run might not reliably hit (e.g. exactly filling the /// slab, then revoking and recycling specific ids). contract LibBorrowOffersTest is Test { + using SharesMathLib for uint256; + BorrowOffersHarness internal h; /// @dev Fixed veto window applied to every inserted offer (the timelock configuration itself @@ -418,6 +421,66 @@ contract LibBorrowOffersTest is Test { _assertStructure(); } + /// @notice An offer exactly at the bonus floor fills for a PARTIAL target: the floor is + /// evaluated on the offer's whole remaining amounts, not on the rounded chunk (whose + /// rounded-up share fill prices it a hair below the floor), so partitioning a target + /// cannot skip the most owner-favorable admissible offer. + function test_consume_atFloorOffer_partialFillNotSkipped() public { + uint40 active = uint40(block.timestamp) + TIMELOCK; + // 101 collateral for 100 debt shares at unit price and exact share:asset conversion: exactly + // the 1% floor (excess 1 == ceil(100 * 100 / 10000)). + uint8 id = h.insert(address(0xA0), active, active + 30 days, 101, 100); + vm.warp(active + 1); + + // An odd 51-atom chunk charges ceil(51 * 100 / 101) = 51 shares: evaluated per chunk it would + // be seizedValue 51 vs repaid 51 (not even strictly profitable) and the offer would be skipped. + LibBorrowOffers.ConsumeInput memory inp = _input(51, 0); + inp.minOfferBonusBps = 100; + (uint256 seized, uint256 repaid) = h.consume(inp); + + assertEq(seized, 51, "at-floor offer fills the partial target"); + assertEq(repaid, 51, "chunk shares round up"); + assertTrue(h.isLive(id), "at-floor remainder stays live"); + assertEq(h.slabAt(id).remainingCollateral, 50, "collateral decremented"); + assertEq(h.slabAt(id).remainingDebtShares, 49, "debt shares decremented"); + _assertStructure(); + } + + /// @notice A partial fill whose leftover cannot clear the profitability gate is released + /// (exhausted write-back, slot freed) instead of staying in `liveBits` as a + /// live-but-unfillable dust remainder until expiry or a revoke. + function test_consume_unusableDustRemainder_clearsSlot() public { + uint40 active = uint40(block.timestamp) + TIMELOCK; + uint8 id = h.insert(address(0xA0), active, active + 30 days, 210, 100); + vm.warp(active + 1); + + // At half the unit price the whole offer is profitable (value 105 > debt 100), but the 5-atom + // leftover of a 205-atom fill values to floor(5 / 2) = 2 against 2 debt atoms: no future fill + // from it can pass the profitability gate, so the write-back must release the slot. + LibBorrowOffers.ConsumeInput memory inp = _input(205, 0); + inp.price = ORACLE_PRICE_SCALE / 2; + + vm.recordLogs(); + (uint256 seized, uint256 repaid) = h.consume(inp); + assertEq(seized, 205, "target met"); + assertEq(repaid, 98, "fill shares round up (ceil(205 * 100 / 210))"); + + assertFalse(h.isLive(id), "dust remainder released the slot"); + assertEq(h.slabAt(id).proposer, address(0), "slot zeroed"); + Vm.Log[] memory logs = vm.getRecordedLogs(); + uint256 n; + for (uint256 i; i < logs.length; ++i) { + if (logs[i].topics[0] != IBorrowOffers.OfferConsumed.selector) continue; + (uint128 collFilled, uint128 sharesFilled, bool exhausted) = abi.decode(logs[i].data, (uint128, uint128, bool)); + assertEq(collFilled, 205, "partial fill collateral reported"); + assertEq(sharesFilled, 98, "partial fill shares reported"); + assertTrue(exhausted, "dust release reported as exhausted"); + ++n; + } + assertEq(n, 1, "one offer touched"); + _assertStructure(); + } + /// @notice A slab full of EXPIRED offers self-heals: the allocator prunes expired slots before /// allocating, so a new insert succeeds (recycling the lowest pruned id) instead of /// reverting {TooManyOffers}, and every pruned slot is fully zeroed with `liveBits` in @@ -614,9 +677,9 @@ contract LibBorrowOffersTest is Test { // The OfferConsumed events are the walk's own account of its fills: they must sum to the // returned totals, fire in ascending-price order (on the pre-consume amounts), and match the - // post-consume storage exactly (partial fills decrement BOTH sides; an exhausted offer had one - // side filled completely and its slot deleted). - _assertConsumedEventsMatchState(before, actSeized, actShares); + // post-consume storage exactly (partial fills decrement BOTH sides; an exhausted offer had a + // side filled completely, or its leftover failed the floor gate, and its slot deleted). + _assertConsumedEventsMatchState(before, inp, actSeized, actShares); // Post-consume storage consistency, offer by offer. uint256 seizedAccounted; @@ -652,7 +715,14 @@ contract LibBorrowOffersTest is Test { /// @dev Decodes the recorded OfferConsumed events and asserts they agree with the pre-consume /// snapshot, the post-consume storage and the returned totals (see the fuzz call site). - function _assertConsumedEventsMatchState(Offer[] memory before, uint256 actSeized, uint256 actShares) internal { + /// `inp` supplies the walk's market snapshot, used to re-derive the floor gate for + /// exhausted-by-dust-release offers. + function _assertConsumedEventsMatchState( + Offer[] memory before, + LibBorrowOffers.ConsumeInput memory inp, + uint256 actSeized, + uint256 actShares + ) internal { Vm.Log[] memory logs = vm.getRecordedLogs(); uint256 sumColl; uint256 sumShares; @@ -690,10 +760,11 @@ contract LibBorrowOffersTest is Test { assertFalse(h.isLive(uint8(id)), "exhausted offer still live"); assertLe(collFilled, before[id].remainingCollateral, "fill exceeds offered collateral"); assertLe(sharesFilled, before[id].remainingDebtShares, "fill exceeds offered debt shares"); - assertTrue( - collFilled == before[id].remainingCollateral || sharesFilled == before[id].remainingDebtShares, - "exhausted offer had neither side filled completely" - ); + // An offer is released when a side was filled completely, or (dust release) when its + // leftover is no longer strictly profitable at the walk's snapshot. + if (collFilled != before[id].remainingCollateral && sharesFilled != before[id].remainingDebtShares) { + _assertLeftoverUnprofitable(before[id], inp, collFilled, sharesFilled); + } } else { assertTrue(h.isLive(uint8(id)), "partially-filled offer not live"); assertEq( @@ -708,6 +779,25 @@ contract LibBorrowOffersTest is Test { assertEq(sumShares, actShares, "event fills do not sum to repaid total"); } + /// @dev Asserts a dust-released offer's leftover fails strict profitability (a zero floor) at + /// the walk's snapshot (the only way a partially-filled offer may be released). Separate + /// frame to keep {_assertConsumedEventsMatchState} within the non-via-IR stack limits. + function _assertLeftoverUnprofitable( + Offer memory pre, + LibBorrowOffers.ConsumeInput memory inp, + uint128 collFilled, + uint128 sharesFilled + ) internal pure { + uint256 leftoverValue = + (uint256(pre.remainingCollateral) - collFilled) * inp.price / ORACLE_PRICE_SCALE; + uint256 leftoverDebtValue = + (uint256(pre.remainingDebtShares) - sharesFilled).toAssetsUp(inp.totalBorrowAssets, inp.totalBorrowShares); + assertFalse( + LibBorrowOffers.isProfitableAboveBonusFloor(leftoverValue, leftoverDebtValue, 0), + "released offer's leftover is still strictly profitable" + ); + } + /// @dev Full structural integrity check (mirrors {LibBorrowOffersInvariantTest}'s invariants): /// the bitmap and the slab agree slot by slot (bit set <=> live, non-degenerate offer; bit /// clear <=> fully zeroed slot), no bits beyond `MAX_OFFERS`, and `count`/`listOffers` diff --git a/test/borrow/MorphoBorrowPositionOffers.t.sol b/test/borrow/MorphoBorrowPositionOffers.t.sol index 05c50826..57272279 100644 --- a/test/borrow/MorphoBorrowPositionOffers.t.sol +++ b/test/borrow/MorphoBorrowPositionOffers.t.sol @@ -270,18 +270,20 @@ contract MorphoBorrowPositionOffersTest is Test { } function test_revokeOffers_revertsForNonGuardian() public { + _enterBand(0.7e18); uint8[] memory ids = new uint8[](1); - ids[0] = 0; + ids[0] = _proposeAtPrice(_positionShares() / 4, 1.2e18); vm.prank(makeAddr("rando")); vm.expectRevert(Unauthorized.selector); pos.revokeOffers(ids); } - /// @notice The position owner (PositionManager) cannot revoke either: revoke stays with the - /// registry guardians and the registry owner. + /// @notice The position owner (PositionManager) cannot revoke someone else's offer: revoke + /// stays with the offer's proposer, the registry guardians and the registry owner. function test_revokeOffers_revertsForPositionOwner() public { + _enterBand(0.7e18); uint8[] memory ids = new uint8[](1); - ids[0] = 0; + ids[0] = _proposeAtPrice(_positionShares() / 4, 1.2e18); vm.prank(positionManager); vm.expectRevert(Unauthorized.selector); pos.revokeOffers(ids); @@ -337,6 +339,8 @@ contract MorphoBorrowPositionOffersTest is Test { _enterBand(0.7e18); uint256 shares = _positionShares() / 4; uint128 coll = _collForPrice(shares, 1.2e18); // precompute (external calls would consume the prank) + // A standing offer from a plain proposer, the target of the old owner's revoke attempt below. + uint8 standing = _propose(coll, uint128(shares)); // Two-step handover: the new owner requests, the current owner completes. address newOwner = makeAddr("newRegistryOwner"); @@ -345,23 +349,27 @@ contract MorphoBorrowPositionOffersTest is Test { vm.prank(registryOwner); registry.completeOwnershipHandover(newOwner); - // The old owner is a stranger to the registry now: no propose, no revoke. + // The old owner is a stranger to the registry now: no propose, no revoke of others' offers. vm.prank(registryOwner); vm.expectRevert(Unauthorized.selector); pos.proposeOffer(coll, uint128(shares), uint40(block.timestamp + 30 days)); uint8[] memory ids = new uint8[](1); - ids[0] = 0; + ids[0] = standing; vm.prank(registryOwner); vm.expectRevert(Unauthorized.selector); pos.revokeOffers(ids); - // The new owner holds the full owner powers by derivation. + // The new owner holds the full owner powers by derivation: propose, revoke its own offer, + // and revoke the plain proposer's standing offer. vm.prank(newOwner); uint8 id = pos.proposeOffer(coll, uint128(shares), uint40(block.timestamp + 30 days)); assertEq(pos.offer(id).proposer, newOwner, "new registry owner may propose"); ids[0] = id; vm.prank(newOwner); pos.revokeOffers(ids); + ids[0] = standing; + vm.prank(newOwner); + pos.revokeOffers(ids); assertEq(pos.offerCount(), 0, "new registry owner may revoke"); } @@ -778,6 +786,100 @@ contract MorphoBorrowPositionOffersTest is Test { assertEq(n, 2, "both offers touched"); } + /// @notice Settlement guard regression: a dust-sized fill can pass the walk's snapshot checks + /// while Morpho's settled totals round the remaining debt back to its pre-fill value, + /// so the LTV does not strictly decrease (repeated flat fills would bleed collateral + /// and walk the LTV upward). The call must revert instead of committing such a fill, + /// in both target modes; the walk-only previewConsume still quotes the fill (the guard + /// runs solely on the real call). + function test_preLiquidate_dustFillLeavesLtvFlat_reverts() public { + _enterBand(0.7e18); + _proposeAtPrice(_positionShares() / 2, 1.2e18); + _warpActive(); + + // A one-atom seize target repays fewer borrow shares than one loan atom is worth (Morpho's + // virtual ratio is 1e6 shares per asset), so the settled remaining debt re-rounds up to its + // entry value while the position's collateral (value) drops by one atom. + (uint256 previewSeized, uint256 previewShares) = pos.previewConsume(1, 0); + assertEq(previewSeized, 1, "the walk itself quotes the dust fill"); + assertLt(previewShares, 1e6, "dust fill repays less than one loan atom in shares"); + vm.expectRevert(LibBorrowErrors.LtvNotReduced.selector); + pos.preLiquidate(address(pos), 1, 0, ""); + + // Same flat fill through the repaidShares-mode target: the guard takes no mode-dependent + // input, so the shares-mode dust call must revert identically. + vm.expectRevert(LibBorrowErrors.LtvNotReduced.selector); + pos.preLiquidate(address(pos), 0, previewShares, ""); + } + + /// @notice Partition invariance: splitting a seize target across two calls must fill from the + /// same (best) offer as the single-call fill. The bonus floor is evaluated on the + /// offer's whole remaining amounts, so an exactly-at-floor offer (the most + /// owner-favorable admissible price) cannot be skipped for a worse one just because a + /// chunk's rounded-up share fill prices it a hair below the floor. + function test_consume_partitionedTarget_doesNotSkipAtFloorOffer() public { + _enterBand(0.7e18); + uint256 shares = _positionShares() / 4; + + // Offer A sits on the 1% bonus floor (smallest admissible collateral, mirroring the + // proposal-time boundary in {test_proposeOffer_minBonusExactBoundary}, plus a few atoms of + // slack so the post-chunk remainder's floored value stays admissible too; the zero-slack + // chunk decision is pinned in LibBorrowOffers.t.sol); offer B is clearly worse for the owner. + (uint256 tba, uint256 tbs) = _borrowTotals(); + uint256 debt = shares.toAssetsUp(tba, tbs); + uint256 minExcess = (debt * MIN_OFFER_BONUS_BPS + BPS - 1) / BPS; + uint256 price = oracle.price(); + uint128 collA = uint128(((debt + minExcess) * SCALE + price - 1) / price) + 10; + uint8 a = _propose(collA, uint128(shares)); + uint8 b = _proposeAtPrice(shares, 1.3e18); + uint128 collB = pos.offer(b).remainingCollateral; + _warpActive(); + + (uint256 fullSeized,) = pos.previewConsume(collA, 0); + assertEq(fullSeized, collA, "a single call would fill all of A"); + + // Split A's collateral into two odd-sized chunks (the shape that baits per-chunk rounding). + uint256 firstChunk = uint256(collA) / 2 + 1; + (uint256 seized1,) = pos.preLiquidate(address(pos), firstChunk, 0, ""); + (uint256 seized2,) = pos.preLiquidate(address(pos), collA - firstChunk, 0, ""); + + assertEq(seized1 + seized2, fullSeized, "split target seized the same total collateral"); + assertEq(pos.offer(a).proposer, address(0), "at-floor offer A fully consumed across the split"); + assertEq(pos.offer(b).remainingCollateral, collB, "worse offer B untouched by either chunk"); + assertEq(pos.offerCount(), 1, "only A removed"); + } + + /// @notice A partial fill that leaves an offer holding only unusable dust releases the slot: + /// the leftover cannot clear the profitability gate, so keeping it live would hold a + /// slab slot (and, across the whole book, block new proposals and starve the band) + /// until expiry or a revoke. + function test_consume_unusableDustRemainder_releasesSlot() public { + _enterBand(0.7e18); + uint8 id = _proposeAtPrice(_positionShares() / 2, 1.2e18); + _warpActive(); + + // Seize all but three collateral atoms: the leftover's value rounds down to less than the + // rounded-up debt it still asks, so no future fill from it can pass the gates. + uint128 coll = pos.offer(id).remainingCollateral; + vm.recordLogs(); + (uint256 seized,) = pos.preLiquidate(address(pos), uint256(coll) - 3, 0, ""); + assertEq(seized, uint256(coll) - 3, "target met"); + + // The release is reported as an exhausted consume and the slot is freed for new proposals. + Vm.Log[] memory logs = vm.getRecordedLogs(); + uint256 n; + for (uint256 i; i < logs.length; ++i) { + if (logs[i].emitter != address(pos) || logs[i].topics[0] != IBorrowOffers.OfferConsumed.selector) continue; + (uint128 collFilled,, bool exhausted) = abi.decode(logs[i].data, (uint128, uint128, bool)); + assertEq(collFilled, coll - 3, "partial fill amounts reported"); + assertTrue(exhausted, "dust release reported as exhausted"); + ++n; + } + assertEq(n, 1, "one offer touched"); + assertEq(pos.offerCount(), 0, "dust remainder released the slot"); + assertEq(pos.offer(id).proposer, address(0), "slot zeroed"); + } + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ /* TIMELOCK / EXPIRY */ /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ @@ -1070,6 +1172,89 @@ contract MorphoBorrowPositionOffersTest is Test { pos.revokeOffers(ids); } + /// @notice Liveness is resolved before authorization: an unknown id reverts {OfferNotFound} for + /// every caller, including one with no registry role at all. + function test_revokeOffers_unknownIdRevertsNotFound_forUnauthorizedCaller() public { + uint8[] memory ids = new uint8[](1); + ids[0] = 5; + vm.prank(makeAddr("rando")); + vm.expectRevert(LibBorrowErrors.OfferNotFound.selector); + pos.revokeOffers(ids); + + ids[0] = uint8(MAX_OFFERS); // out-of-range ids take the same path + vm.prank(makeAddr("rando")); + vm.expectRevert(LibBorrowErrors.OfferNotFound.selector); + pos.revokeOffers(ids); + } + + /// @notice The recorded proposer can cancel its own offer while it is still timelocked: a + /// mistaken offer does not have to stand until a guardian reacts. + function test_revokeOffers_proposerCancelsOwnOffer_beforeActive() public { + _enterBand(0.7e18); + uint8 id = _proposeAtPrice(_positionShares() / 4, 1.2e18); + assertLt(block.timestamp, pos.offer(id).activeAt, "offer still timelocked"); + + uint8[] memory ids = new uint8[](1); + ids[0] = id; + vm.expectEmit(true, true, false, false, address(pos)); + emit IBorrowOffers.OfferRevoked(id, proposer); + vm.prank(proposer); + pos.revokeOffers(ids); + assertEq(pos.offerCount(), 0, "proposer cancelled its own timelocked offer"); + assertEq(pos.offer(id).proposer, address(0), "slot freed"); + } + + /// @notice Proposer self-revoke is not limited to the timelock window: the proposer can also + /// pull its own offer once it is active (and consumable). + function test_revokeOffers_proposerCancelsOwnOffer_afterActive() public { + _enterBand(0.7e18); + uint8 id = _proposeAtPrice(_positionShares() / 4, 1.2e18); + _warpActive(); + assertTrue(pos.isConsumable(id), "offer is live and consumable"); + + uint8[] memory ids = new uint8[](1); + ids[0] = id; + vm.prank(proposer); + pos.revokeOffers(ids); + assertEq(pos.offerCount(), 0, "proposer cancelled its own active offer"); + } + + /// @notice Self-revoke does not extend to other proposers' offers: a batch touching a foreign + /// offer requires the guardian/registry-owner power and reverts atomically without it. + function test_revokeOffers_proposerCannotRevokeForeignOffer() public { + _enterBand(0.7e18); + address proposerB = makeAddr("proposerB"); + vm.prank(registryOwner); + registry.setProposer(proposerB, true); + + uint256 shares = _positionShares() / 8; + uint8 own = _proposeAtPrice(shares, 1.2e18); + uint128 collB = _collForPrice(shares, 1.3e18); // precompute (external calls would consume the prank) + vm.prank(proposerB); + uint8 foreign = pos.proposeOffer(collB, uint128(shares), uint40(block.timestamp + 30 days)); + + // The foreign offer alone is out of reach. + uint8[] memory ids = new uint8[](1); + ids[0] = foreign; + vm.prank(proposer); + vm.expectRevert(Unauthorized.selector); + pos.revokeOffers(ids); + + // A mixed batch (own + foreign) reverts atomically: the own offer stays live too. + uint8[] memory mixed = new uint8[](2); + mixed[0] = own; + mixed[1] = foreign; + vm.prank(proposer); + vm.expectRevert(Unauthorized.selector); + pos.revokeOffers(mixed); + assertEq(pos.offerCount(), 2, "both offers still live after the failed batch"); + + // A guardian can revoke the same mixed batch. + vm.prank(guardian); + pos.revokeOffers(mixed); + assertEq(pos.offerCount(), 0, "guardian revoked both"); + } + function test_slab_recyclesFreedSlots() public { _enterBand(0.7e18); uint256 shares = _positionShares() / 8; From 17835254988fe4bbbd0aedff8d92a0f1d0fec655 Mon Sep 17 00:00:00 2001 From: Maxence Raballand Date: Fri, 7 Aug 2026 10:14:41 +0200 Subject: [PATCH 2/3] refactor: per-offer revoke auth via registry boolean read, hoist marketId, silence cast lints Review follow-ups on the offer lifecycle hardening: - revokeOffers resolves the registry guardian/owner power once as a boolean (canRevokeOffer replaces the reverting checkCanRevokeOffer) and the per-offer proposer check moves into LibBorrowOffers.removeOffer; the Unauthorized error keeps Solady's selector so external behavior is unchanged - _positionLtvFrame and _consumeOffers read marketId from storage once - _checkLtvReduced NatSpec records why the cross-multiplication needs fullMulDiv's 512-bit intermediate (price-scaled values can overflow 256 bits) - forge 1.5.1 unsafe-typecast lints in LibBorrowOffers silenced with justified disable comments - new tests: empty revoke batch no-op, guardian revoke of an active offer, per-offer OfferRevoked events in a batch --- src/borrow/BorrowOffersRegistry.sol | 4 +- src/borrow/MorphoBorrowPosition.sol | 42 +++++++-------- src/interfaces/borrow/IBorrowOffers.sol | 9 ++-- .../borrow/IBorrowOffersRegistry.sol | 12 +++-- src/libs/borrow/LibBorrowErrors.sol | 6 +++ src/libs/borrow/LibBorrowOffers.sol | 19 +++++++ test/borrow/BorrowOffersRegistry.t.sol | 31 +++++------ test/borrow/MorphoBorrowPositionOffers.t.sol | 51 +++++++++++++++++++ 8 files changed, 123 insertions(+), 51 deletions(-) diff --git a/src/borrow/BorrowOffersRegistry.sol b/src/borrow/BorrowOffersRegistry.sol index e92c78b9..4136014e 100644 --- a/src/borrow/BorrowOffersRegistry.sol +++ b/src/borrow/BorrowOffersRegistry.sol @@ -171,8 +171,8 @@ contract BorrowOffersRegistry is IBorrowOffersRegistry, Initializable, OwnableRo } /// @inheritdoc IBorrowOffersRegistry - function checkCanRevokeOffer(address account) external view override { - _checkHasRolesOrOwner(account, GUARDIAN_ROLE); + function canRevokeOffer(address account) external view override returns (bool) { + return hasAnyRole(account, GUARDIAN_ROLE) || account == owner(); } /// @dev Reverts with Solady's `Unauthorized` unless `account` holds any of `roles` or is the diff --git a/src/borrow/MorphoBorrowPosition.sol b/src/borrow/MorphoBorrowPosition.sol index 213ddc35..95440f69 100644 --- a/src/borrow/MorphoBorrowPosition.sol +++ b/src/borrow/MorphoBorrowPosition.sol @@ -465,9 +465,9 @@ contract MorphoBorrowPosition is IBorrowPosition, IBorrowOffers, Initializable, /// {_isHealthy}). Called by {_offerPreLiquidate} before the consume and after the repay /// settles, so the two frames differ only by the fill's real effect on Morpho's totals. function _positionLtvFrame(uint256 price) internal view returns (uint256 debtValue, uint256 collateralValue) { - BorrowPositionStorage storage _storage = _borrowPositionStorage(); - Position memory position = MORPHO.position(_storage.marketId, address(this)); - Market memory market = MORPHO.market(_storage.marketId); + Id _marketId = _borrowPositionStorage().marketId; + Position memory position = MORPHO.position(_marketId, address(this)); + Market memory market = MORPHO.market(_marketId); debtValue = uint256(position.borrowShares).toAssetsUp(uint256(market.totalBorrowAssets), uint256(market.totalBorrowShares)); collateralValue = uint256(position.collateral).mulDiv(price, ORACLE_PRICE_SCALE); @@ -475,9 +475,13 @@ contract MorphoBorrowPosition is IBorrowPosition, IBorrowOffers, Initializable, /// @dev Reverts {LibBorrowErrors.LtvNotReduced} unless the settled position's LTV is strictly /// below the entry frame's: `exitDebt / exitCollateral < entryDebt / entryCollateral`, - /// compared exactly by cross-multiplication (`fullMulDiv` floors, so - /// `floor(a / d) < y <=> a < y * d` makes the comparison exact). `entryDebtValue > 0` in - /// the band (the position has debt, rounded up). + /// cross-multiplied to `exitDebt * entryCollateral < entryDebt * exitCollateral` so the + /// comparison is exact. The product goes through `fullMulDiv`'s 512-bit intermediate + /// rather than a plain multiplication: the collateral values are price-scaled (`price` is + /// an unbounded oracle value at 1e36 scale) and can exceed 128 bits, so the direct product + /// can overflow 256 bits and checked multiplication would turn a legitimate fill into a + /// spurious revert. Flooring keeps it exact (`floor(a * b / d) >= c <=> a * b >= c * d` + /// for integers). `entryDebtValue > 0` in the band (the position has debt, rounded up). function _checkLtvReduced(uint256 price, uint256 entryDebtValue, uint256 entryCollateralValue) internal view { (uint256 exitDebtValue, uint256 exitCollateralValue) = _positionLtvFrame(price); if (FixedPointMathLib.fullMulDiv(exitDebtValue, entryCollateralValue, entryDebtValue) >= exitCollateralValue) { @@ -493,9 +497,9 @@ contract MorphoBorrowPosition is IBorrowPosition, IBorrowOffers, Initializable, internal returns (uint256 totalSeized, uint256 totalDebtShares) { - BorrowPositionStorage storage _storage = _borrowPositionStorage(); - Position memory position = MORPHO.position(_storage.marketId, borrower); - Market memory market = MORPHO.market(_storage.marketId); + Id _marketId = _borrowPositionStorage().marketId; + Position memory position = MORPHO.position(_marketId, borrower); + Market memory market = MORPHO.market(_marketId); LibBorrowOffers.BorrowOffersStorage storage o = LibBorrowOffers.borrowOffersStorage(); LibBorrowOffers.ConsumeInput memory input = LibBorrowOffers.ConsumeInput({ @@ -960,25 +964,17 @@ contract MorphoBorrowPosition is IBorrowPosition, IBorrowOffers, Initializable, /// @inheritdoc IBorrowOffers /// @dev Authorization is per offer: the recorded proposer may revoke its own offer at any time /// (a mistaken offer must not stand just because no guardian reacted before `activeAt`); - /// any other caller must pass {IBorrowOffersRegistry.checkCanRevokeOffer} (a guardian, or - /// the registry owner), the veto inside the timelock window and the kill switch for a bad - /// standing offer at any later stage. Liveness is checked before the proposer read so an - /// unknown id reverts {LibBorrowErrors.OfferNotFound} for every caller; an empty batch - /// performs no authorization check and is a no-op. + /// any other caller needs the registry's revoke power + /// ({IBorrowOffersRegistry.canRevokeOffer}: a guardian, or the registry owner), read once + /// for the whole batch and enforced per offer inside {LibBorrowOffers.removeOffer}. An + /// empty batch is a no-op. function revokeOffers(uint8[] calldata ids) external override { LibBorrowOffers.BorrowOffersStorage storage o = LibBorrowOffers.borrowOffersStorage(); - bool checkedCanRevokeAny; // the registry guardian/owner check passes at most once - // Cache the calldata length once, and each id into a stack variable, so the loop performs a - // single CALLDATALOAD per iteration instead of re-reading `ids.length` and `ids[i]` twice each. + bool isGuardian = OFFERS_REGISTRY.canRevokeOffer(msg.sender); uint256 length = ids.length; for (uint256 i; i < length; ++i) { uint8 id = ids[i]; - if (!o.isLive(id)) revert LibBorrowErrors.OfferNotFound(); - if (o.slab[id].proposer != msg.sender && !checkedCanRevokeAny) { - OFFERS_REGISTRY.checkCanRevokeOffer(msg.sender); - checkedCanRevokeAny = true; - } - o.removeOffer(id); + o.removeOffer(id, msg.sender, isGuardian); emit OfferRevoked(id, msg.sender); } } diff --git a/src/interfaces/borrow/IBorrowOffers.sol b/src/interfaces/borrow/IBorrowOffers.sol index a0f8a2b0..a41295fa 100644 --- a/src/interfaces/borrow/IBorrowOffers.sol +++ b/src/interfaces/borrow/IBorrowOffers.sol @@ -96,9 +96,12 @@ interface IBorrowOffers { function proposeOffer(uint128 collateral, uint128 debtShares, uint40 expiresAt) external returns (uint8 id); /// @notice Revokes one or more offers (batch). Authorization is per offer: the recorded - /// proposer may revoke its own offer at any time; any other caller must pass - /// {IBorrowOffersRegistry.checkCanRevokeOffer} (a guardian, or the registry owner). - /// @dev Reverts {LibBorrowErrors.OfferNotFound} if any id is not a currently-live offer. + /// proposer may revoke its own offer at any time; any other caller needs the registry's + /// revoke power ({IBorrowOffersRegistry.canRevokeOffer}: a guardian, or the registry + /// owner). + /// @dev Reverts {LibBorrowErrors.OfferNotFound} if any id is not a currently-live offer, and + /// {LibBorrowErrors.Unauthorized} if any offer in the batch belongs to another proposer + /// while the caller lacks the registry's revoke power. /// @param ids The slab ids to revoke. function revokeOffers(uint8[] calldata ids) external; diff --git a/src/interfaces/borrow/IBorrowOffersRegistry.sol b/src/interfaces/borrow/IBorrowOffersRegistry.sol index 31285c1b..5d711975 100644 --- a/src/interfaces/borrow/IBorrowOffersRegistry.sol +++ b/src/interfaces/borrow/IBorrowOffersRegistry.sol @@ -49,11 +49,13 @@ interface IBorrowOffersRegistry { /// a role, so an ownership handover never leaves the new owner without powers. function checkCanCreateOffer(address account) external view; - /// @notice Reverts unless `account` may revoke offers (a guardian, or the registry owner): the - /// veto power inside the timelock window, and the kill switch for a bad standing offer. - /// @dev The authorization check used by {MorphoBorrowPosition.revokeOffers}; reverts with - /// Solady's `Unauthorized()`. - function checkCanRevokeOffer(address account) external view; + /// @notice Returns whether `account` may revoke any offer regardless of its proposer (a + /// guardian, or the registry owner): the veto power inside the timelock window, and the + /// kill switch for a bad standing offer. + /// @dev Read once per batch by {MorphoBorrowPosition.revokeOffers}, which enforces the + /// per-offer proposer check itself for callers without this power. A boolean read rather + /// than a reverting check: a proposer revoking its own offer is authorized without it. + function canRevokeOffer(address account) external view returns (bool); /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ /* CONFIGURATION */ diff --git a/src/libs/borrow/LibBorrowErrors.sol b/src/libs/borrow/LibBorrowErrors.sol index c2780d57..b6c16b2c 100644 --- a/src/libs/borrow/LibBorrowErrors.sol +++ b/src/libs/borrow/LibBorrowErrors.sol @@ -78,6 +78,12 @@ library LibBorrowErrors { /// @notice Thrown when the callback is called by an address other than Morpho. error NotMorpho(); + /// @notice Thrown when the caller of {IBorrowOffers.revokeOffers} is neither the offer's + /// recorded proposer nor a registry guardian/owner. Deliberately the same selector as + /// Solady's `Unauthorized()`, which the registry's own authorization checks revert + /// with, so callers see one selector for every authorization failure. + error Unauthorized(); + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ /* OFFERS */ /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ diff --git a/src/libs/borrow/LibBorrowOffers.sol b/src/libs/borrow/LibBorrowOffers.sol index 506eb405..e62d9769 100644 --- a/src/libs/borrow/LibBorrowOffers.sol +++ b/src/libs/borrow/LibBorrowOffers.sol @@ -143,6 +143,17 @@ library LibBorrowOffers { delete s.slab[id]; } + /// @notice Authorized removal for {IBorrowOffers.revokeOffers}: `isGuardian` callers (the + /// registry's guardian/owner revoke power, resolved once per batch by the caller) may + /// remove any offer; any other caller must be the offer's recorded proposer. + /// @dev Liveness resolves first, so an unknown id reverts {LibBorrowErrors.OfferNotFound} for + /// every caller; an unauthorized caller then reverts {LibBorrowErrors.Unauthorized}. + function removeOffer(BorrowOffersStorage storage s, uint8 id, address caller, bool isGuardian) internal { + if (!isLive(s, id)) revert LibBorrowErrors.OfferNotFound(); + if (!isGuardian && s.slab[id].proposer != caller) revert LibBorrowErrors.Unauthorized(); + removeOffer(s, id); + } + /// @dev Allocates the lowest free slab index and marks it live, or reverts /// {LibBorrowErrors.TooManyOffers} when all `MAX_OFFERS` slots hold unexpired offers. /// Expired offers are pruned first (bit cleared, slot zeroed; housekeeping only, mirroring @@ -162,6 +173,8 @@ library LibBorrowOffers { uint256 freeBits = ~liveBits & ((uint256(1) << MAX_OFFERS) - 1); if (freeBits == 0) revert LibBorrowErrors.TooManyOffers(); id = uint8(LibBit.ffs(freeBits)); + // Only bits below `MAX_OFFERS` (32) survive the mask above, so the cast cannot truncate. + // forge-lint: disable-next-line(unsafe-typecast) s.liveBits = uint32(liveBits | (uint256(1) << id)); } @@ -224,6 +237,8 @@ library LibBorrowOffers { } emit IBorrowOffers.OfferConsumed(walkOffer.id, walkOffer.filledCollateral, walkOffer.filledShares, exhausted); } + // Clearing bits of an already-`uint32` value cannot truncate. + // forge-lint: disable-next-line(unsafe-typecast) if (clearBits != 0) s.liveBits = uint32(s.liveBits & ~clearBits); } @@ -263,6 +278,8 @@ library LibBorrowOffers { } if (block.timestamp < offer.activeAt) continue; WalkOffer memory walkOffer = offers[walkLength]; + // `id` indexes a set bit of the 32-bit live bitmap, so the cast cannot truncate. + // forge-lint: disable-next-line(unsafe-typecast) walkOffer.id = uint8(id); walkOffer.remainingCollateral = offer.remainingCollateral; walkOffer.remainingDebtShares = offer.remainingDebtShares; @@ -331,10 +348,12 @@ library LibBorrowOffers { // CONSUME. `fillCollateral <= remainingCollateral` and `fillShares <= remainingDebtShares` // by construction, so the uint128 casts and subtractions cannot overflow/underflow. + // forge-lint: disable-start(unsafe-typecast) walkOffer.remainingCollateral -= uint128(fillCollateral); walkOffer.remainingDebtShares -= uint128(fillShares); walkOffer.filledCollateral = uint128(fillCollateral); walkOffer.filledShares = uint128(fillShares); + // forge-lint: disable-end(unsafe-typecast) totalSeized += fillCollateral; totalDebtShares += fillShares; diff --git a/test/borrow/BorrowOffersRegistry.t.sol b/test/borrow/BorrowOffersRegistry.t.sol index e4e4839a..2f8401af 100644 --- a/test/borrow/BorrowOffersRegistry.t.sol +++ b/test/borrow/BorrowOffersRegistry.t.sol @@ -162,37 +162,34 @@ contract BorrowOffersRegistryTest is Test { registry.checkCanCreateOffer(proposer); } - function test_CheckCanRevokeOffer_GuardianRoundTrip() public { - // Not a guardian yet: rejected - vm.expectRevert(Unauthorized.selector); - registry.checkCanRevokeOffer(guardian); + function test_CanRevokeOffer_GuardianRoundTrip() public { + // Not a guardian yet: no revoke power + assertFalse(registry.canRevokeOffer(guardian), "non-guardian should have no revoke power"); - // Enabled: passes (no revert) + // Enabled: revoke power granted vm.prank(owner); registry.setGuardian(guardian, true); - registry.checkCanRevokeOffer(guardian); + assertTrue(registry.canRevokeOffer(guardian), "guardian should have revoke power"); assertTrue(registry.hasAnyRole(guardian, registry.GUARDIAN_ROLE()), "guardian role bit should be set"); - // Disabled again: rejected + // Disabled again: revoke power revoked vm.prank(owner); registry.setGuardian(guardian, false); - vm.expectRevert(Unauthorized.selector); - registry.checkCanRevokeOffer(guardian); + assertFalse(registry.canRevokeOffer(guardian), "removed guardian should lose revoke power"); } function test_Checks_OwnerPassesWithoutRoles() public view { // The owner is authorized by derivation, without holding any role bit assertEq(registry.rolesOf(owner), 0, "owner should hold no explicit role"); registry.checkCanCreateOffer(owner); - registry.checkCanRevokeOffer(owner); + assertTrue(registry.canRevokeOffer(owner), "owner should have revoke power by derivation"); } function test_Checks_StrangerRejected() public { vm.expectRevert(Unauthorized.selector); registry.checkCanCreateOffer(stranger); - vm.expectRevert(Unauthorized.selector); - registry.checkCanRevokeOffer(stranger); + assertFalse(registry.canRevokeOffer(stranger), "stranger should have no revoke power"); } function test_Checks_RoleSeparation() public { @@ -201,9 +198,8 @@ contract BorrowOffersRegistryTest is Test { registry.setGuardian(guardian, true); vm.stopPrank(); - // A proposer cannot revoke offers - vm.expectRevert(Unauthorized.selector); - registry.checkCanRevokeOffer(proposer); + // A proposer holds no batch-wide revoke power (it may still revoke its own offers) + assertFalse(registry.canRevokeOffer(proposer), "proposer should have no revoke power"); // A guardian cannot create offers vm.expectRevert(Unauthorized.selector); @@ -525,13 +521,12 @@ contract BorrowOffersRegistryTest is Test { // The new owner is authorized by derivation registry.checkCanCreateOffer(newOwner); - registry.checkCanRevokeOffer(newOwner); + assertTrue(registry.canRevokeOffer(newOwner), "new owner should have revoke power"); // The old owner has no lingering powers vm.expectRevert(Unauthorized.selector); registry.checkCanCreateOffer(owner); - vm.expectRevert(Unauthorized.selector); - registry.checkCanRevokeOffer(owner); + assertFalse(registry.canRevokeOffer(owner), "old owner should lose revoke power"); vm.prank(owner); vm.expectRevert(Unauthorized.selector); diff --git a/test/borrow/MorphoBorrowPositionOffers.t.sol b/test/borrow/MorphoBorrowPositionOffers.t.sol index 57272279..4fa8a687 100644 --- a/test/borrow/MorphoBorrowPositionOffers.t.sol +++ b/test/borrow/MorphoBorrowPositionOffers.t.sol @@ -1255,6 +1255,57 @@ contract MorphoBorrowPositionOffersTest is Test { assertEq(pos.offerCount(), 0, "guardian revoked both"); } + /// @notice An empty batch is a no-op for every caller class: no revert, no state change and no + /// events (the registry revoke-power read is a state-neutral view). + function test_revokeOffers_emptyBatchIsNoOp() public { + _enterBand(0.7e18); + _proposeAtPrice(_positionShares() / 4, 1.2e18); + + uint8[] memory none = new uint8[](0); + vm.recordLogs(); + vm.prank(makeAddr("rando")); + pos.revokeOffers(none); + vm.prank(guardian); + pos.revokeOffers(none); + assertEq(pos.offerCount(), 1, "book untouched by empty batches"); + assertEq(vm.getRecordedLogs().length, 0, "no events emitted"); + } + + /// @notice The guardian power is not limited to the timelock window: a standing (active, + /// consumable) offer can still be pulled at any later stage. + function test_revokeOffers_guardianRevokesActiveOffer() public { + _enterBand(0.7e18); + uint8 id = _proposeAtPrice(_positionShares() / 4, 1.2e18); + _warpActive(); + assertTrue(pos.isConsumable(id), "offer is live and consumable"); + + uint8[] memory ids = new uint8[](1); + ids[0] = id; + vm.prank(guardian); + pos.revokeOffers(ids); + assertEq(pos.offerCount(), 0, "guardian revoked the active offer"); + } + + /// @notice A multi-offer batch emits one {OfferRevoked} per removed offer, each attributed to + /// the batch caller. + function test_revokeOffers_batchEmitsPerOffer() public { + _enterBand(0.7e18); + uint256 shares = _positionShares() / 8; + uint8 a = _proposeAtPrice(shares, 1.2e18); + uint8 b = _proposeAtPrice(shares, 1.3e18); + + uint8[] memory ids = new uint8[](2); + ids[0] = a; + ids[1] = b; + vm.expectEmit(true, true, false, false, address(pos)); + emit IBorrowOffers.OfferRevoked(a, guardian); + vm.expectEmit(true, true, false, false, address(pos)); + emit IBorrowOffers.OfferRevoked(b, guardian); + vm.prank(guardian); + pos.revokeOffers(ids); + assertEq(pos.offerCount(), 0, "both revoked"); + } + function test_slab_recyclesFreedSlots() public { _enterBand(0.7e18); uint256 shares = _positionShares() / 8; From 9dd75eac4d74ef7b9f50d693975af4595df0e05b Mon Sep 17 00:00:00 2001 From: Maxence Raballand Date: Mon, 10 Aug 2026 12:21:14 +0200 Subject: [PATCH 3/3] fix: derive band settlement LTV frame before repay to close reentrancy drain The offer-band pre-liquidation guard re-read live position state after the Morpho repay, whose onPreLiquidate callback (plus permissionless repay / supplyCollateral) could drain the position through the reentrant proportional path and re-supply a dust atom so the post-repay check passed. Compute the settled frame deterministically from the fill amounts before the repay (reproducing Morpho shares-mode rounding), so no callback can move it. --- src/borrow/MorphoBorrowPosition.sol | 82 +++++++----- test/borrow/ReentrancyGuardBypassPoC.t.sol | 140 +++++++++++++++++++++ 2 files changed, 191 insertions(+), 31 deletions(-) create mode 100644 test/borrow/ReentrancyGuardBypassPoC.t.sol diff --git a/src/borrow/MorphoBorrowPosition.sol b/src/borrow/MorphoBorrowPosition.sol index 95440f69..eeb52d45 100644 --- a/src/borrow/MorphoBorrowPosition.sol +++ b/src/borrow/MorphoBorrowPosition.sol @@ -419,20 +419,20 @@ contract MorphoBorrowPosition is IBorrowPosition, IBorrowOffers, Initializable, /// /// Settlement guard: the walk's per-fill checks run against a snapshot of Morpho's totals, /// and on a virtual-share boundary the settled totals can round the remaining debt such - /// that an accepted dust-sized fill leaves the LTV flat or higher (and repeated fills could - /// walk it past `liquidationLtv` into the proportional path). After the repay settles, the - /// position is re-quoted at the entry price and the call reverts unless the LTV strictly - /// decreased ({_checkLtvReduced}); with the entry LTV at or below `liquidationLtv`, a - /// strict decrease also keeps the settled LTV within the band's bound. + /// that an accepted dust-sized fill leaves the LTV flat or higher (and, at the band's upper + /// edge, tips the floored health check into the proportional path). The call reverts unless + /// the fill strictly decreased the LTV ({_checkLtvStrictlyReduced}); with the entry LTV at or + /// below `liquidationLtv`, a strict decrease also keeps the settled LTV within the band's + /// bound. The guard runs before the repay on a frame derived from the fill amounts, so a + /// liquidator callback cannot manipulate the state it reads. function _offerPreLiquidate(address borrower, uint256 seizedAssets, uint256 repaidShares, bytes calldata data) internal returns (uint256, uint256) { - // One oracle read for the whole call: the walk, the entry quote and the settlement guard all - // price collateral identically, so a mid-call price move (e.g. from the liquidator callback) - // cannot skew the entry/exit comparison. + // One oracle read for the whole call: the walk and the settlement guard price collateral + // identically, so a mid-call price move (e.g. from the liquidator callback) cannot skew the + // entry/exit comparison. uint256 price = IOracle(_borrowPositionStorage().marketParams.oracle).price(); - (uint256 entryDebtValue, uint256 entryCollateralValue) = _positionLtvFrame(price); // The walk reads market/position once (post-accrual, so exact) and applies all offer mutations // as effects before the repay below. Scoped in a helper to keep this function's stack shallow. @@ -442,9 +442,15 @@ contract MorphoBorrowPosition is IBorrowPosition, IBorrowOffers, Initializable, // gives liquidators a clear signal when the band is entered but nothing is fillable. if (totalSeized == 0 || totalDebtShares == 0) revert LibBorrowErrors.NoConsumableOffer(); - uint256 repaidAssets = _settleOfferRepay(borrower, totalSeized, totalDebtShares, data); + // Settlement guard, evaluated BEFORE the repay on the frame DERIVED from the fill amounts, so a + // liquidator callback cannot move the state it reads. The Morpho `repay` withdraws the seized + // collateral and then hands control to `onPreLiquidate`; since `repay` and `supplyCollateral` + // are permissionless, a post-repay live re-quote could be neutralised by a callback that drains + // the position through the proportional path and re-supplies a dust atom, letting a fill that + // does not de-risk the position stand. The derived frame is immune to that. + _checkLtvStrictlyReduced(price, totalSeized, totalDebtShares); - _checkLtvReduced(price, entryDebtValue, entryCollateralValue); + uint256 repaidAssets = _settleOfferRepay(borrower, totalSeized, totalDebtShares, data); return (totalSeized, repaidAssets); } @@ -460,30 +466,44 @@ contract MorphoBorrowPosition is IBorrowPosition, IBorrowOffers, Initializable, (repaidAssets,) = MORPHO.repay(_borrowPositionStorage().marketParams, 0, totalDebtShares, borrower, callbackData); } - /// @dev Quotes the position's debt and collateral values at `price` from the current Morpho - /// state (post-accrual totals; debt rounds up, collateral rounds down, matching - /// {_isHealthy}). Called by {_offerPreLiquidate} before the consume and after the repay - /// settles, so the two frames differ only by the fill's real effect on Morpho's totals. - function _positionLtvFrame(uint256 price) internal view returns (uint256 debtValue, uint256 collateralValue) { + /// @dev Reverts {LibBorrowErrors.LtvNotReduced} unless the fill strictly lowers the position LTV: + /// `exitDebt / exitCollateral < entryDebt / entryCollateral`, cross-multiplied to + /// `exitDebt * entryCollateral < entryDebt * exitCollateral` so the comparison is exact. + /// + /// The exit frame is DERIVED from the fill amounts against the current (pre-repay) Morpho + /// snapshot rather than re-read live after the repay, so it measures the fill alone and no + /// liquidator callback can move it. It reproduces Morpho's shares-mode `repay` settlement + /// exactly: the repaid assets round up, the market totals drop by (`repaidAssets`, + /// `totalDebtShares`) with a saturating subtraction on the assets (Morpho's `repaidAssets` + /// may exceed `totalBorrowAssets` by one), and the remaining debt rounds up on the reduced + /// totals. Interest was accrued at entry and the in-flight repay re-accrues with zero elapsed + /// time, so these totals equal the settled ones; the derived frame is therefore the exact + /// state a post-repay quote would read absent a callback (debt rounds up, collateral rounds + /// down, matching {_isHealthy}). + /// + /// The product goes through `fullMulDiv`'s 512-bit intermediate rather than a plain + /// multiplication: the collateral values are price-scaled (`price` is an unbounded oracle + /// value at 1e36 scale) and can exceed 128 bits, so the direct product can overflow 256 bits + /// and checked multiplication would turn a legitimate fill into a spurious revert. Flooring + /// keeps it exact (`floor(a * b / d) >= c <=> a * b >= c * d` for integers). `entryDebtValue` + /// and `totalDebtShares` are non-zero in the band (the position has debt; the fill repaid at + /// least one share). + function _checkLtvStrictlyReduced(uint256 price, uint256 totalSeized, uint256 totalDebtShares) internal view { Id _marketId = _borrowPositionStorage().marketId; Position memory position = MORPHO.position(_marketId, address(this)); Market memory market = MORPHO.market(_marketId); - debtValue = - uint256(position.borrowShares).toAssetsUp(uint256(market.totalBorrowAssets), uint256(market.totalBorrowShares)); - collateralValue = uint256(position.collateral).mulDiv(price, ORACLE_PRICE_SCALE); - } + uint256 totalBorrowAssets = uint256(market.totalBorrowAssets); + uint256 totalBorrowShares = uint256(market.totalBorrowShares); + + uint256 entryDebtValue = uint256(position.borrowShares).toAssetsUp(totalBorrowAssets, totalBorrowShares); + uint256 entryCollateralValue = uint256(position.collateral).mulDiv(price, ORACLE_PRICE_SCALE); + + // Settled frame derived from the fill (matches Morpho's shares-mode repay rounding). + uint256 repaidAssets = totalDebtShares.toAssetsUp(totalBorrowAssets, totalBorrowShares); + uint256 exitDebtValue = (uint256(position.borrowShares) - totalDebtShares) + .toAssetsUp(totalBorrowAssets.zeroFloorSub(repaidAssets), totalBorrowShares - totalDebtShares); + uint256 exitCollateralValue = (uint256(position.collateral) - totalSeized).mulDiv(price, ORACLE_PRICE_SCALE); - /// @dev Reverts {LibBorrowErrors.LtvNotReduced} unless the settled position's LTV is strictly - /// below the entry frame's: `exitDebt / exitCollateral < entryDebt / entryCollateral`, - /// cross-multiplied to `exitDebt * entryCollateral < entryDebt * exitCollateral` so the - /// comparison is exact. The product goes through `fullMulDiv`'s 512-bit intermediate - /// rather than a plain multiplication: the collateral values are price-scaled (`price` is - /// an unbounded oracle value at 1e36 scale) and can exceed 128 bits, so the direct product - /// can overflow 256 bits and checked multiplication would turn a legitimate fill into a - /// spurious revert. Flooring keeps it exact (`floor(a * b / d) >= c <=> a * b >= c * d` - /// for integers). `entryDebtValue > 0` in the band (the position has debt, rounded up). - function _checkLtvReduced(uint256 price, uint256 entryDebtValue, uint256 entryCollateralValue) internal view { - (uint256 exitDebtValue, uint256 exitCollateralValue) = _positionLtvFrame(price); if (FixedPointMathLib.fullMulDiv(exitDebtValue, entryCollateralValue, entryDebtValue) >= exitCollateralValue) { revert LibBorrowErrors.LtvNotReduced(); } diff --git a/test/borrow/ReentrancyGuardBypassPoC.t.sol b/test/borrow/ReentrancyGuardBypassPoC.t.sol new file mode 100644 index 00000000..3fecb1f0 --- /dev/null +++ b/test/borrow/ReentrancyGuardBypassPoC.t.sol @@ -0,0 +1,140 @@ +// SPDX-License-Identifier: BUSL-1.1 +pragma solidity ^0.8.20; + +import {console2} from "forge-std/console2.sol"; +import {MorphoBorrowPositionOffersTest} from "./MorphoBorrowPositionOffers.t.sol"; +import {MorphoBorrowPosition} from "src/borrow/MorphoBorrowPosition.sol"; +import {IPreLiquidationCallback} from "src/interfaces/borrow/IPreliquidationCallback.sol"; +import {LibBorrowErrors} from "src/libs/borrow/LibBorrowErrors.sol"; +import {IMorpho, Id, MarketParams, Position, Market} from "lib/morpho-blue/src/interfaces/IMorpho.sol"; +import {MockERC20} from "test/mock/MockERC20.sol"; +import {SharesMathLib} from "lib/morpho-blue/src/libraries/SharesMathLib.sol"; + +/// @notice Regression test for the Cantina reentrancy finding on the band settlement guard. +/// The reviewer's exploit ran end-to-end against the real Morpho + MorphoBorrowPosition: a +/// liquidator's `onPreLiquidate` callback re-entered the PROPORTIONAL path, repaid all +/// remaining shares (seizing the rest of the collateral), supplied a few collateral atoms +/// back on behalf of the position, and returned with `exitDebt = 0`, so a post-repay live +/// `_checkLtvReduced` passed and the whole owner position was drained at the proportional +/// discount. +/// +/// Precondition: the position sits at `margin == 0` (its floored borrowing capacity at +/// `liquidationLtv` exactly equals its debt, i.e. it is at the offer-band/proportional +/// boundary). At `margin == 0` a one-atom "flat LTV" offer fill (the 3F-469 residual) ticks +/// the floored `_isHealthy(liquidationLtv)` check false at the callback point, which is what +/// made the reentrant proportional path reachable. +/// +/// The fix ({_checkLtvStrictlyReduced}) evaluates the guard BEFORE the repay on a frame +/// derived from the fill amounts, so the non-de-risking dust fill is rejected up front and +/// the callback never runs. This test asserts the exploit is blocked. +contract ReentrancyGuardBypassPoC is MorphoBorrowPositionOffersTest { + using SharesMathLib for uint256; + + function test_poc_confirmExploitAtBoundaryPrice() public { + _enterBand(0.7e18); // establish the position (collateral + borrow) + + // Binary-search the smallest price that dispatches to the offer band (isHealthy at + // liquidationLtv). At that price the floored borrowing capacity first reaches the debt, so + // `margin == 0`. Higher price => lower LTV => healthier, so isHealthy(liqLtv) is monotonic. + uint256 lo = BORROW * SCALE / COLLATERAL * 1e18 / uint256(LIQ_LTV); + uint256 hi = lo * 2; + for (uint256 it; it < 256 && hi - lo > 1; ++it) { + uint256 mid = lo + (hi - lo) / 2; + oracle.setPrice(mid); + if (pos.isHealthy(uint256(LIQ_LTV))) hi = mid; + else lo = mid; + } + oracle.setPrice(hi); // offer-band dispatch, closest to the boundary + assertTrue(pos.isHealthy(uint256(LIQ_LTV)) && !pos.isHealthy(uint256(SAFE_LTV)), "offer band at entry"); + + _proposeAtPrice(_positionShares() / 2, 1.2e18); + _warpActive(); + + uint256 ownerCollBefore = _positionCollateral(); + PureExploitLiquidator attacker = + new PureExploitLiquidator(pos, morpho, marketParams, marketId, loanToken, collateralToken, LIQ_LTV); + loanToken.setBalance(address(attacker), 100_000_000e18); + collateralToken.setBalance(address(attacker), 1_000e18); + + uint256 attackerCollBefore = collateralToken.balanceOf(address(attacker)); + + (bool ok, bytes4 sel) = attacker.attackSeize(1); // one-atom seize target => the 3F-469 residual fill + + uint256 collGained = collateralToken.balanceOf(address(attacker)) - attackerCollBefore; + + console2.log("outerOk=%s exploitRan=%s", ok, attacker.exploitRan()); + console2.log( + "ownerCollBefore=%e ownerCollAfter=%e ownerDebtAfter=%e", + ownerCollBefore, + _positionCollateral(), + _positionShares() + ); + console2.log("attacker collateralGained=%e", collGained); + + // Fix regression: the settlement guard now runs BEFORE the repay on a derived frame, so the + // non-de-risking dust fill is rejected up front. The outer call reverts LtvNotReduced, the + // callback (and its reentrant proportional drain) never runs, and the owner is untouched. + assertFalse(ok, "exploit blocked: outer preLiquidate reverts"); + assertEq(sel, LibBorrowErrors.LtvNotReduced.selector, "reverts with LtvNotReduced before settlement"); + assertFalse(attacker.exploitRan(), "callback never reached: guard runs before the repay"); + assertEq(_positionCollateral(), ownerCollBefore, "owner position untouched"); + assertEq(collGained, 0, "attacker seized nothing"); + } +} + +/// @notice Liquidator that runs the reviewer's exploit steps: if the reentrant proportional path is +/// reachable at the callback (position unhealthy at liquidationLtv), repay all remaining +/// shares (seizing the rest of the collateral) then supply a few collateral atoms back on +/// behalf of the position so the outer exit frame reads (debt=0, collateral>0). +contract PureExploitLiquidator is IPreLiquidationCallback { + MorphoBorrowPosition internal pos; + IMorpho internal morpho; + MarketParams internal mp; + Id internal id; + MockERC20 internal loanToken; + MockERC20 internal collateralToken; + uint128 internal liqLtv; + bool internal entered; + bool public exploitRan; + + constructor( + MorphoBorrowPosition _pos, + IMorpho _morpho, + MarketParams memory _mp, + Id _id, + MockERC20 _loan, + MockERC20 _coll, + uint128 _liqLtv + ) { + pos = _pos; + morpho = _morpho; + mp = _mp; + id = _id; + loanToken = _loan; + collateralToken = _coll; + liqLtv = _liqLtv; + loanToken.approve(address(_pos), type(uint256).max); + loanToken.approve(address(_morpho), type(uint256).max); + collateralToken.approve(address(_morpho), type(uint256).max); + } + + function attackSeize(uint256 seizeTarget) external returns (bool ok, bytes4 sel) { + try pos.preLiquidate(address(pos), seizeTarget, 0, abi.encode("go")) { + ok = true; + } catch (bytes memory reason) { + if (reason.length >= 4) sel = bytes4(reason); + } + } + + function onPreLiquidate(uint256, bytes calldata) external override { + if (entered) return; + entered = true; + if (pos.isHealthy(uint256(liqLtv))) return; // proportional path NOT reachable + uint256 rem = uint256(morpho.position(id, address(pos)).borrowShares); + pos.preLiquidate(address(pos), 0, rem, ""); // reentrant proportional: drain all collateral + // Supply enough atoms that the exit-collateral VALUE is strictly positive (one atom rounds to + // zero value when price/scale < 1, which would trip the guard's 0 >= 0 branch). + morpho.supplyCollateral(mp, 1e9, address(pos), ""); + exploitRan = true; + } +}