fix(oracle): use 512 bit multiplication

[Rubilmax]

• Fixes Risk of multiplication overflow #15

[Rubilmax]

Just pushed an alternative order for the price which should be "safer", because chainlink price feeds are 18 decimals max per their specs, which leaves 41 decimals to the vault before overflowing

[QGarchery]

Just pushed an alternative order for the price which should be "safer", because chainlink price feeds are 18 decimals max per their specs, which leaves 41 decimals to the vault before overflowing

Sounds good, although if the base prices are over 1 it can be less than 41 decimals. For example, if you are pricing BTC/ETH, you get an answer that is greater than 1e18. I think that overall this has a very low chance of overflowing, but it should be checked at every oracle deployment

[MerlinEgalite]

Just pushed an alternative order for the price which should be "safer", because chainlink price feeds are 18 decimals max per their specs, which leaves 41 decimals to the vault before overflowing

Sounds good, although if the base prices are over 1 it can be less than 41 decimals. For example, if you are pricing BTC/ETH, you get an answer that is greater than 1e18. I think that overall this has a very low chance of overflowing, but it should be checked at every oracle deployment

We could check it in the constructor maybe?

[QGarchery]

We could check it in the constructor maybe?

I don't think it would be enough because prices can fluctuate. So, for each oracle, we have to make sure that there is a very big buffer that cannot get reached in practice

[Rubilmax]

LGTM!

[QGarchery]

Ok to use this solution in theory, because of this and because the definition of SCALE_FACTOR would revert at deployment if it over/underflows (which is a nice property). I have the following reservations though:

• I would like to have an opinion on Alternative oracle to avoid overflows #37 because I think that this solution is actually pretty clean, cc @MathisGD
• it would be nice to document the risk of overflow, there is a check that has to be done before deployment
• everyone must be aware that we are using a complex library to do the computation. Should someone check it out ? (I have not done it yet)

[MerlinEgalite]

I'm still in favor of this solution and I'm fine with using the uniswap library. And if we want better confidence we can even use OZ version: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/932fddf69a699a9a80fd2396fd1a2ab91cdda123/contracts/utils/math/Math.sol#L123

which I would be in favor.

Doing it on top #42

[MerlinEgalite]

I think we should add the assumptions in terms of decimals for the vaultConversionSample

[Rubilmax]

I think we should add the assumptions in terms of decimals for the vaultConversionSample

Something along the lines of "vaultConversionSample should be large enough to provide enough precision when quoting from the vault, but small enough to avoid overflowing."

We can't be more precise in the general case because it also depends on the prices of base assets involved (restricting decimals is not enough to prevent overflowing)

[MerlinEgalite]

I think we should add the assumptions in terms of decimals for the vaultConversionSample

Something along the lines of "vaultConversionSample should be large enough to provide enough precision when quoting from the vault, but small enough to avoid overflowing."

We can't be more precise in the general case because it also depends on the prices of base assets involved (restricting decimals is not enough to prevent overflowing)

Maybe I can add it to #44 instead

[Rubilmax]

LGTM

[QGarchery]

One change requested, otherwise LGTM