feat: implement ERC4626 #240
Conversation
👷 Deploy request for compose-diamonds pending review.Visit the deploys page to approve it
|
|
Hi @maxnorm! This is a draft PR regarding the |
There was a problem hiding this comment.
The overall structure looks good to me. There's still work to do, even after my requested changes (i didn't want to make you wait longer. I'll review more in depth later this week)
Let's start by making sure it builds.
I left 2 Open Questions on how we handle Math operations inside Compose. I can see some functions that need to be inside the module and not the facet, like mint for example.
@farismln @heypran Since both of you participated in the past on the ERC4626 dev, if you'll like to, please help us review this
@mudgen Do you mind jumping in to get your opinion too
| if (totalShare == 0) { | ||
| return shares; | ||
| } | ||
| return shares * totalAssets() / totalShare; |
There was a problem hiding this comment.
Open Question : how do we handle potential overflow on this calculation?
Qpen Question 2 : Is it worth making a Math module to reuse the same calculation logic across all facets & modules that need it? this would ensure uniformity in calculations
Both OZ (mulDiv) and Solmate (mulDivDown & mulDivUp) using a Math library to handle prevent overflow and do rounding on mulDiv.
OZ implementation: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L248
Solmate implementation: https://github.com/transmissions11/solmate/blob/main/src/tokens/ERC4626.sol#L124
There was a problem hiding this comment.
We could create a dedicated math library to handle all edge cases across Compose, so we can reuse the arithmetic logic wherever required.
There was a problem hiding this comment.
@mudgen what do you think about the Math Module?
I think it's would be a great idea to centralize the arithmetic logic and avoid potential mismatch.
There was a problem hiding this comment.
Making a Math Module to use across Compose is a definite possibility in the future. But I want to see how far we can go without using modules in facets.
In general I want to avoid abstractions, avoid abstractions, avoid abstractions until we see we absolutely should use one and have a perfect fit.
There was a problem hiding this comment.
These math operations of course need to be safe and secure. Gas efficiency is of course great, but at this point it is better for the code to be more readable than gas efficient. So the math operations shouldn't be made more complicated or difficult to understand for gas efficiency.
Priorities are:
- Safe and secure.
- Easy to read and understand.
- Gas efficient.
There was a problem hiding this comment.
I have a feeling that existing math libraries are optimized for gas efficiency. I do not want our math operations optimized for gas efficiency unless they are more readable and easy to understand that way.
There was a problem hiding this comment.
Look at what the overall problems that are being solved here, and with this implementation and with the ERC standard. Maybe there is another way to solve the problems than using complicated math, but maybe not. It is worth looking. Complexity is solved by more looking, more design.
|
Hi @maxnorm! Thanks for reviewing. In this draft, my primary goal was to get feedback on the overall structure, so I intentionally left out the build-ready parts. I’ll make the necessary fixes so the code compiles cleanly. Another inspiration for the overflow handling could be from Uniswap V3 - https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/FullMath.sol |
Totally understand that! (and you did the right thing). Structure looks good to me. Thanks for the Uniswap ref. I'll go that a look into it too. |
Coverage Report
Last updated: Fri, 12 Dec 2025 09:25:47 GMT for commit |
Gas ReportNo gas usage changes detected between All functions maintain the same gas costs. ✅ Last updated: Fri, 12 Dec 2025 09:26:25 GMT for commit |
0x3b33
left a comment
0x3b33
left a comment
There was a problem hiding this comment.
The vault is vulnerable to first depositor attack
| * @param assets Amount of asset tokens. | ||
| * @return The computed amount of shares for `assets`. | ||
| */ | ||
| function convertToShares(uint256 assets) public view returns (uint256) { |
There was a problem hiding this comment.
The vault is vulnerable to the first depositor attack
Summary
The vault uses basic conversion between assets and shares, which does not control for inflation when ratios are low (usually done when the vault is first initialized).
Jumping strait into the example makes it easier to understand:
- User1 deposits 1 wei and receives 1 share
- User1 transfers 1000e18 assets to the vault, making the new share ratio
1 share = 1000e18 + 1 assets - User2 (victim) deposits 1500e18, but receives 1 share
Both users have 1 share and the vault has a total of 2500e18 + 1 assets. When both of them withdraw (or even only the user1) the vault is gonna assume the share ratio is 1250e18 for 1 share, which means that user1 is gonna receive 1250e18 assets.
Recommendation
Would be useful to add a decimal offset and change the formula to resemble the one OZ uses:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L245-L257
function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256) {
return assets.mulDiv(totalSupply() + 10 ** _decimalsOffset(), totalAssets() + 1, rounding);
}There was a problem hiding this comment.
I also wrote a basic harness and a tests to make sure the bug is valid, if you want i can share them <3
There was a problem hiding this comment.
this is a valid issue. I also recommending using the non zero decimal offset on previous Compose ERC4626 implementation but seems the new one did not done the same.
consider to add the decimal offset @akronim26 you can look on previous PR or like what @0x3b33 linked. cant link the old PR now because I am on mobile.
There was a problem hiding this comment.
Thanks to both of you for reviewing @akronim26 work!
We should add a decimal offset of 1, like recommended by @farismln on the past PR #154.
@0x3b33 yes, we want to see and add these tests.
There was a problem hiding this comment.
Awesome, but I am pretty bad with git D:
I can only push and pull.
So I've made these 3 gists, one with the test and the other 2 with the harness, they are pretty basic and only made to demonstrate this single bug.
https://gist.github.com/0x3b33/bbcd0dc0d42902efa1bf294a076688fa
https://gist.github.com/0x3b33/b2fb4cabe66ee428c50dce57c8756946
https://gist.github.com/0x3b33/9b9b93d4bee6126572c047e240250a79
All 3 follow the standard patter you have, where harnesses are in a special harness folder and tests are in token/ERC20/ERC4626/ folder.
If you want I have some free time this week and can developed a full test suite. Just let me know if you or someone else already started it.
There was a problem hiding this comment.
@maxnom I’ve pushed only the facet for now.
Regarding mulDiv and mulDivRoundingUp, I am fully open to make them more readable. For the moment, I have kept them aligned with the Uniswap V3 implementation, but I am happy to adjust the style once we settle on the preferred approach.
There was a problem hiding this comment.
@akronim26 after searching for a bit, this implementation looks good to me.
Here is the original work/article that Uniswap & OZ used as their foundation (i'll like to reference it on the func natspec to give credit)
https://xn--2-umb.com/21/muldiv/
If you can make the implementation more readable, we'll be happy with this. We can also add comments at every step to explain them and make them more understandable about what they are doing.
For now, we will use them as internal function inside both the Facet and Modules since it's only them that need it. We will see later if we extract them into a Math Module, depending if we reuse them elsewhere.
Does it sounds good to you? Please provide your feedback so we make a better informed decision on this
btw, thanks for your great work on this.
There was a problem hiding this comment.
Awesome, but I am pretty bad with git D: I can only push and pull.
Heyy @0x3b33 , first of thanks for the test implementation! We really appreciate.
For using Git & contribute to Compose, we recommend reading our contribution page: https://compose.diamonds/docs/contribution/how-to-contribute
The best way is via the Github UI.
- Fork the repo. That will create a new public repo under your profile link to Compose repo
- You can clone your fork locally
- Do your changes and push to your fork
- You have 2 nice buttons on the Github UI.
4.1. You can see there if you fork is ahead or behind based on the branch main on Compose Repo
4.2 The "Contribute" button allow you to create a new PR to Compose Repo
4.3 If you behind changes, you can easily sync your fork with the latest changes from the branch main on Compose Repo
Tell me if you have more questions! I'll be happy to help.
For the test suite, we will like that maybe talk with @akronim26 to sync both of your work. You will be able to create a new PR with the tests after we merge this one. I would also like complete test for the mulDiv functions to ensure we don't have any flaws in our arithmetic.
There was a problem hiding this comment.
@maxnorm, yes I completely agree with you. For now, the two functions are good as internal functions. I have added the comments throughout the file and tried to make muldiv more readable by adding comments to it. If this direction looks good to you, I can also replace the Yul implementation with an equivalent pure-Solidity version for clarity, and we can deal with the gas later.
There was a problem hiding this comment.
I'll let @mudgen decide on this.
Perso, i don't mind the Yul, if it's well documented.
I'll review the added comments later today.
farismln
left a comment
farismln
left a comment
There was a problem hiding this comment.
there are issue because the return value check of a transfer function, this would not work on weird token that does not have return value (typically USDT ETH Mainnet which is a quite famous token for asset in vault contract)
I suggest some changes. also lets open discussion whether it is good idea if we compromise the clean code to enabling this contract interact with USDT on Mainnet.
but the argument is we should enable this and support as many token as possible.
| if (!getStorage().asset.transferFrom(msg.sender, address(this), assets)) { | ||
| revert ERC4626TransferFailed(msg.sender, address(this), assets); | ||
| } |
There was a problem hiding this comment.
the expectation of return value from this transferFrom would cause USDT on ETH Mainnet to be incompatible as an asset. because it does not contain any return value.
the rationale that USDT is a popular asset in vault contract, so it is worth it to look for workaround.
the suggestion is to change all return value check of transfer to instead checking the balance increase and compare it to expected amount sent.
uint256 balanceBefore = getStorage().asset.balanceOf(address(this));
getStorage().asset.transferFrom(msg.sender, address(this), assets)
uint256 balanceAfter = getStorage().asset.balanceOf(address(this));
// do the check if assets == balanceAfter - balanceBeforeThere was a problem hiding this comment.
Use safeTransfer instead, it's gonna be much simpler.
There was a problem hiding this comment.
true. but I think we refrain using external library.
the elegant way is to use safeTransfer indeed.
need clarification @mudgen
There was a problem hiding this comment.
Using safeTransfer directly from OZ cannot be possible due to design pattern of Compose, however we could introduce our version for ERC4626Facet. @mudgen, @maxnorm - what are your views on this?
That said, the problem is also addressed by the approach suggested by @farismln, so I’m open to whichever direction you feel is more consistent with Compose’s design philosophy.
There was a problem hiding this comment.
Correct that we should not import anything into the facet or module for this or other things.
We are a new library and things change over time, so we shouldn't just follow what other smart contracts have done without looking, researching and thinking for ourselves. Maybe we can find a better way, maybe not, but it is worth looking. Each time we find a better way to do things, we provide more value to the library. We are certainly not a copy of existing libraries and we write our own code the Compose way.
Is USDT on Ethereum the only edge case that we need to handle? Or are there more? Will this facet ever be used with USDT on Ethereum? Has anyone ever written or implemented different approaches to solve safe ERC20 transfer? Can you find or think of a better way to do things with all factors considered?
I like the approach that @farismln gave of checking balances, but it is gas expensive to make external function calls, and that approach adds two external calls, so I would look for another way.
Please review the design docs:https://compose.diamonds/docs/design/.
I know this is sort of a rough draft, please keep in mind to replace "public" with "external".
Avoid internal functions, repeat yourself.
I really appreciate and am glad to see this work being done.
There was a problem hiding this comment.
Using
safeTransferdirectly from OZ cannot be possible due to design pattern of Compose, however we could introduce our version forERC4626Facet. @mudgen, @maxnorm - what are your views on this? That said, the problem is also addressed by the approach suggested by @farismln, so I’m open to whichever direction you feel is more consistent with Compose’s design philosophy.
I have created a personal version of safeTransfer and safeTransferFrom functions tailored for ERC4626. They address the edge-cases we discussed related to token transfers.
| * @title ERC4626Facet | ||
| * @notice Implementation of the ERC-4626 Tokenized Vault standard | ||
| */ | ||
| contract ERC4626Facet { |
There was a problem hiding this comment.
Sidenote: Compose now uses Solidity Modules (aka declaring free functions inside a .sol file)
The new terminology was changed from LibERC4626 to ERC4626Mod
See for more info: https://compose.diamonds/docs/foundations/solidity-modules
This is intended to make Compose future proof of Solidity changes, recently announced.
5 participants
Summary
Implement
ERC4626FacetandLibERC4626.Changes Made
Checklist
Before submitting this PR, please ensure:
Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions,
using fordirectives, orselfdestructCode follows Design Principles - Readable, uses diamond storage, favors composition over inheritance
Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)
Code is formatted with
forge fmtExisting tests pass - Run tests to be sure existing tests pass.
New tests are optional - If you don't provide tests for new functionality or changes then please create a new issue so this can be assigned to someone.
All tests pass - Run
forge testand ensure everything worksDocumentation updated - If applicable, update relevant documentation
Make sure to follow the contributing guidelines.
Additional Notes