Previous NFT owner can burn NFT from the new owner
Short records can be transferred as NFTs. Internally, the short record is deleted from the sender and re-created for the new owner (receiver). However, the `tokenId` of the deleted short record is not reset, allowing the previous NFT owner to burn the NFT from the new owner.\\nShort positions, i.e., short records, can be represented as an NFT (ERC-721) with a specific `tokenId`, storing the reference to the short record id in the `shortRecordId` property of the `nftMapping` mapping.\\nSuch a short record can be transferred to another address by sending the NFT to the new owner. Internally, when transferring the ERC-721 token, the `transferShortRecord` function is called (e.g., in line 162 of the `ERC721Facet.transferFrom` function).\\nThe `transferShortRecord` function first validates if the short record is transferable (e.g., not flagged and not canceled) and then calls the `deleteShortRecord` function in line 132 to delete the short record from the `shortRecords` mapping. Thereafter, a new short record with the values of the transferred short record is created with the new owner as the shorter, and the `nftMapping` struct is updated accordingly.\\ncontracts/libraries/LibShortRecord.sol#L132\\n```\\nfunction transferShortRecord(\\n    address asset,\\n    address from,\\n    address to,\\n    uint40 tokenId,\\n    STypes.NFT memory nft\\n) internal {\\n    AppStorage storage s = appStorage();\\n    STypes.ShortRecord storage short = s.shortRecords[asset][from][nft.shortRecordId];\\n    if (short.status == SR.Cancelled) revert Errors.OriginalShortRecordCancelled();\\n    if (short.flaggerId != 0) revert Errors.CannotTransferFlaggedShort();\\n❌  deleteShortRecord(asset, from, nft.shortRecordId);\\n    uint8 id = createShortRecord(\\n        asset,\\n        to,\\n        SR.FullyFilled,\\n        short.collateral,\\n        short.ercDebt,\\n        short.ercDebtRate,\\n        short.zethYieldRate,\\n        tokenId\\n    );\\n    if (id == Constants.SHORT_MAX_ID) {\\n        revert Errors.ReceiverExceededShortRecordLimit();\\n    }\\n    s.nftMapping[tokenId].owner = to;\\n    s.nftMapping[tokenId].shortRecordId = id;\\n}\\n```\\n\\nHowever, the `LibShortRecord.deleteShortRecord` function neglects to reset and delete the short record's `tokenId`, which is initially set to the `tokenId` of the newly minted NFT in line of the `ERC721Facet.mintNFT` function. Consequently, upon transferring the short record, the deleted short record still references the transferred NFT's `tokenId`, in addition to the new short record which also references the same `tokenId`. Thus, two short records (with different owners), one being even deleted, reference the same NFT token.\\nThis oversight leads to the following issues (with number 3 being the most severe):\\nThe `ERC721Facet.balanceOf` function will report an incorrect NFT token balance for the previous NFT owner: If the short record was only partially filled before transferring it as a NFT, the remaining short record can still be fully filled, resetting the `SR.Cancelled` status. This will cause the `balanceOf` function to include this short record, and due to the short record still referencing the transferred NFT's `tokenId`, this NFT is still counted as owned by the previous owner.\\nThe previous NFT owner can not tokenize the remaining short record: As the `tokenId` of the deleted short record is not reset, the previous owner can not tokenize the remaining short record as any attempt to mint a new NFT via the `ERC721Facet.mintNFT` function will revert with the `Errors.AlreadyMinted` error.\\nThe previous NFT owner can burn the NFT from the new owner: As the `tokenId` of the deleted and partially filled short record is not reset, the short can be fully filled, resetting the `SR.Cancelled` status. By subsequently combining this short with another short using the `ShortRecordFacet.combineShorts` function, the combined shorts will have their associated NFT burned.\\nPlease note that the owner of the transferred short record can re-mint a NFT for the short via the `ERC721Facet.mintNFT`, but if the owner is a contract, the contract may lack the required functionality to do so.\\nThe following test case demonstrates the outline issue 3 above:\\n
Consider resetting the `tokenId` of the deleted short record in the `LibShortRecord.deleteShortRecord` function.
The previous NFT owner can burn the NFT from the new owner.\\nIf this NFT transfer was part of a trade and, for instance, sent to an escrow contract, the previous NFT owner can burn the NFT from the escrow contract, while the escrow contract lacks the functionality to re-mint the NFT for the short record. This renders the short record unusable, and funds (collateral) associated with the short record are lost.
```\\nfunction transferShortRecord(\\n    address asset,\\n    address from,\\n    address to,\\n    uint40 tokenId,\\n    STypes.NFT memory nft\\n) internal {\\n    AppStorage storage s = appStorage();\\n    STypes.ShortRecord storage short = s.shortRecords[asset][from][nft.shortRecordId];\\n    if (short.status == SR.Cancelled) revert Errors.OriginalShortRecordCancelled();\\n    if (short.flaggerId != 0) revert Errors.CannotTransferFlaggedShort();\\n❌  deleteShortRecord(asset, from, nft.shortRecordId);\\n    uint8 id = createShortRecord(\\n        asset,\\n        to,\\n        SR.FullyFilled,\\n        short.collateral,\\n        short.ercDebt,\\n        short.ercDebtRate,\\n        short.zethYieldRate,\\n        tokenId\\n    );\\n    if (id == Constants.SHORT_MAX_ID) {\\n        revert Errors.ReceiverExceededShortRecordLimit();\\n    }\\n    s.nftMapping[tokenId].owner = to;\\n    s.nftMapping[tokenId].shortRecordId = id;\\n}\\n```\\n