-
Notifications
You must be signed in to change notification settings - Fork 81
refactor(shares): extract shares math to lib #94
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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
41639ff
refactor(shares): extract shares math to lib
Rubilmax 7a16e0d
Merge branch 'refactor/morpho-utils' of github.com:morpho-labs/blue i…
Rubilmax 2d83d77
Merge branch 'refactor/morpho-utils' of github.com:morpho-labs/blue i…
Rubilmax 4f141f8
Merge branch 'refactor/morpho-utils' of github.com:morpho-labs/blue i…
Rubilmax c3171a8
Merge branch 'refactor/morpho-utils' of github.com:morpho-labs/blue i…
Rubilmax 2a07f34
refactor(math): use solmate only
Rubilmax 19ac274
fix(shares-math): increase virtual shares
Rubilmax df2013c
Merge branch 'refactor/morpho-utils' of github.com:morpho-labs/blue i…
Rubilmax 80785b0
refactor(shares-math): extract 1 to VIRTUAL_ASSETS
Rubilmax File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
// SPDX-License-Identifier: UNLICENSED | ||
pragma solidity ^0.8.0; | ||
|
||
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol"; | ||
Rubilmax marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/// @notice Shares management library. | ||
/// @dev This implementation mitigates share price manipulations, using OpenZeppelin's virtual shares: https://docs.openzeppelin.com/contracts/4.x/erc4626#inflation-attack. | ||
library SharesMath { | ||
using FixedPointMathLib for uint256; | ||
|
||
uint256 internal constant VIRTUAL_SHARES = 1e18; | ||
uint256 internal constant VIRTUAL_ASSETS = 1; | ||
|
||
/// @dev Calculates the value of the given assets quoted in shares, rounding down. | ||
/// Note: provided that assets <= totalAssets, this function satisfies the invariant: shares <= totalShares. | ||
function toSharesDown(uint256 assets, uint256 totalAssets, uint256 totalShares) internal pure returns (uint256) { | ||
return assets.mulDivDown(totalShares + VIRTUAL_SHARES, totalAssets + VIRTUAL_ASSETS); | ||
} | ||
|
||
/// @dev Calculates the value of the given shares quoted in assets, rounding down. | ||
/// Note: provided that shares <= totalShares, this function satisfies the invariant: assets <= totalAssets. | ||
function toAssetsDown(uint256 shares, uint256 totalAssets, uint256 totalShares) internal pure returns (uint256) { | ||
return shares.mulDivDown(totalAssets + VIRTUAL_ASSETS, totalShares + VIRTUAL_SHARES); | ||
} | ||
|
||
/// @dev Calculates the value of the given assets quoted in shares, rounding up. | ||
/// Note: provided that assets <= totalAssets, this function satisfies the invariant: shares <= totalShares + VIRTUAL_SHARES. | ||
function toSharesUp(uint256 assets, uint256 totalAssets, uint256 totalShares) internal pure returns (uint256) { | ||
return assets.mulDivUp(totalShares + VIRTUAL_SHARES, totalAssets + VIRTUAL_ASSETS); | ||
} | ||
|
||
/// @dev Calculates the value of the given shares quoted in assets, rounding up. | ||
/// Note: provided that shares <= totalShares, this function satisfies the invariant: assets <= totalAssets + VIRTUAL_SHARES. | ||
function toAssetsUp(uint256 shares, uint256 totalAssets, uint256 totalShares) internal pure returns (uint256) { | ||
return shares.mulDivUp(totalAssets + VIRTUAL_ASSETS, totalShares + VIRTUAL_SHARES); | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I feel like there is a vulnerability in this implementation.
Virtual shares are taken into account for the shares calculations (in the SharesMath lib), but are never added to totalSupplyShares.
So it seems that the first supplier still owns all the market shares, and can still perform the inflation attack.
Shouldn’t we add the virtual shares to totalSupplyShares at the initialization of the market (or if totalSupplyShares==0) ?
Maybe I’m wrong
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.
Because what I understand from https://docs.openzeppelin.com/contracts/4.x/erc4626#inflation-attack is that the virtual shares are meant to catch part of the attacker’s donation (that aims to inflate the rate) and therefore make his attack unprofitable.
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.
And same for totalSupply : if totalSupply[id] == 0, we sould add 1 to totalSupply[id]
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.
@Rubilmax
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.
In this implementation, when you do any computation with the shares, you take into account the virtual shares and the virtual assets (only 1 virtual asset actually). You don't add those virtual shares and assets to the mappings representing the actual values.
I think that this implementation works, because those virtual shares actually catch part of the attacker's donation. For example, by writing$10^\delta$ ):
V
as the virtual shares (V
=a0
assets.The added shares are
toSharesDown(a0, 0, 0) = a0 * V
a1
assets.u
assets.The added shares are
toSharesDown(u, a0 + a1, a0 * V) = u * (V + a0 * V) / (1 + a0 + a1) = V * u * (1 + a0) / (1 + a0 + a1)
, which is what is written in the openzeppelin page for the number of shares that the user getsKeep in mind that this manipulation cannot be done on Morpho Blue, because it is not relying on the balance (so there is no way to modify the total assets simply by transferring tokens to the contract). Instead, the purpose of this PR is to ensure a high initial conversion rate
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.
Discussed with @QGarchery I agree it's ok