Funds may be stuck when using a contract

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L511

Vulnerability details

Impact

In the EIP721, onERC721Received is a safeguard to avoid NFTs to be stuck on contracts. But here it could turn out to actually create this situation: it may happen that the NFTs are stuck on PuttyV2 if a user uses a contract not suited to receive NFTs to create and fill orders.

Proof of Concept

An example scenario would be:

• Alice uses a contract to create a short put option.
• Bob fills the order. A long NFT is minted to Alice's contract but there is no check
• Bob exercise the option
• Alice cannot withdraw: here contract cannot receive NFTs

Recommended Mitigation Steps

Check in the appropriate case if the maker or msg.sender is a contract and if it is the case, if they can receive NFTs

[GalloDaSballo]

The finding implies that the caller is a contract, and that the contract was programmed to handle all operations but somehow wouldn't be able to handle the NFTs

[outdoteth]

echo what @GalloDaSballo said. It's unlikely a contract will have all the setup required to interact with PuttyV2 but not be able to handle ERC721 tokens. Adding a check via safeMint adds a gas overhead as well as another re-entrancy attack vector so there is a tradeoff.

[outdoteth]

Duplicate: Contracts that can’t handle ERC721 tokens will lose their Putty ERC721 position tokens: https://github.com/code-423n4/2022-06-putty-findings/issues/327