Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OwnershipNFTs.sol is not ERC721 compliant, doesnt implement the ERC165 interface. #161

Open
howlbot-integration bot opened this issue Sep 16, 2024 · 5 comments
Open

OwnershipNFTs.sol is not ERC721 compliant, doesnt implement the ERC165 interface. #161

howlbot-integration bot opened this issue Sep 16, 2024 · 5 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-49 edited-by-warden grade-b Q-06 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_56_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

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

Vulnerability details

Bug Description

The contract OwnershipNFTs is supposed to be an ERC721 standard contract but it lacks the ERC165's supportInterface() function which is mandatory for all erc721 contracts as stated by the eip-721

Every ERC-721 compliant contract must implement the ERC721 and ERC165 interfaces (subject to “caveats” below):
https://eips.ethereum.org/EIPS/eip-721#specification

Lack of the supportInterface() function will cause issues for external contracts that interact with the OwnershipNFTs.sol as they will be unable to verify the behaviour of the OwnershipNFTs i.e if it does indeed support ERC721 functions.

Further reading

Solidity issue #3419: A contract that implements ERC721Metadata or ERC721Enumerable SHALL also implement ERC721. ERC-721 implements the requirements of interface ERC-165

Impact

OwnershipNFTs.sol is not ERC-721 compliant as it doesnt implement the ERC165 interface.

Tools Used

Manual review

Recommended Mitigation Steps

Implement the ERC165 interface in OwnershipNFTs.sol

Assessed type

ERC721
@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_56_group AI based duplicate group recommendation bug Something isn't working duplicate-49 edited-by-warden sufficient quality report This report is of sufficient quality labels Sep 16, 2024
howlbot-integration bot added a commit that referenced this issue Sep 16, 2024
@howlbot-integration
adeolu issue #161
70dbc40
@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Sep 23, 2024
@c4-judge
Copy link
Contributor

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

@c4-judge
Copy link
Contributor

alex-ppg marked the issue as grade-c

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Sep 23, 2024
@ZanyBonzy ZanyBonzy mentioned this issue Sep 25, 2024
Open
@c4-judge
Copy link
Contributor

c4-judge commented Oct 7, 2024

This previously downgraded issue has been upgraded by alex-ppg

@c4-judge c4-judge reopened this Oct 7, 2024
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Oct 7, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 7, 2024

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

@c4-judge c4-judge closed this as completed Oct 7, 2024
@c4-judge c4-judge added grade-b and removed grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Oct 7, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 7, 2024

alex-ppg marked the issue as grade-b

@C4-Staff C4-Staff reopened this Oct 7, 2024
@C4-Staff C4-Staff added the Q-06 label Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-49 edited-by-warden grade-b Q-06 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_56_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@c4-judge @C4-Staff