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

Cally Protocol Does Not Support Cryptopunk or Cryptokitties Tokens #207

Open
code423n4 opened this issue May 14, 2022 · 2 comments
Open

Cally Protocol Does Not Support Cryptopunk or Cryptokitties Tokens #207

code423n4 opened this issue May 14, 2022 · 2 comments
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L198-L200

Vulnerability details

Impact

While the Cally.sol contract is compatible with standard ERC20 and ERC721 tokens, it is not able to handle popular non-standard ERC721 tokens such as cryptopunks. The typical transferFrom() call will fail as a result, and these users will likely opt to use another protocol which does support options on their specific NFT.

Recommended Mitigation Steps

NFTX protocol has implemented a way to handle the transfer of both standard and non-standard ERC721 tokens. The relevant implementation can be found here. The solution provided also utilises OpenZeppelin's safeTransferFrom() function on most transfers as this ensures the function reverts on failure.
@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 labels May 14, 2022
code423n4 added a commit that referenced this issue May 14, 2022
@code423n4
BondiPestControl issue #207
d6e38de
@outdoteth outdoteth added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label May 15, 2022
@outdoteth
Copy link
Collaborator

Technically correct but we have no intention to support these tokens. users can use those tokens by getting a wrapped version of them that conforms to the erc721 spec

@outdoteth outdoteth added the disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) label May 15, 2022
@HardlyDifficult
Copy link
Collaborator

This is a common issue when working with NFTs. Wrappers, native support, or simply not supporting these non-standard tokens are reasonable courses. Lowing to 1 (Low) and 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 May 22, 2022
This was referenced Dec 17, 2022
Open
Closed
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 disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@HardlyDifficult @outdoteth @code423n4