Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tokenExists ==> cardExists #9

Open
code423n4 opened this issue Aug 21, 2021 · 1 comment
Open

tokenExists ==> cardExists #9

code423n4 opened this issue Aug 21, 2021 · 1 comment

Comments

@code423n4
Copy link
Contributor

Handle

gpersoon

Vulnerability details

Impact

In previous versions of the code, cards where referred to as tokens.
There is still one trace left of this old name, in the function: tokenExists

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) {
...

Tools Used

Recommended Mitigation Steps

Rename the function tokenExists to something like: cardExists

@code423n4 code423n4 added bug Something isn't working 0 (Non-critical) labels Aug 21, 2021
code423n4 added a commit that referenced this issue Aug 21, 2021
@Splidge
Copy link
Collaborator

Splidge commented Aug 24, 2021

Yes, this is a problem we have.
Originally only the name 'Token' was used everywhere. However when we moved over to Matic we started using ERC20 tokens and so the decision was made to call the new ERC20s 'Tokens' and the existing NFTs changed to 'Cards'. This worked well enough until we started using the OpenZeppelin ERC721 library and integrated with Matic Mintable which both refer to the NFTs as 'Tokens'. So for a while we ended up using card and token interchangeably for the NFTs which wasn't ideal.
Further complicating things is that the NFTs all have a unique tokenID, but the markets refer to them using a non-unique zero index. The decision was then made to differentiate between them by calling the unique ID the 'tokenID' and the non-unique ID the 'cardID'.
The name card was almost removed entirely and the tokenID used throughout, however the introduction of the leaderboard and being able to mint copies of the NFTs changed this token to card relationship from one to one, to many to one. So it seemed sensible to have a separate cardID in the market which is associated with several tokenIDs in the NFT hub.

So right now we are calling both ERC20 and ERC721 'Tokens' and when referring to the non-unique market specific version they are 'Cards'.

Saying all that, I believe that the function tokenExists is correctly named because it is checking if the primary token for that card has been minted yet. i.e. does the token (or NFT) exist? However there are still inconsistencies and this naming should be more clearly stated so I'll mark this as acknowledged.

P.S. for further fun in naming try getting out of different RC team members if it's called a 'market' or an 'event'? or maybe the cards/tokens/NFTs are now 'outcomes'!? 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants