hats-finance / Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad

4 stars 4 forks source link

OTAP contract doesn't comply with EIP-721 standard #9

Open hats-bug-reporter[bot] opened 6 months ago

hats-bug-reporter[bot] commented 6 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x8a7ca528c43b47fc92a615819e8d6013a8b69d760c87cfaaa7be34b39f75b88d Severity: medium

Description: Description\ According to the EIP-721 standard, the tokenURI function must revert if a non-existent tokenId is passed. In OTAP contract, this requirement is ignored. This leads to violation of the EIP-712 spec

OTAP.sol: 

    function tokenURI(uint256 tokenId) public view override(ERC721, ERC721NftLoader) returns (string memory) {
        return ERC721NftLoader.tokenURI(tokenId);
    }

ERC721NftLoader.sol:

/**
     * @notice Returns the token URI for a given token ID. If the NFT loader contract is not set, it returns an empty string.
     * @inheritdoc ERC721
     */
    function tokenURI(uint256 tokenId) public view virtual override returns (string memory) {
        if (address(nftLoader) == address(0)) {
            return "";
        }
        return INFTLoader(nftLoader).tokenURI(tokenId);
    }

Similar issue can be seen here: https://github.com/code-423n4/2023-04-caviar-findings/issues/44

Recommendation:

Add a token existence check

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

0xRektora commented 5 months ago

Even though it was marked as medium previously on C4, it shouldn't have been the case. We deem this as a low.

0xRektora commented 5 months ago

Marking this as invalid.

Blngogo commented 5 months ago

Aaa just saw the tweet with low severity winners, then the fix with my issue invalidated :( . All the best though, wishing you a successful project.