search

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

2 stars 0 forks source link

Use ERC721.safertransferFrom instead of ERC721.transferFrom when you send out NFT #300

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L295 https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L431-L453

Vulnerability details

Impact

Buyers can accidentally lose their NFT if they send to incorrect address.

Proof of Concept

When the buyer decide to call exercise the NFT is transfered using transferFrom. This is risky because if the destination (msg.sender) is a contract and it is unable to handle NFT then it will be locked forever. This function in my opinion is Ok when you transfer to your contract. 

  vault.tokenType == TokenType.ERC721
  ? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount)
  : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);

Similar issues

https://github.com/code-423n4/2022-04-backed-findings/issues/83

Recommended Mitigation Steps

Implement safeTransferFrom

outdoteth commented 2 years ago

use safeTransferFrom to prevent stuck NFTs: https://github.com/code-423n4/2022-05-cally-findings/issues/38