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.
af-afk
commented
4 weeks ago
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
_tokenIdis 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