ERC721 Underlying Can Be Stuck in Unsupported Contract

[code423n4]

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#L344

Vulnerability details

Impact

The underlying asset can be transferd out to the caller (msg.sender) using exercise() or withdraw() to the caller indicated by msg.sender. If the underlying asset is an ERC721 token, and the msg.sender is a contract address that does not support ERC721, the underlying asset can be stucked in the contract.

As proposed in the EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

Proof of Concept

This can be indicated manually by the use of ERC721.transferFrom() instead of ERC721.safeTransferFrom(), which includes a safety check for ERC721 support on the destination address.

Tool Used

Manual review

Recommended Mitigation Steps

Use ERC721.safeTransferFrom() instead which includes a safety check for ERC721 support on the destination address.

[outdoteth]

use safeTransferFrom to prevent stuck NFTs: #38