First Flight #9: Soulmate

Summary

The soulbound NFT is implemented by inheriting the ERC721.sol NFT standard from OpenZeppelin into the Soulmate.sol contract. ERC721.sol implements its own data structure to keep track of who owns the NFTs and how many there are. But Soulmate.sol does not use ownerOf and tokenId from ERC721.sol - instead it creates a second set of ownership and token ID data structure (see below - idToOwners, nextID etc)

The problem with doing this is when you forget to update one of the data structure and they conflict

Vulnerability Details

Here is the data structure for NFT ownership in Soulmate.sol but there are also functions and variables to manage the same thing in ERC721.sol:

Impact

The parallel data structure can be out of sync with each other which can create problems. For example, it is possible to use safeTransferFrom() in ERC721.sol to transfer your NFT (even though you aren't supposed to be able to transfer it) and the variables in ERC721.sol will show that the transferee now owns that NFT but Soulmate.sol will still show that that NFT is owned by the transferor.

Tools Used

Manual review

Recommendations

Use the variables implemented by ERC721.sol to track who owns what NFT and how many there are