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.

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

There is no Support For The Trading of Cryptopunks #248

Open
code423n4 opened this issue Sep 19, 2022 · 3 comments
Open

There is no Support For The Trading of Cryptopunks #248

code423n4 opened this issue Sep 19, 2022 · 3 comments
Labels
bug Something isn't working old-submission-method QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/BuyCrowdfundBase.sol#L137

Vulnerability details

Impact

Cryptopunks are at the core of the NFT ecosystem. As one of the first NFTs, it embodies the culture of NFT marketplaces. By not supporting the trading of cryptopunks, Foundation is at a severe disadvantage when compared to other marketplaces. Cryptopunks have their own internal marketplace which allows users to trade their NFTs to other users. As such, cryptopunks does not adhere to the ERC721 standard, it will always fail when the protocol attempts to trade them.

Proof of concept

Here is an example implementation of what it might look like to integrate cryptopunks into the Foundation protocol.

Tools Used

Manual review

Recommended Mitigation Steps

Consider designing a wrapper contract for cryptopunks to facilitate standard ERC721 transfers. The logic should be abstracted away from the user such that their user experience is not impacted.

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working old-submission-method labels Sep 19, 2022
code423n4 added a commit that referenced this issue Sep 19, 2022
@merklejerk
Copy link
Collaborator

Don't consider this a vulnerability. The protocol is only designed to work with ERC721s. The FE will only suggest ERC721s to crowdfund on.

@merklejerk merklejerk added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Sep 22, 2022
@HardlyDifficult
Copy link
Collaborator

At least change the name of the protocol when copy pasting another report 🙏

This is a NC feature suggestion, converting into a QA report for the warden.

@HardlyDifficult HardlyDifficult added 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 Oct 4, 2022
@HardlyDifficult
Copy link
Collaborator

Merging with #249

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working old-submission-method QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants