Sablier

Summary

The SablierV2Lockup contract inherits the OpenZeppelin ERC-721 template but uses _transfer instead of _safeTransfer in several places, not checking the return value of onERC721Received hook. This hook ensures contract recipients can handle ERC-721 tokens, preventing tokens from being locked if the recipient is unaware of the transfer.

Vulnerability Details

The SablierV2Lockup contract inherits the OpenZeppelin ERC-721 template, which includes two versions of ERC-721 minting functions: _mint and _safeMint. The difference between them is the verification of the onERC721Received hook. If the receiver is a contract account (CA) rather than an externally owned account (EOA), the hook will be invoked to ensure the contract can transfer or withdraw the ERC-721 token. There are also two versions of transfer functions, _transfer and _safeTransfer, with the latter verifying whether onERC721Received is implemented and returns the correct value.

Although LightChase already spotted this issue, there are more instances of using _mint instead of _safeMint throughout the v2-core codebase.

https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/SablierV2LockupDynamic.sol#L354
https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/SablierV2LockupLinear.sol#L274
https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/SablierV2LockupTranched.sol#L258

Additionally, there are instances of using _transfer instead of _safeTransfer:
https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/abstracts/SablierV2Lockup.sol#L432

Checking the onERC721Received hook in these functions will not lead to potential reentrancy issues since the operations follow the standard check-effects-interactions pattern.

Impact

Verification of the onERC721Received hook’s return value ensures that the recipient, if it is a contract, has implemented functions related to transfer or withdrawal. If the recipient is a contract and is not aware of the incoming ERC-721 token, the transferred ERC-721 might be locked in the recipient forever.

Tools Used

Manual Review

Recommendations

Use _safeTransfer instead of _transfer and _safeMint instead of _mint