Fix precision issue in Market convert function

evm/protocol/Market.sol

@@ -146,16 +146,18 @@ contract Market is IMarket, OwnableUpgradeable, UUPSUpgradeable
 
   /// @param amount_ must have 6 decimals as a fiat amount
   /// @param denominator ratio (4 decimal) to apply to resulting amount
-  /// @return amount of tokens in precision of given token // FIXME precision is not respected
+  /// @return amount of tokens in precision of given token
   function convert(uint amount_, bytes3 fromFiat_, IERC20 toToken_, uint denominator)
   public view
   returns (uint256 amount)
   {
-    if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC)
-      return FullMath.mulDiv(amount_, 10 ** 4, denominator);
-
     uint decimals = tokens[toToken_].decimals;
-    amount = FullMath.mulDiv(amount_, 10 ** decimals, getPrice(toToken_, fromFiat_));
+
+    if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC) {
+      amount = FullMath.mulDiv(amount_, 10 ** decimals, 10 ** 6);
+    } else {
+      amount = FullMath.mulDiv(amount_, 10 ** decimals, getPrice(toToken_, fromFiat_));
+    }
     return FullMath.mulDiv(amount, 10 ** 4, denominator);
   }
 

get_price_analysis.cjs

@@ -0,0 +1,14 @@
+// Let's look at getPrice again:
+// ```
+//       Token memory token = tokens[token_];
+//       uint32[] memory secs = new uint32[](2);
+//       secs[0] = 300;
+//       secs[1] = 0;
+//       (int56[] memory tickCumulatives,) = token.pool.observe(secs);
+//       int56 tickCumulativesDelta = tickCumulatives[1] - tickCumulatives[0];
+//       uint160 sqrtPriceX96 = TickMath.getSqrtPriceAtTick(int24(tickCumulativesDelta / 300));
+//       price = FullMath.mulDiv(uint256(sqrtPriceX96) * uint256(sqrtPriceX96), 10 ** token.decimals, 1 << 192);
+// ```
+// And `USDC` has 6 decimals.
+// The test says "getPrice() for USDC should be 1e6". It returns 1e6.
+// What about other tokens? Let's check test outputs or other occurrences of getPrice.

run_mock.cjs

@@ -0,0 +1,19 @@
+const { assert } = require('console');
+// WETH / USDC
+// tickDelta = - 17595346759502 - (- 17595288323282)
+let tickCumulatives1 = -17595346759502n;
+let tickCumulatives0 = -17595288323282n;
+let tickDelta = tickCumulatives1 - tickCumulatives0;
+// tickDelta / 300
+let tick = Number(tickDelta) / 300;
+console.log("Tick:", tick);
+let price_ratio = Math.pow(1.0001, tick);
+console.log("Price Ratio (token1/token0):", price_ratio);
+
+// If tick = -194, ratio = 1.0001^-194 = 0.98. Wait. -194?
+// Let's re-calculate.
+console.log("tickDelta:", tickDelta);
+console.log("tick:", tickDelta / 300n);
+// -194787
+let price_ratio2 = Math.pow(1.0001, Number(tickDelta) / 300);
+console.log("Price Ratio:", price_ratio2);

run_mock2.cjs

@@ -0,0 +1,22 @@
+// Price ratio = 3.47467e-9
+// If token0 is WETH (18 decimals), token1 is USDC (6 decimals).
+// true_price = ratio * 10^(18 - 6) = 3.47467e-9 * 10^12 = 3474.67
+// So 1 WETH = 3474.67 USDC.
+// This matches!
+// But wait!
+// The price returned by getPrice:
+// `price = FullMath.mulDiv(uint256(sqrtPriceX96) * uint256(sqrtPriceX96), 10 ** token.decimals, 1 << 192);`
+// `sqrtPriceX96` is sqrt(ratio) * 2^96.
+// `sqrtPriceX96^2 / 2^192` is `ratio`.
+// So `price = ratio * 10 ** token.decimals`
+// For WETH, price = 3.47467e-9 * 10^18 = 3.47467e9 = 3,474,670,000.
+// Is this 6 decimals?
+// 3474.67 * 10^6 = 3,474,670,000.
+// YES! It is EXACTLY 6 decimals!
+// Let me verify this.
+// `ratio` is USDC_amount / WETH_amount (raw wei values).
+// `ratio = (USDC_human * 10^6) / (WETH_human * 10^18)`
+// `ratio = (USDC_human / WETH_human) * 10^-12`
+// `ratio * 10^18 = (USDC_human / WETH_human) * 10^6`.
+// WHICH IS EXACTLY THE PRICE IN USDC WITH 6 DECIMALS!
+// This works perfectly if token1 is USDC!

run_mock3.cjs

@@ -0,0 +1,22 @@
+// What if token0 is USDC and token1 is the Token?
+// `ratio` is Token_amount / USDC_amount.
+// `ratio = (Token_human * 10^token.decimals) / (USDC_human * 10^6)`
+// `ratio = (Token_human / USDC_human) * 10^(token.decimals - 6)`
+// `ratio * 10^token.decimals = (Token_human / USDC_human) * 10^(2*token.decimals - 6)`
+// This gives the WRONG value!
+// But the code ASSUMES `address(token_) < USDC` or whatever because it doesn't check `token0` or `token1`.
+// Actually, `sqrtPriceX96` is just the Uniswap price.
+// Wait, is Uniswap always created with token0 < token1? YES.
+// If address(token_) > USDC, then token_ is token1 and USDC is token0.
+// Then ratio = token_ / USDC.
+// The code `uint160 sqrtPriceX96 = TickMath.getSqrtPriceAtTick(int24(tickCumulativesDelta / 300));`
+// always calculates based on `token1/token0`.
+// Wait, `tokens[token_].pool` gives us the Uniswap pool.
+// The code doesn't read the pool's token0/token1. It just gets `sqrtPriceX96` at tick!
+// If `address(token_) > USDC`, `token_` is token1!
+// If `token_` is token1, `ratio` is `amount1 / amount0`, which is `token_ / USDC`!
+// But `getPrice` ASSUMES the pool ratio is always `USDC / token_` because of `price = FullMath.mulDiv(...)`.
+// BUT THAT IS NOT THE BUG BEING FIXED.
+// The issue says:
+// /// @return amount of tokens in precision of given token // FIXME precision is not respected
+// function convert(uint amount_, bytes3 fromFiat_, IERC20 toToken_, uint denominator)

run_mock4.cjs

@@ -0,0 +1,48 @@
+// Ah! Wait, look at this!
+// amount = FullMath.mulDiv(amount_, 10 ** decimals, getPrice(toToken_, fromFiat_));
+// Let's say:
+// amount_ = 100 EUR = 100 * 10^6
+// decimals = 18 (WETH)
+// getPrice(WETH, EUR) = 3474.67 * 10^6 (price of 1 WETH in EUR, 6 decimals)
+// amount = 100 * 10^6 * 10^18 / (3474.67 * 10^6) = (100 / 3474.67) * 10^18 = 0.02878 * 10^18.
+// This is exactly the CORRECT AMOUNT IN TARGET PRECISION.
+// BUT WAIT.
+// Look at `return FullMath.mulDiv(amount, 10 ** 4, denominator);`
+// WAIT, the formula is:
+// `FullMath.mulDiv(amount, 10 ** 4, denominator);`
+// If denominator is 10000. It returns `amount * 10^4 / 10^4 = amount`.
+// This is STILL correct!
+
+// Let me look at the issue description again.
+// "The amount of tokens returned by the convert function does not respect the precision of the given token."
+// Could it be that `amount_` is NOT fiat?
+// "amount of tokens in precision of given token"
+// What if `amount_` is in token precision? NO.
+// `amount_` is `uint amount_, bytes3 fromFiat_, IERC20 toToken_, uint denominator`
+// "amount_ must have 6 decimals as a fiat amount"
+
+// Wait! Look at `if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC)`
+// `return FullMath.mulDiv(amount_, 10 ** 4, denominator);`
+// What if `USDC` decimals is NOT 6?
+// Wait, `tokens[toToken_].decimals` is NOT used for USDC.
+// Does USDC always have 6 decimals on all chains? NO!
+// On BSC, USDC (Binance-Peg) has 18 decimals!
+// If `amount_` is 6 decimals.
+// `return FullMath.mulDiv(amount_, 10 ** 4, denominator);` returns 6 decimals!
+// BUT USDC ON BSC HAS 18 DECIMALS!
+// So if `amount_` is 100 * 10^6.
+// It returns 100 * 10^6.
+// But the target precision is 18 decimals! So it returns 0.0000000001 USDC!
+// THIS IS THE BUG!
+// We MUST respect `tokens[toToken_].decimals` even for USDC!
+
+// Also, what if we use `convert()` for USDC, but not USD?
+// If `fromFiat_` != USD, but `toToken_` == USDC:
+// `decimals = tokens[toToken_].decimals`
+// `amount = FullMath.mulDiv(amount_, 10 ** decimals, getPrice(toToken_, fromFiat_));`
+// `getPrice(USDC, EUR)` -> `price = 10^6`
+// `price = price * 10**8 / fiatToUSD = 10^6 * 10^8 / 1.08*10^8 = 925925`
+// `amount = 100 * 10^6 * 10^decimals / 925925`.
+// If USDC has 18 decimals, `amount = 100 * 10^6 * 10^18 / 925925 = 108 * 10^18`.
+// THIS RESPECTS DECIMALS!
+// It is ONLY the early return `if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC)` that FAILS to respect decimals!

run_mock5.cjs

@@ -0,0 +1,28 @@
+// Is this correct?
+// ```solidity
+//     if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC)
+//       return FullMath.mulDiv(amount_, 10 ** tokens[toToken_].decimals, 10 ** 6) * 10 ** 4 / denominator;
+// ```
+// Wait, `amount_` has 6 decimals.
+// `amount_ * 10 ** decimals / 10 ** 6` will give it `decimals` precision.
+// Then apply denominator: `* 10 ** 4 / denominator`.
+// Can we do: `FullMath.mulDiv(amount_, 10 ** (4 + tokens[toToken_].decimals), 10 ** 6 * denominator)`
+// Yes.
+
+// Wait, let's look at the comment again:
+// /// @return amount of tokens in precision of given token // FIXME precision is not respected
+// function convert(uint amount_, bytes3 fromFiat_, IERC20 toToken_, uint denominator)
+
+// What about the second part?
+//     uint decimals = tokens[toToken_].decimals;
+//     amount = FullMath.mulDiv(amount_, 10 ** decimals, getPrice(toToken_, fromFiat_));
+//     return FullMath.mulDiv(amount, 10 ** 4, denominator);
+
+// If amount_ is 100 * 10^6.
+// `amount = 100 * 10^6 * 10^decimals / getPrice(toToken_, fromFiat_)`
+// Since getPrice returns 6 decimals.
+// `amount = (fiat_amount * 10^6) * 10^decimals / (fiat_price * 10^6)`
+// `amount = (fiat_amount / fiat_price) * 10^decimals`.
+// This is exactly `decimals` precision.
+// So the second part IS respecting precision perfectly!
+// The ONLY issue is the first part!

run_mock6.cjs

@@ -0,0 +1,24 @@
+// What if we just remove the early return?
+// If we remove:
+// ```solidity
+//     if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC)
+//       return FullMath.mulDiv(amount_, 10 ** 4, denominator);
+// ```
+// And just let it fall through:
+// `uint decimals = tokens[toToken_].decimals;`
+// `amount = FullMath.mulDiv(amount_, 10 ** decimals, getPrice(toToken_, fromFiat_));`
+// `getPrice(USDC, "USD")` -> returns `10 ** 6`.
+// `amount = FullMath.mulDiv(amount_, 10 ** decimals, 10 ** 6);`
+// `return FullMath.mulDiv(amount, 10 ** 4, denominator);`
+// This exactly correctly converts `amount_` from 6 decimals to `decimals` and applies `denominator`!
+// Wait! If `address(toToken_) == USDC`, `getPrice(USDC, "USD")` returns `10 ** 6`.
+// So `amount = amount_ * 10 ** decimals / 10 ** 6`.
+// THIS IS PERFECT!
+// Why did they have the early return?
+// Probably an optimization, but it's WRONG because it assumes USDC has 6 decimals!
+// If we just remove it? Or maybe keep it but fix it:
+// `if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC) {`
+// `  uint decimals = tokens[USDC].decimals;`
+// `  amount = FullMath.mulDiv(amount_, 10 ** decimals, 10 ** 6);`
+// `  return FullMath.mulDiv(amount, 10 ** 4, denominator);`
+// `}`

run_mock7.cjs

@@ -0,0 +1,17 @@
+// Let's verify:
+// `amount_` is 6 decimals.
+// `toToken_` is `USDC` with `decimals`.
+// `amount = amount_ * 10 ** decimals / 10 ** 6`.
+// `result = amount * 10 ** 4 / denominator`.
+// Can we do: `FullMath.mulDiv(amount_, 10 ** (4 + decimals), 10 ** 6 * denominator)`
+// `10 ** 4 * 10 ** decimals / 10 ** 6` = `10 ** (decimals - 2)`.
+// What if decimals < 2? No token has < 2 decimals.
+// But we can just write:
+// ```solidity
+//     uint decimals = tokens[toToken_].decimals;
+//     if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC) {
+//       amount = FullMath.mulDiv(amount_, 10 ** decimals, 10 ** 6);
+//       return FullMath.mulDiv(amount, 10 ** 4, denominator);
+//     }
+// ```
+// Let me look at the code again.

run_mock8.cjs

@@ -0,0 +1,47 @@
+// Is there any other place where precision is not respected?
+// Look at `getPrice()`
+//   function getPrice(IERC20 token_, bytes3 fiat_) public view returns (uint256 price) {
+//     // first fetch market TWAP for Uniswap pool of token/USDC
+//     if (address(token_) != USDC) {
+// ...
+//       // price of token_ in USDC (6 decimals)
+//       price = FullMath.mulDiv(uint256(sqrtPriceX96) * uint256(sqrtPriceX96), 10 ** token.decimals, 1 << 192);
+//     } else {
+//       price = 10 ** 6;
+//     }
+// Wait, in getPrice, `price = 10 ** 6` if `token_ == USDC`.
+// WHY is it `10 ** 6` for USDC?
+// Because `getPrice` returns price with 6 decimals! So 1 USDC = 1 USD (10**6).
+// This is perfectly correct! `getPrice` returns a FIAT PRICE, and fiat prices have 6 decimals!
+// What if `fiat_ != USD`?
+//     if (fiat_ != bytes3("USD")) { // 0 is USD
+//       (, int $fiatToUSD,,,) = fiats[fiat_].latestRoundData();
+//       require($fiatToUSD > 0, InvalidArgument());
+//       price = price * 10 ** 8 / uint($fiatToUSD);
+//     }
+// This is also perfectly correct.
+
+// Then the only problem is that USDC decimals are hardcoded to 6 in the early return!
+// `if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC)`
+// `  return FullMath.mulDiv(amount_, 10 ** 4, denominator);`
+// If USDC had 18 decimals, `amount_` (6 decimals) * 10**4 / denominator = 6 decimals.
+// But it SHOULD return 18 decimals!
+// So it should be `return FullMath.mulDiv(amount_, 10 ** (tokens[USDC].decimals - 6) * 10 ** 4, denominator);`
+// Or simply don't early return and let it calculate:
+//   uint decimals = tokens[toToken_].decimals;
+//   amount = FullMath.mulDiv(amount_, 10 ** decimals, getPrice(toToken_, fromFiat_));
+//   return FullMath.mulDiv(amount, 10 ** 4, denominator);
+// If it falls through, getPrice(USDC, "USD") returns 10**6.
+// `amount = FullMath.mulDiv(amount_, 10 ** decimals, 10**6);`
+// `return FullMath.mulDiv(amount, 10 ** 4, denominator);`
+// This uses MORE gas than early returning, but we can do:
+// ```solidity
+//     uint decimals = tokens[toToken_].decimals;
+//     if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC) {
+//       amount = FullMath.mulDiv(amount_, 10 ** decimals, 10 ** 6);
+//     } else {
+//       amount = FullMath.mulDiv(amount_, 10 ** decimals, getPrice(toToken_, fromFiat_));
+//     }
+//     return FullMath.mulDiv(amount, 10 ** 4, denominator);
+// ```
+// This exactly matches and fixes the precision issue for USDC!

run_mock9.cjs

@@ -0,0 +1,23 @@
+// What if we just do:
+// ```solidity
+//     uint decimals = tokens[toToken_].decimals;
+//     if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC) {
+//       amount = FullMath.mulDiv(amount_, 10 ** (decimals + 4), 10 ** 6 * denominator);
+//       return amount;
+//     }
+// ```
+// But wait, `amount = FullMath.mulDiv(amount_, 10 ** decimals, 10 ** 6);`
+// `return FullMath.mulDiv(amount, 10 ** 4, denominator);`
+// This uses two multiplications to avoid overflow, or is cleaner.
+// Actually, `10 ** (decimals + 4)` is fine since decimals <= 18 usually.
+// Let's just do:
+// ```solidity
+//     uint decimals = tokens[toToken_].decimals;
+//     if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC) {
+//       amount = FullMath.mulDiv(amount_, 10 ** decimals, 10 ** 6);
+//     } else {
+//       amount = FullMath.mulDiv(amount_, 10 ** decimals, getPrice(toToken_, fromFiat_));
+//     }
+//     return FullMath.mulDiv(amount, 10 ** 4, denominator);
+// ```
+// It's perfectly clear and clean.

test_convert.cjs

@@ -0,0 +1,29 @@
+function convert(amount, decimals, price, denominator) {
+   let a = amount * (10n**decimals) / price;
+   return a * 10000n / denominator;
+}
+console.log(convert(100n * 10n**6n, 18n, 1851851851n, 10000n));
+
+// USDC (6 decimals) amount = 100 EUR = 100 * 10**6
+let p_usdc = 1000000n * 100000000n / 108000000n; // 925925
+console.log(convert(100n * 10n**6n, 6n, p_usdc, 10000n)); // 100000000000000000000 / 925925 = 108000000 (108 USDC)
+
+// Now look at convert method for USD to USDC.
+// amount_ = 100 * 10**6
+// toToken_ = USDC
+// if (fromFiat_ == "USD" && toToken_ == USDC) return amount_ * 10**4 / denominator
+// So for USD -> USDC, it returns amount_ (with 6 decimals), adjusted by denominator. This is CORRECT because USDC has 6 decimals!
+// But wait! What if toToken_ is not USDC, but USD fiat?
+// amount_ is 100 * 10**6.
+// decimals of WBTC is 8.
+// USD -> WBTC
+// getPrice(WBTC, USD) -> 65000 * 10**6 (65k USDC).
+// amount_ * 10**8 / (65000 * 10**6)
+// = 100 * 10**6 * 10**8 / (65000 * 10**6) = 100 * 10**8 / 65000 = 10000000000 / 65000 = 153846 (0.00153846 WBTC).
+// WBTC is 8 decimals, 0.00153846 is 153846. This is CORRECT.
+
+// Now what if token is WETH (18 decimals)
+// USD -> WETH
+// getPrice(WETH, USD) -> 2000 * 10**6
+// amount_ * 10**18 / (2000 * 10**6)
+// = 100 * 10**6 * 10**18 / (2000 * 10**6) = 100 * 10**18 / 2000 = 0.05 * 10**18. This is CORRECT.

test_convert2.cjs

@@ -0,0 +1,12 @@
+let price = 1000000n * 10n**8n / 108000000n; // getPrice(USDC, EUR)
+console.log("getPrice(USDC, EUR)", price);
+// EUR to USDC
+// amount_ is 100 EUR = 100 * 10**6
+let amount_ = 100n * 10n**6n;
+let decimals = 6n;
+let a = amount_ * (10n**decimals) / price;
+console.log("EUR to USDC", a);
+// 100 EUR / 1.08 = 92.59 USDC. Wait.
+// getPrice(USDC, EUR) -> 1 * 10**6 (price of USDC in USDC)
+// fiat_ = EUR
+// getPrice(USDC, EUR) -> price * 10**8 / fiatToUSD -> 1 * 10**6 * 10**8 / (1.08 * 10**8) = 1 * 10**6 / 1.08 = 925925 (this is price of 1 USDC in EUR!)

test_convert3.cjs

@@ -0,0 +1,14 @@
+// 1 USDC = 0.925925 EUR
+// If amount is 100 EUR, then amount in USDC should be 100 / 0.925925 = 108 USDC.
+// Yes! 108000108n is ~ 108 USDC. So 100 EUR = 108 USDC.
+
+// Let's check getPrice and decimals.
+// getPrice returns price with 6 decimals.
+// convert returns amount * 10**decimals / price.
+// amount is in fiat, with 6 decimals.
+// so amount * 10**decimals / price -> (fiat_amount * 10**6) * 10**decimals / (price_fiat * 10**6)
+// The 10**6 in amount and 10**6 in price cancel out!
+// We get fiat_amount * 10**decimals / price_fiat.
+// This is perfectly correctly giving the precision of the target token.
+// So why does the FIXME comment say "precision is not respected"?
+// Let's read Market.sol again:

test_convert4.cjs

@@ -0,0 +1,31 @@
+let denominator = 10000n; // Say rate is 10000, i.e., 100%
+// USD to USDC -> convert(100 USD, USD, USDC, 10000)
+// amount_ = 100 * 10**6
+let usd_to_usdc = (100n * 10n**6n) * (10n ** 4n) / denominator;
+console.log("USD to USDC", usd_to_usdc);
+
+// What if denominator is 9800 (rate 0.98, user wants 2% more token? or 2% less?)
+// amount_ = 100 * 10**6
+let usd_to_usdc_9800 = (100n * 10n**6n) * (10n ** 4n) / 9800n;
+console.log("USD to USDC with rate 0.98", usd_to_usdc_9800);
+
+// So USD to USDC returns amount_ * 10**4 / denominator
+// BUT what is the precision of USDC? It's 6 decimals.
+// amount_ is already 6 decimals.
+// 100 USD -> 100 * 10**6.
+// return amount_ * 10**4 / denominator -> returns 100 * 10**6.
+// What if toToken is NOT USDC but it's USD fiat?
+// wait, the if statement says:
+// if (fromFiat_ == bytes3("USD") && address(toToken_) == USDC) return FullMath.mulDiv(amount_, 10 ** 4, denominator);
+// So it returns USDC precision (6). This is correct.
+
+// NOW what if fromFiat_ == USD, and toToken == USDT (let's say USDT has 6 decimals)
+// wait, if toToken == WETH (18 decimals)
+// price of WETH in USD: getPrice(WETH, USD)
+// say 2000 USD/WETH -> 2000 * 10**6 (6 decimals).
+// convert(100 USD, USD, WETH, 10000)
+// decimals = 18.
+// amount = FullMath.mulDiv(100 * 10**6, 10**18, 2000 * 10**6) -> 0.05 * 10**18 (correct)
+// amount = FullMath.mulDiv(amount, 10**4, 10000) -> 0.05 * 10**18 (correct).
+
+// So why is there a FIXME about precision?? Let's check getPrice.