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

Unsafe NFT Transfers Risk Locking Tokens

roccomania

Summary

The EggVault contract uses transferFrom for NFT withdrawals instead of the ERC721-standard safeTransferFrom, creating a risk of permanent NFT loss when users withdraw tokens to smart contracts that do not support ERC721. This violates best practices and can lead to irreversible asset locking.

Vulnerability Details

Location:

  • EggVault::withdrawEgg(uint256 tokenId)

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

Issue:

  • The contract uses transferFrom for NFT transfers, which does not verify if the recipient can handle ERC721 tokens.

  • If an NFT is sent to a contract that does not implement IERC721Receiver, the transfer succeeds, but the NFT becomes permanently stuck—unable to be recovered.

Attack Scenarios:

  1. Accidental Transfer to Incompatible Contracts

    • A user withdraws an NFT to a DEX, lending protocol, or other contract not designed to hold NFTs.

    • The transaction completes, but the NFT is locked forever because the recipient cannot process it.

Root Cause:

  • The contract fails to comply with EIP-721, which mandates the use of safeTransferFrom for secure NFT transfers.

  • Missing IERC721Receiver implementation in the vault contract.

Impact

This leads to permanent, irreversible loss of user NFTs.

Tools Used

  1. Manual Review

Recommendations

1. Use safeTransferFrom for All Withdrawals

function withdrawEgg(uint256 tokenId) public {
// ...existing checks...
eggNFT.safeTransferFrom(address(this), msg.sender, tokenId);
}

2. Implement IERC721Receiver in EggVault and receiver contract

import "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
​
contract EggVault is ERC721Holder { // Auto-implements IERC721Receiver
// ...existing code...
}
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.