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

totalNftMintCount can be replaced with ERC721 totalSupply() #134

Open
code423n4 opened this issue Jun 16, 2021 · 2 comments
Open

totalNftMintCount can be replaced with ERC721 totalSupply() #134

code423n4 opened this issue Jun 16, 2021 · 2 comments

Comments

@code423n4
Copy link
Contributor

Handle

pauliax

Vulnerability details

Impact

I can't find a reason why totalNftMintCount in Factory can't be replaced with ERC721 totalSupply() to make it less error-prone. As nfthub.mint issues a new token it should automatically increment totalSupply and this assignment won't be needed:
totalNftMintCount = totalNftMintCount + _tokenURIs.length;
Also in function setNftHubAddress you need to manually set _newNftMintCount if you want to change nfthub so an invalid value may crash the system. totalSupply() will eliminate totalNftMintCount and make the system more robust.

Recommended Mitigation Steps

Replace totalNftMintCount with nfthub totalSupply() in Factory contract.

@code423n4 code423n4 added 1 (Low Risk) bug Something isn't working labels Jun 16, 2021
code423n4 added a commit that referenced this issue Jun 16, 2021
@mcplums
Copy link
Collaborator

mcplums commented Jun 17, 2021

This is a good one, would indeed make it less error prone

@Splidge
Copy link
Collaborator

Splidge commented Jun 21, 2021

Sometimes what looks like a small fix just takes you all the way down the rabbithole, this was one of them. totalSupply() isn't included by default so I had to import the Enumerable extension.
fixed here

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

3 participants