Summary
DittoEth allow shorts owner to create NFT regarding short position, the first NFT minter will have it's short tokenId equal to 0 as tokenIdCounter value is not set by default to 1 and this if a big problem for a lot of cases I will describe below.
Vulnerability Details
tokenIdCounter is an uint40 declared in AppStorage.sol :
struct AppStorage {
address admin;
address ownerCandidate;
address baseOracle;
uint24 flaggerIdCounter;
uint40 tokenIdCounter;
To create an NFT from a short position we can call ERC721Facet.sol#mintNFT() :
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;
s.tokenIdCounter += 1;
}
As you can see s.tokenIdCounter is incremented after the allocation of it's value to s.nftMapping and short.tokenId so first NFT minter will have 0 as tokenIdCounter so it's NFT will be the 0 tokenId NFT.
Several problems can happen regarding this ( medium to high impact) :
Problem 1 : ERC721.sol#balanceOf() will return a bad number if the tokenId 0 owner calls it :
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) {
balance++;
}
unchecked {
j++;
}
}
unchecked {
++i;
}
}
}
=> As you can see if shortRecords[j].tokenId == 0 , NFT is not accounted
Problem 2 : Combining NFTs will revert if 0 tokenId NFT is the first id of the uint8[] memory ids short orders parameter
function combineShorts(address asset, uint8[] memory ids)
external
isNotFrozen(asset)
nonReentrant
onlyValidShortRecord(asset, msg.sender, ids[0])
{
if (ids.length < 2) revert Errors.InsufficientNumberOfShorts();
STypes.ShortRecord storage firstShort = s.shortRecords[asset][msg.sender][ids[0]];
for (uint256 i = ids.length - 1; i > 0; i--) {
STypes.ShortRecord storage currentShort = s.shortRecords[_asset][msg.sender][_id];
if (currentShort.tokenId != 0) {
if (firstShort.tokenId == 0) {
revert Errors.FirstShortMustBeNFT();
}
}
=> As you can see if tokenId of the firstShort submitted to be combined is 0 it will revert, but it should not
Problem 3 : If the tokenId 0 NFT is combined , owner will keep the NFT with tokenId 0 (so he will have it's merged short position and the non merged tokenid=0 position)
function combineShorts(address asset, uint8[] memory ids)
external
isNotFrozen(asset)
nonReentrant
onlyValidShortRecord(asset, msg.sender, ids[0])
{
if (ids.length < 2) revert Errors.InsufficientNumberOfShorts();
STypes.ShortRecord storage firstShort = s.shortRecords[asset][msg.sender][ids[0]];
for (uint256 i = ids.length - 1; i > 0; i--) {
uint8 _id = ids[i];
_onlyValidShortRecord(_asset, msg.sender, _id);
STypes.ShortRecord storage currentShort = s.shortRecords[_asset][msg.sender][_id];
if (currentShort.tokenId != 0) {
if (firstShort.tokenId == 0) {
revert Errors.FirstShortMustBeNFT();
}
LibShortRecord.burnNFT(currentShort.tokenId);
}
LibShortRecord.deleteShortRecord(_asset, msg.sender, _id);
}
firstShort.merge(ercDebt, ercDebtSocialized, collateral, yield, c.shortUpdatedAt);
}
=> As you can see if tokenId of a short being merged is 0, it won't be burned so user will get both the non merged short NFT position with tokenId = 0 and the merged NFT position , so if this NFT has a short position with a very good health he could be used to improve the health of other NFT with a bad CR indefinitely(by calling combine() multiple times) and prevent some bad shorts from being liquidated
Problem 4 : tokenId 0 NFT can mint as many NFT position as he wants
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;
s.tokenIdCounter += 1;
}
=> As you can see if (short.tokenId == 0) it doesn't revert , so tokenId 0 nft owner can mint as many nft as he wants for it's position which could DOS the NFT mechanisms if tokenIdCounter = max(uint40) - 1 and completely destruct the accounting model because the same short position could be owned,cancel,combined multiple times
Impact
Multiples impact :
bad accounting of balanceOf()
combining mechanisms can be maliciously used to improve health factor of short
multiple NFT can be minted for the same short position
Tools Used
VSCode
Recommendations
Set tokenIdCounter to 1 or increment it before minting the NFT
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.tokenIdCounter += 1;
s.nftMapping[s.tokenIdCounter] = STypes.NFT({
owner: msg.sender,
assetId: s.asset[asset].assetId,
shortRecordId: shortRecordId
});
short.tokenId = s.tokenIdCounter;
}
