ERC721 callback not called when sending token to user.

[code423n4]

Lines of code

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

Vulnerability details

Impact

NFT can get trapped in user contract because of some mistakes.

Proof of Concept

In exercise function, when the contract is transferring NFT to user contract, it use transferFrom instead of safeTransferFrom which skips the check of receiver contract is willing to receive NFT. This might lead to NFT permanently locked in a contract.

There has been debates about whether to use transferFrom or safeTransferFrom, but I personally think it's always better to use safeTransferFrom: if the user use a contract wallet without the receiving callback, he can always transfer the optionToken to another EOA address and exercise; But, if there's a mistake and a contract accidentally receive and NFT, it will get locked forever.

Recommended Mitigation Steps

consider using safeTransferFrom (and safeMint), otherwise explicitly state it somewhere in the code or document that it doesn't fully complied with ERC721.

[outdoteth]

use safeTransferFrom to prevent stuck NFTs: #38