Eggstravaganza
First Flight #37
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Risk of Losing NFTs When Using transferFrom() Instead of safeTransferFrom()

grands9n47

Summary

The withdrawEgg function calls eggNFT.transferFrom() to transfer an NFT back to the depositor. However, this is not as safe as using safeTransferFrom(), which ensures that the recipient can handle ERC721 tokens. If the recipient is a smart contract that does not implement the necessary interface to receive ERC721 tokens, the NFT can be stuck in the contract, leading to permanent loss.

Vulnerability Details

The function withdrawEgg uses transferFrom() to transfer the NFT to the depositor. The problem is that transferFrom() does not check whether the recipient is capable of handling ERC721 tokens. If the recipient is a smart contract that lacks the onERC721Received() function, the transfer will fail silently, and the NFT will be stuck in that contract.

/// @notice Allows the depositor to withdraw their egg from the vault.
function withdrawEgg(uint256 tokenId) public {
require(storedEggs[tokenId], "Egg not in vault");
require(eggDepositors[tokenId] == msg.sender, "Not the original depositor");
storedEggs[tokenId] = false;
delete eggDepositors[tokenId];
​
eggNFT.transferFrom(address(this), msg.sender, tokenId);
emit EggWithdrawn(msg.sender, tokenId);
}

Impact

If the recipient is a smart contract that does not support ERC721 tokens, the NFT will be stuck and cannot be retrieved. This can lead to permanent loss of the token.

Tools Used

Manual Review

Recommendations

Use safeTransferFrom() instead of transferFrom(). This function ensures the recipient can handle ERC721 tokens and prevents NFTs from being stuck in incompatible contracts.

eggNFT.safeTransferFrom(address(this), msg.sender, tokenId);
Updates

Lead Judging Commences

m3dython Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Unsafe ERC721 Transfer

NFTs are transferred to contracts without onERC721Received implementation.

Support

FAQs

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