From f630fbc0b288ac7e387d1ec1bc213acb086895cb Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Fri, 27 Oct 2023 15:56:23 +0200 Subject: [PATCH 1/7] docs(oracle): clarify SCALE_FACTOR def --- src/ChainlinkOracle.sol | 44 ++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/src/ChainlinkOracle.sol b/src/ChainlinkOracle.sol index 7ca4f5f..e2c86b8 100644 --- a/src/ChainlinkOracle.sol +++ b/src/ChainlinkOracle.sol @@ -53,34 +53,42 @@ contract ChainlinkOracle is IOracle { uint256 baseTokenDecimals, uint256 quoteTokenDecimals ) { - // The vault parameter is used for ERC4626 tokens, to price its shares. - // It is used to price `VAULT_CONVERSION_SAMPLE` of the vault shares, so it requires dividing by that number, - // hence the division by `VAULT_CONVERSION_SAMPLE` in the `SCALE_FACTOR` definition. + // The ERC4626 vault parameter is used to price `VAULT_CONVERSION_SAMPLE` of its shares, so it requires dividing + // by that number, hence the division by `VAULT_CONVERSION_SAMPLE` in the `SCALE_FACTOR` definition. // Verify that vault = address(0) => vaultConversionSample = 1. require( address(vault) != address(0) || vaultConversionSample == 1, ErrorsLib.VAULT_CONVERSION_SAMPLE_IS_NOT_ONE ); + VAULT = vault; VAULT_CONVERSION_SAMPLE = vaultConversionSample; BASE_FEED_1 = baseFeed1; BASE_FEED_2 = baseFeed2; QUOTE_FEED_1 = quoteFeed1; QUOTE_FEED_2 = quoteFeed2; - // Let pB1 and pB2 be the base prices, and pQ1 and pQ2 the quote prices (price taking into account the - // decimals of both tokens), in a common currency. - // We tackle the most general case in the remainder of this comment, where we assume that no feed is the address - // zero. Similar explanations would hold in the case where some of the feeds are the address zero. - // Let dB1, dB2, dB3, and dQ1, dQ2, dQ3 be the decimals of the tokens involved. - // For example, pB1 is the number of 1e(dB2) of the second base asset that can be obtained from 1e(dB1) of - // the first base asset. - // We notably have dB3 = dQ3, because those two quantities are the decimals of the same common currency. - // Let fpB1, fpB2, fpQ1 and fpQ2 be the feed precision of the corresponding prices. - // Chainlink feeds return pB1*1e(fpB1), pB2*1e(fpB2), pQ1*1e(fpQ1) and pQ2*1e(fpQ2). - // Because the Blue oracle does not take into account decimals, `price()` should return - // 1e36 * (pB1*1e(dB2-dB1) * pB2*1e(dB3-dB2)) / (pQ1*1e(dQ2-dQ1) * pQ2*1e(dQ3-dQ2)) - // Yet `price()` returns (pB1*1e(fpB1) * pB2*1e(fpB2) * SCALE_FACTOR) / (pQ1*1e(fpQ1) * pQ2*1e(fpQ2)) - // So 1e36 * pB1 * pB2 * 1e(-dB1) / (pQ1 * pQ2 * 1e(-dQ1)) = - // (pB1*1e(fpB1) * pB2*1e(fpB2) * SCALE_FACTOR) / (pQ1*1e(fpQ1) * pQ2*1e(fpQ2)) + + // In the following comment, we explain in the general case (where we assume that no feed is the address + // zero) how to scale the output price as Morpho Blue expects, given the input feed prices. + // Similar explanations would hold in the case where some of the feeds are the address zero. + + // Let A, B1, B2, Q1, Q2 be 5 assets, each respectively having dA, dB1, dB2, dQ1, dQ2 decimals. + // Let pB1 and pB2 be the base prices, and pQ1 and pQ2 the quote prices, so that: + // - pB1 is the quantity of assets B2 that can be exchanged for 1e(dB1) assets B1, with dB2 decimals. + // - pB2 is the quantity of assets A that can be exchanged for 1e(dB2) assets B2, with dA decimals. + // - pQ1 is the quantity of assets Q2 that can be exchanged for 1e(dQ1) assets Q1, with dQ2 decimals. + // - pQ2 is the quantity of assets A that can be exchanged for 1e(dQ2) assets B2, with dA decimals. + + // Because Blue's oracle does not take into account decimals, this oracle's `price()` should return: + // 1e36 * (pB1 * 1e(dB2 - dB1)) * (pB2 * 1e(dA - dB2)) / (pQ1 * 1e(dQ2 - dQ1)) * (pQ2 * 1e(dA - dQ2)) + // = 1e36 * (pB1 * 1e(-dB1) * pB2) / (pQ1 * 1e(-dQ1) * pQ2) + + // Let fpB1, fpB2, fpQ1, fpQ2 be the feed precision of the respective prices pB1, pB2, pQ1, pQ2. + // Chainlink feeds return pB1 * 1e(fpB1), pB2 * 1e(fpB2), pQ1 * 1e(fpQ1) and pQ2 * 1e(fpQ2). + + // Based on the implementation of `price()` below, the value of `SCALE_FACTOR` should thus satisfy: + // (pB1 * 1e(fpB1)) * (pB2 * 1e(fpB2)) * SCALE_FACTOR / ((pQ1 * 1e(fpQ1)) * (pQ2 * 1e(fpQ2))) + // = 1e36 * pB1 * pB2 * 1e(-dB1) / (pQ1 * pQ2 * 1e(-dQ1)) + // So SCALE_FACTOR = 1e36 * 1e(-dB1) * 1e(dQ1) * 1e(-fpB1) * 1e(-fpB2) * 1e(fpQ1) * 1e(fpQ2) // = 1e(36 + dQ1 + fpQ1 + fpQ2 - dB1 - fpB1 - fpB2) SCALE_FACTOR = 10 From 54e16bc23276a4028c691845f2948080f129fcdb Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Mon, 30 Oct 2023 09:07:00 +0100 Subject: [PATCH 2/7] docs(oracle): rename A to C --- src/ChainlinkOracle.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/ChainlinkOracle.sol b/src/ChainlinkOracle.sol index e2c86b8..af711bb 100644 --- a/src/ChainlinkOracle.sol +++ b/src/ChainlinkOracle.sol @@ -71,15 +71,15 @@ contract ChainlinkOracle is IOracle { // zero) how to scale the output price as Morpho Blue expects, given the input feed prices. // Similar explanations would hold in the case where some of the feeds are the address zero. - // Let A, B1, B2, Q1, Q2 be 5 assets, each respectively having dA, dB1, dB2, dQ1, dQ2 decimals. + // Let B1, B2, Q1, Q2, C be 5 assets, each respectively having dB1, dB2, dQ1, dQ2, dC decimals. // Let pB1 and pB2 be the base prices, and pQ1 and pQ2 the quote prices, so that: // - pB1 is the quantity of assets B2 that can be exchanged for 1e(dB1) assets B1, with dB2 decimals. - // - pB2 is the quantity of assets A that can be exchanged for 1e(dB2) assets B2, with dA decimals. + // - pB2 is the quantity of assets C that can be exchanged for 1e(dB2) assets B2, with dC decimals. // - pQ1 is the quantity of assets Q2 that can be exchanged for 1e(dQ1) assets Q1, with dQ2 decimals. - // - pQ2 is the quantity of assets A that can be exchanged for 1e(dQ2) assets B2, with dA decimals. + // - pQ2 is the quantity of assets C that can be exchanged for 1e(dQ2) assets B2, with dC decimals. - // Because Blue's oracle does not take into account decimals, this oracle's `price()` should return: - // 1e36 * (pB1 * 1e(dB2 - dB1)) * (pB2 * 1e(dA - dB2)) / (pQ1 * 1e(dQ2 - dQ1)) * (pQ2 * 1e(dA - dQ2)) + // Morpho Blue's market oracle expects to price 1 asset B1 quoted in 1 asset of Q2, so `price()` should return: + // 1e36 * (pB1 * 1e(dB2 - dB1)) * (pB2 * 1e(dC - dB2)) / (pQ1 * 1e(dQ2 - dQ1)) * (pQ2 * 1e(dC - dQ2)) // = 1e36 * (pB1 * 1e(-dB1) * pB2) / (pQ1 * 1e(-dQ1) * pQ2) // Let fpB1, fpB2, fpQ1, fpQ2 be the feed precision of the respective prices pB1, pB2, pQ1, pQ2. From c5407136e081d5acb3df720e483e0125f722aa8c Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Mon, 30 Oct 2023 10:30:14 +0100 Subject: [PATCH 3/7] docs(oracle): clarify equations --- src/ChainlinkOracle.sol | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/ChainlinkOracle.sol b/src/ChainlinkOracle.sol index af711bb..9871995 100644 --- a/src/ChainlinkOracle.sol +++ b/src/ChainlinkOracle.sol @@ -67,27 +67,27 @@ contract ChainlinkOracle is IOracle { QUOTE_FEED_1 = quoteFeed1; QUOTE_FEED_2 = quoteFeed2; - // In the following comment, we explain in the general case (where we assume that no feed is the address - // zero) how to scale the output price as Morpho Blue expects, given the input feed prices. + // In the following comment, we explain the general case (where we assume that no feed is the address zero) + // how to scale the output price as Morpho Blue expects, given the input feed prices. // Similar explanations would hold in the case where some of the feeds are the address zero. // Let B1, B2, Q1, Q2, C be 5 assets, each respectively having dB1, dB2, dQ1, dQ2, dC decimals. // Let pB1 and pB2 be the base prices, and pQ1 and pQ2 the quote prices, so that: - // - pB1 is the quantity of assets B2 that can be exchanged for 1e(dB1) assets B1, with dB2 decimals. - // - pB2 is the quantity of assets C that can be exchanged for 1e(dB2) assets B2, with dC decimals. - // - pQ1 is the quantity of assets Q2 that can be exchanged for 1e(dQ1) assets Q1, with dQ2 decimals. - // - pQ2 is the quantity of assets C that can be exchanged for 1e(dQ2) assets B2, with dC decimals. + // - pB1 is the quantity of 1e(dB2) assets B2 that can be exchanged for 1e(dB1) assets B1. + // - pB2 is the quantity of 1e(dC) assets C that can be exchanged for 1e(dB2) assets B2. + // - pQ1 is the quantity of 1e(dQ2) assets Q2 that can be exchanged for 1e(dQ1) assets Q1. + // - pQ2 is the quantity of 1e(dC) assets C that can be exchanged for 1e(dQ2) assets B2. - // Morpho Blue's market oracle expects to price 1 asset B1 quoted in 1 asset of Q2, so `price()` should return: - // 1e36 * (pB1 * 1e(dB2 - dB1)) * (pB2 * 1e(dC - dB2)) / (pQ1 * 1e(dQ2 - dQ1)) * (pQ2 * 1e(dC - dQ2)) - // = 1e36 * (pB1 * 1e(-dB1) * pB2) / (pQ1 * 1e(-dQ1) * pQ2) + // Morpho Blue expects `price()` to be the quantity of 1 asset B1 that can be exchanged for 1 asset Q2: + // 1e36 * (pB1 * 1e(dB2 - dB1)) * (pB2 * 1e(dC - dB2)) / ((pQ1 * 1e(dQ2 - dQ1)) * (pQ2 * 1e(dC - dQ2))) + // = `1e36 * (pB1 * 1e(-dB1) * pB2) / (pQ1 * 1e(-dQ1) * pQ2)` // Let fpB1, fpB2, fpQ1, fpQ2 be the feed precision of the respective prices pB1, pB2, pQ1, pQ2. // Chainlink feeds return pB1 * 1e(fpB1), pB2 * 1e(fpB2), pQ1 * 1e(fpQ1) and pQ2 * 1e(fpQ2). // Based on the implementation of `price()` below, the value of `SCALE_FACTOR` should thus satisfy: // (pB1 * 1e(fpB1)) * (pB2 * 1e(fpB2)) * SCALE_FACTOR / ((pQ1 * 1e(fpQ1)) * (pQ2 * 1e(fpQ2))) - // = 1e36 * pB1 * pB2 * 1e(-dB1) / (pQ1 * pQ2 * 1e(-dQ1)) + // = `1e36 * (pB1 * 1e(-dB1) * pB2) / (pQ1 * 1e(-dQ1) * pQ2)` // So SCALE_FACTOR = 1e36 * 1e(-dB1) * 1e(dQ1) * 1e(-fpB1) * 1e(-fpB2) * 1e(fpQ1) * 1e(fpQ2) // = 1e(36 + dQ1 + fpQ1 + fpQ2 - dB1 - fpB1 - fpB2) From a3ff55c43f417205df80de9ea1163f875f834ba4 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Mon, 30 Oct 2023 11:12:33 +0100 Subject: [PATCH 4/7] docs(oracle): fix price def --- src/ChainlinkOracle.sol | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/ChainlinkOracle.sol b/src/ChainlinkOracle.sol index 9871995..8cd7b2b 100644 --- a/src/ChainlinkOracle.sol +++ b/src/ChainlinkOracle.sol @@ -78,16 +78,17 @@ contract ChainlinkOracle is IOracle { // - pQ1 is the quantity of 1e(dQ2) assets Q2 that can be exchanged for 1e(dQ1) assets Q1. // - pQ2 is the quantity of 1e(dC) assets C that can be exchanged for 1e(dQ2) assets B2. - // Morpho Blue expects `price()` to be the quantity of 1 asset B1 that can be exchanged for 1 asset Q2: + // Morpho Blue expects `price()` to be the quantity of 1 asset Q1 that can be exchanged for 1 asset B1, + // scaled by 1e36: // 1e36 * (pB1 * 1e(dB2 - dB1)) * (pB2 * 1e(dC - dB2)) / ((pQ1 * 1e(dQ2 - dQ1)) * (pQ2 * 1e(dC - dQ2))) - // = `1e36 * (pB1 * 1e(-dB1) * pB2) / (pQ1 * 1e(-dQ1) * pQ2)` + // = 1e36 * (pB1 * 1e(-dB1) * pB2) / (pQ1 * 1e(-dQ1) * pQ2) // Let fpB1, fpB2, fpQ1, fpQ2 be the feed precision of the respective prices pB1, pB2, pQ1, pQ2. // Chainlink feeds return pB1 * 1e(fpB1), pB2 * 1e(fpB2), pQ1 * 1e(fpQ1) and pQ2 * 1e(fpQ2). // Based on the implementation of `price()` below, the value of `SCALE_FACTOR` should thus satisfy: // (pB1 * 1e(fpB1)) * (pB2 * 1e(fpB2)) * SCALE_FACTOR / ((pQ1 * 1e(fpQ1)) * (pQ2 * 1e(fpQ2))) - // = `1e36 * (pB1 * 1e(-dB1) * pB2) / (pQ1 * 1e(-dQ1) * pQ2)` + // = 1e36 * (pB1 * 1e(-dB1) * pB2) / (pQ1 * 1e(-dQ1) * pQ2) // So SCALE_FACTOR = 1e36 * 1e(-dB1) * 1e(dQ1) * 1e(-fpB1) * 1e(-fpB2) * 1e(fpQ1) * 1e(fpQ2) // = 1e(36 + dQ1 + fpQ1 + fpQ2 - dB1 - fpB1 - fpB2) From 5aaaf58e1352b071fe7cecdccbdd0fa635ab68ce Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Fri, 27 Oct 2023 16:56:15 +0200 Subject: [PATCH 5/7] fix(oracle): require vault conversion not zero --- src/ChainlinkOracle.sol | 1 + src/libraries/ErrorsLib.sol | 3 +++ 2 files changed, 4 insertions(+) diff --git a/src/ChainlinkOracle.sol b/src/ChainlinkOracle.sol index 8cd7b2b..fd514d6 100644 --- a/src/ChainlinkOracle.sol +++ b/src/ChainlinkOracle.sol @@ -59,6 +59,7 @@ contract ChainlinkOracle is IOracle { require( address(vault) != address(0) || vaultConversionSample == 1, ErrorsLib.VAULT_CONVERSION_SAMPLE_IS_NOT_ONE ); + require(vaultConversionSample != 0, ErrorsLib.VAULT_CONVERSION_IS_ZERO); VAULT = vault; VAULT_CONVERSION_SAMPLE = vaultConversionSample; diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index dcd6861..7f8f7f3 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -9,6 +9,9 @@ library ErrorsLib { /// @notice Thrown when the answer returned by a Chainlink feed is negative. string constant NEGATIVE_ANSWER = "negative answer"; + /// @notice Thrown when the vault conversion sample is 0. + string constant VAULT_CONVERSION_SAMPLE_IS_ZERO = "vault conversion sample is zero"; + /// @notice Thrown when the vault conversion sample is not 1 while vault = address(0). string constant VAULT_CONVERSION_SAMPLE_IS_NOT_ONE = "vault conversion sample is not one"; } From b62f427b9a587dbc8ce9d625325926e6b844f4be Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Fri, 27 Oct 2023 17:32:19 +0200 Subject: [PATCH 6/7] fix(oracle): prevent instanciating with zero decimals --- src/ChainlinkOracle.sol | 4 ++-- test/ChainlinkOracleTest.sol | 8 +++++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/ChainlinkOracle.sol b/src/ChainlinkOracle.sol index fd514d6..d617d82 100644 --- a/src/ChainlinkOracle.sol +++ b/src/ChainlinkOracle.sol @@ -59,7 +59,7 @@ contract ChainlinkOracle is IOracle { require( address(vault) != address(0) || vaultConversionSample == 1, ErrorsLib.VAULT_CONVERSION_SAMPLE_IS_NOT_ONE ); - require(vaultConversionSample != 0, ErrorsLib.VAULT_CONVERSION_IS_ZERO); + require(vaultConversionSample != 0, ErrorsLib.VAULT_CONVERSION_SAMPLE_IS_ZERO); VAULT = vault; VAULT_CONVERSION_SAMPLE = vaultConversionSample; @@ -97,7 +97,7 @@ contract ChainlinkOracle is IOracle { ** ( 36 + quoteTokenDecimals + quoteFeed1.getDecimals() + quoteFeed2.getDecimals() - baseTokenDecimals - baseFeed1.getDecimals() - baseFeed2.getDecimals() - ) / VAULT_CONVERSION_SAMPLE; + ) / vaultConversionSample; } /* PRICE */ diff --git a/test/ChainlinkOracleTest.sol b/test/ChainlinkOracleTest.sol index 02d91ee..3c994bc 100644 --- a/test/ChainlinkOracleTest.sol +++ b/test/ChainlinkOracleTest.sol @@ -132,8 +132,14 @@ contract ChainlinkOracleTest is Test { assertApproxEqRel(oracle.price(), expectedPrice, deviation); } + function testConstructorZeroVaultConversionSample() public { + vm.expectRevert(bytes(ErrorsLib.VAULT_CONVERSION_SAMPLE_IS_ZERO)); + new ChainlinkOracle(sDaiVault, daiEthFeed, feedZero, usdcEthFeed, feedZero, 0, 18, 6); + } + function testConstructorVaultZeroNonOneSample(uint256 vaultConversionSample) public { - vm.assume(vaultConversionSample != 1); + vm.assume(vaultConversionSample > 1); + vm.expectRevert(bytes(ErrorsLib.VAULT_CONVERSION_SAMPLE_IS_NOT_ONE)); new ChainlinkOracle(vaultZero, daiEthFeed, feedZero, usdcEthFeed, feedZero, vaultConversionSample, 18, 6); } From 25c4ab6137ceb21434ebf36355ba8ab694959297 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Mon, 6 Nov 2023 14:41:01 +0100 Subject: [PATCH 7/7] fix(test): bound instead of assume --- test/ChainlinkOracleTest.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ChainlinkOracleTest.sol b/test/ChainlinkOracleTest.sol index 3c994bc..42f57b9 100644 --- a/test/ChainlinkOracleTest.sol +++ b/test/ChainlinkOracleTest.sol @@ -138,7 +138,7 @@ contract ChainlinkOracleTest is Test { } function testConstructorVaultZeroNonOneSample(uint256 vaultConversionSample) public { - vm.assume(vaultConversionSample > 1); + vaultConversionSample = bound(vaultConversionSample, 2, type(uint256).max); vm.expectRevert(bytes(ErrorsLib.VAULT_CONVERSION_SAMPLE_IS_NOT_ONE)); new ChainlinkOracle(vaultZero, daiEthFeed, feedZero, usdcEthFeed, feedZero, vaultConversionSample, 18, 6);