DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Shorters can avoid being liquidated by reusing the flaggerId of the user who flagged them for liquidation

cosine

Summary

There are two mistakes in the setFlagger function of the LibShortRecord which allows users to delete and claim the flaggerId of other users for themself, even if the flagger id is still valid. This can be misused as griefing attack, as well as to avoid being liquidated.

Vulnerability Details

The setFlagger function of the LibShortRecord.sol contract is used to set and also delete flaggerIds (as flaggerIds can be reused). To check if the id can be reused, the time difference is calculated and compared against the firstLiquidationTime of the CUSD asset:

uint256 timeDiff = flaggerToReplace != address(0)
? LibOrders.getOffsetTimeHours()
- s.assetUser[cusd][flaggerToReplace].g_updatedAt
: 0;
//@dev re-use an inactive flaggerId
if (timeDiff > LibAsset.firstLiquidationTime(cusd)) {
delete s.assetUser[cusd][flaggerToReplace].g_flaggerId;
short.flaggerId = flagStorage.g_flaggerId = flaggerHint;
} ...

Here are two mistakes made:

  1. The time difference is checked against the firstLiquidationTime instead of the secondLiquidationTime or resetTime. The firstLiquidationTime is the time when margin callers who flagged a shortRecord for liquidation are able to actually liquidate it. Therefore, if this time is taken to check if the id can be reused, the shorter who is flagged for liquidation can wait till the margin caller would be able to liquidate and instantly reuse, with front running, the id of the margin caller. So that if the margin caller who calls liquidate now will be reverted.

  2. It does not take the firstLiquidationTime of the given asset and instead uses the firstLiquidationTime of the hardcoded CUSD address. As the firstLiquidationTime of assets can differ and can be updated in the OwnerFacet, this could also lead to problems even if the right time would be used.

Inside the MarginCallPrimaryFacet.sol contract, we can see that the hardcoded CUSD address is used:

constructor(address _cusd) {
**cusd = _cusd;**
}
​
function flagShort(address asset, address shorter, uint8 id, uint16 flaggerHint)
external
isNotFrozen(asset)
nonReentrant
onlyValidShortRecord(asset, shorter, id)
{
if (msg.sender == shorter) revert Errors.CannotFlagSelf();
STypes.ShortRecord storage short = s.shortRecords[asset][shorter][id];
short.updateErcDebt(asset);
​
if (
short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset))
>= LibAsset.primaryLiquidationCR(asset)
) {
revert Errors.SufficientCollateral();
}
​
uint256 adjustedTimestamp = LibOrders.getOffsetTimeHours();
​
// check if already flagged
if (short.flaggerId != 0) {
uint256 timeDiff = adjustedTimestamp - short.updatedAt;
uint256 resetLiquidationTime = LibAsset.resetLiquidationTime(asset);
​
if (timeDiff <= resetLiquidationTime) {
revert Errors.MarginCallAlreadyFlagged();
}
}
​
**short.setFlagger(cusd, flaggerHint);**
emit Events.FlagShort(asset, shorter, id, msg.sender, adjustedTimestamp);
}

We can also see that inside the flagShort function the right check is given to prevent the first mistake declared above:

if (short.flaggerId != 0) {
uint256 timeDiff = adjustedTimestamp - short.updatedAt;
uint256 resetLiquidationTime = LibAsset.resetLiquidationTime(asset);
​
if (timeDiff <= resetLiquidationTime) {
revert Errors.MarginCallAlreadyFlagged();
}
}

However, this does not prevent the bug, as it checks inside the _canLiquidate function, if the liquidate caller is the flagger and this function looks for the flagger inside the flagMapping, which is updated by the setFlagger function in LibShortRecord.sol:

s.flagMapping[m.short.flaggerId] == msg.sender

Therefore, it is possible to call flagShort on another shortRecord to remove the flagger from any wished shortRecord after the firstLiquidationTime.

Impact

Users can misuse this as griefing attack, and shorters can avoid being liquidated.

Tools Used

Manual Review

Recommendations

Check if the timeDiff is bigger than the secondLiquidationTime or even the resetTime and check it for the given asset and not for CUSD.

Updates

Lead Judging Commences

0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-533

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.