From a82d0f498d118e19508fc7d13c6d6c121060a418 Mon Sep 17 00:00:00 2001 From: Jayesh Yadav Date: Mon, 1 Jun 2026 10:51:55 +0530 Subject: [PATCH] fix(security): lock-griefing, asset-denominated pendle withdraw, two-step ownership, strategy fault-tolerance --- .gitignore | 3 + src/Vault.sol | 16 +- src/facets/AllocatorFacet.sol | 62 ++++--- src/facets/OwnershipFacet.sol | 24 ++- .../strategies/PendlePtStrategyFacet.sol | 62 ++++--- src/libraries/LibDiamond.sol | 28 ++++ test/mocks/MockStrategyFacet.sol | 11 ++ test/unit/MorphoStrategy.t.sol | 16 +- test/unit/Ownership.t.sol | 158 ++++++++++++++++++ test/unit/PendleStrategy.t.sol | 81 +++++++-- test/unit/Quarantine.t.sol | 59 ++++++- test/unit/ShareLock.t.sol | 25 +++ test/unit/WithdrawQueue.t.sol | 59 ++++++- 13 files changed, 532 insertions(+), 72 deletions(-) create mode 100644 test/unit/Ownership.t.sol diff --git a/.gitignore b/.gitignore index 7ce6f04..7c2379f 100644 --- a/.gitignore +++ b/.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 diff --git a/src/Vault.sol b/src/Vault.sol index 73b45be..d6922bc 100644 --- a/src/Vault.sol +++ b/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); } diff --git a/src/facets/AllocatorFacet.sol b/src/facets/AllocatorFacet.sol index 623aa9e..9788d75 100644 --- a/src/facets/AllocatorFacet.sol +++ b/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) { diff --git a/src/facets/OwnershipFacet.sol b/src/facets/OwnershipFacet.sol index 5b83ed4..2df3e0a 100644 --- a/src/facets/OwnershipFacet.sol +++ b/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(); } } diff --git a/src/facets/strategies/PendlePtStrategyFacet.sol b/src/facets/strategies/PendlePtStrategyFacet.sol index ce3b7ca..b89a870 100644 --- a/src/facets/strategies/PendlePtStrategyFacet.sol +++ b/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 — diff --git a/src/libraries/LibDiamond.sol b/src/libraries/LibDiamond.sol index d897a33..2f5ebf6 100644 --- a/src/libraries/LibDiamond.sol +++ b/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); diff --git a/test/mocks/MockStrategyFacet.sol b/test/mocks/MockStrategyFacet.sol index 57880f4..8ba5d01 100644 --- a/test/mocks/MockStrategyFacet.sol +++ b/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,6 +58,7 @@ 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); @@ -56,6 +66,7 @@ contract MockStrategyFacet { } function mockWithdraw(uint256 amount) external { + if (_ms().revertOnMove) revert("mock: withdraw reverted"); _ms().protocol.withdraw(amount); } diff --git a/test/unit/MorphoStrategy.t.sol b/test/unit/MorphoStrategy.t.sol index b2bc57c..78628af 100644 --- a/test/unit/MorphoStrategy.t.sol +++ b/test/unit/MorphoStrategy.t.sol @@ -150,7 +150,7 @@ contract MorphoStrategyTest is Test { ); } - function test_Deposit_RevertsOnSlippage() public { + function test_Deposit_SkippedOnSlippage() public { _configure(); _register(); uint256 amount = 1000 * 1e6; @@ -159,15 +159,15 @@ contract MorphoStrategyTest is Test { _setSingleAllocation(MORPHO_ID, 10_000); vm.roll(block.number + 1); - // The whole rebalance deposit is `amount` (100% of NAV); previewDeposit - // on the empty vault is the value the facet sees, and MorphoSlippage - // bubbles up through the allocator's self-dispatch. - uint256 expected = morphoVault.previewDeposit(amount); - uint256 received = (expected * (10_000 - 100)) / 10_000; - + // MorphoSlippage bubbles up through the allocator's self-dispatch, where + // F05 turns it into a per-strategy SKIP rather than a whole-rebalance + // revert: the funds stay idle and the skip is recorded. + vm.expectEmit(true, false, false, true, address(vault)); + emit AllocatorFacet.StrategyRebalanceSkipped(MORPHO_ID, MorphoStrategyFacet.morphoDeposit.selector); vm.prank(owner); - vm.expectRevert(abi.encodeWithSelector(MorphoStrategyFacet.MorphoSlippage.selector, expected, received)); AllocatorFacet(address(vault)).rebalance(); + + assertEq(usdc.balanceOf(address(vault)), amount, "funds stayed idle; over-slippage Morpho deposit skipped"); } function test_Withdraw_ReturnsUnderlyingToDiamond() public { diff --git a/test/unit/Ownership.t.sol b/test/unit/Ownership.t.sol new file mode 100644 index 0000000..5917bb0 --- /dev/null +++ b/test/unit/Ownership.t.sol @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import { Test } from "forge-std/Test.sol"; +import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; + +import { Vault } from "../../src/Vault.sol"; +import { IDiamond } from "../../src/interfaces/IDiamond.sol"; +import { IDiamondCut } from "../../src/interfaces/IDiamondCut.sol"; +import { IDiamondLoupe } from "../../src/interfaces/IDiamondLoupe.sol"; +import { IERC173 } from "../../src/interfaces/IERC173.sol"; +import { DiamondCutFacet } from "../../src/facets/DiamondCutFacet.sol"; +import { DiamondLoupeFacet } from "../../src/facets/DiamondLoupeFacet.sol"; +import { OwnershipFacet } from "../../src/facets/OwnershipFacet.sol"; +import { LibDiamond } from "../../src/libraries/LibDiamond.sol"; + +contract MockUSDC is ERC20 { + constructor() ERC20("USD Coin", "USDC") { } + + function decimals() public pure override returns (uint8) { + return 6; + } +} + +/// @title OwnershipTest +/// @notice Coverage for the two-step ownership transfer (F02): `transferOwnership` +/// only NOMINATES a pending owner; the transfer completes only when that +/// address calls `acceptOwnership`. This makes a mistaken or malicious +/// handoff recoverable and makes it impossible to lose ownership to the +/// zero address, since address(0) can never accept. +contract OwnershipTest is Test { + MockUSDC internal usdc; + Vault internal vault; + + address internal owner = makeAddr("owner"); + address internal newOwner = makeAddr("newOwner"); + address internal stranger = makeAddr("stranger"); + + function setUp() public { + usdc = new MockUSDC(); + vault = _deployVault(); + } + + function _of() internal view returns (OwnershipFacet) { + return OwnershipFacet(address(vault)); + } + + function test_TransferOwnership_NominatesButDoesNotTransfer() public { + vm.prank(owner); + _of().transferOwnership(newOwner); + + assertEq(_of().owner(), owner, "ownership unchanged until accepted"); + assertEq(_of().pendingOwner(), newOwner, "pending owner recorded"); + } + + function test_TransferOwnership_OwnerOnly() public { + vm.prank(stranger); + vm.expectRevert(abi.encodeWithSelector(LibDiamond.NotContractOwner.selector, stranger, owner)); + _of().transferOwnership(newOwner); + } + + function test_AcceptOwnership_CompletesTransfer() public { + vm.prank(owner); + _of().transferOwnership(newOwner); + + vm.prank(newOwner); + _of().acceptOwnership(); + + assertEq(_of().owner(), newOwner, "ownership moved to acceptor"); + assertEq(_of().pendingOwner(), address(0), "pending cleared after acceptance"); + } + + function test_AcceptOwnership_RevertsForNonPending() public { + vm.prank(owner); + _of().transferOwnership(newOwner); + + vm.prank(stranger); + vm.expectRevert(abi.encodeWithSelector(OwnershipFacet.NotPendingOwner.selector, stranger, newOwner)); + _of().acceptOwnership(); + } + + function test_TransferOwnership_CanBeCancelled() public { + vm.prank(owner); + _of().transferOwnership(newOwner); + + // Owner cancels by nominating the zero address. + vm.prank(owner); + _of().transferOwnership(address(0)); + assertEq(_of().pendingOwner(), address(0), "nomination cleared"); + + // The former nominee can no longer take over. + vm.prank(newOwner); + vm.expectRevert(abi.encodeWithSelector(OwnershipFacet.NotPendingOwner.selector, newOwner, address(0))); + _of().acceptOwnership(); + assertEq(_of().owner(), owner, "owner retained control"); + } + + /// @notice The headline F02 property: ownership can never be lost to the zero + /// address. Even a "transfer" to address(0) does not move ownership, + /// and nobody can accept it. + function test_OwnershipCannotBeLostToZeroAddress() public { + vm.prank(owner); + _of().transferOwnership(address(0)); + + assertEq(_of().owner(), owner, "owner unchanged by a zero-address transfer"); + + // address(0) cannot call acceptOwnership, so ownership stays put — there is + // no path by which a fat-fingered zero address bricks governance. + vm.prank(stranger); + vm.expectRevert(abi.encodeWithSelector(OwnershipFacet.NotPendingOwner.selector, stranger, address(0))); + _of().acceptOwnership(); + } + + function _deployVault() internal returns (Vault) { + DiamondCutFacet cut = new DiamondCutFacet(); + DiamondLoupeFacet loupe = new DiamondLoupeFacet(); + OwnershipFacet ownership = new OwnershipFacet(); + + IDiamond.FacetCut[] memory cuts = new IDiamond.FacetCut[](3); + cuts[0] = IDiamond.FacetCut({ + facetAddress: address(cut), action: IDiamond.FacetCutAction.Add, functionSelectors: _diamondCutSelectors() + }); + cuts[1] = IDiamond.FacetCut({ + facetAddress: address(loupe), + action: IDiamond.FacetCutAction.Add, + functionSelectors: _diamondLoupeSelectors() + }); + cuts[2] = IDiamond.FacetCut({ + facetAddress: address(ownership), + action: IDiamond.FacetCutAction.Add, + functionSelectors: _ownershipSelectors() + }); + + return new Vault(IERC20(address(usdc)), "Vault Router", "vUSDC", owner, cuts, address(0), ""); + } + + function _diamondCutSelectors() internal pure returns (bytes4[] memory s) { + s = new bytes4[](1); + s[0] = IDiamondCut.diamondCut.selector; + } + + function _diamondLoupeSelectors() internal pure returns (bytes4[] memory s) { + s = new bytes4[](4); + s[0] = IDiamondLoupe.facets.selector; + s[1] = IDiamondLoupe.facetFunctionSelectors.selector; + s[2] = IDiamondLoupe.facetAddresses.selector; + s[3] = IDiamondLoupe.facetAddress.selector; + } + + function _ownershipSelectors() internal pure returns (bytes4[] memory s) { + s = new bytes4[](4); + s[0] = IERC173.owner.selector; + s[1] = IERC173.transferOwnership.selector; + s[2] = OwnershipFacet.acceptOwnership.selector; + s[3] = OwnershipFacet.pendingOwner.selector; + } +} diff --git a/test/unit/PendleStrategy.t.sol b/test/unit/PendleStrategy.t.sol index 8374caa..a4737bb 100644 --- a/test/unit/PendleStrategy.t.sol +++ b/test/unit/PendleStrategy.t.sol @@ -186,7 +186,12 @@ contract PendleStrategyTest is Test { assertEq(PendlePtStrategyFacet(address(vault)).pendleTotalAssets(), expectedPt, "marked at par == face"); } - function test_Deposit_RevertsWhenExpired() public { + // F05: a strategy's protective deposit refusal (expired market / missing + // oracle / slippage breach) must SKIP that strategy, not brick the whole + // rebalance. The protection still works — the funds simply stay idle and a + // StrategyRebalanceSkipped event records the skip. + + function test_Deposit_SkippedWhenExpired() public { _configureWithOracle(); _register(); usdc.mint(address(vault), 1000 * 1e6); @@ -194,26 +199,34 @@ contract PendleStrategyTest is Test { vm.warp(expiry); // at/after expiry the market is closed for buys vm.roll(block.number + 1); + vm.expectEmit(true, false, false, true, address(vault)); + emit AllocatorFacet.StrategyRebalanceSkipped(PENDLE_ID, PendlePtStrategyFacet.pendleDeposit.selector); vm.prank(owner); - vm.expectRevert(PendlePtStrategyFacet.PendleMarketExpired.selector); AllocatorFacet(address(vault)).rebalance(); + + assertEq(usdc.balanceOf(address(vault)), 1000 * 1e6, "funds stayed idle; expired-market deposit skipped"); + assertEq(PendlePtStrategyFacet(address(vault)).pendleTotalAssets(), 0, "no PT bought"); } - function test_Deposit_RevertsWhenNoOracle() public { + function test_Deposit_SkippedWhenNoOracle() public { // Mandatory oracle: a deposit on an unpriced market is refused outright - // rather than swapped with minOut = 0. + // rather than swapped with minOut = 0 — and the refusal skips, not bricks. _configure(); // router/market/pt, but NO oracle _register(); usdc.mint(address(vault), 1000 * 1e6); _setSingleAllocation(PENDLE_ID, 10_000); vm.roll(block.number + 1); + vm.expectEmit(true, false, false, true, address(vault)); + emit AllocatorFacet.StrategyRebalanceSkipped(PENDLE_ID, PendlePtStrategyFacet.pendleDeposit.selector); vm.prank(owner); - vm.expectRevert(PendlePtStrategyFacet.PendleOracleRequired.selector); AllocatorFacet(address(vault)).rebalance(); + + assertEq(usdc.balanceOf(address(vault)), 1000 * 1e6, "funds stayed idle; unpriced deposit skipped"); + assertEq(PendlePtStrategyFacet(address(vault)).pendleTotalAssets(), 0, "no PT bought"); } - function test_Deposit_RevertsWhenFillBelowOracleSlippageBound() public { + function test_Deposit_SkippedWhenFillBelowOracleSlippageBound() public { _configure(); _setOracle(0.95e18); // oracle: 1 PT = 0.95 USDC -> a buy should yield ~1.053x PT _register(); @@ -222,10 +235,15 @@ contract PendleStrategyTest is Test { vm.roll(block.number + 1); // Router fills at par (1000 PT) — ~5% worse than the oracle-implied amount, - // beyond the default 1% tolerance. The router enforces our derived minPtOut. + // beyond the default 1% tolerance. The router enforces our derived minPtOut, + // so the deposit reverts internally and rebalance skips it. + vm.expectEmit(true, false, false, true, address(vault)); + emit AllocatorFacet.StrategyRebalanceSkipped(PENDLE_ID, PendlePtStrategyFacet.pendleDeposit.selector); vm.prank(owner); - vm.expectRevert("MockPendle: minPtOut"); AllocatorFacet(address(vault)).rebalance(); + + assertEq(usdc.balanceOf(address(vault)), 1000 * 1e6, "funds stayed idle; over-slippage buy skipped"); + assertEq(PendlePtStrategyFacet(address(vault)).pendleTotalAssets(), 0, "no PT bought"); } function test_Deposit_SucceedsWhenFillWithinOracleSlippageBound() public { @@ -282,6 +300,33 @@ contract PendleStrategyTest is Test { assertEq(PendlePtStrategyFacet(address(vault)).pendleTotalAssets(), 600 * 1e6, "remaining position reported"); } + /// @notice Regression for F03: rebalance computes withdrawal deltas in ASSET + /// units, and pendleWithdraw must convert them to a PT quantity at the + /// oracle mark — NOT treat the asset delta as a raw PT amount. At a + /// non-par mark the two diverge, which is where the original bug bit. + function test_Withdraw_PreMaturity_AssetDenominatedAtDiscount() public { + _configureWithOracle(); + _register(); + _deployAll(1000 * 1e6); // 1000 PT bought at par + + // Mark the live position down to a 0.8 rate: 1000 PT now == 800 asset. + oracle.setRate(0.8e18); + assertEq(PendlePtStrategyFacet(address(vault)).pendleTotalAssets(), 800 * 1e6, "PT marked to 800 asset"); + + // Target the Pendle sleeve at 50% of NAV. NAV == 800 (idle 0 + pendle 800), + // so target == 400 and the allocator asks to withdraw 400 of ASSET value. + // Correct behavior: liquidate 400 / 0.8 == 500 PT (the old bug sold 400). + _setSingleAllocation(PENDLE_ID, 5000); + vm.roll(block.number + 1); + vm.prank(owner); + AllocatorFacet(address(vault)).rebalance(); + + assertEq(pt.balanceOf(address(vault)), 500 * 1e6, "liquidated asset-denominated PT (400 / 0.8)"); + assertEq( + PendlePtStrategyFacet(address(vault)).pendleTotalAssets(), 400 * 1e6, "Pendle sleeve converged to target" + ); + } + function test_Withdraw_PostMaturity_RedeemsAtFaceValue() public { _configureWithOracle(); _register(); @@ -299,21 +344,26 @@ contract PendleStrategyTest is Test { assertEq(pt.balanceOf(address(vault)), 0, "PT fully burned"); } - function test_Withdraw_PreMaturity_RevertsWhenNoOracle() public { + function test_Withdraw_PreMaturity_SkippedWhenNoOracle() public { // Mandatory oracle on the pre-maturity sell. Build a PT position directly - // (no oracle) so we can reach the sell path, then rebalance to 0%. + // (no oracle) so we can reach the sell path, then rebalance to 0%. The + // unpriced-sell refusal skips the strategy rather than bricking rebalance. _configure(); // no oracle _register(); pt.mint(address(vault), 1000 * 1e6); _setSingleAllocation(PENDLE_ID, 0); vm.roll(block.number + 1); + vm.expectEmit(true, false, false, true, address(vault)); + emit AllocatorFacet.StrategyRebalanceSkipped(PENDLE_ID, PendlePtStrategyFacet.pendleWithdraw.selector); vm.prank(owner); - vm.expectRevert(PendlePtStrategyFacet.PendleOracleRequired.selector); AllocatorFacet(address(vault)).rebalance(); + + assertEq(pt.balanceOf(address(vault)), 1000 * 1e6, "PT retained; unpriced sell skipped"); + assertEq(usdc.balanceOf(address(vault)), 0, "no underlying freed"); } - function test_Withdraw_RevertsWhenAmmHaircutExceedsSlippageBound() public { + function test_Withdraw_SkippedWhenAmmHaircutExceedsSlippageBound() public { _configureWithOracle(); // par mark: minTokenOut = 99% of PT sold _register(); _deployAll(1000 * 1e6); @@ -321,9 +371,14 @@ contract PendleStrategyTest is Test { router.setWithdrawHaircutBps(500); // 5% AMM haircut, beyond the 1% tolerance _setSingleAllocation(PENDLE_ID, 6000); // sell 400 vm.roll(block.number + 1); + + vm.expectEmit(true, false, false, true, address(vault)); + emit AllocatorFacet.StrategyRebalanceSkipped(PENDLE_ID, PendlePtStrategyFacet.pendleWithdraw.selector); vm.prank(owner); - vm.expectRevert("MockPendle: minTokenOut"); AllocatorFacet(address(vault)).rebalance(); + + assertEq(pt.balanceOf(address(vault)), 1000 * 1e6, "PT retained; over-slippage sell skipped"); + assertEq(usdc.balanceOf(address(vault)), 0, "no underlying freed"); } function test_Withdraw_SucceedsWhenHaircutWithinSlippageBound() public { diff --git a/test/unit/Quarantine.t.sol b/test/unit/Quarantine.t.sol index aa614a7..613d1db 100644 --- a/test/unit/Quarantine.t.sol +++ b/test/unit/Quarantine.t.sol @@ -135,6 +135,59 @@ contract QuarantineTest is Test { AllocatorFacet(address(vault)).rebalance(); } + // ----------------------------------------------------------------------- + // F06: permissionless recovery from a strategy whose NAV read reverts + // ----------------------------------------------------------------------- + + function test_QuarantineFailedStrategy_PermissionlesslyUnbricks() public { + MockStrategyFacet(address(vault)).mockSetReverting(true); // NAV read now reverts + + // The vault is bricked: totalAssets (and thus every ERC-4626 entrypoint) reverts. + vm.expectRevert(abi.encodeWithSelector(Vault.StrategyTotalAssetsCallFailed.selector, MOCK_ID)); + vault.totalAssets(); + + // ANY caller — no role required — can isolate a strategy whose read is + // actually failing, so recovery no longer waits on the owner. + address keeper = makeAddr("keeper"); + vm.prank(keeper); + AllocatorFacet(address(vault)).quarantineFailedStrategy(MOCK_ID); + + assertTrue(AllocatorFacet(address(vault)).isQuarantined(MOCK_ID), "permissionlessly quarantined"); + assertEq(vault.totalAssets(), 500 * 1e6, "vault live again; failing strategy excluded from NAV"); + } + + function test_QuarantineFailedStrategy_RevertsOnHealthyStrategy() public { + // A healthy strategy cannot be griefed offline: the on-chain liveness + // probe only permits quarantine when the NAV read genuinely reverts. + address keeper = makeAddr("keeper"); + vm.prank(keeper); + vm.expectRevert(abi.encodeWithSelector(AllocatorFacet.StrategyHealthy.selector, MOCK_ID)); + AllocatorFacet(address(vault)).quarantineFailedStrategy(MOCK_ID); + } + + // ----------------------------------------------------------------------- + // F05: a strategy that prices fine but cannot move funds is skipped, not + // allowed to brick the whole rebalance. + // ----------------------------------------------------------------------- + + function test_Rebalance_SkipsStrategyThatRevertsOnMove() public { + // Reads succeed (NAV still 500), but deposits/withdrawals revert — e.g. a + // paused lending pool. Quarantine's read-probe wouldn't catch this, so the + // rebalancer itself must tolerate the failed move. + MockStrategyFacet(address(vault)).mockSetRevertOnMove(true); + + _setSingleAllocation(MOCK_ID, 8000); // wants to deposit 300 more into the mock + vm.roll(block.number + 1); + + vm.expectEmit(true, false, false, true, address(vault)); + emit AllocatorFacet.StrategyRebalanceSkipped(MOCK_ID, MockStrategyFacet.mockDeposit.selector); + vm.prank(owner); + AllocatorFacet(address(vault)).rebalance(); // must NOT revert + + assertEq(AllocatorFacet(address(vault)).strategyTotalAssets(MOCK_ID), 500 * 1e6, "mock position unchanged"); + assertEq(usdc.balanceOf(address(vault)), 500 * 1e6, "idle unchanged; failed move skipped"); + } + // ----------------------------------------------------------------------- // harvestAll isolation // ----------------------------------------------------------------------- @@ -336,7 +389,7 @@ contract QuarantineTest is Test { } function _allocatorSelectors() internal pure returns (bytes4[] memory s) { - s = new bytes4[](16); + s = new bytes4[](17); s[0] = AllocatorFacet.registerStrategy.selector; s[1] = AllocatorFacet.removeStrategy.selector; s[2] = AllocatorFacet.setAllocation.selector; @@ -353,6 +406,7 @@ contract QuarantineTest is Test { s[13] = AllocatorFacet.quarantineStrategy.selector; s[14] = AllocatorFacet.releaseStrategy.selector; s[15] = AllocatorFacet.isQuarantined.selector; + s[16] = AllocatorFacet.quarantineFailedStrategy.selector; } function _harvestSelectors() internal pure returns (bytes4[] memory s) { @@ -362,7 +416,7 @@ contract QuarantineTest is Test { } function _mockSelectors() internal pure returns (bytes4[] memory s) { - s = new bytes4[](7); + s = new bytes4[](8); s[0] = MockStrategyFacet.mockSetProtocol.selector; s[1] = MockStrategyFacet.mockProtocol.selector; s[2] = MockStrategyFacet.mockTotalAssets.selector; @@ -370,5 +424,6 @@ contract QuarantineTest is Test { s[4] = MockStrategyFacet.mockWithdraw.selector; s[5] = MockStrategyFacet.mockHarvest.selector; s[6] = MockStrategyFacet.mockSetReverting.selector; + s[7] = MockStrategyFacet.mockSetRevertOnMove.selector; } } diff --git a/test/unit/ShareLock.t.sol b/test/unit/ShareLock.t.sol index e575568..e693d57 100644 --- a/test/unit/ShareLock.t.sol +++ b/test/unit/ShareLock.t.sol @@ -74,6 +74,31 @@ contract ShareLockTest is Test { LockFacet(address(vault)).setShareLockPeriod(tooLong); } + /// @notice Regression for F01: the lock is armed only when the depositor locks + /// their OWN shares (caller == receiver). A third party naming someone + /// else as receiver must not be able to freeze that account's balance. + function test_ThirdPartyDepositDoesNotLockReceiver() public { + vm.prank(owner); + LockFacet(address(vault)).setShareLockPeriod(PERIOD); + + address attacker = makeAddr("attacker"); + usdc.mint(attacker, 10 * 1e6); + vm.startPrank(attacker); + usdc.approve(address(vault), type(uint256).max); + vault.deposit(10 * 1e6, alice); // caller (attacker) != receiver (alice) + vm.stopPrank(); + + // Alice's lock must NOT be armed by someone else. + assertEq(LockFacet(address(vault)).lockedUntil(alice), 0, "third-party deposit must not lock the receiver"); + + // And alice can immediately move the shares she received. + uint256 bal = vault.balanceOf(alice); + assertGt(bal, 0, "alice received shares"); + vm.prank(alice); + vault.transfer(bob, bal); + assertEq(vault.balanceOf(bob), bal, "alice freely moved her unlocked shares"); + } + function test_SetShareLockPeriod_RevertsForNonOwner() public { vm.prank(alice); vm.expectRevert(abi.encodeWithSelector(LibDiamond.NotContractOwner.selector, alice, owner)); diff --git a/test/unit/WithdrawQueue.t.sol b/test/unit/WithdrawQueue.t.sol index 93b0d4b..6a79443 100644 --- a/test/unit/WithdrawQueue.t.sol +++ b/test/unit/WithdrawQueue.t.sol @@ -15,6 +15,7 @@ import { DiamondLoupeFacet } from "../../src/facets/DiamondLoupeFacet.sol"; import { OwnershipFacet } from "../../src/facets/OwnershipFacet.sol"; import { AllocatorFacet } from "../../src/facets/AllocatorFacet.sol"; import { WithdrawQueueFacet } from "../../src/facets/WithdrawQueueFacet.sol"; +import { LockFacet } from "../../src/facets/LockFacet.sol"; import { LibAllocator } from "../../src/libraries/LibAllocator.sol"; import { LibWithdrawQueue } from "../../src/libraries/LibWithdrawQueue.sol"; import { LibRoles } from "../../src/libraries/LibRoles.sol"; @@ -242,6 +243,51 @@ contract WithdrawQueueTest is Test { assertEq(WithdrawQueueFacet(address(vault)).pendingWithdrawShares(), 0, "queue drained"); } + // ----------------------------------------------------------------------- + // Griefing regression (F01): an attacker must not be able to brick the queue + // by arming the share lock on the vault's own escrow address. + // ----------------------------------------------------------------------- + + function test_Griefing_DepositToVaultCannotBrickQueue() public { + // Enable the anti-MEV share lock — the precondition the old bug needed. + vm.prank(owner); + LockFacet(address(vault)).setShareLockPeriod(1 hours); + + // Alice escrows half her shares for an async exit (her setUp deposit + // predates the lock, so her shares are unlocked and movable). + uint256 shares = vault.balanceOf(alice) / 2; + vm.prank(alice); + uint256 id = vault.requestWithdraw(shares, alice); + + // Attacker attempts the brick: a 1-wei deposit naming the VAULT as + // receiver, trying to arm lockedUntil[address(this)] so the escrow + // _burn/_transfer in fulfill/cancel revert SharesLocked. + address attacker = makeAddr("attacker"); + usdc.mint(attacker, 1); + vm.startPrank(attacker); + usdc.approve(address(vault), 1); + vault.deposit(1, address(vault)); + vm.stopPrank(); + + // The vault address must remain unlocked (caller != receiver ⇒ not armed). + assertEq( + LockFacet(address(vault)).lockedUntil(address(vault)), 0, "vault escrow address must never be lock-armed" + ); + + // Fulfill still works — the queue is not bricked. + vm.prank(owner); + vault.fulfillWithdraw(id); + assertGt(usdc.balanceOf(alice), 0, "curator fulfilled despite griefing attempt"); + + // And the cancel path is not bricked either. + uint256 rest = vault.balanceOf(alice); + vm.prank(alice); + uint256 id2 = vault.requestWithdraw(rest, alice); + vm.prank(alice); + vault.cancelWithdraw(id2); + assertEq(vault.balanceOf(alice), rest, "alice reclaimed escrowed shares via cancel"); + } + // ----------------------------------------------------------------------- // Helpers // ----------------------------------------------------------------------- @@ -273,8 +319,9 @@ contract WithdrawQueueTest is Test { AllocatorFacet allocator = new AllocatorFacet(); MockStrategyFacet mock = new MockStrategyFacet(); WithdrawQueueFacet queue = new WithdrawQueueFacet(); + LockFacet lock = new LockFacet(); - IDiamond.FacetCut[] memory cuts = new IDiamond.FacetCut[](6); + IDiamond.FacetCut[] memory cuts = new IDiamond.FacetCut[](7); cuts[0] = IDiamond.FacetCut({ facetAddress: address(cut), action: IDiamond.FacetCutAction.Add, functionSelectors: _diamondCutSelectors() }); @@ -301,6 +348,9 @@ contract WithdrawQueueTest is Test { action: IDiamond.FacetCutAction.Add, functionSelectors: _withdrawQueueSelectors() }); + cuts[6] = IDiamond.FacetCut({ + facetAddress: address(lock), action: IDiamond.FacetCutAction.Add, functionSelectors: _lockSelectors() + }); return new Vault(IERC20(address(usdc)), "Vault Router", "vUSDC", owner, cuts, address(0), ""); } @@ -350,4 +400,11 @@ contract WithdrawQueueTest is Test { s[1] = WithdrawQueueFacet.pendingWithdrawShares.selector; s[2] = WithdrawQueueFacet.withdrawRequest.selector; } + + function _lockSelectors() internal pure returns (bytes4[] memory s) { + s = new bytes4[](3); + s[0] = LockFacet.setShareLockPeriod.selector; + s[1] = LockFacet.shareLockPeriod.selector; + s[2] = LockFacet.lockedUntil.selector; + } }