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

Recommend using safeTransferFrom() instead of transferFrom() for NFTs #318

Closed
HardlyDifficult opened this issue May 29, 2022 · 2 comments
Closed
Labels
invalid This doesn't seem right

Comments

@HardlyDifficult
Copy link
Collaborator

From MiloTruck in #131

Recommend using safeTransferFrom() instead of transferFrom() for NFTs
The EIP-721 standard states:

/// @notice Transfer ownership of an NFT -- THE CALLER IS RESPONSIBLE
///  TO CONFIRM THAT `_to` IS CAPABLE OF RECEIVING NFTS OR ELSE
///  THEY MAY BE PERMANENTLY LOST

Cally.sol uses transferFrom() to facillitate the transfer of ERC721 NFTs from the contract to users, as shown below.

In exercise():

Cally.sol:295 ? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount)
In withdraw():

Cally.sol:344 ? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount)
To prevent a permanent loss of NFTs, there should be checks in place to ensure msg.sender is capable of receiving NFTs. Otherwise, consider using safeTransferFrom() instead of transferFrom().

@HardlyDifficult HardlyDifficult added bug Something isn't working duplicate This issue or pull request already exists 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels May 29, 2022
@HardlyDifficult
Copy link
Collaborator Author

Dupe of #38

@JeeberC4 JeeberC4 added invalid This doesn't seem right and removed bug Something isn't working duplicate This issue or pull request already exists 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jun 6, 2022
@JeeberC4
Copy link
Contributor

JeeberC4 commented Jun 6, 2022

Issue recreated with script that includes all required data.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants