Skip to content

Commit 09c55d9

Browse files
jayeshy14jayeshy14
authored
Merge pull request #11 from jayeshy14/fix/slither-findings
fix: enforce strategy slippage bounds, precise fee math, and vault reentrancy guard
2 parents 57ed113 + 630d267 commit 09c55d9

4 files changed

Lines changed: 162 additions & 21 deletions

File tree

‎src/Vault.sol‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ pragma solidity ^0.8.24;
44
import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
55
import { ERC4626 } from "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol";
66
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
7+
import { ReentrancyGuard } from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
8+
import { Math } from "@openzeppelin/contracts/utils/math/Math.sol";
79

810
import { IDiamond } from "./interfaces/IDiamond.sol";
911
import { Diamond } from "./Diamond.sol";
@@ -19,7 +21,7 @@ import { LibGuard } from "./libraries/LibGuard.sol";
1921
/// @dev Inflation attack mitigation comes from OZ ERC-4626's `_decimalsOffset`
2022
/// virtual shares. The ERC-4626 surface is native (non-facet) and therefore
2123
/// non-upgradeable, so it cannot be altered by a later diamondCut.
22-
contract Vault is Diamond, ERC4626 {
24+
contract Vault is Diamond, ERC4626, ReentrancyGuard {
2325
error StrategyTotalAssetsCallFailed(bytes32 strategyId);
2426

2527
constructor(
@@ -70,7 +72,7 @@ contract Vault is Diamond, ERC4626 {
7072
// Fee-accrual hooks
7173
// -----------------------------------------------------------------------
7274

73-
function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal override {
75+
function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal override nonReentrant {
7476
// Circuit breaker: revert if paused, or if the current share price has
7577
// moved beyond the configured bound since the last checkpoint.
7678
_guard();
@@ -93,6 +95,7 @@ contract Vault is Diamond, ERC4626 {
9395
)
9496
internal
9597
override
98+
nonReentrant
9699
{
97100
_guard();
98101
_accrueFees();
@@ -148,10 +151,13 @@ contract Vault is Diamond, ERC4626 {
148151

149152
// Performance fee — proportional to share-price gain above HWM.
150153
if (f.performanceFeeBps > 0 && sharePrice > f.highWaterMark) {
154+
// Full-precision mulDiv throughout: each step multiplies at 512-bit
155+
// width before dividing, so no intermediate truncation feeds the next
156+
// multiplication (fixes the divide-before-multiply rounding).
151157
uint256 profitPerShare = sharePrice - f.highWaterMark;
152-
uint256 profitValue = (profitPerShare * supply) / 1e18;
153-
uint256 feeValue = (profitValue * uint256(f.performanceFeeBps)) / LibFees.BPS_DENOMINATOR;
154-
if (ta > 0) feeShares += (feeValue * supply) / ta;
158+
uint256 profitValue = Math.mulDiv(profitPerShare, supply, 1e18);
159+
uint256 feeValue = Math.mulDiv(profitValue, uint256(f.performanceFeeBps), LibFees.BPS_DENOMINATOR);
160+
if (ta > 0) feeShares += Math.mulDiv(feeValue, supply, ta);
155161
f.highWaterMark = sharePrice;
156162
}
157163

‎src/facets/strategies/MorphoStrategyFacet.sol‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,11 @@ contract MorphoStrategyFacet {
114114
function morphoWithdraw(uint256 amount) external {
115115
MorphoStorage storage s = _ms();
116116
if (address(s.vault) == address(0)) revert MorphoVaultNotConfigured();
117-
s.vault.withdraw(amount, address(this), address(this));
117+
// Bound the shares burned: burning more than previewWithdraw predicted
118+
// means a worse-than-quoted price. Mirrors the morphoDeposit slippage check.
119+
uint256 expected = s.vault.previewWithdraw(amount);
120+
uint256 shares = s.vault.withdraw(amount, address(this), address(this));
121+
if (shares > expected) revert MorphoSlippage(expected, shares);
118122
}
119123

120124
/// @notice No-op for Metamorpho — supply yield auto-compounds into the

‎src/facets/strategies/PendlePtStrategyFacet.sol‎

Lines changed: 68 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,9 @@ contract PendlePtStrategyFacet {
6363
/// zero TWAP duration.
6464
error PendleInvalidOracle();
6565

66+
/// @notice Thrown when a configured slippage tolerance exceeds 100%.
67+
error PendleInvalidSlippage(uint16 bps);
68+
6669
// -----------------------------------------------------------------------
6770
// Events
6871
// -----------------------------------------------------------------------
@@ -73,13 +76,22 @@ contract PendlePtStrategyFacet {
7376
/// @notice Emitted when the mark-to-market oracle is set (or cleared).
7477
event PendleOracleSet(address indexed oracle, uint32 twapDuration);
7578

79+
/// @notice Emitted when the max AMM slippage tolerance is set.
80+
event PendleSlippageSet(uint16 maxSlippageBps);
81+
7682
// -----------------------------------------------------------------------
7783
// Storage
7884
// -----------------------------------------------------------------------
7985

8086
/// @dev erc7201:vaultrouter.strategy.pendle
8187
bytes32 internal constant PENDLE_STORAGE_SLOT = 0xb0e016db49ce2cfbe35770c2200cbf5f1a9b502bca57dbaaddf328cb9e0cef00;
8288

89+
/// @dev Basis-points denominator.
90+
uint16 internal constant PENDLE_BPS = 10_000;
91+
92+
/// @dev Slippage tolerance (bps) used when none is explicitly configured: 1%.
93+
uint16 internal constant DEFAULT_MAX_SLIPPAGE_BPS = 100;
94+
8395
struct PendleStorage {
8496
/// @notice PendleRouterV4 — handles all swap and redemption paths.
8597
IPendleRouter router;
@@ -92,6 +104,9 @@ contract PendlePtStrategyFacet {
92104
IPYLpOracle oracle;
93105
/// @notice TWAP window (seconds) passed to the oracle.
94106
uint32 twapDuration;
107+
/// @notice Max AMM slippage tolerance (bps) applied against the oracle
108+
/// mark when deriving minOut for swaps. Zero => 1% default.
109+
uint16 maxSlippageBps;
95110
}
96111

97112
function _ps() internal pure returns (PendleStorage storage s) {
@@ -146,6 +161,24 @@ contract PendlePtStrategyFacet {
146161
emit PendleOracleSet(address(oracle), twapDuration);
147162
}
148163

164+
/// @notice Set the maximum AMM slippage tolerance (bps) for pre-maturity
165+
/// swaps, applied against the oracle mark when deriving minOut.
166+
/// @dev Owner-gated. Only enforceable when an oracle is configured — without
167+
/// an on-chain mark there is no reference to bound the swap against.
168+
/// @param bps Tolerance in basis points (<= 10_000). Zero selects the 1% default.
169+
function pendleSetSlippage(uint16 bps) external {
170+
LibDiamond.enforceIsContractOwner();
171+
if (bps > PENDLE_BPS) revert PendleInvalidSlippage(bps);
172+
_ps().maxSlippageBps = bps;
173+
emit PendleSlippageSet(bps);
174+
}
175+
176+
/// @dev Effective slippage tolerance: the configured value, or the 1% default.
177+
function _maxSlippageBps(PendleStorage storage s) private view returns (uint16) {
178+
uint16 bps = s.maxSlippageBps;
179+
return bps == 0 ? DEFAULT_MAX_SLIPPAGE_BPS : bps;
180+
}
181+
149182
// -----------------------------------------------------------------------
150183
// Strategy surface (pendle* prefix)
151184
// -----------------------------------------------------------------------
@@ -209,20 +242,32 @@ contract PendlePtStrategyFacet {
209242
// Empty limit order, strategy does not participate in the limit book.
210243
IPendleRouter.LimitOrderData memory limit;
211244

212-
uint256 ptBefore = s.pt.balanceOf(address(this));
245+
// Derive an on-chain minimum from the oracle mark when available: invert
246+
// the PT->asset rate to value `amount` in PT, then haircut by the
247+
// slippage tolerance. The router reverts if it cannot meet minPtOut.
248+
// Without an oracle there is no mark to bound against, so we fall back to
249+
// 0 (unchanged behaviour for unpriced markets) and rely on the post-call
250+
// zero check.
251+
uint256 minPtOut;
252+
if (address(s.oracle) != address(0)) {
253+
uint256 rate = s.oracle.getPtToAssetRate(s.market, s.twapDuration);
254+
if (rate > 0) {
255+
uint256 expectedPt = amount * 1e18 / rate;
256+
minPtOut = expectedPt * (PENDLE_BPS - _maxSlippageBps(s)) / PENDLE_BPS;
257+
}
258+
}
213259

214-
s.router
260+
(uint256 netPtOut,,) = s.router
215261
.swapExactTokenForPt(
216262
address(this), // PT receiver is the vault itself
217263
s.market,
218-
0, // minPtOut: checked post-call below
264+
minPtOut,
219265
approx,
220266
input,
221267
limit
222268
);
223269

224-
uint256 ptReceived = s.pt.balanceOf(address(this)) - ptBefore;
225-
if (ptReceived == 0) revert PendleDepositFailed(ptReceived);
270+
if (netPtOut == 0) revert PendleDepositFailed(netPtOut);
226271
}
227272

228273
/// @notice Return `amount` of underlying from the Pendle position to the vault.
@@ -242,10 +287,11 @@ contract PendlePtStrategyFacet {
242287
if (amount > ptBalance) revert PendleInsufficientPt(amount, ptBalance);
243288

244289
IERC20 underlying = IERC20(IERC4626(address(this)).asset());
245-
uint256 underlyingBefore = underlying.balanceOf(address(this));
246290

247291
IERC20(address(s.pt)).forceApprove(address(s.router), amount);
248292

293+
uint256 received;
294+
249295
if (s.pt.isExpired()) {
250296
// Post-maturity: PT redeems 1:1. minTokenOut = 99% (dust tolerance).
251297
IPendleRouter.TokenOutput memory output = IPendleRouter.TokenOutput({
@@ -259,13 +305,24 @@ contract PendlePtStrategyFacet {
259305
});
260306

261307
// redeemPyToToken burns PT (YT is implicitly 0 post-maturity).
262-
s.router.redeemPyToToken(address(this), s.pt.YT(), amount, output);
308+
(received,) = s.router.redeemPyToToken(address(this), s.pt.YT(), amount, output);
263309
} else {
264-
// Pre-maturity: sell PT on the Pendle AMM.
265-
// minTokenOut = 0 here; slippage is validated post-call.
310+
// Pre-maturity: sell PT on the Pendle AMM. Derive minTokenOut from the
311+
// oracle mark when available (PT->asset rate, haircut by slippage) so
312+
// the router itself enforces the bound; fall back to 0 only when no
313+
// oracle is configured (unchanged behaviour for unpriced markets).
314+
uint256 minTokenOut;
315+
if (address(s.oracle) != address(0)) {
316+
uint256 rate = s.oracle.getPtToAssetRate(s.market, s.twapDuration);
317+
if (rate > 0) {
318+
uint256 expected = amount * rate / 1e18;
319+
minTokenOut = expected * (PENDLE_BPS - _maxSlippageBps(s)) / PENDLE_BPS;
320+
}
321+
}
322+
266323
IPendleRouter.TokenOutput memory output = IPendleRouter.TokenOutput({
267324
tokenOut: address(underlying),
268-
minTokenOut: 0,
325+
minTokenOut: minTokenOut,
269326
tokenRedeemSy: address(underlying),
270327
pendleSwap: address(0),
271328
swapData: IPendleRouter.SwapData({
@@ -275,10 +332,9 @@ contract PendlePtStrategyFacet {
275332

276333
IPendleRouter.LimitOrderData memory limit;
277334

278-
s.router.swapExactPtForToken(address(this), s.market, amount, output, limit);
335+
(received,,) = s.router.swapExactPtForToken(address(this), s.market, amount, output, limit);
279336
}
280337

281-
uint256 received = underlying.balanceOf(address(this)) - underlyingBefore;
282338
if (received == 0) revert PendleWithdrawFailed(amount, received);
283339
}
284340

‎test/unit/PendleStrategy.t.sol‎

Lines changed: 78 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,52 @@ contract PendleStrategyTest is Test {
198198
PendlePtStrategyFacet(address(vault)).pendleDeposit(1000 * 1e6);
199199
}
200200

201+
function test_Deposit_RevertsWhenFillBelowOracleSlippageBound() public {
202+
_configure();
203+
_setOracle(0.95e18); // oracle: 1 PT = 0.95 USDC -> a buy should yield ~1.053x PT
204+
uint256 amount = 1000 * 1e6;
205+
usdc.mint(address(vault), amount);
206+
207+
// Router fills at par (1000 PT) — ~5% worse than the oracle-implied amount,
208+
// beyond the default 1% tolerance. The router enforces our derived minPtOut.
209+
vm.expectRevert("MockPendle: minPtOut");
210+
PendlePtStrategyFacet(address(vault)).pendleDeposit(amount);
211+
}
212+
213+
function test_Deposit_SucceedsWhenFillWithinOracleSlippageBound() public {
214+
_configure();
215+
_setOracle(0.95e18);
216+
router.setDepositRateBps(10_500); // 1050 PT, within 1% of the ~1052.6 oracle mark
217+
uint256 amount = 1000 * 1e6;
218+
usdc.mint(address(vault), amount);
219+
220+
PendlePtStrategyFacet(address(vault)).pendleDeposit(amount);
221+
assertEq(pt.balanceOf(address(vault)), (amount * 10_500) / 10_000, "fill within tolerance accepted");
222+
}
223+
224+
// -----------------------------------------------------------------------
225+
// Slippage config — gating & validation
226+
// -----------------------------------------------------------------------
227+
228+
function test_SetSlippage_SetsAndEmits() public {
229+
vm.expectEmit(false, false, false, true, address(vault));
230+
emit PendlePtStrategyFacet.PendleSlippageSet(250);
231+
vm.prank(owner);
232+
PendlePtStrategyFacet(address(vault)).pendleSetSlippage(250);
233+
}
234+
235+
function test_SetSlippage_RevertsAboveBps() public {
236+
vm.prank(owner);
237+
vm.expectRevert(abi.encodeWithSelector(PendlePtStrategyFacet.PendleInvalidSlippage.selector, 10_001));
238+
PendlePtStrategyFacet(address(vault)).pendleSetSlippage(10_001);
239+
}
240+
241+
function test_SetSlippage_RevertsForNonOwner() public {
242+
vm.prank(alice);
243+
vm.expectRevert(abi.encodeWithSelector(LibDiamond.NotContractOwner.selector, alice, owner));
244+
PendlePtStrategyFacet(address(vault)).pendleSetSlippage(100);
245+
}
246+
201247
// -----------------------------------------------------------------------
202248
// Withdraw — pre-maturity (sell on AMM) vs post-maturity (redeem 1:1)
203249
// -----------------------------------------------------------------------
@@ -252,6 +298,30 @@ contract PendleStrategyTest is Test {
252298
PendlePtStrategyFacet(address(vault)).pendleWithdraw(400 * 1e6);
253299
}
254300

301+
function test_Withdraw_RevertsWhenAmmHaircutExceedsSlippageBound() public {
302+
_configure();
303+
uint256 amount = 1000 * 1e6;
304+
usdc.mint(address(vault), amount);
305+
PendlePtStrategyFacet(address(vault)).pendleDeposit(amount);
306+
_setOracle(1e18); // par mark: minTokenOut derived as 99% of PT sold
307+
308+
router.setWithdrawHaircutBps(500); // 5% AMM haircut, beyond the 1% tolerance
309+
vm.expectRevert("MockPendle: minTokenOut");
310+
PendlePtStrategyFacet(address(vault)).pendleWithdraw(400 * 1e6);
311+
}
312+
313+
function test_Withdraw_SucceedsWhenHaircutWithinSlippageBound() public {
314+
_configure();
315+
uint256 amount = 1000 * 1e6;
316+
usdc.mint(address(vault), amount);
317+
PendlePtStrategyFacet(address(vault)).pendleDeposit(amount);
318+
_setOracle(1e18);
319+
320+
router.setWithdrawHaircutBps(50); // 0.5% haircut, within the 1% tolerance
321+
PendlePtStrategyFacet(address(vault)).pendleWithdraw(400 * 1e6);
322+
assertEq(usdc.balanceOf(address(vault)), (400 * 1e6 * 9950) / 10_000, "fill within tolerance settled");
323+
}
324+
255325
// -----------------------------------------------------------------------
256326
// Oracle config — gating & validation
257327
// -----------------------------------------------------------------------
@@ -291,10 +361,14 @@ contract PendleStrategyTest is Test {
291361

292362
function test_TotalAssets_MarksToMarketWhenOracleSet() public {
293363
_configure();
294-
_setOracle(0.95e18); // PT marked at a 5% discount pre-maturity
295364
uint256 amount = 1000 * 1e6;
296365
usdc.mint(address(vault), amount);
366+
// Acquire the position at par, then mark it down. The oracle drives
367+
// totalAssets() valuation, not deposit execution, so it is set after the
368+
// buy — a discounted mark against a par fill would otherwise (correctly)
369+
// trip pendleDeposit's slippage guard.
297370
PendlePtStrategyFacet(address(vault)).pendleDeposit(amount);
371+
_setOracle(0.95e18); // PT marked at a 5% discount pre-maturity
298372

299373
// Face value is 1000 USDC, but the oracle marks it down to 950.
300374
assertEq(pt.balanceOf(address(vault)), amount, "holds PT at face");
@@ -313,10 +387,10 @@ contract PendleStrategyTest is Test {
313387

314388
function test_TotalAssets_PostMaturityIgnoresOracle() public {
315389
_configure();
316-
_setOracle(0.95e18);
317390
uint256 amount = 1000 * 1e6;
318391
usdc.mint(address(vault), amount);
319392
PendlePtStrategyFacet(address(vault)).pendleDeposit(amount);
393+
_setOracle(0.95e18); // set after the buy; see MarksToMarketWhenOracleSet
320394

321395
vm.warp(expiry + 1); // PT now redeems 1:1 — oracle discount must be bypassed
322396

@@ -512,7 +586,7 @@ contract PendleStrategyTest is Test {
512586
}
513587

514588
function _pendleSelectors() internal pure returns (bytes4[] memory s) {
515-
s = new bytes4[](13);
589+
s = new bytes4[](14);
516590
s[0] = PendlePtStrategyFacet.pendleSetConfig.selector;
517591
s[1] = PendlePtStrategyFacet.pendleTotalAssets.selector;
518592
s[2] = PendlePtStrategyFacet.pendleDeposit.selector;
@@ -526,5 +600,6 @@ contract PendleStrategyTest is Test {
526600
s[10] = PendlePtStrategyFacet.pendleSetOracle.selector;
527601
s[11] = PendlePtStrategyFacet.pendleOracle.selector;
528602
s[12] = PendlePtStrategyFacet.pendleTwapDuration.selector;
603+
s[13] = PendlePtStrategyFacet.pendleSetSlippage.selector;
529604
}
530605
}

0 commit comments

Comments
 (0)