Summary

The MarginCallPrimaryFacet::flagShort() faces a rounding-down issue when verifying the margin call end time (resetLiquidationTime).

Subsequently, a short flagger cannot re-flag the target Short suddenly after 16 hours (margin call end time -- resetLiquidationTime) as expected since the margin call end time will be shifted forward by 1 hour.

Vulnerability Details

Once a flagger invokes the flagShort() on the target Short, the function will verify that if the target Short was already flagged, the flagger will be able to flag the target Short again if and only if the previous flag has reached the margin call end time (resetLiquidationTime).

For example, if the resetLiquidationTime is 16 hours, the flagger can suddenly flag the target Short again after the timeDiff variable has passed 16 hours.

However, I discovered that the flagShort() does not function according to the above spec. Since the flagShort() 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 end time (resetLiquidationTime) will be shifted forward by 1 hour.

As a result, the flagger can re-flag the target Short when the timeDiff variable has passed 17 hours (not 16 hours as designed).

• The adjustedTimestamp is in an hour unit: https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/MarginCallPrimaryFacet.sol#L60

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

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

• 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, a short flagger can re-flag the target Short when the timeDiff variable has passed 17 hours (not 16 hours as designed).

This will break the functionality of the flagShort(), 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, remove the edge case of timeDiff == resetLiquidationTime like the below snippet.