fix: mint PM fee shares against fee-adjusted base#175
Merged
maximebrugel merged 1 commit intoMay 4, 2026
Merged
Conversation
Convert combined management + performance fee assets to shares against `totalAssets - totalFeeAssets` so the minted shares redeem to exactly the advertised fee assets, instead of underpaying fees by pricing them against the pre-fee pool that still contains the fee slice.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Free Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
maxencerb
approved these changes
Apr 28, 2026
maximebrugel
deleted the
feature/3f-258-pm-fee-shares-are-minted-against-the-pre-fee-base-and
branch
maximebrugel
added a commit
that referenced
this pull request
May 14, 2026
* feat: Superstate allowlist restrictions for WrappedAsset and TransferGuard (#174) * feat: add Superstate allowlist restrictions for WrappedAsset and TransferGuard Enforce Superstate's `isAllowed()` compliance check at the token transfer and transfer guard levels to prevent addresses outside the USCC allowlist from holding or transferring wrapped tokens. New contracts: - SuperstateRestrictedWrappedAsset: extends WrappedAsset, checks both transfer parties against ISuperstateToken(underlying).isAllowed() - SuperstateRestrictedTransferGuard: extends TransferGuard with Superstate allowlist enforcement on both non-null parties - WhitelistedPartyTransferGuard: standalone guard allowing transfers when at least one party is on a whitelist (e.g., the Facility) - SuperstateRestrictedWhitelistedPartyTransferGuard: combines whitelisted- party logic with Superstate allowlist checks - Beacon proxy factories for all three guard variants Restructures guard folder layout into base/, superstate/, and whitelisted-party/ subfolders for separation of concerns. All new contracts have 100% test coverage (line, branch, function). * refactor: extract TransferGuardFactoryBase to deduplicate factory logic * style: apply forge fmt formatting * refactor: unify TransferGuard with NATIVE status, TokenMode enum, and collateral isAllowed check Replace 4 guard contracts (TransferGuard, SuperstateRestrictedTransferGuard, WhitelistedPartyTransferGuard, SuperstateRestrictedWhitelistedPartyTransferGuard) with a single TransferGuard that supports 4 token modes: - BLOCKLIST (default): all allowed except BLOCKLIST addresses - WHITELIST: only WHITELIST/NATIVE addresses allowed - NATIVE_ONLY: at least one NATIVE party required, no BLOCKLIST - NATIVE_WHITELIST: all parties WHITELIST/NATIVE, at least one NATIVE Add NATIVE address status for protocol addresses (e.g., Facility). Add checkCollateralAllowed flag to call WrappedAsset.isAllowed() via PositionManager.assets() for Superstate-style compliance delegation. Add virtual isAllowed(address, uint256) hook to WrappedAsset (base returns true). SuperstateRestrictedWrappedAsset overrides to delegate to ISuperstateToken.isAllowed(). Base _beforeTokenTransfer now calls isAllowed for non-null parties. Delete 6 guard source files, 6 test files, and 2 directories. Net: -872 lines. All 1660 tests pass. * refactor: remove TransferGuardFactoryBase, flatten guard folder structure Inline TransferGuardFactoryBase logic into TransferGuardFactory (only one factory remains). Move TransferGuard and TransferGuardFactory from src/guard/base/ to src/guard/. Remove base/ subdirectories from source, test, and mock folders. Update all imports. * refactor: make isAllowed public to avoid external self-call * fix: update comments to say "all modes" instead of "both modes" * refactor: simplify canTransfer to one storage read per address Read each address status once upfront, then do all mode checks on the loaded values. Remove _isAllowed, _hasNative, _isWhitelistedOrNative helpers. Lift BLOCKLIST check to top (shared across all modes). * refactor: decompose canTransfer mode logic into noneBlocked/nativeRequired properties * docs: update README tree and TransferGuard section for unified guard Update directory tree with SuperstateRestrictedWrappedAsset and isAllowed hook. Rewrite TransferGuard section with 4 token modes (BLOCKLIST, WHITELIST, NATIVE_ONLY, NATIVE_WHITELIST), NATIVE address status, collateral allowlist check, and updated flow diagram. * test: harden MockSuperstateToken with transfer restrictions and add integration tests Tighten SuperstateRestrictedWrappedAsset NatSpec to clarify two-layer enforcement. Add _beforeTokenTransfer to MockSuperstateToken to mirror production USCC allowlist enforcement. Add tests proving burn-to-disallowed-recipient and mint-by-disallowed-issuer revert at the underlying token layer. * fix: mint PM fee shares against fee-adjusted base (#175) Convert combined management + performance fee assets to shares against `totalAssets - totalFeeAssets` so the minted shares redeem to exactly the advertised fee assets, instead of underpaying fees by pricing them against the pre-fee pool that still contains the fee slice. * fix: enforce transfer guard in zero-delta and zero-mint settle branches (#176) In PositionManagerLP._settleShares(), the no-share-movement branches only checked paused() and skipped blocklist, whitelist, native-only, and collateral-allowed checks. A blocklisted MINTER_ROLE caller could perform a neutral deposit/withdraw and bypass the guard entirely. Replace the paused()-only check with canTransfer(msg.sender) — the same convention used by _beforeTokenTransfer — and add coverage for blocklisted callers in both zero-delta and zero-mint branches. * fix: extend reentrancy guard to pullFunds and authorizeMinting (#177) The onRequestConsumed callback in consume() could re-enter pullFunds() or authorizeMinting() because those entrypoints lacked the nonReentrant guard the rest of the contract relies on. A maker holding PULLER_ROLE could withdraw and re-deposit the pending principal to mint PT/YT without net new capital; a maker owning the Request could mint a fresh authorization mid-consume. Bring the whole contract under one reentrancy domain and cover the new vectors with mirroring tests. * fix: PT/YT ERC-20 compliance gaps (#178) Closes 3F-244. Three deviations from EIP-20 are addressed on the user-facing PT/YT surface (transfer, transferFrom, approve): 1. Transfer event is now emitted on zero-value transfers, on the relevant token only (no spurious zero-value event on the sibling token). 2. Approval event is emitted on every successful approve, including when the stored value is unchanged. 3. Approve amounts in [type(uint128).max, type(uint256).max - 1] used to be silently clamped to the infinite-allowance sentinel and read back as type(uint256).max. They now revert with AllowanceTooLarge. Only values strictly below type(uint128).max (stored exactly) and exactly type(uint256).max (stored as the infinite sentinel) are accepted. Implementation: event emission was lifted out of the inner _transfer and _setAllowance helpers up to their public callers, where the relevant token is known; _approve now bypasses _setAllowance and writes the sibling allowance verbatim so the sentinel is never re-validated. Unused LibAllowance.normalize was removed since the public allowance() view inlines its logic. * fix: validate Pareto vault mode gates in ParetoFund.create (#179) ParetoFund.create() previously accepted orders that were guaranteed to revert at commit() time when the upstream CDO had `isDepositDuringEpochDisabled` or `allowAAWithdrawRequest=false`. Such stale ACCEPTED orders blocked subsequent orders and bound Facility intents until manually canceled. Reject deposit orders when the vault is running an epoch with `depositDuringEpoch` disabled, and redeem orders when AA withdrawal requests are disabled, with dedicated errors so failures are unambiguous. * fix: reject USCCFund redeems while Superstate accounting is paused (#180) Without this check, a redeem order would be accepted but its commit would always revert in offchainRedeem (a burn path), leaving the order stuck in ACCEPTED and blocking subsequent orders until manually canceled. * fix: settle shares before outbound transfer in deposit/withdraw (#183) * fix: settle shares before outbound transfer in deposit/withdraw `deposit()` and `withdraw()` previously transferred tokens to the caller between `processDeposit/processWithdrawal` (which mutates NAV) and `_settleShares()` (which mints/burns the matching shares), leaving `totalAssets()` and `totalSupply()` desynced during the external call. Move `_settleShares()` ahead of the outbound transfer so any token hook or third-party view observes a consistent share price. Add Solady's `nonReadReentrant` to `totalAssets()`, `totalSupply()` (via override), `pendingFees()`, `collateralAmount()`, `collateralAmountQuoted()`, and `debtAmount()` as defense-in-depth. Internal callers in the guarded scope read `ERC20.totalSupply()` directly to bypass the override. * fix: forge fmt * docs: explain why preLiquidate omits nonReentrant (3F-262) (#184) Audit finding 3F-262 flagged the absence of a reentrancy guard on MorphoBorrowPosition.preLiquidate() and onMorphoRepay(); its own recommendation was to keep them unguarded since callback re-entry is economically equivalent to sequential liquidation calls. Capture that rationale and the load-bearing role assumption (liquidators must never hold MINTER_ROLE / FACILITATOR_ROLE) in NatSpec so the design decision is visible at the point of reading and is not re-flagged in future audits. * fix: add zero-address owner check to Request and PositionManager initialize (#187) Solady's _initializeOwner does not revert when owner is address(0), so each contract inheriting OwnableRoles must guard its own initialize. Mirrors the existing Facility.initialize check using LibChecks.checkNotZero. * docs: clarify totalAssets reflects wrapper-wide AUM (3F-238) (#186) When a WrappedAsset is shared across fund instances, totalAssets() returns the aggregate supply across all instances, not per-fund AUM. Document this on IFund and on USCCFund/CentrifugeFund/ParetoFund so integrators don't assume per-fund scoping. * fix(pareto): allow redeems when vault is in keyringAllowWithdraw mode (#181) * fix: allow ParetoFund redeems when vault is in keyringAllowWithdraw mode ParetoFund.create/commit always required isWalletAllowed(address(this)), which is stricter than the upstream IdleCDOEpochVariant — the vault skips the wallet allowlist when keyringAllowWithdraw is true (used for liquidations and other open-withdraw modes). Wrapped AA holders could therefore be locked out of Grunt while the underlying vault still accepted withdrawals. Align the check with upstream for REDEEM orders: pass if isWalletAllowed or keyringAllowWithdraw. DEPOSIT orders remain strictly gated on isWalletAllowed. * test: align invariant_safeLtvEnforcement with _isHealthy debt predicate bp.totalBorrowed() rounds down via toAssetsDown, but _isHealthy short-circuits on borrowShares == 0 and otherwise rounds up via toAssetsUp. Dust shares left after a burn could leave totalBorrowed() at 0 while _isHealthy still treated the position as having debt, producing a false PM-7 failure under extreme oracle prices. Use the raw Morpho borrowShares as the "no debt" predicate so the invariant agrees with _isHealthy. * forge fmt * fix: validate wUSCC receiver against Superstate allowlist in USCCFund (#182) * fix: validate wUSCC receiver against Superstate allowlist in USCCFund create() and commit() previously only checked that the fund itself was Superstate-allowlisted. Once wUSCC mint gates on the receiver's allowlist status, an order could pass create()/commit() with an ineligible receiver and then get permanently stuck at unlock()/recover() when the wrapper mint reverts. Re-validate order.receiver in both entry points so bad orders are rejected before USDC leaves the fund. * forge fmt * fix: 3F-248 ERC-4626 views reflect real-time withdrawal state (#185) Decouple Request._canWithdraw() from the stored `repaid` flag so that canWithdraw, convertToAssets, maxWithdraw, and maxRedeem reflect effective state once the repayment deadline has passed, without requiring a prior syncRepaidStatus() call. Reorder _redeem/_withdraw to sync before pricing, which prevents the first post-deadline redeem from reverting against an empty balance. * docs: align NatSpec with code + small audit fixes (#188) Documentation pass addressing audit-flagged drift between code and comments, plus one missing invariant and two minor code tweaks. - Fix `intialBeaconOwner` typo in RequestFactory. - Rewrite mode-aware NatSpec for IFund.commit/recover/unlock and IFacilityFunds.create to spell out DEPOSIT vs REDEEM semantics. - Fix LibIntent.init NatSpec to mention transferableIntent flag. - Loosen README updateTarget allowed-states (DEPOSITING / RESOLVING). - Clarify guardKey field doc in IntentProperties. - Fix ParetoFund.totalAssets virtualPrice decimals comment (WAD-scaled). - Fix LibPause.pauseFor cast-safety comment. - Document WrappedAsset.burn redemption flow and zero-address transfer behavior under Solady ERC20. - Document Solady ERC20 storage compatibility in LibTokenController. - Document seizedAssets source on MorphoBorrowPosition.onMorphoRepay. - Document maxRebalanceLoss==0 caveat for 1-2 wei phantom NAV losses. - Add USCCFund constructor invariant: WUSCC.underlying() == USCC, mirroring Pareto/Centrifuge. - Switch VaultController._assetsAndSupplies to IERC20.balanceOf. --------- Co-authored-by: Maxence Raballand <dev@maxencerb.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
https://cantina.xyz/code/7164dd7a-a679-463f-b2b9-7b39f182e758/findings/29
PositionManagerBase._pendingFees()was carving the management/performance fee assets out oftotalAssets_, then converting those fee assets to shares against the pre-feetotalAssets_. The minted shares redeemed for less than the originally computed fee assets (e.g. 200e18 advertised fee → ~196.08e18 actually claimable on a 1y/2% accrual against a 10,000e18 deposit), silently understating the configured management/performance fee percentages.totalAssets_ - totalFeeAssets. The minted shares now redeem to exactly the advertised fee assets.managementFeeAssetsattotalAssets_so very long accrual intervals don't underflowfeeAdjustedAssets. The combined sum is mathematically bounded bytotalAssets_givenMAX_PERFORMANCE_FEE = 5000, so no extra cap is needed.managementFeeShares/performanceFeeSharesso their sum exactly matches the combined-base conversion and_accrueFees()mints the right total.