Description

When a user calls withdrawEgg(tokenId), the vault does the following:

1. Clears the egg's state (storedEggs[tokenId] = false, delete eggDepositors[tokenId])

2. Transfers the NFT to msg.sender using eggNFT.transferFrom(...)

However, if the transfer fails silently, the vault has already updated its internal state. This creates a false positive:

• The egg is marked as no longer deposited,

• But it physically remains inside the vault contract,

• The original depositor can no longer claim it (their address was deleted).

• No one else can interact with it either.

This condition bricks the egg indefinitely, with no recovery path.

How Can a Transfer Fail?

Even though ERC721 transferFrom is supposed to revert on failure, some edge cases still apply:

• Transferring to a contract that does not implement ERC721Receiver will fail if the NFT was minted with safeMint or if safeTransferFrom is used elsewhere.

• If the token has internal restrictions (e.g., locked NFTs, time-based constraints) within the ERC721 implementation.

• If the NFT contract has bugs or non-standard behavior.

In any of these cases, even though the transfer might revert, if it doesn't and fails silently or is caught and skipped due to some higher-level logic (e.g., try/catch), the vault’s state will already have changed.

Impact

Low to Medium

• Low if only technical edge cases arise — e.g., someone manually sends NFTs from a contract address, and it breaks.

• Medium if this becomes a frequent UX trap — especially if players try to withdraw to wallets like smart contract safes or custom game agents.

Even one broken NFT might result in frustration or support overhead if the user has no way to recover it.

Recommended Mitigation

Use the checks-effects-interactions pattern properly and ensure the withdrawal only proceeds if the transfer is successful.

Instead of:

Use:

This way, if the transfer reverts, the state remains unchanged.

Additionally, consider using safeTransferFrom to guarantee compatibility with contracts that implement onERC721Received:

This ensures that tokens are not sent to contracts that cannot handle them.

Summary

This is an edge case, but it’s a valid concern — especially in an ecosystem where NFTs are game items. One mishandled withdrawal could brick a valuable egg, and from a security standpoint, consistency between internal state and actual token ownership is crucial.