OwnershipNFTs do not comply with ERC721, breaking composability

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/OwnershipNFTs.sol#L13

Vulnerability details

OwnershipNFTs within the contract deviate from the ERC721 standard, leading to issues with composability and interoperability. Following are the missing things which are mandated by the ERC721 specification:

• transfer() Function: The ERC721 standard specifies that a transfer to address(0) must revert, as it would destroy the token and lead to issues with tracking asset ownership. Failing to handle this can break the lifecycle of the NFT and introduce bugs or loss of tokens.

• Missing Events: Transfer and approval events are critical for tracking changes in ownership and enabling third-party systems to respond to changes in asset state.

• balanceOf() Function: This function should revert if queried for an invalid owner address (address(0)).

• ownerOf() Function: This function should revert if called with a token ID not owned by any address or if the owner is address(0).

• getApproved() Function: The function should throw an error if the queried _tokenId is not valid (i.e., the token does not exist).

Impact

The non-compliance with ERC721 results in Breaking Composability and also

• Potential Loss of Funds: Since tokens can be transferred to address(0) without reverts, this could result in permanent loss of NFTs. (This could be user error but here it is protocol mistake for not adhering the standard).

• Lack of Traceability: Missing events make it difficult to track token transfers and approvals, which can hamper monitoring systems monitoring malicious activities.

Proof of Concept

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/OwnershipNFTs.sol#L13

Tools Used

Manual Review

Recommended Mitigation Steps

Align OwnershipNFT contract with the ERC721 standard.

Assessed type

ERC721

[af-afk]

I don't believe we made compatibility with ERC721 a goal of the contest, but we'll fix this anyway.

[alex-ppg]

The submission and its duplicates detail various deviancies from the EIP-721 standard that are not imperative to the functionality of the NFT itself.

The contract does not indicate that it must be strictly compliant with the standard, and the documentation of the project does not indicate so.

As such, I consider all submissions of this duplicate set to be QA recommendations as to how the contract can become compliant.

To note, submission #55 was considered a proper vulnerability due to how the presence of the code itself solely serves the purpose of satisfying the EIP-721 callback mechanism rather than being a generic NFT feature.

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-c

[c4-judge]

This previously downgraded issue has been upgraded by alex-ppg

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-b