Use safeTransferFrom for ERC721

[code423n4]

Lines of code

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

Vulnerability details

Use safeTransferFrom for ERC721

In the contract Cally.sol every transfer of ERC721 are done with the transferFrom() instead of the recommended safeTransferFrom(). This transferFrom() does not check, whether the receiver is capable of proper handling of NFTs.

Impact

If the buyers of a call option with a ERC721 collateral is a smart contract (could be a multisig wallet) which does not implement the onERC721Received and do not have proper handling of ERC721 tokens, then the NFT would be lost if the option is exercised.

Proof of concept

1. Alice creates a vault with a BAYC NFT

2. SomeDAO choose to buy the call option from their multisig wallet

3. SomeDAO exercises the option when there is a change of making a profit

4. The BAYC NFT is transferred to SomeDAO’s multisig wallet, but by a mistake the wallet does not support handling of ERC721 tokens.

5. SomeDAO have no way of retrieving the BAYC NFT, and has hence paid for a NFT they cannot get.

Even though the scenario is unlikely, and it would be the users own fault, there is no doubt that when it requires so small an effort, it makes sense to make it impossible to do this rooky mistake.

A similar issue was also scored as a medium see here.

The relevant code:

L199, L295, and L344.

Recommended mitigation

It is recommended to use the safe version safeTransferFrom() since this will require the receiving contract to implement IERC721Receiver.onERC721Received

[outdoteth]

use safeTransferFrom to prevent stuck NFTs: #38