code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df42/contracts/NFTDropCollection.sol#L195-L201

Vulnerability details

Impact

postRevealBaseURIHash in NFTDropCollection contract is never checked against hash of the revealed baseURI. Also this storage variable can be updated by the creator/admin at anytime before the reveal. The arbitrary update of this parameter would defeat having an immutable baseURI value post reveal (commit/reveal scheme), if that was what the devs intended. Another thing to note is that the commit/reveal schemes for baseURIs cannot really work content-wise. Since the baseURI servers can share different contents at will for off-chain hosted content.

In the current code base postRevealBaseURIHash is only used to check if certain operations/methods are valid to perform based on the pre/post reveal status which is determined by the nullity of postRevealBaseURIHash using the onlyWhileUnrevealed modifier.

Proof of Concept

An admin calls reveal (line 195) with a _baseURI which its hash does not equal postRevealBaseURIHash. Since there is no check, the baseURI gets updated.

Tools Used

N/A

Recommended Mitigation Steps

Add a hash check in the reveal method and also a new Error.

error NFTDropCollection_REVEAL_BASE_URI_HASH_DOES_NOT_MATCH();

...

function reveal(string calldata _baseURI) external onlyAdmin validBaseURI(_baseURI) onlyWhileUnrevealed {
    if(postRevealBaseURIHash != keccak256(abi.encodePacked(_baseURI))) {
        revert NFTDropCollection_REVEAL_BASE_URI_HASH_DOES_NOT_MATCH();
    }
    // `postRevealBaseURIHash` == 0 indicates that the collection has been revealed.
    delete postRevealBaseURIHash;

    // Set the new base URI.
    baseURI = _baseURI;
    emit URIUpdated(_baseURI, "");
}