If users don’t handle ERC721 received, the ERC721 token will be frozen

[code423n4]

Lines of code

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

Impact

It doesn't check whether users will handle ERC721 received. If a user is a contract but doesn’t handle ERC721 received, the ERC721 token will be frozen when receiving ERC721 tokens.

Proof of Concept

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

It doesn’t check whether the receiver has implemented the onERC721Received function. If a user buys an option but the user doesn't handle ERC721 received, the ERC721(vault.token) will be frozen after the user exercises the option.

Tools Used

vim

Recommended Mitigation Steps

Use safeTransferFrom rather than transferFrom when transferring ERC721 tokens to users.

[outdoteth]

use safeTransferFrom to prevent stuck NFTs: #38