GHSA-xh2p-7p87-fhgh: @liquity/contracts Incorrect TCR Calculation Vulnerability

{
  "schema_version" : "1.4.0",
  "id" : "GHSA-xh2p-7p87-fhgh",
  "modified" : "2021-07-29T20:43:48Z",
  "published" : "2021-08-05T16:56:58Z",
  "aliases" : [ ],
  "summary" : "Incorrect TCR calculation in batchLiquidateTroves() during Recovery Mode",
  "details" : "TCR is temporarily miscalculated in the `batchLiquidateTroves` function during Recovery Mode.\n\nThe bug lies in [`batchLiquidateTroves` of `TroveManager`](https://github.com/liquity/dev/blob/7c83ea11378454629618b3808b16fbfda69ee3e5/packages/contracts/contracts/TroveManager.sol#L722). \n\nWhen calculating system's entire collateral, we should also exclude the liquidated trove's surplus collateral, since liquidation closes the trove and makes the surplus collateral claimable by the trove owner. This means, this line of code should look like this: \n```\nvars.entireSystemColl = vars.entireSystemColl.sub(singleLiquidation.collToSendToSP).sub(singleLiquidation.collSurplus);\n```\n### Impact\n\nThe miscalculated entire collateral is used only to calculate the TCR and check if the system has been able to exit Recovery Mode. The miscalulation only persists temporarily, and within the`batchLiquidateTroves` transaction. Once the transaction completes the TCR and Recovery Mode will be calculated properly again. However, the bug could negatively impact the liquidation throughput and the gas efficiency gains from batching multiple liquidations in a single transaction.\n\nIn normal situations, the impact of the collateral surplus of a Trove on the global TCR would be tiny. For instance, we have calculated that liquidating a trove with a collateral representing 1% of the total system collateral (so in the order of at least $10M at current values), would lead to an extra 0.53% in the temporary miscalculation of TCR. So for this bug to be meaningful, in such a scenario, the resulting real TCR must be already be very close to the Recovery Mode boundary anyway - i.e. between 149.47% and 150%. The batch liquidation transaction should also be executed with a particular trove ordering to achieve the TCR distortion. When a different trove order for the liquidation transaction is selected, the bug has no impact. In summary, the bug only has a non-negligible impact in a very narrow, specific set of circumstances.\n\nThe potential effects of the bug after it occurs are:\n\n- The next trove in the sequence is not liquidated because the `batchLiquidateTroves` function calculates a premature exit from Recovery Mode. It could be liquidated in a subsequent transaction if the price of Ether doesn’t recover.\n- The next trove in the sequence has an ICR below 100% and it’s offset against the Stability Pool instead of redistributed among other troves because the function calculates a premature exit from Recovery Mode. For this to happen, the Ether price must have instantly plummeted by more than 10% (otherwise, the trove would have been already liquidated before).\n- The next trove in the sequence is liquidated while its ICR is over the real TCR: the function calculates the TCR as being slightly too high, and thus can liquidate a trove that has ICR less than the calculated TCR, but greater than the true TCR. This is probably the worst outcome - however it is already possible to achieve the same effect, regardless of the bug.  A liquidator can craft a `batchLiquidateTroves` transaction whereby they select troves for liquidation such that the TCR increases and makes a given trove liquidateable. To liquidate trove A, they can order troves such that they first liquidate troves which raise the TCR to between A's ICR and 150%. This is intended and expected behavior. As clearly stated in Liquity documentation, to be completely safe and guarantee immunity from liquidation in Recovery Mode, borrowers should keep their ICR above 150%.\n\nWe don't believe this bug creates a profitable exploit. Theoretically, and only in a very narrow set of circumstances, a liquidator could try to send a batch liquidation during Recovery Mode that lets the system very temporarily return to Normal Mode earlier than it should. In that case - _and only if the Ether price also happens to suddenly plummet by more than 10%_ - stability providers might take the haircut that should be taken by the borrowers (through redistribution).\n\n### Patches\nThe problem has been patched in the source code but not on mainnet contracts. Liquity protocol is immutable, and this issue is not critical, so it doesn't merit a launch of a new version.\n\n### Bug bounty\n\nA reward of $1,000 (the maximum for its category) was awarded to Xiahong (`gaoxh06`) for reporting this bug.\n\n### For more information\n\nIf you have any questions or comments about this advisory:\n* Open an issue in [our repo](https://github.com/liquity/dev)\n* Email us at [security@liquity.org](mailto:security@liquity.org)\n",
  "severity" : [ {
    "type" : "CVSS_V3",
    "score" : "CVSS:3.1/AV:N/AC:H/PR:N/UI:R/S:U/C:N/I:L/A:N"
  } ],
  "affected" : [ {
    "package" : {
      "ecosystem" : "npm",
      "name" : "@liquity/contracts"
    },
    "ranges" : [ {
      "type" : "ECOSYSTEM",
      "events" : [ {
        "introduced" : "0"
      }, {
        "last_affected" : "1.0.0"
      } ]
    } ]
  } ],
  "references" : [ {
    "type" : "WEB",
    "url" : "https://github.com/liquity/dev/security/advisories/GHSA-xh2p-7p87-fhgh"
  }, {
    "type" : "WEB",
    "url" : "https://github.com/liquity/dev/commit/c69d0bae30b5457e89724d880851a03ba7477905"
  }, {
    "type" : "WEB",
    "url" : "https://github.com/liquity/dev/blob/7c83ea11378454629618b3808b16fbfda69ee3e5/packages/contracts/contracts/TroveManager.sol#L722"
  }, {
    "type" : "WEB",
    "url" : "https://www.npmjs.com/package/@liquity/contracts"
  } ],
  "database_specific" : {
    "cwe_ids" : [ "CWE-682" ],
    "severity" : "LOW",
    "github_reviewed" : true,
    "github_reviewed_at" : "2021-07-29T20:43:48Z",
    "nvd_published_at" : null
  }
}