DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Missing to call the ERC721Facet#`mintNFT()`

0xmuxyz

Summary

Before the LibShortRecord#burnNFT() would be called with a tokenID of the Short Position NFT in the ShortRecordFacet#combineShorts(), the tokenID of the Short Position NFT is supposed to be minted by calling the ERC721Facet#mintNFT().

However, the ERC721Facet#mintNFT() would never be called in any contracts and any functions. As a result, a Short Position NFT will never be minted.
If a NFT will never be minted, a Short Position NFT will never be burned in the ShortRecordFacet#combineShorts().

Because the currentShort.tokenId would always 0 and therefore the condition (if (currentShort.tokenId != 0)) would always be skipped in the ShortRecordFacet#combineShorts(). Due to that, if-block under the condition would also always be skipped.
https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/ShortRecordFacet.sol#L162-L169

This is unintended behavior of the Ditto protocol as a design.

In addition to that, the DittoETH protocol assume that a current shorter can transfer their short position to another user (a potential future shorter) as a design by transferring the Short Position NFT from a current shorter to another user.
However, the assumption (design) would also not work due to missing to call the ERC721Facet#mintNFT().

Vulnerability Details

Within the ERC721Facet#mintNFT(), a Short Position NFT that represents the Short Position would be minted like this:
https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/ERC721Facet.sol#L260-L284

/**
* @notice Mints NFT for active shortRecord
* @dev Minters can only mint their own shortRecords
*
* @param asset The market that will be impacted
* @param shortRecordId Id of active shortRecord
*/
​
function mintNFT(address asset, uint8 shortRecordId)
external
isNotFrozen(asset)
nonReentrant
onlyValidShortRecord(asset, msg.sender, shortRecordId)
{
if (shortRecordId == Constants.SHORT_MAX_ID) {
revert Errors.CannotMintLastShortRecord();
}
STypes.ShortRecord storage short =
s.shortRecords[asset][msg.sender][shortRecordId];
​
if (short.tokenId != 0) revert Errors.AlreadyMinted();
​
s.nftMapping[s.tokenIdCounter] = STypes.NFT({
owner: msg.sender,
assetId: s.asset[asset].assetId,
shortRecordId: shortRecordId
});
​
short.tokenId = s.tokenIdCounter;
​
//@dev never decreases
s.tokenIdCounter += 1;
}

Within the LibShortRecord#burnNFT(), a Short Position NFT would be burned like this:
https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/libraries/LibShortRecord.sol#L363-L375

function burnNFT(uint256 tokenId) internal {
//@dev No need to check downcast tokenId because it is handled in function that calls burnNFT
AppStorage storage s = appStorage();
STypes.NFT storage nft = s.nftMapping[tokenId];
if (nft.owner == address(0)) revert Errors.NotMinted();
address asset = s.assetMapping[nft.assetId];
STypes.ShortRecord storage short =
s.shortRecords[asset][nft.owner][nft.shortRecordId];
delete s.nftMapping[tokenId];
delete s.getApproved[tokenId];
delete short.tokenId;
emit Events.Transfer(nft.owner, address(0), tokenId);
}

Within the ShortRecordFacet#combineShorts(), the LibShortRecord#burnNFT() would be called with a tokenID (currentShort.tokenId) of the Short Position NFT to be combined like this:
https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/ShortRecordFacet.sol#L168

/**
* @notice Combine active shorts into one short
* @dev If any shorts are flagged, the resulting short must have c-ratio > primaryLiquidationCR
*
* @param asset The market that will be impacted
* @param ids Array of short ids to be combined
*
*/
​
function combineShorts(address asset, uint8[] memory ids)
external
isNotFrozen(asset)
nonReentrant
onlyValidShortRecord(asset, msg.sender, ids[0])
{
...
for (uint256 i = ids.length - 1; i > 0; i--) {
...
if (currentShort.tokenId != 0) {
//@dev First short needs to have NFT so there isn't a need to burn and re-mint
if (firstShort.tokenId == 0) {
revert Errors.FirstShortMustBeNFT();
}
​
LibShortRecord.burnNFT(currentShort.tokenId); ///<--------- @audit
}
​
// Cancel this short and combine with short in ids[0]
LibShortRecord.deleteShortRecord(_asset, msg.sender, _id);
}

Before the LibShortRecord#burnNFT() would be called with a tokenID of the Short Position NFT in the ShortRecordFacet#combineShorts(), the tokenID of the Short Position NFT is supposed to be minted by calling the ERC721Facet#mintNFT().

However, the ERC721Facet#mintNFT() would never be called in any contracts and any functions. As a result, a Short Position NFT will never be minted.
If a NFT will never be minted, a Short Position NFT will never be burned in the ShortRecordFacet#combineShorts().

Because the currentShort.tokenId would always 0 and therefore the condition (if (currentShort.tokenId != 0)) would always be skipped in the ShortRecordFacet#combineShorts(). Due to that, if-block under the condition would also always be skipped.
https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/ShortRecordFacet.sol#L162-L169

This is unintended behavior of the Ditto protocol as a design.

In addition to that, the DittoETH protocol assume that a current shorter can transfer their short position to another user (a potential future shorter) as a design by transferring the Short Position NFT from a current shorter to another user.
However, the assumption (design) would also not work due to missing to call the ERC721Facet#mintNFT().

Impact

This lead to unintended behavior of the Ditto protocol as a design.

In addition to that, the DittoETH protocol assume that a current shorter can transfer their short position to another user (a potential future shorter) as a design by transferring the Short Position NFT from a current shorter to another user.
However, the assumption (design) would also not work due to missing to call the ERC721Facet#mintNFT().

Tools Used

  • Foundry

Recommendations

Consider calling the ERC721Facet#mintNFT() in order to mint a new Short Position NFT when a new ShortRecord (shortRecordId) would be created.

Updates

Lead Judging Commences

0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.