code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L693-L712

Vulnerability details

Impact

validation check for gobblerId is missing for other case inside the function tokenURI.

Proof of Concept

https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L695-L699

function tokenURI(uint256 gobblerId) public view virtual override returns (string memory) {
    // Between 0 and lastRevealed are revealed normal gobblers.
    if (gobblerId <= gobblerRevealsData.lastRevealedId) {
        if (gobblerId == 0) revert("NOT_MINTED"); // 0 is not a valid id for Art Gobblers.

        return string.concat(BASE_URI, uint256(getGobblerData[gobblerId].idx).toString());
    }

    // Between lastRevealed + 1 and currentNonLegendaryId are minted but not revealed.
    if (gobblerId <= currentNonLegendaryId) return UNREVEALED_URI;

    // Between currentNonLegendaryId and FIRST_LEGENDARY_GOBBLER_ID are unminted.
    if (gobblerId < FIRST_LEGENDARY_GOBBLER_ID) revert("NOT_MINTED");

    // Between FIRST_LEGENDARY_GOBBLER_ID and FIRST_LEGENDARY_GOBBLER_ID + numSold are minted legendaries.
    if (gobblerId < FIRST_LEGENDARY_GOBBLER_ID + legendaryGobblerAuctionData.numSold)
        return string.concat(BASE_URI, gobblerId.toString());

if (gobblerId == 0) revert("NOT_MINTED"); check is done only for if (gobblerId <= gobblerRevealsData.lastRevealedId) and for other's it does not.

Tools Used

VS code

Recommended Mitigation Steps

Add the validation if (gobblerId == 0) revert("NOT_MINTED"); at the start of the function.

Shungy commented 1 year ago

if (gobblerId == 0) revert("NOT_MINTED"); check is done only for if (gobblerId <= gobblerRevealsData.lastRevealedId) and for other's it does not.

Zero is always inclusive in that greater or equal check. So the path is never missed.

This saves gas as opposed to it being executed every time. That is why it is not its own statement at the top.

GalloDaSballo commented 1 year ago

The zero check is present: https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L696

code-423n4 / 2022-09-artgobblers-findings