Rounding-up of user's `cRatio` causes loss for the protocol
At multiple places in the code, user's collateral ratio has been calculated in a manner which causes loss of precision (rounding-up) due to division before multiplication. This causes potential loss for the DittoETH protocol, among other problems.\\nRoot Cause\\nUse of the following piece of code causes rounding-up:\\nStyle 1\\n```\\nuint256 cRatio = short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset));\\n```\\n\\nStyle 2\\n```\\nuint256 oraclePrice = LibOracle.getOraclePrice(asset); // or uint256 oraclePrice = LibOracle.getSavedOrSpotOraclePrice(asset); // or uint256 oraclePrice = LibOracle.getPrice(asset);\\n // rest of code\\n // rest of code\\n // rest of code\\nuint256 cRatio = short.getCollateralRatioSpotPrice(oraclePrice);\\n```\\n\\n\\nLet's break the issue down into 4 smaller parts:\\nPART 1:\\nLet us first look inside getOraclePrice():\\n```\\n  File: contracts/libraries/LibOracle.sol\\n\\n  20            function getOraclePrice(address asset) internal view returns (uint256) {\\n  21                AppStorage storage s = appStorage();\\n  22                AggregatorV3Interface baseOracle = AggregatorV3Interface(s.baseOracle);\\n  23                uint256 protocolPrice = getPrice(asset);\\n  24                // prettier-ignore\\n  25                (\\n  26                    uint80 baseRoundID,\\n  27                    int256 basePrice,\\n  28                    /*uint256 baseStartedAt*/\\n  29                    ,\\n  30                    uint256 baseTimeStamp,\\n  31                    /*uint80 baseAnsweredInRound*/\\n  32                ) = baseOracle.latestRoundData();\\n  33\\n  34                AggregatorV3Interface oracle = AggregatorV3Interface(s.asset[asset].oracle);\\n  35                if (address(oracle) == address(0)) revert Errors.InvalidAsset();\\n  36\\n  37              if (oracle == baseOracle) {\\n  38                    //@dev multiply base oracle by 10**10 to give it 18 decimals of precision\\n  39                    uint256 basePriceInEth = basePrice > 0\\n  40                      ? uint256(basePrice * Constants.BASE_ORACLE_DECIMALS).inv()\\n  41                        : 0;\\n  42                    basePriceInEth = baseOracleCircuitBreaker(\\n  43                        protocolPrice, baseRoundID, basePrice, baseTimeStamp, basePriceInEth\\n  44                    );\\n  45                  return basePriceInEth;\\n  46                } else {\\n  47                    // prettier-ignore\\n  48                    (\\n  49                        uint80 roundID,\\n  50                        int256 price,\\n  51                        /*uint256 startedAt*/\\n  52                        ,\\n  53                        uint256 timeStamp,\\n  54                        /*uint80 answeredInRound*/\\n  55                    ) = oracle.latestRoundData();\\n  56                  uint256 priceInEth = uint256(price).div(uint256(basePrice));\\n  57                    oracleCircuitBreaker(\\n  58                        roundID, baseRoundID, price, basePrice, timeStamp, baseTimeStamp\\n  59                    );\\n  60                  return priceInEth;\\n  61                }\\n  62            }\\n```\\n\\nBased on whether the `oracle` is `baseOracle` or not, the function returns either `basePriceEth` or `priceInEth`.\\n`basePriceEth` can be `uint256(basePrice * Constants.BASE_ORACLE_DECIMALS).inv()` which is basically `1e36 / (basePrice * Constants.BASE_ORACLE_DECIMALS)` or simply written, of the form `oracleN / oracleD` where `oracleN` is the numerator with value 1e36 (as defined here) and `oracleD` is the denominator.\\n`priceInEth` is given as uint256 `priceInEth` = uint256(price).div(uint256(basePrice)) which again is of the form `oracleN / oracleD`.\\n\\nPART 2:\\ngetSavedOrSpotOraclePrice() too internally calls the above `getOraclePrice()` function, if it has been equal to or more than 15 minutes since the last time `LibOrders.getOffsetTime()` was set:\\n```\\n  File: contracts/libraries/LibOracle.sol\\n\\n  153           function getSavedOrSpotOraclePrice(address asset) internal view returns (uint256) {\\n  154               if (LibOrders.getOffsetTime() - getTime(asset) < 15 minutes) {\\n  155                   return getPrice(asset);\\n  156               } else {\\n  157                 return getOraclePrice(asset);\\n  158               }\\n  159           }\\n```\\n\\n\\nPART 3:\\ngetCollateralRatioSpotPrice() calculates `cRatio` as:\\n```\\n  File: contracts/libraries/LibShortRecord.sol\\n\\n  30            function getCollateralRatioSpotPrice(\\n  31                STypes.ShortRecord memory short,\\n  32                uint256 oraclePrice\\n  33            ) internal pure returns (uint256 cRatio) {\\n  34              return short.collateral.div(short.ercDebt.mul(oraclePrice));\\n  35            }\\n```\\n\\n\\nPART 4 (FINAL PART):\\nThere are multiple places in the code (mentioned below under Impacts section) which compare the user's `cRatio` to `initialCR` or `LibAsset.primaryLiquidationCR(_asset)` in the following manner:\\n```\\nif (short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset)) < LibAsset.primaryLiquidationCR(asset))\\n```\\n\\n\\nCalling `short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset))` means the value returned from it would be:\\n```\\n      // @audit-issue : Potential precision loss. Division before multiplication should not be done.\\n      shortCollateral / (shortErcDebt * (oracleN / oracleD))           // return short.collateral.div(short.ercDebt.mul(oraclePrice));\\n```\\n\\nwhich has the potential for precision loss (rounding-up) due to division before multiplication. The correct style ought to be:\\n```\\n// Add the line below\\n      (shortCollateral * oracleD) / (shortErcDebt * oracleN)\\n```\\n\\n\\nHave attempted to keep all values in close proximity to the ones present in forked mainnet tests.\\nLet's assume some values for numerator & denominator and other variables:\\n```\\n    uint256 private short_collateral = 100361729669569000000; // ~ 100 ether\\n    uint256 private short_ercDebt = 100000000000000000000000; // 100_000 ether\\n    uint256 private price = 99995505; // oracleN\\n    uint256 private basePrice = 199270190598; // oracleD\\n    uint256 private primaryLiquidationCR = 2000000000000000000; // 2 ether (as on forked mainnet)\\n\\n// For this example, we assume that oracle != baseOracle, so that the below calculation would be done by the protocol\\nSo calculated priceInEth = price.div(basePrice) = 501808648347845  // ~ 0.0005 ether\\n```\\n\\n\\nLet's calculate for the scenario of `flagShort()` where the code logic says:\\n```\\n  53                if (\\n  54                  short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset))\\n  55                      >= LibAsset.primaryLiquidationCR(asset)      // @audit-issue : this will evaluate to `true`, then revert, due to rounding-up and the short will incorrectly escape flagging\\n  56                ) {\\n  57                    revert Errors.SufficientCollateral();\\n  58                }\\n```\\n\\n\\nCreate a file named `test/IncorrectCRatioCheck.t.sol` and paste the following code in it. Some mock functions are included here which mirror protocol's calculation style:\\n```\\n// SPDX-License-Identifier: GPL-3.0-only\\npragma solidity 0.8.21;\\n\\nimport {U256} from "contracts/libraries/PRBMathHelper.sol";\\nimport {OBFixture} from "test/utils/OBFixture.sol";\\nimport {console} from "contracts/libraries/console.sol";\\n\\ncontract IncorrectCRatioCheck is OBFixture {\\n    using U256 for uint256;\\n\\n    uint256 private short_collateral = 85307470219133700000; // ~ 85.3 ether\\n    uint256 private short_ercDebt = 100000000000000000000000; // 100_000 ether\\n    uint256 private price = 99995505; // oracleN\\n    uint256 private basePrice = 199270190598; // (as on forked mainnet)  // oracleD\\n    uint256 private primaryLiquidationCR = 1700000000000000000; // 1.7 ether (as on forked mainnet)\\n\\n    function _getSavedOrSpotOraclePrice() internal view returns (uint256) {\\n        uint256 priceInEth = price.div(basePrice);\\n        return priceInEth; // will return 501808648347845 =~ 0.0005 ether  // (as on forked mainnet)\\n    }\\n\\n    function getCollateralRatioSpotPrice_IncorrectStyle_As_In_Existing_DittoProtocol(\\n        uint256 oraclePrice\\n    ) internal view returns (uint256) {\\n        return short_collateral.div(short_ercDebt.mul(oraclePrice));\\n    }\\n\\n    function getCollateralRatioSpotPrice_CorrectStyle(uint256 oracleN, uint256 oracleD)\\n        internal\\n        view\\n        returns (uint256)\\n    {\\n        return (short_collateral.mul(oracleD)).div(short_ercDebt.mul(oracleN));\\n    }\\n\\n    /* solhint-disable no-console */\\n    function test_GetCollateralRatioSpotPrice_IncorrectStyle_As_In_Existing_DittoProtocol(\\n    ) public view {\\n        uint256 cRatio =\\n        getCollateralRatioSpotPrice_IncorrectStyle_As_In_Existing_DittoProtocol(\\n            _getSavedOrSpotOraclePrice()\\n        );\\n        console.log("cRatio calculated (existing style) =", cRatio);\\n        if (cRatio >= primaryLiquidationCR) {\\n            console.log("Errors.SufficientCollateral; can not be flagged");\\n        } else {\\n            console.log("InsufficientCollateral; can be flagged");\\n        }\\n    }\\n\\n    /* solhint-disable no-console */\\n    function test_GetCollateralRatioSpotPrice_CorrectStyle() public view {\\n        uint256 cRatio = getCollateralRatioSpotPrice_CorrectStyle(price, basePrice);\\n        console.log("cRatio calculated (correct style) =", cRatio);\\n        if (cRatio >= primaryLiquidationCR) {\\n            console.log("Errors.SufficientCollateral; can not be flagged");\\n        } else {\\n            console.log("InsufficientCollateral; can be flagged");\\n        }\\n    }\\n}\\n```\\n\\n\\nFirst, let's see the output as per protocol's calculation. Run forge test --mt test_GetCollateralRatioSpotPrice_IncorrectStyle_As_In_Existing_DittoProtocol -vv:\\n```\\nLogs:\\n  cRatio calculated (existing style) = 1700000000000000996\\n  Errors.SufficientCollateral; can not be flagged\\n```\\n\\nSo the short can not be flagged as `cRatio > primaryLiquidationCR` of 1700000000000000000.\\nNow, let's see the output as per the correct calculation. Run forge test --mt test_GetCollateralRatioSpotPrice_CorrectStyle -vv:\\n```\\nLogs:\\n  cRatio calculated (correct style) = 1699999999999899995\\n  InsufficientCollateral; can be flagged\\n```\\n\\nShort's cRatio is actually below primaryLiquidationCR. Should have been flagged ideally.\\n
These steps need to be taken to fix the issue. Developer may have to make some additional changes since `.mul`, `.div`, etc are being used from the `PRBMathHelper.sol` library. Following is the general workflow required:\\nCreate additional functions to fetch oracle parameters instead of price: Create copies of `getOraclePrice()` and `getSavedOrSpotOraclePrice()`, but these ones return `oracleN` & `oracleD` instead of the calculated price. Let's assume the new names to be `getOraclePriceParams()` and `getSavedOrSpotOraclePriceParams()`.\\nCreate a new function to calculate cRatio which will be used in place of the above occurences of getCollateralRatioSpotPrice():\\n```\\n    function getCollateralRatioSpotPriceFromOracleParams(\\n        STypes.ShortRecord memory short,\\n        uint256 oracleN,\\n        uint256 oracleD\\n    ) internal pure returns (uint256 cRatio) {\\n        return (short.collateral.mul(oracleD)).div(short.ercDebt.mul(oracleN));\\n    }\\n```\\n\\n\\nFor fixing the last issue of `oraclePrice.mul(1.01 ether)` on L847, first call `getOraclePriceParams()` to get `oracleN` & `oracleD` and then:\\n```\\n  845                       //@dev: force hint to be within 1% of oracleprice\\n  846                       bool startingShortWithinOracleRange = shortPrice\\n// Remove the line below\\n 847                           <= oraclePrice.mul(1.01 ether)\\n// Add the line below\\n 847                           <= (oracleN.mul(1.01 ether)).div(oracleD)\\n  848                           && s.shorts[asset][prevId].price >= oraclePrice;\\n```\\n
```\\n  File: contracts/facets/YieldFacet.sol\\n\\n  76            function _distributeYield(address asset)\\n  77                private\\n  78                onlyValidAsset(asset)\\n  79                returns (uint88 yield, uint256 dittoYieldShares)\\n  80            {\\n  81                uint256 vault = s.asset[asset].vault;\\n  82                // Last updated zethYieldRate for this vault\\n  83                uint80 zethYieldRate = s.vault[vault].zethYieldRate;\\n  84                // Protocol time\\n  85                uint256 timestamp = LibOrders.getOffsetTimeHours();\\n  86                // Last saved oracle price\\n  87              uint256 oraclePrice = LibOracle.getPrice(asset);\\n  88                // CR of shortRecord collateralized at initialMargin for this asset\\n  89                uint256 initialCR = LibAsset.initialMargin(asset) + 1 ether;\\n  90                // Retrieve first non-HEAD short\\n  91                uint8 id = s.shortRecords[asset][msg.sender][Constants.HEAD].nextId;\\n  92                // Loop through all shorter's shorts of this asset\\n  93                while (true) {\\n  94                    // One short of one shorter in this market\\n  95                    STypes.ShortRecord storage short = s.shortRecords[asset][msg.sender][id];\\n  96                    // To prevent flash loans or loans where they want to deposit to claim yield immediately\\n  97                    bool isNotRecentlyModified =\\n  98                        timestamp - short.updatedAt > Constants.YIELD_DELAY_HOURS;\\n  99                    // Check for cancelled short\\n  100                   if (short.status != SR.Cancelled && isNotRecentlyModified) {\\n  101                       uint88 shortYield =\\n  102                           short.collateral.mulU88(zethYieldRate - short.zethYieldRate);\\n  103                       // Yield earned by this short\\n  104                       yield += shortYield;\\n  105                       // Update zethYieldRate for this short\\n  106                       short.zethYieldRate = zethYieldRate;\\n  107                       // Calculate CR to modify ditto rewards\\n  108                     uint256 cRatio = short.getCollateralRatioSpotPrice(oraclePrice);\\n  109                     if (cRatio <= initialCR) {\\n  110                         dittoYieldShares += shortYield;\\n  111                       } else {\\n  112                           // Reduce amount of yield credited for ditto rewards proportional to CR\\n  113                           dittoYieldShares += shortYield.mul(initialCR).div(cRatio);\\n  114                       }\\n  115                   }\\n  116                   // Move to next short unless this is the last one\\n  117                   if (short.nextId > Constants.HEAD) {\\n  118                       id = short.nextId;\\n  119                   } else {\\n  120                       break;\\n  121                   }\\n  122               }\\n  123           }\\n```\\n\\nThis rounding-up can lead to user's `cRatio` to be considered as `>initialCR` even when it's slightly lower. This results in greater `dittoYieldShares` being calculated.\\n```\\n  File: contracts/facets/MarginCallPrimaryFacet.sol\\n\\n  43            function flagShort(address asset, address shorter, uint8 id, uint16 flaggerHint)\\n  44                external\\n  45                isNotFrozen(asset)\\n  46                nonReentrant\\n  47                onlyValidShortRecord(asset, shorter, id)\\n  48            {\\n  49                if (msg.sender == shorter) revert Errors.CannotFlagSelf();\\n  50                STypes.ShortRecord storage short = s.shortRecords[asset][shorter][id];\\n  51                short.updateErcDebt(asset);\\n  52\\n  53                if (\\n  54                  short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset))\\n  55                      >= LibAsset.primaryLiquidationCR(asset)      // @audit-issue : this will evaluate to `true` due to rounding-up and the short will not be eligible for flagging\\n  56                ) {\\n  57                    revert Errors.SufficientCollateral();\\n  58                }\\n  59\\n  60                uint256 adjustedTimestamp = LibOrders.getOffsetTimeHours();\\n  61\\n  62                // check if already flagged\\n  63                if (short.flaggerId != 0) {\\n  64                    uint256 timeDiff = adjustedTimestamp - short.updatedAt;\\n  65                    uint256 resetLiquidationTime = LibAsset.resetLiquidationTime(asset);\\n  66\\n  67                    if (timeDiff <= resetLiquidationTime) {\\n  68                        revert Errors.MarginCallAlreadyFlagged();\\n  69                    }\\n  70                }\\n  71\\n  72                short.setFlagger(cusd, flaggerHint);\\n  73                emit Events.FlagShort(asset, shorter, id, msg.sender, adjustedTimestamp);\\n  74            }\\n```\\n\\n\\n```\\n  File: contracts/facets/MarginCallSecondaryFacet.sol\\n\\n  38            function liquidateSecondary(\\n  39                address asset,\\n  40                MTypes.BatchMC[] memory batches,\\n  41                uint88 liquidateAmount,\\n  42                bool isWallet\\n  43            ) external onlyValidAsset(asset) isNotFrozen(asset) nonReentrant {\\n  44                STypes.AssetUser storage AssetUser = s.assetUser[asset][msg.sender];\\n  45                MTypes.MarginCallSecondary memory m;\\n  46                uint256 minimumCR = LibAsset.minimumCR(asset);\\n  47                uint256 oraclePrice = LibOracle.getSavedOrSpotOraclePrice(asset);\\n  48                uint256 secondaryLiquidationCR = LibAsset.secondaryLiquidationCR(asset);\\n  49\\n  50                uint88 liquidatorCollateral;\\n  51                uint88 liquidateAmountLeft = liquidateAmount;\\n  52                for (uint256 i; i < batches.length;) {\\n  53                  m = _setMarginCallStruct(\\n  54                        asset, batches[i].shorter, batches[i].shortId, minimumCR, oraclePrice\\n  55                    );\\n  56\\n\\n                // rest of code// rest of code\\n                // rest of code// rest of code\\n                // rest of code// rest of code\\n\\n\\n  129           function _setMarginCallStruct(\\n  130               address asset,\\n  131               address shorter,\\n  132               uint8 id,\\n  133               uint256 minimumCR,\\n  134               uint256 oraclePrice\\n  135           ) private returns (MTypes.MarginCallSecondary memory) {\\n  136               LibShortRecord.updateErcDebt(asset, shorter, id);\\n  137\\n  138               MTypes.MarginCallSecondary memory m;\\n  139               m.asset = asset;\\n  140               m.short = s.shortRecords[asset][shorter][id];\\n  141               m.vault = s.asset[asset].vault;\\n  142               m.shorter = shorter;\\n  143               m.minimumCR = minimumCR;\\n  144             m.cRatio = m.short.getCollateralRatioSpotPrice(oraclePrice);\\n  145               return m;\\n  146           }\\n```\\n\\n\\n```\\n  File: contracts/facets/ShortRecordFacet.sol\\n\\n  117           function combineShorts(address asset, uint8[] memory ids)\\n  118               external\\n  119               isNotFrozen(asset)\\n  120               nonReentrant\\n  121               onlyValidShortRecord(asset, msg.sender, ids[0])\\n  122           {\\n  123               if (ids.length < 2) revert Errors.InsufficientNumberOfShorts();\\n  124               // First short in the array\\n  125               STypes.ShortRecord storage firstShort = s.shortRecords[asset][msg.sender][ids[0]];\\n  \\n                // rest of code// rest of code\\n                // rest of code// rest of code\\n                // rest of code// rest of code\\n\\n  174\\n  175               // Merge all short records into the short at position id[0]\\n  176               firstShort.merge(ercDebt, ercDebtSocialized, collateral, yield, c.shortUpdatedAt);\\n  177\\n  178               // If at least one short was flagged, ensure resulting c-ratio > primaryLiquidationCR\\n  179               if (c.shortFlagExists) {\\n  180                   if (\\n  181                     firstShort.getCollateralRatioSpotPrice(\\n  182                         LibOracle.getSavedOrSpotOraclePrice(_asset)\\n  183                     ) < LibAsset.primaryLiquidationCR(_asset)\\n  184                   ) revert Errors.InsufficientCollateral();\\n  185                   // Resulting combined short has sufficient c-ratio to remove flag\\n  186                   firstShort.resetFlag();\\n  187               }\\n  188               emit Events.CombineShorts(asset, msg.sender, ids);\\n  189           }\\n```\\n\\n\\nNOTE:\\nWhile the operation done in this piece of code is a bit different from the above analysis, I am clubbing it with this bug report as the underlying issue is the same (and the resolution would be similar): Multiplication and division operations should not be done directly on top of fetched oracle price, without paying attention to new order of evaluation:\\n```\\n  File: contracts/libraries/LibOrders.sol\\n\\n  812           function _updateOracleAndStartingShort(address asset, uint16[] memory shortHintArray)\\n  813               private\\n  814           {\\n  815               AppStorage storage s = appStorage();\\n  815               uint256 oraclePrice = LibOracle.getOraclePrice(asset);\\n  \\n                // rest of code// rest of code\\n                // rest of code// rest of code\\n                // rest of code// rest of code\\n\\n  845                       //@dev: force hint to be within 1% of oracleprice\\n  846                       bool startingShortWithinOracleRange = shortPrice\\n  847                         <= oraclePrice.mul(1.01 ether)                     // @audit-issue : division before multiplication\\n  848                           && s.shorts[asset][prevId].price >= oraclePrice;\\n  \\n                // rest of code// rest of code\\n                // rest of code// rest of code\\n                // rest of code// rest of code\\n\\n  866           }\\n```\\n\\n\\nThe effective calculation being done above is:\\n```\\n    (oracleN / oracleD) * (1.01 ether)        // division before multiplication\\n```\\n\\n\\nWhich should have been:\\n```\\n    (oracleN * 1.01 ether) / oracle\\n```\\n\\n\\nSimilar multiplication or division operations have been done on `price` at various places throughout the code, which can be clubbed under this root cause itself.
```\\nuint256 cRatio = short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset));\\n```\\n