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

`EggVault::withdrawEgg` using `ERC721::transferFrom`, not `ERC721::safeTransferFrom`

jjhddtt

Summary

The EggVault::withdrawEgg calls the ERC721::transferFrom function, not ERC721::safeTransferFrom. This does not check whether the to address is capable of receiving the NFT, in this case msg.sender, which may lead to loss of the NFT.

Vulnerability Details

The following function uses transferFrom, which does not check whether the target is able to receive the NFT:

/// @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);
}

Whereas, the safeTransferFrom, as described in EIP-721:

When transfer is complete, this function checks if _to is a smart contract (code size > 0). If so, it calls onERC721Received on _to and throws if the return value is not >bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))

Impact

This could lead to the permanent loss of the NFT if the transfer is unsuccessful.

Tools Used

Manual review.

Recommendations

Replace the use of transferFrom with safeTransferFrom

Updates

Lead Judging Commences

m3dython Lead Judge about 2 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.