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

Set base URI on reveal #19

Closed
wants to merge 1 commit into from
Closed

Set base URI on reveal #19

wants to merge 1 commit into from

Conversation

aalmada
Copy link

@aalmada aalmada commented Dec 2, 2021

Setting the base URI in the constructor allows the hackers to find the location of the unrevealed metadata. They can call baseURI() or look for the deployment transaction. This lets them find which are the rarest tokens and are also able to steal the artwork.

This PR changes the behavior to set the base at the reveal moment. It changes the reveal() method to take the base URI as a parameter and only allows calling setBaseURI() when revealed is true.

It also moves the check for token existence in tokenURI() to make it somewhat harder for hackers to find what tokens have already been minted before the reveal.

@laygir
Copy link

laygir commented Dec 27, 2021

I think the _baseURI() is an internal function, not sure if it can be called from outside. But not an expert here, would love to learn how it can be called or how to figure out the baseURI from the deployment transaction 👀

Though, I'm having similar concerns and I think the problem is the following line

shouldn't that be string baseURI;

as it used to be here

https://github.com/HashLips/hashlips_nft_contract/blob/975d6d60454f9de9ba4aa60914cca63c6e145924/contract/SimpleNft.sol#L24

@aalmada
Copy link
Author

aalmada commented Jan 1, 2022

@laygir Being able to change the base URI is a feature defined by @HashLips. This PR does not change it and this feature it's rightfully questioned in #20.

Adding public makes the compiler generate a read-only method with the same name as the variable. In this case baseURI(). This simply allows getting the current value. It can be useful when developing a frontend to interact with the contract.

@HashLips added a setBaseURI() that allows changing the value of baseURI:

function setBaseURI(string memory _newBaseURI) public onlyOwner {

The _baseURI() is overridden to return baseURI:

function _baseURI() internal view virtual override returns (string memory) {

This is later used in tokenURI() method definition:

string memory currentBaseURI = _baseURI();

This PR only proposes setting the "revealed" URI only when reveal() is called so that hackers don't get early clues of the metadata location.

@aalmada aalmada closed this by deleting the head repository Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants