From 630d2677b3be55e70ae35d8bd9e70548cd26f39a Mon Sep 17 00:00:00 2001 From: Jayesh Yadav Date: Sat, 30 May 2026 18:52:28 +0530 Subject: [PATCH] fix: enforce strategy slippage bounds, precise fee math, and vault reentrancy guard --- src/Vault.sol | 16 ++-- src/facets/strategies/MorphoStrategyFacet.sol | 6 +- .../strategies/PendlePtStrategyFacet.sol | 80 +++++++++++++++--- test/unit/PendleStrategy.t.sol | 81 ++++++++++++++++++- 4 files changed, 162 insertions(+), 21 deletions(-) diff --git a/src/Vault.sol b/src/Vault.sol index 76d6062..b091cde 100644 --- a/src/Vault.sol +++ b/src/Vault.sol @@ -4,6 +4,8 @@ pragma solidity ^0.8.24; import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; import { ERC4626 } from "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol"; import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import { ReentrancyGuard } from "@openzeppelin/contracts/utils/ReentrancyGuard.sol"; +import { Math } from "@openzeppelin/contracts/utils/math/Math.sol"; import { IDiamond } from "./interfaces/IDiamond.sol"; import { Diamond } from "./Diamond.sol"; @@ -19,7 +21,7 @@ import { LibGuard } from "./libraries/LibGuard.sol"; /// @dev Inflation attack mitigation comes from OZ ERC-4626's `_decimalsOffset` /// virtual shares. The ERC-4626 surface is native (non-facet) and therefore /// non-upgradeable, so it cannot be altered by a later diamondCut. -contract Vault is Diamond, ERC4626 { +contract Vault is Diamond, ERC4626, ReentrancyGuard { error StrategyTotalAssetsCallFailed(bytes32 strategyId); constructor( @@ -70,7 +72,7 @@ contract Vault is Diamond, ERC4626 { // Fee-accrual hooks // ----------------------------------------------------------------------- - function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal override { + function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal override nonReentrant { // Circuit breaker: revert if paused, or if the current share price has // moved beyond the configured bound since the last checkpoint. _guard(); @@ -93,6 +95,7 @@ contract Vault is Diamond, ERC4626 { ) internal override + nonReentrant { _guard(); _accrueFees(); @@ -148,10 +151,13 @@ contract Vault is Diamond, ERC4626 { // Performance fee — proportional to share-price gain above HWM. if (f.performanceFeeBps > 0 && sharePrice > f.highWaterMark) { + // Full-precision mulDiv throughout: each step multiplies at 512-bit + // width before dividing, so no intermediate truncation feeds the next + // multiplication (fixes the divide-before-multiply rounding). uint256 profitPerShare = sharePrice - f.highWaterMark; - uint256 profitValue = (profitPerShare * supply) / 1e18; - uint256 feeValue = (profitValue * uint256(f.performanceFeeBps)) / LibFees.BPS_DENOMINATOR; - if (ta > 0) feeShares += (feeValue * supply) / ta; + uint256 profitValue = Math.mulDiv(profitPerShare, supply, 1e18); + uint256 feeValue = Math.mulDiv(profitValue, uint256(f.performanceFeeBps), LibFees.BPS_DENOMINATOR); + if (ta > 0) feeShares += Math.mulDiv(feeValue, supply, ta); f.highWaterMark = sharePrice; } diff --git a/src/facets/strategies/MorphoStrategyFacet.sol b/src/facets/strategies/MorphoStrategyFacet.sol index 74704fa..587c5e1 100644 --- a/src/facets/strategies/MorphoStrategyFacet.sol +++ b/src/facets/strategies/MorphoStrategyFacet.sol @@ -114,7 +114,11 @@ contract MorphoStrategyFacet { function morphoWithdraw(uint256 amount) external { MorphoStorage storage s = _ms(); if (address(s.vault) == address(0)) revert MorphoVaultNotConfigured(); - s.vault.withdraw(amount, address(this), address(this)); + // Bound the shares burned: burning more than previewWithdraw predicted + // means a worse-than-quoted price. Mirrors the morphoDeposit slippage check. + uint256 expected = s.vault.previewWithdraw(amount); + uint256 shares = s.vault.withdraw(amount, address(this), address(this)); + if (shares > expected) revert MorphoSlippage(expected, shares); } /// @notice No-op for Metamorpho — supply yield auto-compounds into the diff --git a/src/facets/strategies/PendlePtStrategyFacet.sol b/src/facets/strategies/PendlePtStrategyFacet.sol index 3378445..edb39fd 100644 --- a/src/facets/strategies/PendlePtStrategyFacet.sol +++ b/src/facets/strategies/PendlePtStrategyFacet.sol @@ -63,6 +63,9 @@ contract PendlePtStrategyFacet { /// zero TWAP duration. error PendleInvalidOracle(); + /// @notice Thrown when a configured slippage tolerance exceeds 100%. + error PendleInvalidSlippage(uint16 bps); + // ----------------------------------------------------------------------- // Events // ----------------------------------------------------------------------- @@ -73,6 +76,9 @@ contract PendlePtStrategyFacet { /// @notice Emitted when the mark-to-market oracle is set (or cleared). event PendleOracleSet(address indexed oracle, uint32 twapDuration); + /// @notice Emitted when the max AMM slippage tolerance is set. + event PendleSlippageSet(uint16 maxSlippageBps); + // ----------------------------------------------------------------------- // Storage // ----------------------------------------------------------------------- @@ -80,6 +86,12 @@ contract PendlePtStrategyFacet { /// @dev erc7201:vaultrouter.strategy.pendle bytes32 internal constant PENDLE_STORAGE_SLOT = 0xb0e016db49ce2cfbe35770c2200cbf5f1a9b502bca57dbaaddf328cb9e0cef00; + /// @dev Basis-points denominator. + uint16 internal constant PENDLE_BPS = 10_000; + + /// @dev Slippage tolerance (bps) used when none is explicitly configured: 1%. + uint16 internal constant DEFAULT_MAX_SLIPPAGE_BPS = 100; + struct PendleStorage { /// @notice PendleRouterV4 — handles all swap and redemption paths. IPendleRouter router; @@ -92,6 +104,9 @@ contract PendlePtStrategyFacet { IPYLpOracle oracle; /// @notice TWAP window (seconds) passed to the oracle. uint32 twapDuration; + /// @notice Max AMM slippage tolerance (bps) applied against the oracle + /// mark when deriving minOut for swaps. Zero => 1% default. + uint16 maxSlippageBps; } function _ps() internal pure returns (PendleStorage storage s) { @@ -146,6 +161,24 @@ contract PendlePtStrategyFacet { emit PendleOracleSet(address(oracle), twapDuration); } + /// @notice Set the maximum AMM slippage tolerance (bps) for pre-maturity + /// swaps, applied against the oracle mark when deriving minOut. + /// @dev Owner-gated. Only enforceable when an oracle is configured — without + /// an on-chain mark there is no reference to bound the swap against. + /// @param bps Tolerance in basis points (<= 10_000). Zero selects the 1% default. + function pendleSetSlippage(uint16 bps) external { + LibDiamond.enforceIsContractOwner(); + if (bps > PENDLE_BPS) revert PendleInvalidSlippage(bps); + _ps().maxSlippageBps = bps; + emit PendleSlippageSet(bps); + } + + /// @dev Effective slippage tolerance: the configured value, or the 1% default. + function _maxSlippageBps(PendleStorage storage s) private view returns (uint16) { + uint16 bps = s.maxSlippageBps; + return bps == 0 ? DEFAULT_MAX_SLIPPAGE_BPS : bps; + } + // ----------------------------------------------------------------------- // Strategy surface (pendle* prefix) // ----------------------------------------------------------------------- @@ -209,20 +242,32 @@ contract PendlePtStrategyFacet { // Empty limit order, strategy does not participate in the limit book. IPendleRouter.LimitOrderData memory limit; - uint256 ptBefore = s.pt.balanceOf(address(this)); + // Derive an on-chain minimum from the oracle mark when available: invert + // the PT->asset rate to value `amount` in PT, then haircut by the + // slippage tolerance. The router reverts if it cannot meet minPtOut. + // Without an oracle there is no mark to bound against, so we fall back to + // 0 (unchanged behaviour for unpriced markets) and rely on the post-call + // zero check. + uint256 minPtOut; + if (address(s.oracle) != address(0)) { + uint256 rate = s.oracle.getPtToAssetRate(s.market, s.twapDuration); + if (rate > 0) { + uint256 expectedPt = amount * 1e18 / rate; + minPtOut = expectedPt * (PENDLE_BPS - _maxSlippageBps(s)) / PENDLE_BPS; + } + } - s.router + (uint256 netPtOut,,) = s.router .swapExactTokenForPt( address(this), // PT receiver is the vault itself s.market, - 0, // minPtOut: checked post-call below + minPtOut, approx, input, limit ); - uint256 ptReceived = s.pt.balanceOf(address(this)) - ptBefore; - if (ptReceived == 0) revert PendleDepositFailed(ptReceived); + if (netPtOut == 0) revert PendleDepositFailed(netPtOut); } /// @notice Return `amount` of underlying from the Pendle position to the vault. @@ -242,10 +287,11 @@ contract PendlePtStrategyFacet { if (amount > ptBalance) revert PendleInsufficientPt(amount, ptBalance); IERC20 underlying = IERC20(IERC4626(address(this)).asset()); - uint256 underlyingBefore = underlying.balanceOf(address(this)); 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). IPendleRouter.TokenOutput memory output = IPendleRouter.TokenOutput({ @@ -259,13 +305,24 @@ contract PendlePtStrategyFacet { }); // redeemPyToToken burns PT (YT is implicitly 0 post-maturity). - s.router.redeemPyToToken(address(this), s.pt.YT(), amount, output); + (received,) = s.router.redeemPyToToken(address(this), s.pt.YT(), amount, output); } else { - // Pre-maturity: sell PT on the Pendle AMM. - // minTokenOut = 0 here; slippage is validated post-call. + // Pre-maturity: sell PT on the Pendle AMM. Derive minTokenOut from the + // oracle mark when available (PT->asset rate, haircut by slippage) so + // the router itself enforces the bound; fall back to 0 only when no + // oracle is configured (unchanged behaviour for unpriced markets). + uint256 minTokenOut; + if (address(s.oracle) != address(0)) { + uint256 rate = s.oracle.getPtToAssetRate(s.market, s.twapDuration); + if (rate > 0) { + uint256 expected = amount * rate / 1e18; + minTokenOut = expected * (PENDLE_BPS - _maxSlippageBps(s)) / PENDLE_BPS; + } + } + IPendleRouter.TokenOutput memory output = IPendleRouter.TokenOutput({ tokenOut: address(underlying), - minTokenOut: 0, + minTokenOut: minTokenOut, tokenRedeemSy: address(underlying), pendleSwap: address(0), swapData: IPendleRouter.SwapData({ @@ -275,10 +332,9 @@ contract PendlePtStrategyFacet { IPendleRouter.LimitOrderData memory limit; - s.router.swapExactPtForToken(address(this), s.market, amount, output, limit); + (received,,) = s.router.swapExactPtForToken(address(this), s.market, amount, output, limit); } - uint256 received = underlying.balanceOf(address(this)) - underlyingBefore; if (received == 0) revert PendleWithdrawFailed(amount, received); } diff --git a/test/unit/PendleStrategy.t.sol b/test/unit/PendleStrategy.t.sol index 5124518..a1f4e89 100644 --- a/test/unit/PendleStrategy.t.sol +++ b/test/unit/PendleStrategy.t.sol @@ -198,6 +198,52 @@ contract PendleStrategyTest is Test { PendlePtStrategyFacet(address(vault)).pendleDeposit(1000 * 1e6); } + function test_Deposit_RevertsWhenFillBelowOracleSlippageBound() public { + _configure(); + _setOracle(0.95e18); // oracle: 1 PT = 0.95 USDC -> a buy should yield ~1.053x PT + uint256 amount = 1000 * 1e6; + usdc.mint(address(vault), amount); + + // Router fills at par (1000 PT) — ~5% worse than the oracle-implied amount, + // beyond the default 1% tolerance. The router enforces our derived minPtOut. + vm.expectRevert("MockPendle: minPtOut"); + PendlePtStrategyFacet(address(vault)).pendleDeposit(amount); + } + + function test_Deposit_SucceedsWhenFillWithinOracleSlippageBound() public { + _configure(); + _setOracle(0.95e18); + router.setDepositRateBps(10_500); // 1050 PT, within 1% of the ~1052.6 oracle mark + uint256 amount = 1000 * 1e6; + usdc.mint(address(vault), amount); + + PendlePtStrategyFacet(address(vault)).pendleDeposit(amount); + assertEq(pt.balanceOf(address(vault)), (amount * 10_500) / 10_000, "fill within tolerance accepted"); + } + + // ----------------------------------------------------------------------- + // Slippage config — gating & validation + // ----------------------------------------------------------------------- + + function test_SetSlippage_SetsAndEmits() public { + vm.expectEmit(false, false, false, true, address(vault)); + emit PendlePtStrategyFacet.PendleSlippageSet(250); + vm.prank(owner); + PendlePtStrategyFacet(address(vault)).pendleSetSlippage(250); + } + + function test_SetSlippage_RevertsAboveBps() public { + vm.prank(owner); + vm.expectRevert(abi.encodeWithSelector(PendlePtStrategyFacet.PendleInvalidSlippage.selector, 10_001)); + PendlePtStrategyFacet(address(vault)).pendleSetSlippage(10_001); + } + + function test_SetSlippage_RevertsForNonOwner() public { + vm.prank(alice); + vm.expectRevert(abi.encodeWithSelector(LibDiamond.NotContractOwner.selector, alice, owner)); + PendlePtStrategyFacet(address(vault)).pendleSetSlippage(100); + } + // ----------------------------------------------------------------------- // Withdraw — pre-maturity (sell on AMM) vs post-maturity (redeem 1:1) // ----------------------------------------------------------------------- @@ -252,6 +298,30 @@ contract PendleStrategyTest is Test { PendlePtStrategyFacet(address(vault)).pendleWithdraw(400 * 1e6); } + function test_Withdraw_RevertsWhenAmmHaircutExceedsSlippageBound() public { + _configure(); + uint256 amount = 1000 * 1e6; + usdc.mint(address(vault), amount); + PendlePtStrategyFacet(address(vault)).pendleDeposit(amount); + _setOracle(1e18); // par mark: minTokenOut derived as 99% of PT sold + + router.setWithdrawHaircutBps(500); // 5% AMM haircut, beyond the 1% tolerance + vm.expectRevert("MockPendle: minTokenOut"); + PendlePtStrategyFacet(address(vault)).pendleWithdraw(400 * 1e6); + } + + function test_Withdraw_SucceedsWhenHaircutWithinSlippageBound() public { + _configure(); + uint256 amount = 1000 * 1e6; + usdc.mint(address(vault), amount); + PendlePtStrategyFacet(address(vault)).pendleDeposit(amount); + _setOracle(1e18); + + router.setWithdrawHaircutBps(50); // 0.5% haircut, within the 1% tolerance + PendlePtStrategyFacet(address(vault)).pendleWithdraw(400 * 1e6); + assertEq(usdc.balanceOf(address(vault)), (400 * 1e6 * 9950) / 10_000, "fill within tolerance settled"); + } + // ----------------------------------------------------------------------- // Oracle config — gating & validation // ----------------------------------------------------------------------- @@ -291,10 +361,14 @@ contract PendleStrategyTest is Test { function test_TotalAssets_MarksToMarketWhenOracleSet() public { _configure(); - _setOracle(0.95e18); // PT marked at a 5% discount pre-maturity uint256 amount = 1000 * 1e6; usdc.mint(address(vault), amount); + // Acquire the position at par, then mark it down. The oracle drives + // totalAssets() valuation, not deposit execution, so it is set after the + // buy — a discounted mark against a par fill would otherwise (correctly) + // trip pendleDeposit's slippage guard. PendlePtStrategyFacet(address(vault)).pendleDeposit(amount); + _setOracle(0.95e18); // PT marked at a 5% discount pre-maturity // Face value is 1000 USDC, but the oracle marks it down to 950. assertEq(pt.balanceOf(address(vault)), amount, "holds PT at face"); @@ -313,10 +387,10 @@ contract PendleStrategyTest is Test { function test_TotalAssets_PostMaturityIgnoresOracle() public { _configure(); - _setOracle(0.95e18); uint256 amount = 1000 * 1e6; usdc.mint(address(vault), amount); PendlePtStrategyFacet(address(vault)).pendleDeposit(amount); + _setOracle(0.95e18); // set after the buy; see MarksToMarketWhenOracleSet vm.warp(expiry + 1); // PT now redeems 1:1 — oracle discount must be bypassed @@ -512,7 +586,7 @@ contract PendleStrategyTest is Test { } function _pendleSelectors() internal pure returns (bytes4[] memory s) { - s = new bytes4[](13); + s = new bytes4[](14); s[0] = PendlePtStrategyFacet.pendleSetConfig.selector; s[1] = PendlePtStrategyFacet.pendleTotalAssets.selector; s[2] = PendlePtStrategyFacet.pendleDeposit.selector; @@ -526,5 +600,6 @@ contract PendleStrategyTest is Test { s[10] = PendlePtStrategyFacet.pendleSetOracle.selector; s[11] = PendlePtStrategyFacet.pendleOracle.selector; s[12] = PendlePtStrategyFacet.pendleTwapDuration.selector; + s[13] = PendlePtStrategyFacet.pendleSetSlippage.selector; } }