StableMath::_calculateInvariant can, in fact, overflow

[xenide]

Would like to highlight 2 things:

1. the comments written for the unchecked arithmetic in StableMath doesn't have much implication because almost all of the arithmetic done in _calculateInvariant actually makes use of the Math library, which actually checks for overflow.
2. Albeit the point about normalized balances fitting into 172 bits is true, consider the following conditions:
   • if we have a token that has 0 decimal places
   • amount of tokens are at/near type(uint112).max

That would result in the invariant and D_P in the range of uint172, which will then cause an overflow when multiplying D_P with invariant on this line

[nventuro]

Hello @xenide!

You are correct in that the comments don't make sense given the current state of the code. I believe we wrote those a while ago, and later decided that the risk of undetected overflow wasn't worth it and switched to using checked arithmetic.

That would result in the invariant and D_P in the range of uint172, which will then cause an overflow when multiplying D_P with invariant on this line

Hm, this seems correct at a glance, but I'd need to review the implementation more thoroughly to be sure. I don't think this is much of an issue however as this would be a huge edge case for a number of reasons. Was there anything about this that had you worried?

[xenide]

@nventuro nope not any real life cases in particular. Just occurred to me that this is a possibility.

Not sure if it's a concern if / when it happens and withdrawing liquidity is affected. A quick glance at _onExitPool and _doExit seem to show that the stable invariant is calculated during withdrawing liq as well.

[nventuro]

The reason why it's not a concern is that we added recovery mode, which completely bypasses any invariant calculations and simply performs a proportional withdrawal. That's (if I recall correctly) when we added checked arithmetic in order to avoid silent errors.