Skip to content
Open
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
243 changes: 243 additions & 0 deletions src/token/ERC20/ERC4626/ERC4626Facet.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,243 @@
// SPDX-License-Identifier: MIT
pragma solidity >=0.8.30;

/**
* @dev Implementation of the ERC-4626
*/
contract ERC4626Facet {
event Deposit(address indexed caller, address indexed owner, uint256 assets, uint256 shares);
event Withdraw(address indexed caller, address indexed receiver, address indexed owner, uint256 assets, uint256 shares);
event Approval(address indexed owner, address indexed spender, uint256 value);
event Transfer(address indexed from, address indexed to, uint256 value);

error ERC4626ExceededMaxDeposit(address receiver, uint256 assets, uint256 max);
error ERC4626ExceededMaxMint(address receiver, uint256 shares, uint256 max);
error ERC4626ExceededMaxWithdraw(address owner, uint256 assets, uint256 max);
error ERC4626ExceededMaxRedeem(address owner, uint256 shares, uint256 max);
error ERC4626ZeroAmount();
error ERC4626ZeroAddress();

bytes32 constant STORAGE_POSITION = keccak256("compose.erc4626");
bytes32 constant ERC20_STORAGE_POSITION = keccak256("compose.erc20");

struct ERC20Storage {
mapping(address owner => uint256 balance) balanceOf;
uint256 totalSupply;
mapping(address owner => mapping(address spender => uint256 allowance)) allowances;
uint8 decimals;
string name;
string symbol;
}

struct ERC4626Storage {
IERC20 asset;
}

function getStorage() internal pure returns (ERC4626Storage storage s) {
bytes32 position = STORAGE_POSITION;
assembly {
s.slot := position
}
}

function getERC20Storage() internal pure returns (ERC20Storage storage s) {
bytes32 position = ERC20_STORAGE_POSITION;
assembly {
s.slot := position
}
}

function asset() public view returns (address) {
return address(getStorage().asset);
}

function totalAssets() public view returns (uint256) {
return getStorage().asset.balanceOf(address(this));
}

function decimals() public view returns (uint8) {
ERC20Storage storage erc20s = getERC20Storage();
return erc20s.decimals;
}

function balanceOf(address account) public view returns (uint256) {
ERC20Storage storage erc20s = getERC20Storage();
return erc20s.balanceOf[account];
}

function totalShares() public view returns (uint256) {
ERC20Storage storage s = getERC20Storage();
return s.totalSupply;
}

function convertToShares(uint256 assets) public view returns (uint256) {
Copy link

Choose a reason for hiding this comment

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

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:

  1. User1 deposits 1 wei and receives 1 share
  2. User1 transfers 1000e18 assets to the vault, making the new share ratio 1 share = 1000e18 + 1 assets
  3. 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);
    }

Copy link

Choose a reason for hiding this comment

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

I also wrote a basic harness and a tests to make sure the bug is valid, if you want i can share them <3

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@maxnorm maxnorm Dec 8, 2025

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

@maxnorm maxnorm Dec 10, 2025

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

@maxnorm maxnorm Dec 10, 2025

Choose a reason for hiding this comment

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

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.

  1. Fork the repo. That will create a new public repo under your profile link to Compose repo
fork-example
  1. You can clone your fork locally
  2. Do your changes and push to your fork
  3. 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
fork-sync-contribute

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

uint256 totalShare = totalSupply();
/** This is the state when the vault was used for the first time.
* The ratio between the assets and the shares is 1:1 when the vault is in initial state.
*/
if (totalShare == 0) {
return assets;
}
return assets * totalShare / totalAssets();
}

function convertToAssets(uint256 shares) public view returns (uint256) {
uint256 totalShare = totalShares();
/** This is the state when the vault was used for the first time.
* The ratio between the assets and the shares is 1:1 when the vault is in initial state.
*/
if (totalShare == 0) {
return shares;
}
return shares * totalAssets() / totalShare;
Copy link
Collaborator

@maxnorm maxnorm Dec 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@akronim26 akronim26 Dec 4, 2025

Choose a reason for hiding this comment

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

We could create a dedicated math library to handle all edge cases across Compose, so we can reuse the arithmetic logic wherever required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@akronim26
@maxnorm

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@akronim26
@maxnorm

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:

  1. Safe and secure.
  2. Easy to read and understand.
  3. Gas efficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@mudgen mudgen Dec 8, 2025

Choose a reason for hiding this comment

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

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.

}


function previewDeposit(uint256 assets) public view returns (uint256) {
return convertToShares(assets);
}

function previewMint(uint256 shares) public view returns (uint256) {
uint256 totalShare = totalShares();
uint256 totalAsset = totalAssets();
if (totalShare == 0) {
return shares;
}

// rounded up
return (shares * totalAssets + totalShare - 1) / totalShare;
}

function previewWithdraw(uint256 assets) public view returns (uint256) {
uint256 totalShare = totalShares();
uint256 totalAsset = totalAssets();
if (totalShare == 0) {
return assets;
}
return (assets * totalShare + totalAsset - 1) / totalAsset;
}

function previewRedeem(uint256 shares) public view returns (uint256) {
return convertToAssets(shares);
}

function maxDeposit(address) public view returns (uint256) {
return type(uint256).max;
}

function maxMint(address) public view returns (uint256) {
return type(uint256).max;
}

function maxWithdraw(address owner) public view returns (uint256) {
return previewRedeem(maxRedeem(owner));
}

function maxRedeem(address owner) public view returns (uint256) {
return balanceOf(owner);
}


function deposit(uint256 assets, address receiver) public returns (uint256) {
uint256 maxAssets = maxDeposit(receiver);
if (assets > maxAssets) {
revert ERC4626ExceededMaxDeposit(receiver, assets, maxAssets);
}
uint256 shares = previewDeposit(assets);
if (shares == 0) {
revert ERC4626ZeroAmount();
}

_deposit(_msgSender(), receiver, assets, shares);
return shares;
}

function mint(uint256 shares, address receiver) public returns (uint256) {
uint256 maxShares = maxMint(receiver);
if (shares > maxShares) {
revert ERC4626ExceededMaxMint(receiver, shares, maxShares);
}
uint256 assets = previewMint(shares);
if (assets == 0) {
revert ERC4626ZeroAmount();
}

_deposit(_msgSender(), receiver, assets, shares);
return assets;
}


function withdraw(uint256 assets, address receiver, address owner) public returns (uint256) {
uint256 maxAssets = maxWithdraw(owner);
if (assets > maxAssets) {
revert ERC4626ExceededMaxWithdraw(owner, assets, maxAssets);
}
uint256 shares = previewWithdraw(assets);
if (shares == 0) {
revert ERC4626ZeroAmount();
}

_withdraw(_msgSender(), receiver, owner, assets, shares);
return shares;
}

function redeem(uint256 shares, address receiver, address owner) public returns (uint256) {
uint256 maxShares = maxRedeem(owner);
if (shares > maxShares) {
revert ERC4626ExceededMaxRedeem(owner, shares, maxShares);
}
uint256 assets = previewRedeem(shares);
if (assets == 0) {
revert ERC4626ZeroAmount();
}

_withdraw(_msgSender(), receiver, owner, assets, shares);
return assets;
}



function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal {
if (receiver == address(0)) {
revert ERC4626ZeroAddress();
}
ERC20Storage storage erc20s = getERC20Storage();
getStorage().asset.transferFrom(caller, address(this), assets);

erc20s.totalSupply += shares;
erc20s.balanceOf[receiver] += shares;

emit Transfer(address(0), receiver, shares);
emit Deposit(caller, receiver, assets, shares);
}

function _withdraw(
address caller,
address receiver,
address owner,
uint256 assets,
uint256 shares
) internal {
ERC20Storage storage erc20s = getERC20Storage();

if (caller != owner) {
uint256 allowed = erc20s.allowances[owner][caller];
require(allowed >= shares, "INSUFFICIENT_ALLOWANCE");
if (allowed != type(uint256).max) {
erc20s.allowances[owner][caller] = allowed - shares;
}
}
if (receiver == address(0)) {
revert ERC4626ZeroAddress();
}

erc20s.balanceOf[owner] -= shares;
erc20s.totalSupply -= shares;

emit Transfer(owner, address(0), shares);

getStorage().asset.transfer(receiver, assets);

emit Withdraw(caller, receiver, owner, assets, shares);
}
}
Loading