DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

Emitting incorrect event value

serialcoder

Summary

The LibShortRecord::burnNFT() emits an incorrect event value.

Vulnerability Details

The burnNFT() emits an incorrect event value: nft.owner. Specifically, the nft variable will point to the storage object specified by the tokenId. However, the pointing storage object will be deleted before emitting the Transfer event.

Subsequently, the ERC721::Transfer event will be emitted with nft.owner == address(0).

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

  • https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/libraries/LibShortRecord.sol#L366

  • https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/libraries/LibShortRecord.sol#L371

  • https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/libraries/LibShortRecord.sol#L374

Impact

The ERC721::Transfer is an important event. The incorrect event logs may cause off-chain services to malfunction.

Tools Used

Manual Review

Recommendations

Emit the Transfer event before the delete operations.

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];
+ emit Events.Transfer(nft.owner, address(0), tokenId);
delete s.nftMapping[tokenId];
delete s.getApproved[tokenId];
delete short.tokenId;
- emit Events.Transfer(nft.owner, address(0), tokenId);
}
Updates

Lead Judging Commences

0xnevi Lead Judge
about 2 years ago
0xnevi Lead Judge about 2 years ago
Submission Judgement Published
Invalidated
Reason: Other
serialcoder Submitter
about 2 years ago
0xnevi Lead Judge
about 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-648

Support

FAQs

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