Sablier
Sablier
DeFiFoundry
53,440 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Using ERC721._transfer() instead of _safeTransfer() may cause the user's NFT to be frozen in a contract that does not support ERC721

ak1

Summary

Since the bot report did not mention the issue location and nature of issue, we are submitting this again.

There are certain smart contracts that do not support ERC721, using _transfer() may result in the NFT being sent to such contracts.

 * @dev Safely transfers `tokenId` token from `from` to `to`, checking that contract recipients
 * are aware of the ERC-721 standard to prevent tokens from being forever locked.
 *
 * `data` is additional data, it has no specified format and it is sent in call to `to`.
 *
 * This internal function is like {safeTransferFrom} in the sense that it invokes
 * {IERC721Receiver-onERC721Received} on the receiver, and can be used to e.g.
 * implement alternative mechanisms to perform token transfer, such as signature-based.
 *

Vulnerability Details

SablierV2Lockup.sol#L410-L433

function withdrawMaxAndTransfer(
uint256 streamId,
address newRecipient
)
external
override
noDelegateCall
notNull(streamId)
{
// Check: the caller is the current recipient. This also checks that the NFT was not burned.
address currentRecipient = _ownerOf(streamId);
if (msg.sender != currentRecipient) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
​
// Skip the withdrawal if the withdrawable amount is zero.
uint128 withdrawableAmount = _withdrawableAmountOf(streamId);
if (withdrawableAmount > 0) {
withdraw({ streamId: streamId, to: currentRecipient, amount: withdrawableAmount });
}
​
// Checks and Effects: transfer the NFT.
_transfer({ from: currentRecipient, to: newRecipient, tokenId: streamId });
}

As shown above, the function withdrawMaxAndTransfer is used to withdraw the ERC20 tokens to the current NFT owner and then this owner transfers the NFT to the new owner - newRecipient.

This function uses the ERC721._transfer to transfer the NFT.

Impact

The NFT may get stuck in the contract that does support ERC721.

Tools Used

Manual review

Recommendations

Consider using _safeTransfer() instead of _transfer().

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

Info/Gas/Invalid as per Docs

https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity

ak1 Submitter
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Info/Gas/Invalid as per Docs

https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.