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

fix(mintYieldFee): check for partial collateralization (c4-issue-435) #13

Merged

Conversation

PierrickGT
Copy link
Contributor

@@ -1,312 +1,313 @@
TN:
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be committed

✂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The repo is not configured properly but we need to display the coverage in PRs: https://github.com/zgosalvez/github-actions-report-lcov

src/Vault.sol Outdated
_withdrawableAssets = _withdrawableAssets - (_withdrawableAssets - _totalSupplyToAssets);
// If the Vault is collateralized, yield is excluded from the withdrawable amount
// and users can only withdraw the amount of underlying assets they deposited.
// Otherwise, any unclaimed yield is included and shared proportionally amongst depositors.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment needs to be updated I think

The additional assets are not shared proportionally, AFAIK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is once the Vault is undercollateralized since _withdrawableAssets won't be equal to _depositedAssets, so any underlying assets withdrawable from the YieldVault will be available and the exchange will be computed using the following formula:
return _withdrawableAssets.mulDiv(_assetUnit, _totalSupplyAmount, Math.Rounding.Down);

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 language thing; there is no yield if it's undercollateralized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed wording in this commit: 81fe36a

Base automatically changed from gen-224-c4-issue-129 to main August 16, 2023 02:40
@PierrickGT PierrickGT force-pushed the gen-197-c4-issue-435-mintyieldfee-does-not-check-maxmint branch from 1017a67 to 8097392 Compare August 16, 2023 21:05
@PierrickGT PierrickGT merged commit 8985bea into main Aug 16, 2023
1 of 2 checks passed
@PierrickGT PierrickGT deleted the gen-197-c4-issue-435-mintyieldfee-does-not-check-maxmint branch August 16, 2023 23:03
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.

3 participants