safer implementation of tokenExists

[code423n4]

Handle

gpersoon

Vulnerability details

Impact

The function tokenExists does only limited checks on the existence of cards.
It doesn't doublecheck that tokenIds[_card] != 0
This is relevant because 0 is the default value of empty array elements. Although this isn't a problem in the current code,
future changes might accidentally introduce vulnerabilities.

Also cards are only valid if they are below numberOfCards. This has led to vulnerabilities in previous versions of the contract
(e.g. previous contest)

Proof of Concept

// https://github.com/code-423n4/2021-08-realitycards/blob/main/contracts/RCMarket.sol#L1139
function tokenExists(uint256 _card) internal view returns (bool) {
return tokenIds[_card] != type(uint256).max;
}

Tools Used

Recommended Mitigation Steps

Change the function to something like the following:

function tokenExists(uint256 _card) internal view returns (bool) {
if (_cardId >= numberOfCards) return false;
if (tokenIds[_card] == 0) return false;
return tokenIds[_card] != type(uint256).max;
}

[Splidge]

Obviously I didn't learn my lesson from the last contest, I've added in the check that the card Id is less than the numberOfCards.

However, I'm reluctant to disallow the use of tokenId 0. This would mean that the first NFT we mint wouldn't be usable in a market, so we would need to create a dead market just to get rid of that first NFT. All things going well the first NFT minted could end up being a valuable one, who knows..? 🚀
An alternative would be for the factory to start minting from index 1 but this would mean instead of using the totalSupply() as the next token to mint, we would need to use totalSupply() + 1 , I'm unsure how this would affect integration with services such as opensea where they may expect the first NFT to have index 0, I feel like at this late stage it's too much of a change that could introduce other issues later, and so for now I'll not be adding if (tokenIds[_card] == 0) return false;

[Splidge]

Fix to the check that the _card is a valid card number added in this commit