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

`short.tokenId` starting from 0 will result in an incorrect behaviour of some protocol functions

CipherWizard

Summary

The fact that short.tokenId starts from 0 will lead to incorrect behaviour of the functions : ShortRecordFacet.combineShorts, ERC721Facet.balanceOf, ERC721Facet.mintNFT as they all assume that the short tokenId starts from 1 (cannot be 0).

Vulnerability Details

The issue occurs when the first short is minted with the ERC721Facet.mintNFT function :

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
});
//@audit tokenId will starts from 0
short.tokenId = s.tokenIdCounter;
​
//@dev never decreases
s.tokenIdCounter += 1;
}

As you can see from the code above the s.tokenIdCounter is first assigned to short.tokenId before its value get incremented, and because s.tokenIdCounter is never initialized to 1 this will result in the first short having a tokenId equal to 0.

This will lead an incorrect behaviour in three functions of the protocol : ERC721Facet.balanceOf, ERC721Facet.mintNFT and ShortRecordFacet.combineShorts.

First we start ERC721Facet.mintNFT itself, we can see from the code snippet above that because the first short have a tokenId equal to 0, the shorter can mint a second NFT for the same short as the check if (short.tokenId != 0) revert Errors.AlreadyMinted(); will not revert.

In the ERC721Facet.balanceOf functions we find that if counts only the NFTs with tokenId different from 0 and so this first short NFT will not be accounted for in the shorted balance.

function balanceOf(address owner) external view returns (uint256 balance) {
if (owner == address(0)) {
revert Errors.ERC721InvalidOwner(address(0));
}
​
uint256 length = s.assets.length;
for (uint256 i; i < length; ) {
STypes.ShortRecord[] memory shortRecords = IDiamond(
payable(address(this))
).getShortRecords(s.assets[i], owner);
for (uint256 j; j < shortRecords.length; ) {
if (shortRecords[j].tokenId != 0) {
//@audit will not account for first minted short
balance++;
}
unchecked {
j++;
}
}
unchecked {
++i;
}
}
}

And finally in ShortRecordFacet.combineShorts function there is the following lines of code :

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);
}

We find the function will always misbehave when the first minted short record NFT is used, if it's currentShort then the check currentShort.tokenId != 0 will be false and thus the NFT won't be burned which is wrong, and in the other case if it's firstShort the check will firstShort.tokenId == 0 will be true and the call will revert which is not supposed to happen.

Impact

The fact that short.tokenId starts from 0 will lead to incorrect behaviour of the functions : ShortRecordFacet.combineShorts, ERC721Facet.balanceOf, ERC721Facet.mintNFT as they all assume that the short tokenId starts from 1 (cannot be 0).

Tools Used

Manual review

Recommended Mitigation

Make sure that short.tokenId starts from 1 by first incrementing s.tokenIdCounter and then assigning its value, this avoid any misbehaviour of the aforementioned functions :

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
});
​
//@dev never decreases
s.tokenIdCounter += 1;
​
//@audit will starts from 1 now
short.tokenId = s.tokenIdCounter;
}
Updates

Lead Judging Commences

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

Support

FAQs

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