search

morpho-org / morpho-blue-oracles

Morpho Blue Oracles
https://morpho.org
Other
35 stars 28 forks source link

fix(oracle): use 512 bit multiplication #36

Closed Rubilmax closed 11 months ago

Rubilmax commented 1 year ago
Rubilmax commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

MerlinEgalite commented 12 months ago

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

Rubilmax commented 12 months ago

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 commented 12 months ago

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