refactor(shares): extract shares math to lib

[Rubilmax]

• Fixes Share prices can be inflated #83
• Fixes Internalize share to amount logic #64
• Fixes Use wad-based operations in test suite #72

[QGarchery]

I like this idea, it looks cleaner. Not sure about the dependencies though, this will have to be decided in another discussion

[MerlinEgalite]

Maybe we want to put 1 or 2 as exponent to make the attack very unprofitable?

[Rubilmax]

With exponent 0, the attack costs the attacker as much as they steal from the first deposit. I don't think we need to make it less profitable.

[MerlinEgalite]

@Rubilmax

[Jean-Grimal]

What are the drawbacks of increasing the offset?

[Rubilmax]

It seems there's no drawback in increasing the offset, so I agree with the suggestion but even suggest to change it to 1e18.

[MerlinEgalite]

It's not obvious what are the side effect of increasing this much though, is it clear for you?

[Rubilmax]

Don't trust me then, run the maths! I may also have made a mistake (there's a high chance I did).

[MerlinEgalite]

No worries I don't trust you 😛 don't have the time rn to do it, I was curious if you did

[MerlinEgalite]

After some discussion I think it's ok to put a huge number as long as it's not creating overflows. 1e18 seems safe (you could easily deposit 100M worth of an asset with 18 decimals costing 0.01$) but we could use 1e9 to make sure it would work in any configuration.

[Jean-Grimal]

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

[Jean-Grimal]

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.

[Jean-Grimal]

And same for totalSupply : if totalSupply[id] == 0, we sould add 1 to totalSupply[id]

[Jean-Grimal]

@Rubilmax

[QGarchery]

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 V as the virtual shares (V = $10^\delta$):

1. The attacker supplies a0 assets.
   The added shares are toSharesDown(a0, 0, 0) = a0 * V

assets	shares
a0	a0 * V

2. The attacker transfers a1 assets.

assets	shares
a0 + a1	a0 * V

3. A user supplies 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 gets

Keep 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

[MerlinEgalite]

Discussed with @QGarchery I agree it's ok

[QGarchery]

Approving the changes about the shares math. We still need to decide about the math library used, and the actual computation of mulDiv.

I also think that we have a strong invariant with this PR: the ratio shares / assets is greater than the virtual shares. I'm adding it to the invariant list #96