fix(security): lock-griefing, asset-denominated pendle withdraw, two-…

.gitignore

@@ -1,6 +1,7 @@
 # Compiler files
 cache/
 out/
+artifacts/
 
 # Ignores development broadcast logs
 !/broadcast
@@ -9,6 +10,7 @@ out/
 
 # Docs
 docs/
+PROJECT_PLAN.md
 
 # Dotenv file
 .env
@@ -19,6 +21,7 @@ docs/
 .vscode/
 .idea/
 .DS_Store
+.claude/
 
 # Foundry
 foundry.lock

src/Vault.sol

@@ -100,11 +100,17 @@ contract Vault is Diamond, ERC4626, ReentrancyGuard {
         // Post-accrue handles the first-deposit bootstrap: now that supply > 0,
         // initialise HWM and the accrual timestamp. No-op for subsequent deposits.
         _accrueFees();
-        // Lock the freshly minted shares for the configured window so an attacker
+        // Lock the receiver's shares for the configured window so an attacker
         // cannot deposit, manipulate, and withdraw within the same block. Skipped
         // when the period is 0 (disabled). Re-deposits refresh the window.
+        //
+        // Armed ONLY when the depositor is locking their own shares
+        // (caller == receiver). A third party must never be able to set the lock
+        // on an arbitrary receiver: doing so would let an attacker freeze a
+        // victim's entire balance, or — with receiver == address(this) — lock the
+        // vault's own escrowed withdraw-queue shares and brick cancel/fulfill.
         LibLock.LockStorage storage l = LibLock.lockStorage();
-        if (l.shareLockPeriod > 0) {
+        if (l.shareLockPeriod > 0 && caller == receiver) {
             l.lockedUntil[receiver] = block.timestamp + uint256(l.shareLockPeriod);
         }
     }
@@ -133,7 +139,11 @@ contract Vault is Diamond, ERC4626, ReentrancyGuard {
     ///      catching transfers stops the transfer-then-withdraw bypass, so no
     ///      separate check is needed in `_withdraw`.
     function _update(address from, address to, uint256 value) internal override {
-        if (from != address(0)) {
+        // Mints (from == 0) are exempt — that is how the lock is armed. The
+        // vault's own address is also exempt: shares it custodies are protocol
+        // escrow (the withdraw queue), not user funds subject to the anti-MEV
+        // lock, and blocking their movement would brick cancel/fulfill.
+        if (from != address(0) && from != address(this)) {
             uint256 unlockAt = LibLock.lockStorage().lockedUntil[from];
             if (block.timestamp < unlockAt) revert LibLock.SharesLocked(from, unlockAt);
         }

src/facets/AllocatorFacet.sol

@@ -31,6 +31,7 @@ contract AllocatorFacet {
     error AllocationToQuarantined(bytes32 strategyId);
     error RebalanceDeltaTooLarge(uint256 movementBps, uint16 maxBps);
     error IdleReserveBreached(uint256 idleAfter, uint256 requiredIdle);
+    error StrategyHealthy(bytes32 strategyId);
 
     event StrategyRegistered(bytes32 indexed strategyId, LibAllocator.StrategyConfig config);
     event StrategyRemoved(bytes32 indexed strategyId);
@@ -42,6 +43,7 @@ contract AllocatorFacet {
     event StrategyQuarantined(bytes32 indexed strategyId);
     event StrategyReleased(bytes32 indexed strategyId);
     event MaxRebalanceDeltaSet(uint16 bps);
+    event StrategyRebalanceSkipped(bytes32 indexed strategyId, bytes4 selector);
 
     // -----------------------------------------------------------------------
     // Owner-gated governance / risk bounds
@@ -194,6 +196,29 @@ contract AllocatorFacet {
         emit StrategyReleased(strategyId);
     }
 
+    /// @notice Permissionlessly quarantine a strategy whose NAV read is currently
+    ///         reverting, so a single broken strategy can no longer brick
+    ///         `totalAssets` and every ERC-4626 entrypoint while waiting on the
+    ///         owner to react.
+    /// @dev Guarded by an on-chain liveness probe: it staticcalls the strategy's
+    ///      own `totalAssetsSelector` and only quarantines if that call REVERTS.
+    ///      A healthy strategy therefore cannot be griefed offline by anyone. The
+    ///      effect mirrors `quarantineStrategy` (excluded from NAV, target zeroed);
+    ///      lifting it stays owner-gated via `releaseStrategy`, since re-including
+    ///      a position is a trust decision.
+    function quarantineFailedStrategy(bytes32 strategyId) external {
+        LibAllocator.AllocatorStorage storage s = LibAllocator.allocatorStorage();
+        if (!s.configs[strategyId].active) revert StrategyNotRegistered(strategyId);
+        if (s.quarantined[strategyId]) revert StrategyAlreadyQuarantined(strategyId);
+
+        (bool ok,) = address(this).staticcall(abi.encodeWithSelector(s.configs[strategyId].totalAssetsSelector));
+        if (ok) revert StrategyHealthy(strategyId);
+
+        s.quarantined[strategyId] = true;
+        s.targetBps[strategyId] = 0;
+        emit StrategyQuarantined(strategyId);
+    }
+
     // -----------------------------------------------------------------------
     // Rebalance
     // -----------------------------------------------------------------------
@@ -253,7 +278,11 @@ contract AllocatorFacet {
             uint256 target = (totalCached * uint256(s.targetBps[id])) / LibAllocator.BPS_DENOMINATOR;
             if (currentAssets[i] > target) {
                 uint256 delta = currentAssets[i] - target;
-                _dispatchStrategyCall(id, s.configs[id].withdrawSelector, delta);
+                // Skip (don't brick the batch) if this one strategy's withdraw
+                // reverts; the idle-reserve floor below still backstops safety.
+                if (!_dispatchStrategyCall(s.configs[id].withdrawSelector, delta)) {
+                    emit StrategyRebalanceSkipped(id, s.configs[id].withdrawSelector);
+                }
             }
         }
 
@@ -264,7 +293,9 @@ contract AllocatorFacet {
             uint256 target = (totalCached * uint256(s.targetBps[id])) / LibAllocator.BPS_DENOMINATOR;
             if (currentAssets[i] < target) {
                 uint256 delta = target - currentAssets[i];
-                _dispatchStrategyCall(id, s.configs[id].depositSelector, delta);
+                if (!_dispatchStrategyCall(s.configs[id].depositSelector, delta)) {
+                    emit StrategyRebalanceSkipped(id, s.configs[id].depositSelector);
+                }
             }
         }
 
@@ -357,23 +388,16 @@ contract AllocatorFacet {
         return abi.decode(data, (uint256));
     }
 
-    function _dispatchStrategyCall(bytes32 strategyId, bytes4 selector, uint256 amount) internal {
-        if (selector == bytes4(0)) revert EmptySelector();
-        bytes memory data;
-        if (amount == 0) {
-            data = abi.encodeWithSelector(selector);
-        } else {
-            data = abi.encodeWithSelector(selector, amount);
-        }
-        (bool ok, bytes memory ret) = address(this).call(data);
-        if (!ok) {
-            if (ret.length > 0) {
-                assembly {
-                    revert(add(32, ret), mload(ret))
-                }
-            }
-            revert StrategyCallFailed(strategyId, selector);
-        }
+    /// @dev Self-dispatches a strategy mutator (deposit/withdraw) through the
+    ///      diamond fallback and reports success instead of reverting. Returning
+    ///      false on an unset selector or a failed call lets `rebalance` skip a
+    ///      single misbehaving strategy rather than letting it brick the whole
+    ///      batch; the end-of-rebalance idle-reserve invariant still backstops
+    ///      safety. `amount` is always > 0 here (callers only dispatch a non-zero
+    ///      delta), so the selector is always encoded with the amount argument.
+    function _dispatchStrategyCall(bytes4 selector, uint256 amount) internal returns (bool ok) {
+        if (selector == bytes4(0)) return false;
+        (ok,) = address(this).call(abi.encodeWithSelector(selector, amount));
     }
 
     function _effectiveCap(LibAllocator.AllocatorStorage storage s, bytes32 strategyId) internal view returns (uint16) {

src/facets/OwnershipFacet.sol

@@ -5,14 +5,36 @@ import { IERC173 } from "../interfaces/IERC173.sol";
 import { LibDiamond } from "../libraries/LibDiamond.sol";
 
 contract OwnershipFacet is IERC173 {
+    /// @notice Thrown when a non-pending-owner calls `acceptOwnership`.
+    error NotPendingOwner(address caller, address expected);
+
     /// @inheritdoc IERC173
     function owner() external view override returns (address owner_) {
         owner_ = LibDiamond.contractOwner();
     }
 
+    /// @notice The address nominated to take ownership, pending its acceptance.
+    function pendingOwner() external view returns (address) {
+        return LibDiamond.pendingOwner();
+    }
+
     /// @inheritdoc IERC173
+    /// @dev Two-step transfer: this only NOMINATES `_newOwner`; ownership does not
+    ///      move until they call `acceptOwnership`. This makes a fat-fingered or
+    ///      malicious handoff recoverable and removes the need for an explicit
+    ///      zero-address check — because address(0) can never call
+    ///      `acceptOwnership`, ownership can never be lost to it; passing
+    ///      address(0) here simply cancels any pending transfer.
     function transferOwnership(address _newOwner) external override {
         LibDiamond.enforceIsContractOwner();
-        LibDiamond.setContractOwner(_newOwner);
+        LibDiamond.setPendingOwner(_newOwner);
+    }
+
+    /// @notice Complete a pending ownership transfer. Callable only by the
+    ///         currently nominated pending owner.
+    function acceptOwnership() external {
+        address pending = LibDiamond.pendingOwner();
+        if (msg.sender != pending) revert NotPendingOwner(msg.sender, pending);
+        LibDiamond.acceptPendingOwner();
     }
 }

src/facets/strategies/PendlePtStrategyFacet.sol

@@ -272,34 +272,40 @@ contract PendlePtStrategyFacet {
         if (netPtOut == 0) revert PendleDepositFailed(netPtOut);
     }
 
-    /// @notice Return `amount` of underlying from the Pendle position to the vault.
-    /// @dev Routes through the appropriate path depending on maturity:
-    ///      - Pre-maturity:  sells PT on the Pendle AMM via swapExactPtForToken.
-    ///      - Post-maturity: redeems PT at face value via redeemPyToToken.
-    ///
-    ///      `amount` is treated as the PT quantity to liquidate (face value units).
-    ///      The underlying received may be slightly less pre-maturity due to
-    ///      the AMM discount; post-maturity it is 1:1.
-    /// @param amount PT quantity to liquidate (denominated in underlying units).
-    function pendleWithdraw(uint256 amount) external {
+    /// @notice Liquidate enough of the Pendle position to return `assetAmount` of
+    ///         the underlying asset to the vault.
+    /// @dev `assetAmount` is denominated in the UNDERLYING ASSET — matching the
+    ///      rebalance delta the allocator computes (asset units) and the units
+    ///      `pendleDeposit` consumes — NOT in PT. It is converted to a PT quantity
+    ///      at the oracle mark (pre-maturity) or 1:1 face value (post-maturity)
+    ///      and capped at the held PT balance, so an over-large request liquidates
+    ///      the whole position instead of reverting. Routes:
+    ///      - Pre-maturity:  swapExactPtForToken (sell PT on the AMM, oracle-bounded).
+    ///      - Post-maturity: redeemPyToToken     (burn PT 1:1, hard 99% dust bound).
+    ///      The underlying received may be slightly less than `assetAmount`
+    ///      pre-maturity due to AMM execution; post-maturity it is ~1:1.
+    /// @param assetAmount Target underlying amount to free, in asset units.
+    function pendleWithdraw(uint256 assetAmount) external {
         LibDiamond.enforceIsSelf();
         PendleStorage storage s = _ps();
         if (address(s.router) == address(0)) revert PendleNotConfigured();
 
         uint256 ptBalance = s.pt.balanceOf(address(this));
-        if (amount > ptBalance) revert PendleInsufficientPt(amount, ptBalance);
+        if (ptBalance == 0) revert PendleInsufficientPt(assetAmount, 0);
 
         IERC20 underlying = IERC20(IERC4626(address(this)).asset());
-
-        IERC20(address(s.pt)).forceApprove(address(s.router), amount);
-
         uint256 received;
 
         if (s.pt.isExpired()) {
-            // Post-maturity: PT redeems 1:1. minTokenOut = 99% (dust tolerance).
+            // Post-maturity: PT redeems 1:1 for the asset (shared decimals), so the
+            // requested asset amount maps directly to a PT quantity. Cap at the
+            // balance: an over-large request liquidates the whole position.
+            uint256 ptAmount = assetAmount > ptBalance ? ptBalance : assetAmount;
+            IERC20(address(s.pt)).forceApprove(address(s.router), ptAmount);
+
             IPendleRouter.TokenOutput memory output = IPendleRouter.TokenOutput({
                 tokenOut: address(underlying),
-                minTokenOut: amount * 99 / 100,
+                minTokenOut: ptAmount * 99 / 100,
                 tokenRedeemSy: address(underlying),
                 pendleSwap: address(0),
                 swapData: IPendleRouter.SwapData({
@@ -308,19 +314,25 @@ contract PendlePtStrategyFacet {
             });
 
             // redeemPyToToken burns PT (YT is implicitly 0 post-maturity).
-            (received,) = s.router.redeemPyToToken(address(this), s.pt.YT(), amount, output);
+            (received,) = s.router.redeemPyToToken(address(this), s.pt.YT(), ptAmount, output);
         } else {
-            // Pre-maturity: sell PT on the Pendle AMM. Mandatory oracle — derive
-            // minTokenOut from the mark (PT->asset rate, haircut by slippage) so
-            // the router enforces the bound. Refuse to sell unpriced rather than
-            // run with minTokenOut = 0 (fully sandwichable). Post-maturity redeem
-            // below needs no oracle: it is 1:1 with a hard 99% dust bound.
+            // Pre-maturity: sell PT on the Pendle AMM. Mandatory oracle — convert
+            // the requested asset value into a PT quantity at the mark, cap at the
+            // balance, and derive minTokenOut from the PT actually being sold
+            // (haircut by slippage). Refuse to sell unpriced rather than run with
+            // minTokenOut = 0 (fully sandwichable).
             if (address(s.oracle) == address(0)) revert PendleOracleRequired();
             uint256 rate = s.oracle.getPtToAssetRate(s.market, s.twapDuration);
             if (rate == 0) revert PendleOracleRequired();
-            uint256 expected = amount * rate / 1e18;
+
+            uint256 ptAmount = assetAmount * 1e18 / rate;
+            if (ptAmount > ptBalance) ptAmount = ptBalance;
+
+            uint256 expected = ptAmount * rate / 1e18;
             uint256 minTokenOut = expected * (PENDLE_BPS - _maxSlippageBps(s)) / PENDLE_BPS;
 
+            IERC20(address(s.pt)).forceApprove(address(s.router), ptAmount);
+
             IPendleRouter.TokenOutput memory output = IPendleRouter.TokenOutput({
                 tokenOut: address(underlying),
                 minTokenOut: minTokenOut,
@@ -333,10 +345,10 @@ contract PendlePtStrategyFacet {
 
             IPendleRouter.LimitOrderData memory limit;
 
-            (received,,) = s.router.swapExactPtForToken(address(this), s.market, amount, output, limit);
+            (received,,) = s.router.swapExactPtForToken(address(this), s.market, ptAmount, output, limit);
         }
 
-        if (received == 0) revert PendleWithdrawFailed(amount, received);
+        if (received == 0) revert PendleWithdrawFailed(assetAmount, received);
     }
 
     /// @notice No-op. PT yield accrues entirely to face value at maturity —

src/libraries/LibDiamond.sol

@@ -23,6 +23,10 @@ library LibDiamond {
         address[] facetAddresses;
         mapping(bytes4 => bool) supportedInterfaces;
         address contractOwner;
+        // Two-step ownership: the address nominated by `transferOwnership` that
+        // must call `acceptOwnership` to take over. Appended last to preserve the
+        // existing storage layout.
+        address pendingOwner;
     }
 
     error NotContractOwner(address caller, address expected);
@@ -49,6 +53,7 @@ library LibDiamond {
     }
 
     event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
+    event OwnershipTransferStarted(address indexed previousOwner, address indexed newOwner);
 
     function setContractOwner(address _newOwner) internal {
         DiamondStorage storage ds = diamondStorage();
@@ -61,6 +66,29 @@ library LibDiamond {
         contractOwner_ = diamondStorage().contractOwner;
     }
 
+    /// @notice The address nominated to take ownership via the two-step transfer.
+    function pendingOwner() internal view returns (address pendingOwner_) {
+        pendingOwner_ = diamondStorage().pendingOwner;
+    }
+
+    /// @notice Nominate `_pendingOwner` for ownership (step 1). Setting it to
+    ///         address(0) cancels a pending transfer.
+    function setPendingOwner(address _pendingOwner) internal {
+        diamondStorage().pendingOwner = _pendingOwner;
+        emit OwnershipTransferStarted(diamondStorage().contractOwner, _pendingOwner);
+    }
+
+    /// @notice Promote the pending owner to owner (step 2) and clear the
+    ///         nomination. Caller authorization is enforced by the facet.
+    function acceptPendingOwner() internal returns (address newOwner) {
+        DiamondStorage storage ds = diamondStorage();
+        newOwner = ds.pendingOwner;
+        ds.pendingOwner = address(0);
+        address previousOwner = ds.contractOwner;
+        ds.contractOwner = newOwner;
+        emit OwnershipTransferred(previousOwner, newOwner);
+    }
+
     function enforceIsContractOwner() internal view {
         if (msg.sender != diamondStorage().contractOwner) {
             revert NotContractOwner(msg.sender, diamondStorage().contractOwner);

test/mocks/MockStrategyFacet.sol

@@ -18,6 +18,7 @@ contract MockStrategyFacet {
         MockProtocol protocol;
         uint256 harvestCount;
         bool reverting;
+        bool revertOnMove;
     }
 
     function _ms() internal pure returns (MockStorage storage s) {
@@ -41,6 +42,14 @@ contract MockStrategyFacet {
         _ms().reverting = v;
     }
 
+    /// @notice Test-only — when set, the strategy's deposit and withdraw movers
+    ///         revert while its totalAssets read still succeeds, simulating a
+    ///         strategy that prices fine but cannot move funds (e.g. a paused
+    ///         lending pool). Used to exercise rebalance's per-strategy skip.
+    function mockSetRevertOnMove(bool v) external {
+        _ms().revertOnMove = v;
+    }
+
     function mockTotalAssets() external view returns (uint256) {
         if (_ms().reverting) revert("mock: totalAssets reverted");
         MockProtocol p = _ms().protocol;
@@ -49,13 +58,15 @@ contract MockStrategyFacet {
     }
 
     function mockDeposit(uint256 amount) external {
+        if (_ms().revertOnMove) revert("mock: deposit reverted");
         MockProtocol p = _ms().protocol;
         IERC20 token = IERC20(IERC4626(address(this)).asset());
         token.approve(address(p), amount);
         p.deposit(amount);
     }
 
     function mockWithdraw(uint256 amount) external {
+        if (_ms().revertOnMove) revert("mock: withdraw reverted");
         _ms().protocol.withdraw(amount);
     }