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 IERC721 transfers #39

Closed
code423n4 opened this issue Nov 10, 2022 · 1 comment
Closed
Labels
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 unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelERC20Transfer.sol#L24

Vulnerability details

Impact

The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:

OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible
Given that any NFT can be used for the call option, there are a few NFTs that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()

Proof of Concept

code-423n4/2022-05-cally-findings#38 (comment)

Tools Used

Manual

Recommended Mitigation Steps

Call the safeTransferFrom() method instead of transferFrom() for NFT transfers. Note that the looksrare contract should inherit the ERC721TokenReceiver contract as a consequence.

@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 Nov 10, 2022
code423n4 added a commit that referenced this issue Nov 10, 2022
@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Nov 21, 2022
@c4-judge
Copy link
Contributor

Picodes marked the issue as unsatisfactory:
Invalid

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
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 unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants