DittoETH

Summary

The MarginCallPrimaryFacet::liquidate() faces a rounding-down issue when verifying the margin call time window.

Subsequently, a flagged Short will not be liquidable by a short flagger suddenly after 10 hours have passed (firstLiquidationTime), and it will not be liquidable by other margin callers immediately after 12 hours have passed (secondLiquidationTime) as expected.

Vulnerability Details

The liquidate() is one of the most essential functions of the Ditto protocol. The malfunctioning of the liquidate() can lead to the disruption of the Ditto protocol and their minted stable assets (e.g., cUSD).

The liquidate() executes the _canLiquidate() to verify that the function must be called within the designed margin call time window according to the following timeline.

• By default, the firstLiquidationTime is 10 hours, the secondLiquidationTime is 12 hours, and the resetLiquidationTime is 16 hours.

• Between the updatedAt and firstLiquidationTime, the liquidate() is not callable.

• Between the firstLiquidationTime and secondLiquidationTime, the liquidate() is only callable by the short flagger.

• Between the secondLiquidationTime and resetLiquidationTime, the liquidate() is callable by anyone.

• After the resetLiquidationTime, the liquidate() is not callable, and the Short's flag can be reset.

However, I discovered that the _canLiquidate() does not function according to the above spec. Since the _canLiquidate() computes the timeDiff variable using an hour unit, the resulting timeDiff will face a rounding-down issue because Solidity has no fixed-point numbers (please refer to the Proof of Concept section below for more details).

Consequently, the margin call time window will be shifted forward by 1 hour.

To elaborate, the above timeline will become the below.

• By default, the firstLiquidationTime is 10 hours, the secondLiquidationTime is 12 hours, and the resetLiquidationTime is 16 hours. (Using the same settings)

• Between the updatedAt and firstLiquidationTime (+ 1 hour), the liquidate() is not callable.

• Between the firstLiquidationTime (+ 1 hour) and secondLiquidationTime (+ 1 hour), the liquidate() is only callable by the short flagger.

• Between the secondLiquidationTime (+ 1 hour) and resetLiquidationTime (+ 1 hour), the liquidate() is callable by anyone.

• After the resetLiquidationTime (+ 1 hour), the liquidate() is not callable, and the Short's flag can be reset.

• The liquidate() executes the _canLiquidate() to check if the function call is within the expected margin call time window: https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/MarginCallPrimaryFacet.sol#L118

• The _canLiquidate() computes the timeDiff (in an hour unit): https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/MarginCallPrimaryFacet.sol#L384

• Since the timeDiff is in an hour unit, the margin call time window is shifted forward by 1 hour (not the expected time window): https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/MarginCallPrimaryFacet.sol#L391-L395

• The getOffsetTimeHours()'s resulting timeInHours faces a rounding-down issue (coarse-grained timestamp unit): https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/libraries/LibOrders.sol#L31

Proof of Concept

This section will demonstrate how the timeDiff variable will face the rounding-down issue.

• Assuming that the variable timeInSeconds = 3600 (1 hour)

• When we compute the variable timeInHours = 1

• But if the timeInSeconds = 5400 (1.5 hours)

• The timeInHours = 1 (not 1.5)

As you can see, the resulting timeInHours will become 1 (not 1.5) due to the rounding down since Solidity has no fixed-point numbers.

Impact

Subsequently, the flagged Short will not be liquidable by the short flagger suddenly after 10 hours have passed (firstLiquidationTime), and it will not be liquidable by other margin callers immediately after 12 hours have passed (secondLiquidationTime) as expected.

This will break the functionality of the liquidate(), affecting the user experience. Moreover, the shift of 1 hour can cause the debts of all Short positions in the protocol to be larger than expected, affecting the stability of the Ditto protocol and its minted stable assets (e.g., cUSD).

Tools Used

Manual Review

Recommendations

To fix the vulnerability, change the conditional logic for the isBetweenFirstAndSecondLiquidationTime and isBetweenSecondAndResetLiquidationTime variables like the below snippet.