-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(morpho): protect from slippage #314
feat(morpho): protect from slippage #314
Conversation
Rubilmax
commented
Oct 23, 2023
•
edited by MerlinEgalite
Loading
edited by MerlinEgalite
- Fixes https://github.com/cantinasec/review-morpho-blue-1/issues/33
- Fixes M-03 Bundler Slippage May Cause Unexpected Loss of Funds #344
Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> Signed-off-by: Jean-Grimal <83286814+Jean-Grimal@users.noreply.github.com>
Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> Signed-off-by: Jean-Grimal <83286814+Jean-Grimal@users.noreply.github.com>
…of github.com:morpho-labs/morpho-stack into feat/async-slippage-protection
…to feat/async-slippage-protection
…bs/morpho-stack into feat/morpho-slippage-protection
3ea66a7
to
4e7913e
Compare
…bs/morpho-stack into feat/morpho-slippage-protection
Are all those commits necessary? 👀 |
They are due to the mix between rebase & merge |
…bs/morpho-blue-bundlers into feat/morpho-slippage-protection
…bs/morpho-blue-bundlers into feat/morpho-slippage-protection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also add revert test for slippage
This PR should not be merged yet |
…undlers into feat/morpho-slippage-protection
feat(morpho): protect from slippage
🚨 Vulnerabilities Summary
For more details view the full report in OpenZeppelin Code |
…undlers into feat/morpho-slippage-protection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/// @param shares The amount of shares to burn. | ||
/// @param slippageAmount The minimum amount of shares to mint in exchange for `assets` when it is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error: shares
are burned in withdraw and not minted
/// @notice Warning: should only be called via the bundler's `multicall` function. | ||
/// @dev Either `assets` or `shares` should be zero. Most usecases should rely on `assets` as an input so the | ||
/// initiator is guaranteed to withdraw `assets` tokens, but the possibility to mint a specific amount of shares is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error: shares
are burned in withdraw and not minted