Use safeTransferFrom instead of transferFrom for ERC721 transfers

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/VirtualAccount.sol#L36-L37

Vulnerability details

Impact

In the function withdrawERC721 of contract VirtualAccount, when transferring ERC721 tokens to the recipient, the transferFrom keyword is used instead of safeTransferFrom. If the recipient is a contract and is not capable of handling ERC721 tokens, the sent tokens could be locked.

Proof of Concept

When the function transferFrom is used for transferring ERC721 token , there is no way of checking if the recipient address has capability to handle incoming ERC721 token. If there is no checking, the transaction will push through and the ERC721 token will be locked in the recipient contract address.
https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/VirtualAccount.sol#L36-L37

Similar issue:
code-423n4/2022-05-cally-findings#38

Tools Used

Manual Review

Recommended Mitigation Steps

Call the safeTransferFrom() method instead of transferFrom() for NFT transfers. This call will revert if the onERC721Received function is not implemented in the receiving contract, not allowing an incompatible contract to exercise the option.

Assessed type

ERC721

[trust1995]

Not Med-severity impact without additional evidence.

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-c