Sablier

Summary

In v2-core/src/abstracts/SablierV2Lockup.sol:withdrawMaxAndTransfer, It is recommended to use _safeTransfer() instead of _transfer when transferring ERC721s out of the vault.

Vulnerability Details

In Use of _transfer method for ERC721 transfer is discouraged and recommended to use _safeTransfer whenever possible by OpenZeppelin.

This is because _transfer cannot check whether the receiving address know how to handle ERC721 tokens.

In the function shown at below PoC, ERC721 token is sent to newRecipient with the _transfer method.

POC:

https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/abstracts/SablierV2Lockup.sol#L432

If this newRecipient is a contract and is not aware of incoming ERC721 tokens, the sent token could be locked up in the contract forever.

Reference: https://docs.openzeppelin.com/contracts/3.x/api/token/erc721

OpenZeppelin’s documentation discourages the use of transferFrom(); use safeTransferFrom() whenever possible
The recipient could have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom(). A notable example of such contracts is the Sudoswap pair:

Impact

While unlikely because the recipient is the function caller, there is the potential loss of NFTs should the recipient is unable to handle the sent ERC721s.

Tools Used

Manual Review

Recommendations

Use _safeTransfer when sending out the NFT from the vault.