fix(erc4626): make decimals offset adapt

[Rubilmax]

• Fixes https://cantina.xyz/code/8409a0ce-6c21-4cc9-8ef2-bd77ce7425af/findings/414d3776-a3c9-450f-aca6-549a7cff2d41

[adhusson]

I'm getting a failing testRedeemTooMuch(0).

Also, is the following (from the cantina issue) true?

Most protocols only support tokens with up to 18 decimals

Finally this can lower the virtual shares : virtual asset ratio down to 1, is this safe?

[MerlinEgalite]

Code looks good

[Rubilmax]

Also, is the following (from the cantina issue) true?

Most protocols only support tokens with up to 18 decimals

Well, we consider it being quite relevant because Blue itself was not designed specifically for tokens with over 18 decimals, even though it does support them to some extent

Finally this can lower the virtual shares : virtual asset ratio down to 1, is this safe?

Indeed for tokens with 18 decimals
As showcased by OZ a decimals offset of 0 is not an issue with regard to the inflation attack

However, a ratio of virtual assets over virtual shares of 1 means that 1 share is worth 1 asset and the conversion rate is expected to grow (unless a lot of bad debt is realized) so it is expected that roundings upon interactions with MetaMorpho grow in size
For example, one MetaMorpho lender may expect to lose up to 1 asset (18 decimals) upon deposit/withdraw/mint/redeem due to roundings at the beginning and as the conversion rate grows with interest (for example 10x), the same supplier may experience a loss of 10 assets upon interaction, due to roundings. I consider it not an issue, as these assets are expected to be worth almost nothing

It may be an issue for high value, low decimals tokens such as WBTC though. Hence why we don't simply choose a decimals offset of 0 : we instead choose a decimals offset of 18 - decimals so that for the case of high value, low decimals tokens such as WBTC, the ratio is so low (1e-12) that enough precision on shares is given to mitigate the worth of roundings

[adhusson]

Also, is the following (from the cantina issue) true?

Most protocols only support tokens with up to 18 decimals

Well, we consider it being quite relevant because Blue itself was not designed specifically for tokens with over 18 decimals, even though it does support them to some extent

What about decoupling decimals() from decimalsOffset? Maybe I'm missing something but they seem unrelated in principle. You could have 18 decimals shares, 1e6 virtual shares, and an 18 decimals asset token.

Indeed for tokens with 18 decimals

In discussions related to Blue I'm seeing references to trying to maintain the property #shares > #assets as long as possible, and those discussions led to 1e6 virtual shares. Since this PR reduces virtual shares to 1 for most tokens I think it's worth rechecking the reasons for having 1e6 virtual shares in the first place.

References:

• https://morpholabs.slack.com/archives/C02N7CZ088N/p1697097108798919?thread_ts=1697096627.341509&cid=C02N7CZ088N
• refactor: lower virtual shares morpho-blue#283 (comment)
• https://github.com/morpho-org/morpho-blue/pull/361/files

[Rubilmax]

What about decoupling decimals() from decimalsOffset? Maybe I'm missing something but they seem unrelated in principle. You could have 18 decimals shares, 1e6 virtual shares, and an 18 decimals asset token.

That is correct: it is possible but you end up with a share token that has a 1e6 times larger totalSupply than the underlying asset of the vault, so it may be an issue for protocols that don't handle tokens with large totalSupply (for example a MetaMorpho vault share token with a MakerDAO subDAO token as underlying would have a totalSupply in the order of 1e34 = 1e(6 + 10 + 18) if decimalsOffset = 6, which is close to the limit supported by Blue [1e35])

In discussions related to Blue I'm seeing references to trying to maintain the property #shares > #assets as long as possible, and those discussions led to 1e6 virtual shares. Since this PR reduces virtual shares to 1 for most tokens I think it's worth rechecking the reasons for having 1e6 virtual shares in the first place.

While it seems this was chosen because we had the freedom of having the precision we want on shares because they are not transferrable and are not expected to be handled anywhere outside Blue

[MathisGD]

It's kind of weird to have 1e6 virtual shares in Blue and here only have 1. If 1 isn't enough in Blue, why would it be enough in a user of Blue? This downside is clearer to me than the one we are trying to fix, of a protocol not handling tokens with 24 decimals. I also like the proposal of @adhusson of hardcoding the decimals. I can approve because it doesn't seem really dangerous just a bit weird.

[QGarchery]

If 1 isn't enough in Blue, why would it be enough in a user of Blue?

Because Blue is a primitive, so we want it to be robust to different kind of attacks. In this case, it's good that the inflation attack is completely removed from it, they layers on top will not have to worry about it at all.
Besides, it's not that 1 decimal offset isn't enough for Blue, it's more that it doesn't cost much to make it bigger. This is because, as @Rubilmax said, shares in Blue are

not transferrable and are not expected to be handled anywhere outside Blue

I also like the proposal of @adhusson of hardcoding the decimals

It has other issues, as pointed out here. Do you think they are negligible ?

[adhusson]

In this case, it's good that the inflation attack is completely removed from it, they layers on top will not have to worry about it at all.

At first glance it seems like you could inflation-attack MM by donating to the vault, is it clear why not? (edit: nevermind I forgot that 4626 is safe with decimalOffset = 0)

[MathisGD]

It has other issues, as pointed out here. Do you think they are negligible ?

I think yes