Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove obsolete ERC4626 mint NatSpec #4837

Merged
merged 3 commits into from
Jan 17, 2024

Conversation

dglowinski
Copy link
Contributor

@dglowinski dglowinski commented Jan 17, 2024

Removes NatSpec for ERC4626 mint function, which says that minting is allowed when the price of a share is zero. With the introduction of virtual shares and assets mitigation mechanism for the inflation attack, this is no longer true, because a price of a share can no longer be zero.

Zero price of a share in this context means that shares amount requested in mint would be converted to zero assets by previewMint. It would only be possible if totalSupply was greater than zero, and totalAssets was zero in the previous version of the code (given that previewMint function rounds up):

function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256) {
        uint256 supply = totalSupply();
        return
            (supply == 0) ? _initialConvertToAssets(shares, rounding) : shares.mulDiv(totalAssets(), supply, rounding);
}

The new version of the function unconditionally adds 1 virtual asset to the numerator in the conversion, making 'zero price of a share' impossible, given upward rounding.

function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256) {
     return shares.mulDiv(totalAssets() + 1, totalSupply() + 10 ** _decimalsOffset(), rounding);
}

The original comment was introduced in: Improve some NatSpec and revert reasons #3809

It was made obsolete in ERC4626 inflation attack mitigation #3979

Where the test of the original behaviour was removed

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

Copy link

changeset-bot bot commented Jan 17, 2024

⚠️ No Changeset found

Latest commit: 7e8aaef

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dglowinski dglowinski changed the title remove obsolete ERC4626 mint NatSpec Remove obsolete ERC4626 mint NatSpec Jan 17, 2024
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

LGTM. Needs @ernestognw approval.

Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for clearly documenting @dglowinski!

@ernestognw ernestognw merged commit 06eb785 into OpenZeppelin:master Jan 17, 2024
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants