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

Use of `transferFrom` Instead of `safeTransferFrom` in `withdrawEgg()` May Cause NFT to Be Stuck in the Vault

0xbyteknight

Summary

The withdrawEgg() function in EggVault.sol uses transferFrom to transfer NFTs back to the depositor. If the recipient address is a contract that does not implement onERC721Received, the transfer will silently succeed, but the contract will not be able to receive or interact with the token — causing it to be lost.

Vulnerability Details

The transferFrom function performs a raw transfer of the NFT without checking whether the recipient address can handle ERC721 tokens. This creates a dangerous edge case:

  • If the depositor is a smart contract that does not implement onERC721Received, the token will be transferred but not safely received, violating the ERC721 standard.

  • Because the contract marks storedEggs[tokenId] = false and deletes the depositor info before the transfer, the token becomes unrecoverable if the transfer fails silently.

OpenZeppelin recommends using the safeTransferFrom function to ensure that ERC721 tokens are only sent to compliant addresses. This function includes an internal call to onERC721Received and reverts if the recipient doesn't support it.

function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual {
transferFrom(from, to, tokenId);
ERC721Utils.checkOnERC721Received(_msgSender(), from, to, tokenId, data);
}

Impact

  • Tokens can be permanently stuck in the vault.

  • Off-chain tracking may show the token as withdrawn, but the depositor will not actually receive it.

  • Users may lose access to assets due to silent failures.

Tools Used

  • Manual Code Review

  • Foundry Test Suite

PoC

// SPDX-License-Identifier: MIT
pragma solidity 0.8.23;
​
contract BlackHoleContract {
// This contract does NOT implement onERC721Received,
// so it can't safely receive ERC721 tokens.
}
​
import "forge-std/Test.sol";
import "../src/EggstravaganzaNFT.sol";
import "../src/EggVault.sol";
import "../src/EggHuntGame.sol";
​
contract EggGameTest is Test {
EggstravaganzaNFT nft;
EggVault vault;
EggHuntGame game;
​
function setUp() public {
// Deploy the contracts.
nft = new EggstravaganzaNFT("Eggstravaganza", "EGG");
vault = new EggVault();
game = new EggHuntGame(address(nft), address(vault));
// Set the allowed game contract for minting eggs in the NFT.
nft.setGameContract(address(game));
// Configure the vault with the NFT contract.
vault.setEggNFT(address(nft));
}
​
/// @notice This test demonstrates that using `transferFrom` instead of `safeTransferFrom`
/// in `withdrawEgg()` can lead to the NFT being lost forever if the recipient
/// is a contract that doesn't support the ERC721 standard.
function test_WithdrawToNonERC721Receiver_LosesNFT() public {
// Deploy a contract that does not implement ERC721 standard
BlackHoleContract nonCompliant = new BlackHoleContract();
// Simulate a user (nonCompliant contract) receiving an NFT after finding an egg
vm.prank(address(game));
nft.mintEgg(address(nonCompliant), 1);
assertEq(nft.ownerOf(1), address(nonCompliant));
// Simulate nonCompliant contract approving the game to move its NFT
vm.prank(address(nonCompliant));
nft.approve(address(game), 1);
// Deposit the NFT into the vault via the EggHuntGame
vm.prank(address(nonCompliant));
game.depositEggToVault(1);
// Confirm it was deposited correctly
assertTrue(vault.isEggDeposited(1));
assertEq(nft.ownerOf(1), address(vault));
// Now the vault sends it back to the nonCompliant contract
vm.startPrank(address(nonCompliant));
vault.withdrawEgg(1);
vm.stopPrank();
// Ownership appears transferred — but no guarantees the token is usable
assertFalse(vault.isEggDeposited(1));
// NFT is no longer in the vault, but blackHole can't interact with it either
assertEq(nft.ownerOf(1), address(nonCompliant)); // Still shows ownership...
// `transferFrom()` succeeded silently, but the recipient cannot handle the NFT.
// In a real system, the user has no way to interact with or recover this NFT.
}
}

Recommendations

Replace the raw transferFrom with safeTransferFrom in withdrawEgg():

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

Lead Judging Commences

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