Vulnerability Details
When an ShortRecord NFT is transferred to another user, the tokenId associated with the original ShortRecord is not cleared. In case of a partially filled ShortRecord, further fills to the associated short order will add the new debt and collateral to this same ShortRecord without clearing the tokenId even when the ShortRecord has been cancelled.
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
To mint an NFT for a ShortRecord, it must not be having a non-zero tokenId. Hence the above situation will result in the user not being able to mint an NFT for the ShortRecord.
function mintNFT(address asset, uint8 shortRecordId)
external
isNotFrozen(asset)
nonReentrant
onlyValidShortRecord(asset, msg.sender, shortRecordId)
{
STypes.ShortRecord storage short =
s.shortRecords[asset][msg.sender][shortRecordId];
if (short.tokenId != 0) revert Errors.AlreadyMinted();
https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/ERC721Facet.sol#L260-L272
POC Test
@@ -453,6 +453,29 @@ contract ERC721Test is OBFixture {
diamond.ownerOf(2);
}
+ function test_PrevOwnerCantMintNFTForNewFills() public {
+ //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);
+ diamond.safeTransferFrom(sender, address(0xBEEF), 1, "");
+ vm.stopPrank();
+
+ //fill the other half of the short order to activate the same short record
+ fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT / 2, sender);
+
+ assertEq(diamond.getShortRecordCount(asset, sender), 1);
+ uint8 shortId = diamond.getShortRecords(asset, sender)[0].id;
+
+ //owner cannot mint nft for this short
+ vm.startPrank(sender);
+ vm.expectRevert(Errors.AlreadyMinted.selector);
+ diamond.mintNFT(asset, shortId);
+ }
+
function test_CombineShort_TwoShortsOneNFT() public {
//@dev first short has nft, second does not
createShortAndMintNFT();
Impact
Some users might not be able to mint NFT for their short records
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(
