Skip to content

feat(4626 wrapper): implements a 4626 wrapping of debt tokens#86

Open
0xMcsweeja wants to merge 17 commits intomainnetfrom
feat-4626-wrapper
Open

feat(4626 wrapper): implements a 4626 wrapping of debt tokens#86
0xMcsweeja wants to merge 17 commits intomainnetfrom
feat-4626-wrapper

Conversation

@0xMcsweeja
Copy link

@0xMcsweeja 0xMcsweeja commented Oct 29, 2025

GH Issue

Factory wrapper for bridging debt tokens

TLDR

wraps a wildcat market debt token and exposes a non-rebasing share supply. Shares represent the underlying scaled balance of the wrapper inside the market; interest accrual is reflected through the share price rather than token balances.

entering vault:

  • deposit: debt token holder wants to deposit X normalized market tokens and receive the matching scaled shares
  • mint: debt token holder wants to mint X shares (scaled units) by transferring in Y normalized market tokens

exiting vault:

  • withdraw: share holder targets X normalized market tokens out; the vault burns the minimum shares (scaled units) needed so that normalized amount can be sent to the receiver
  • redeem: share holder burns X shares (scaled units) and receives the corresponding normalized market tokens that those shares map to at the current scale factor

random notes:

  • do we want any access control / ownership for any reason
  • do we want a way to sweep any tokens that might land in the vault unexpectedly for any reason?
  • pls double check the naming convention for the wrapper name and symbol based on feedback from L (that i may have misinterpreted)

Clanker Feature Summary:

  • Wildcat4626Wrapper (v2-protocol/src/vault/Wildcat4626Wrapper.sol) is a Solady-based ERC‑4626 vault that wraps a Wildcat market debt token. The underlying “asset” is the rebasing market token; the vault “shares” are a
    non‑rebasing ERC20 that track the market’s scaled balances (i.e., share count you can bridge cross‑chain).
  • Conversions don’t use totalAssets/totalSupply; they use the market’s scaleFactor (ray, 1e27).
    • Spec views: convertToShares/convertToAssets and previewDeposit/previewRedeem floor‑round; previewMint/previewWithdraw ceil‑round (EIP‑4626 compliant).
    • Execution: deposit/mint/withdraw/redeem use half‑up rounding to mirror Wildcat’s rayDiv/rayMul, then verify actual scaled deltas.
  • deposit(assets, receiver) pulls market tokens and mints shares equal to scaledBalanceOf(wrapper) increase; reverts on zeroes, cap excess, sanctions, or if minted shares fall below the half‑up expectation.
  • mint(shares, receiver) computes the minimal assets that half‑up‑round to exactly shares, transfers them in, and requires the scaled delta to equal shares (so mint is exact per spec).
  • withdraw(assets, receiver, owner) burns half‑up shares for the asset amount, spends allowance if needed, transfers assets out, and requires the scaled delta to match burned shares. redeem(shares, receiver, owner) is the
    symmetric exact‑shares path.
  • Safety/ops: reentrancy guard on all mutating funcs; sanctions enforced for callers/owners/receivers and on share transfers via _beforeTokenTransfer; cap enforced vs market.maxTotalSupply(); stray asset donations raise
    totalAssets but don’t dilute shares (assets become stranded). Borrower can sweep non‑market ERC20s only.
  • Wildcat4626WrapperFactory (v2-protocol/src/vault/Wildcat4626WrapperFactory.sol) permissionlessly deploys at most one wrapper per registered market via archController.isRegisteredMarket, stored in wrapperForMarket.
  • Tests (v2-protocol/test/vault/4626) cover metadata, rounding rules, scale‑factor changes, sanctions, cap, donation/inflation resistance, fuzzed execution, and a full ERC‑4626 standard test suite against a real
    WildcatMarket.

@laurenceday
Copy link
Contributor

Largely looks fine to me with some caveats - what we're probably going to want here is a primary factory that any market can deploy a wrapper contract for once and once only (so tracking centrally via the factory itself whether a wrapper contract exists), so we can extend the UI with a 'wanna wrap? here. wrapper doesn't exist yet? deploy it yourself!'

To the extent that access control is needed, I'd leave it pretty open ended in the sense that anyone can hit the factory to create a new instance of a vault wrapper if one doesn't exist, since its' existence doesn't affect the market in any form.

For the second question, for the sake of sanity I'd also include a sweep function for arbitrary ERC20s, with the account permitted to do that being the owner of the market itself. Wouldn't necessarily call it 'owner' for the wrapper since the only thing it could do is sweep, but I guess that's six and two threes.

@laurenceday
Copy link
Contributor

Naturally my thoughts on 'LGTM' are to be overridden by @d1ll0n

@kethcode kethcode self-requested a review November 21, 2025 15:16
@kethcode
Copy link

kethcode commented Nov 22, 2025

Why are we using different math for preview vs execution? deposits, mints, burns, all use rayDiv and rayMul, which does half-up rounding the same as the market. convertToShares/Assets uses mulDiv which floors, and mulDivUp which is ceiling. i dont think it's a problem per se, but don't understand why we don't use the _scaledX functions for the previews, so we match the market ray math.

Copy link

@kethcode kethcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see any reason to hold this up, so I'm going to approval it.

I've already mentioned the preview math not matching the execution math, but realistically that not material.

There are some gas optimizations we could consider. Wrapper cap is fied to uint256.max. I don't know if we ever want to allow the borrower to change that. If we don't plan to change that, we could not spend on the store sot and SLOAD... just inline type(unint256).max from maxDeposit/maxMint.

We should fix the SPDX declaration.

For cross-chain visibility and convenience, we could consider assetsPerShareRay() view helpers or something, so convertToAssets doesn't need to choose a sample share size.

We already have wrapperForMarket. If we want to do marketForWrapper, we could consider a view function or an event, but we ealready expse market(), so not really necesssary.

A few nits thrown by aderyn, but outside the scope here, not worth holding things up over.

Good work.

@0xMcsweeja
Copy link
Author

Updates from reviews / feedback:

Feedback from @d1ll0n via tg:

prefer solady over OZ
wrapper cap not needed / uint256.max was wrong anyway as markets used 128 or less

  • i swapped it so that we now use the solady 4626 (and therefore its erc20 too)
  • wrapper cap removed and now the previewMax* will just use underlying market cap.

feedback from @kethcode :

Why are we using different math for preview vs execution? deposits, mints, burns ..
I've already mentioned the preview math not matching the execution math, but realistically that not material.

  • initially i wasnt going to bother doing much here but i ended up aligning the preview and executions but after spending too long on the spec i ended up introducing a _convertToSharesHalfUp which only gets used for Deposit and Withdraw exclusively as they need to perfectly mirror the underlying wildcat market half-up rounding so that the predicted vs actual shares match. (the other approach to avoid needing exact matches is to relax the conditional so that we accept >= shares if its still above the floor. I prefer the exact prediction/actual model tho)

For cross-chain visibility and convenience, we could consider assetsPerShareRay() view helpers or something, so convertToAssets doesn't need to choose a sample share size.

  • added this (assetsPerShareRay ) and a sharesPerAssetRay (the inverse) as views

We should fix the SPDX declaration.

  • for the contracts that are deployed (non-tests) i copied the license thats in the wildcat market contracts. I am absolutely NOT a lawyer or license expert at all so would appreciate if yourself or @laurenceday can confirm the contract licensing

other stuff:

  • added extra test just for some coverage of conversion helpers vs eip requirements
  • added a few extra wrapper tests

return MathUtils.mulDiv(assets, RAY, scaleFactor);
}

function _convertToSharesUp(uint256 assets, uint256 scaleFactor) internal pure returns (uint256) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused why this exists. we are operating on a wrapped market. we already have _scaledSharesForAssets and _scaledAssetsForShares. why not do something like

    function convertToShares(uint256 assets) public view override returns (uint256) {
        if (assets == 0) return 0;
        uint256 scaleFactor = wrappedMarket.scaleFactor();
        return _scaledSharesForAssets(assets, scaleFactor);
    }
    
    function convertToAssets(uint256 shares) public view override returns (uint256) {
        if (shares == 0) return 0;
        uint256 scaleFactor = wrappedMarket.scaleFactor();
        return _scaledAssetsForShares(shares, scaleFactor);
    }

this should match the existing market behavior, and also matches how you implemented maxWithdraw and previewWithdraw.

edit:

ok, i see the conflict. our market does not behave like EIP-4626 requires. convertToShares/convertToAssets and previewDeposit/previewRedeem must round down; previewMint/previewWithdraw must round up. Our market token converts between assets and scaled balances with half-up rounding (via rayDiv/rayMul). We should decide if the preview/runtime mismatch specified by EIP-4626 is preferred, or havin the preview match the market behavior. the difference is at most 1 unit in a uint256, so EIP-4626 alignment may be preferred, in which case your code is correct.

@0xMcsweeja
Copy link
Author

0xMcsweeja commented Dec 11, 2025

updates:

sanctions support:

  • added sentinel checks to deposit withdraw mint redeem and any use of the transfer hook.
  • Theres probably (definitely) some redundancy here ie checking for sanction on a mint and then again on the underlying transfers however im happy having some repeated sanction checks over missing any paths to save gas. happy to be challenged though
  • previews eg maxDeposit will check for sanctions too and return 0 for any naughty addresses

rounding:

  • aligned to market half-up where appropriate and aligned the previews with execution and allow favourable slippage on deposits. also aligned the 4626 rounding requirements

tests:

  • added some fuzz tests for roundtrip flows (theyre probably not perfect so feel free to improve)
  • updated wrapper tests with sanction mocks/stubs
  • imported a16z 4626-tests and wired up the wrapper to use the suite. it validates the main properties and things like spec-compliant rounding. got those passing but could be worth a second look:
image

open questions / comments:

  • tokens sent directly to the wrapper (not via mint / deposit) are accounting-safe but theyre effectively dead. they cant be pulled out or swept so does this make it a partial write-off? and should the wildcat market/frontend then reflect that too ? alternatively we can add some form of admin-sweep for market tokens but would they go to the borrower or us or back to sender?

@kethcode
Copy link

I have some questions about sanctions and nukeFromOrbit(). it looks like the borrower can override a sanction on false positives, but does that create a race condition with nuke? whomever gets to it first wins? We may want offchain monitoring of sanction state of markets so borrowers can be notified and can call overrideSanction(wrapper) before they get nuked.

market.maxTotalSupply - wrapper.totalAssets has already been brought up a couple times. donations could be used for griefing i guess. in theory, could totalAssets exceed the cap, locking deposit/mint? big enough donation disabling new wraps.

the totalAssets thing might cause mispricing if other protocols ignore convertToAssets/assetsPerShareRay.

mulDiv and MulDivUp revert on overflow as desired. not a problem as long as we are operating within realistic inputs. previews of type(uint256).max would revert for instance, but not really an issue.

We should explicitly put cap details and totalAssets behavrio in the documentation we share with integrators.

Signed-off-by: Dave Coleman <dave@wildcat.finance>
Signed-off-by: Dave Coleman <dave@wildcat.finance>
Signed-off-by: Dave Coleman <dave@wildcat.finance>
Signed-off-by: Dave Coleman <dave@wildcat.finance>
Signed-off-by: Dave Coleman <dave@wildcat.finance>
Signed-off-by: Dave Coleman <dave@wildcat.finance>
@kethcode
Copy link

This has been released to the auditor.

…ue 2 and 3 will be acknowledged but ignored.

Signed-off-by: Dave Coleman <dave@wildcat.finance>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants