Skip to content
New issue

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

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

Already on GitHub? # to your account

Use safeTransferFrom instead of transferFrom for ERC721 transfers #330

Closed
code423n4 opened this issue Sep 27, 2022 · 3 comments
Closed

Use safeTransferFrom instead of transferFrom for ERC721 transfers #330

code423n4 opened this issue Sep 27, 2022 · 3 comments
Labels
bug Something isn't working duplicate This issue or pull request already exists QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L748
https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/utils/GobblerReserve.sol#L38

Vulnerability details

Proof of Concept

It is good to add a require() statement that checks the return value of token transfers or to use something like solmate safeTransferFrom(), unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

The same case in : code-423n4/2022-05-cally-findings#38

Tools Used

Manual Review

Recommended Mitigation Steps

It is usually good to add a require-statement that checks the return value or to use something like safeTransferFrom; unless one is sure the given token reverts in case of a failure and Call the safeTransferFrom() method instead of transferFrom() for NFT transfers.Note that the ArtGobblers.sol contract should inherit the ERC721Receiver contract as a consequence but i didn't found any of them on solmate .

@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 Sep 27, 2022
code423n4 added a commit that referenced this issue Sep 27, 2022
@Shungy
Copy link
Member

Shungy commented Sep 28, 2022

This is similar to cally one but I don't think they are the same. Cally one can result in stealing valuable NFTs. (nonetheless I think cally one was also judged too high severity).
Here, gobbling just increases a counter that has no effect in other function logic. If a non-standard NFT contract causes the counter to increase without actually transferring, then it is likely that NFT address is not worthy of whitelisting in the UI to show in the belly gallery.

@GalloDaSballo GalloDaSballo added the duplicate This issue or pull request already exists label Sep 29, 2022
@GalloDaSballo
Copy link
Collaborator

Dup of #322

@GalloDaSballo GalloDaSballo 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 11, 2022
@GalloDaSballo
Copy link
Collaborator

L

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working duplicate This issue or pull request already exists QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

3 participants