search

code-423n4 / 2022-05-cally-findings

2 stars 0 forks source link

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

Closed HardlyDifficult closed 2 years ago

HardlyDifficult commented 2 years ago

From MiloTruck in https://github.com/code-423n4/2022-05-cally-findings/issues/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 commented 2 years ago

Dupe of https://github.com/code-423n4/2022-05-cally-findings/issues/38

JeeberC4 commented 2 years ago

Issue recreated with script that includes all required data.