8000 Share prices can be inflated · Issue #83 · morpho-org/morpho-blue · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Share prices can be inflated #83

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.

Sign up for GitHub

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

Jump to bottom
Closed
pakim249CAL opened this issue Jul 9, 2023 · 6 comments · Fixed by #94
Closed

Share prices can be inflated #83

pakim249CAL opened this issue Jul 9, 2023 · 6 comments · Fixed by #94
Assignees
@Rubilmax

Comments

@pakim249CAL
Copy link
Contributor

Simply setting WAD to be the initial number of shares is vulnerable to the following:

Supply 1.000_000 WBTC for WAD shares.
Withdraw 0.999_999 WBTC for WAD - 1 shares, leaving 1 share left and 0.000_001 WBTC.

Now the intended precision in shares is lost.
@QGarchery
Copy link
Contributor

Assuming 8 decimals for WBTC:

  • Supply 1e8 WEI of WBTC, you get 1e18 shares.
  • Withdraw amount = 1e8 - 1 WEI, this removes amount.wMul(totalSupplyShares[id]).wDiv(totalSupply[id]) shares.
    amount.wMul(WAD) = amount
    amount.wDiv(1e8) = 1e18 - 1e10

So I'm getting WAD - 1e10 shares withdrawn and not WAD - 1.

Maybe I misunderstood the example though. Anyway it's good to check this part because I did not have the time to make sure that it's correct

@MathisGD
Copy link
Contributor

Usually to do this kind of attacks you need to inflate the price per share (typically by a donation to the contract). To me here it's only doable through very high interests (which should not happen if the IRM is not crazy). Nice point of attention though.

@Rubilmax Rubilmax self-assigned this Jul 10, 2023
@Rubilmax Rubilmax mentioned this issue Jul 10, 2023
Merged
@Rubilmax
Copy link
Collaborator
Rubilmax commented Jul 10, 2023 •
edited
Loading

Assuming 8 decimals for WBTC:

  • Supply 1e8 WEI of WBTC, you get 1e18 shares.
  • Withdraw amount = 1e8 - 1 WEI, this removes amount.wMul(totalSupplyShares[id]).wDiv(totalSupply[id]) shares.
    amount.wMul(WAD) = amount
    amount.wDiv(1e8) = 1e18 - 1e10

So I'm getting WAD - 1e10 shares withdrawn and not WAD - 1.

Maybe I misunderstood the example though. Anyway it's good to check this part because I did not have the time to make sure that it's correct

But @pakim249CAL 's point still stands with amount = 1e18, doesn't it?
The fact that it's WBTC isn't relevant here?

The point is that you can always end up with 1 share = 1 wei of asset.
In practice, it would require to have the liquidity for DEFAULT_SHARES, where DEFAULT_SHARES = 1e18 currently.

@QGarchery
Copy link
Contributor
QGarchery commented Jul 10, 2023 •
edited
Loading

But @pakim249CAL 's point still stands with amount = 1e18, doesn't it?
The fact that it's WBTC isn't relevant here?

I can't say, it is a bit vague for me so I tried this example. I think it would be easier to understand with a counter-example with all the details, including the assets decimals, and an approximate expected price (this is why I took actual WBTC data)

The idea for the DEFAULT_SHARES is to have a precision that would work for most assets. This means that you could theoretically create an asset for which it does not work. In essence, what you don't want is to have a single WEI of share that is worth a lot, which you can then attack with some rounding error.

In the counterexample, 1e10 shares are worth 1 WEI of WBTC, which seems ok. For ETH, 1 share is worth 1 WEI of ETH, which also seems ok

@Rubilmax
Copy link
Collaborator

Ah yes, there's no issue with having 1 share = 1 wei of asset a priori (and it's also the default edge case of OZ's ERC4626, with virtual shares offset being set at 0 by default).

@Rubilmax Rubilmax mentioned this issue Jul 10, 2023
Closed
@pakim249CAL
Copy link
Contributor Author

Ah yes, there's no issue with having 1 share = 1 wei of asset a priori (and it's also the default edge case of OZ's ERC4626, with virtual shares offset being set at 0 by default).

The shares offset being zero in OZ actually means that the offset is 1, since the offset is defined as 10**offset.

@Rubilmax Rubilmax linked a pull request Jul 11, 2023 that will close this issue
refactor(shares): extract shares math to lib #94
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Rubilmax Rubilmax

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor(shares): extract shares math to lib morpho-org/morpho-blue
5 participants
@Rubilmax @QGarchery @MerlinEgalite @pakim249CAL @MathisGD
0