Update CeloNFT.sol #2
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The createNFT() function is not marked as internal. This means that anyone can call this function, even though it should only be called internally by the contract. This could be a security risk, as it would allow malicious users to create NFTs and set their prices arbitrarily. The sellNFT() function does not check if the sender actually owns the NFT they are trying to sell. This means that anyone could call this function to sell an NFT that they don't own, which could lead to fraud. The allNfts() function returns an array of all NFTs, even those that have already been sold. This could be confusing for users, as it would make it look like they can still purchase NFTs that are no longer available. The userNfts() function is not very efficient. It iterates over all NFTs and checks if each one is owned by the sender. This could be slow for contracts with a large number of NFTs. Here are some suggestions for fixing these bugs and errors: Mark the createNFT() function as internal. This will prevent anyone from calling this function except for other functions within the contract. Add a check to the sellNFT() function to make sure that the sender actually owns the NFT they are trying to sell. This can be done by using the ownerOf() function from the ERC721 contract. Only return NFTs that have not yet been sold in the allNfts() function. This can be done by checking the sold field of each NFT. Use a more efficient algorithm to retrieve all NFTs owned by a user in the userNfts() function. One way to do this is to use a mapping to track which NFTs are owned by each user. In addition to the above, I would also recommend adding some additional security checks to the contract, such as: Check the msg.value in the sellNFT() function to make sure that it is equal to the price of the NFT. This will prevent users from sending too much or too little money when purchasing an NFT. Check the return value of the call() function in the sellNFT() function to make sure that the payment to the seller was successful. This will prevent fraud in case the seller's address is no longer valid.
|
Someone is attempting to deploy a commit to a Personal Account owned by @Oladayo-Ahmod on Vercel. @Oladayo-Ahmod first needs to authorize it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The createNFT() function is not marked as internal. This means that anyone can call this function, even though it should only be called internally by the contract. This could be a security risk, as it would allow malicious users to create NFTs and set their prices arbitrarily. The sellNFT() function does not check if the sender actually owns the NFT they are trying to sell. This means that anyone could call this function to sell an NFT that they don't own, which could lead to fraud. The allNfts() function returns an array of all NFTs, even those that have already been sold. This could be confusing for users, as it would make it look like they can still purchase NFTs that are no longer available. The userNfts() function is not very efficient. It iterates over all NFTs and checks if each one is owned by the sender. This could be slow for contracts with a large number of NFTs.
Here are some suggestions for fixing these bugs and errors:
Mark the createNFT() function as internal. This will prevent anyone from calling this function except for other functions within the contract. Add a check to the sellNFT() function to make sure that the sender actually owns the NFT they are trying to sell. This can be done by using the ownerOf() function from the ERC721 contract. Only return NFTs that have not yet been sold in the allNfts() function. This can be done by checking the sold field of each NFT. Use a more efficient algorithm to retrieve all NFTs owned by a user in the userNfts() function. One way to do this is to use a mapping to track which NFTs are owned by each user.
In addition to the above, I would also recommend adding some additional security checks to the contract, such as:
Check the msg.value in the sellNFT() function to make sure that it is equal to the price of the NFT. This will prevent users from sending too much or too little money when purchasing an NFT. Check the return value of the call() function in the sellNFT() function to make sure that the payment to the seller was successful. This will prevent fraud in case the seller's address is no longer valid.