c4-bot-10 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/BridgedERC721.sol#L107

Vulnerability details

Impact

The BridgedERC721.sol contract is the bridged representation (on chain B) of a canonical token (on chain A). The issue is that the overriden tokenURI() function in the BridgedERC721 contract does not check whether the _tokenId passed as parameter is a valid NFT or not.

Due to this, the ERC721 specification is not conformed to since it requires the tokenURI() function throw/revert if _tokenId is not a valid NFT.

    /// @notice A distinct Uniform Resource Identifier (URI) for a given asset.
    /// @dev Throws if `_tokenId` is not a valid NFT. URIs are defined in RFC
    ///  3986. The URI may point to a JSON file that conforms to the "ERC721
    ///  Metadata JSON Schema".
    function tokenURI(uint256 _tokenId) external view returns (string);

It is essential for tokens created on the Bridge level to conform to EIP specifications in order to not break composability with other smart contracts and consumers such as digital marketplaces, block explorers etc. In our case, the tokenURI() function would build and return an EIP-681 URI for a non-existing tokenId, which could be consumed by external applications that require the Bridged token to be EIP-721 compliant.

Proof of Concept

Below, we can see the difference between the original tokenURI() function that is overriden by the BridgedERC721 contract. On Line 99 in the first snippet, we can see that token existence check is made but when the function was overriden in the second snippet, the check was not applied.

ERC721 tokenURI() function:

File: ERC721Upgradeable.sol
098:     function tokenURI(uint256 tokenId) public view virtual override returns (string memory) {
099:         _requireMinted(tokenId);
100: 
101:         string memory baseURI = _baseURI();
102:         return bytes(baseURI).length > 0 ? string(abi.encodePacked(baseURI, tokenId.toString())) : "";
103:     }

BridgedERC721 tokenURI() overriden function:

File: BridgedERC721.sol
110:     function tokenURI(uint256 _tokenId) public view virtual override returns (string memory) {
111:         return string(
112:             abi.encodePacked(
113:                 LibBridgedToken.buildURI(srcToken, srcChainId), Strings.toString(_tokenId)
114:             )
115:         );
116:     }

Tools Used

Manual Review

Recommended Mitigation Steps

At the start of the function, call the function _requireMinted() to check if the tokenId exists or not.

Assessed type

Other

c4-pre-sort commented 8 months ago

minhquanym marked the issue as primary issue

c4-pre-sort commented 8 months ago

minhquanym marked the issue as sufficient quality report

adaki2004 commented 7 months ago

I'd say technically it is possible, but it is sucha. niche edge-caee, I'd dispute or maybe best acknowldge as the sentence holds true.

Example: So Bob bridges his NFT A. Then it means this NFT A do exists on chain X, so we are not creating NFT from "thin air".

Also it is possible that for example this vaulted NFT A on canonical chain burnt (because a deployer for example with custom erc721 contract does it with special allowance).

In that case the statement above hold true, on the other hand what kind of contract/procedure it is ? How likely it will happen ? And also, we cannot make our contracts 100% fool-proof. There are multiple ways people could stuck ether/funds/erc20/erc721 assets in our protocol, this is simply unavoidable, and cannot be prepared for all edge cases.

Each individual shall be well aware, if a malicious owner can mint custodied (not onwed) NFTs on behalf of others.

c4-sponsor commented 7 months ago

adaki2004 (sponsor) acknowledged

c4-sponsor commented 7 months ago

adaki2004 marked the issue as disagree with severity

0xean commented 7 months ago

QA (Quality Assurance) Includes both Non-critical (code style, clarity, syntax, versioning, off-chain monitoring (events, etc) and Low risk (e.g. assets are not at risk: state handling, function incorrect as to spec, issues with comments). Excludes Gas optimizations, which are submitted and judged separately.

very clearly function incorrect as to spec... None of the duplicates show a clear impact beyond that. QA is most appropriate.

c4-judge commented 7 months ago

0xean changed the severity to QA (Quality Assurance)