Math operations revert on large inputs

[Rubilmax]

An integrator passing a large value as input to amount may get an empty revert, without knowing why the protocol fails.

A solution to this is to add a check amount <= MAX_INPUT where MAX_INPUT can be defined based on how we do arithmetic.
A side-effect of this is that we'll check for overflow twice: once at the protocol level, the other at the arithmetic level.

[QGarchery]

A side-effect of this is that we'll check for overflow twice: once at the protocol level, the other at the arithmetic level.

Let me underline this last point. To stay minimal I would be in favor of having the appropriate revert errors in the math library and to not change the main contract

[Rubilmax]

However, arithmetics would only be able to check for overflow and throw appropriate errors. The integrator would end up with an overflow error which doesn't indicate to them that they've input something that just doesn't work with the protocol (it could be an internal arithmetic overflow that the integrator is not responsible for).

[MerlinEgalite]

I think tools like tenderly or phalcon should unlock this problem

[Rubilmax]

Debuggers always do solve debugging issues indeed, but it's not because debuggers exist that we shouldn't design our software to be easy to debug.

[MerlinEgalite]

Yes but you can do that for every computation in the protocol no? So you don't remove all the issues with that solution. Or maybe I did not get it

[Rubilmax]

I'm not interested in providing a comprehensive error for every revert that can happen inside the protocol. I'm interested in providing a comprehensive error for arithmetic errors that can happen due to incorrect input (cf issue description).

[MerlinEgalite]

How do you compute the correct MAX_INPUT? I feel that it depends on too many things

[Rubilmax]

How do you compute the correct MAX_INPUT? I feel that it depends on too many things

I did it in tests: https://github.com/morpho-labs/blue/blob/df2013c98e702fa0010b2efe0d908595a0873cc7/test/forge/Blue.t.sol#L525

[MathisGD]

Can we just put that as a dev comment in the (coming) NATSPEC ?

[Rubilmax]

I'm ok with that, because anyway the maximum amount of shares that can be minted given a market depends on the market's state and may be way lower than the amount described above

[MathisGD]

ok let's just write the formula in the NATSPEC then

[MerlinEgalite]

Now that we have shares and amount, should we write 2 different comments?

[Rubilmax]

Something like this should fit:

/// @dev Can revert with underflow/overflow in the case too many shares are repaid/withdrawn or the input amount/shares is too large.

[MerlinEgalite]

Ok I'm questioning whether it's really necessary. It seems obvious that putting large inputs can cause overflows.

[Rubilmax]

Obvious to us because we wrote the code but it may not be to integrators who don't know how Blue behaves under the hood. I mean it costs nothing to write and warn integrators via the interface that Blue can underflow/overflow and to explain the reasons for it

[MathisGD]

I'm ok to add this type of @ dev comment

[MerlinEgalite]

Ok let's do it but if integrators do not know that overflows can happen, please never use their code 😅

[MathisGD]

In the end I'm against doing something for this one, it's kind of well known and common. Feel free to reopen if you disagree.

[Rubilmax]

So we don't add the comment in the end? What's the cost?
It warns integrator that this type of things can happen and explain why precisely, so they don't have to read the code and identify the source of error themselves

[MerlinEgalite]

I'll suggest something