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

EggVault doe snot account for non EOA, leading to potentially locked NFTs

mishoko

Summary

The EggVault contract uses transferFrom instead of safeTransferFrom when withdrawing NFTs, which allows transfers to contracts that don't implement the onERC721Received function. This creates a risk of NFTs becoming permanently locked if they're sent to non-compliant contract addresses with no functions to transfer them back out.
mintEgg uses '_mint' instead of safemint which is also not looking for the specific hook implementation on the receiver side.

Vulnerability Details

In the withdrawEgg function of the EggVault contract, NFTs are transferred using the unsafe transferFrom method:

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

When a contract that doesn't implement onERC721Received calls this function, the transfer will succeed because transferFrom doesn't verify the receiver's ability to handle ERC721 tokens. If the receiving contract has no functionality to move the NFT elsewhere, the token becomes permanently locked.

The ERC721 standard recommends using safeTransferFrom when sending tokens to unknown addresses specifically to prevent this scenario.

PoC

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
​
import "forge-std/Test.sol";
import "../src/EggstravaganzaNFT.sol";
import "../src/EggVault.sol";
import "../src/EggHuntGame.sol";
​
// A contract that doesn't implement onERC721Received and has no NFT management functionality
contract NFTBlackhole {
// This function calls withdraw but has no way to send the NFT out afterwards
function callWithdraw(EggVault vault, uint256 tokenId) external {
vault.withdrawEgg(tokenId);
// No functions to send the NFT anywhere else!
}
// No other functions to interact with NFTs - NFTs sent here are stuck forever
}
​
// A contract that properly implements onERC721Received
contract ProperERC721Receiver {
function onERC721Received(address, address, uint256, bytes calldata) external pure returns (bytes4) {
return this.onERC721Received.selector;
}
function callWithdraw(EggVault vault, uint256 tokenId) external {
vault.withdrawEgg(tokenId);
}
// This contract could have a function to send NFTs out, but safeTransferFrom would
// still protect it from receiving NFTs it can't handle
}
​
contract SafeTransferTest is Test {
EggstravaganzaNFT public nft;
EggVault public vault;
EggHuntGame public game;
NFTBlackhole public blackhole;
ProperERC721Receiver public goodReceiver;
address public owner = address(1);
address public user = address(2);
function setUp() public {
vm.startPrank(owner);
nft = new EggstravaganzaNFT("EggstravaganzaNFT", "EGG");
vault = new EggVault();
game = new EggHuntGame(address(nft), address(vault));
nft.setGameContract(address(game));
vault.setEggNFT(address(nft));
// Deploy our test contracts
blackhole = new NFTBlackhole();
goodReceiver = new ProperERC721Receiver();
// Mint eggs for testing
nft.setGameContract(owner);
nft.mintEgg(user, 1);
nft.mintEgg(owner, 2);
vm.stopPrank();
}
function testNFTLockedInBlackhole() public {
// User deposits their egg in the vault
vm.startPrank(user);
nft.approve(address(vault), 1);
nft.transferFrom(user, address(vault), 1);
vm.stopPrank();
// Register the blackhole as the depositor
vm.prank(owner);
vault.depositEgg(1, address(blackhole));
// The blackhole withdraws the egg
vm.startPrank(address(blackhole));
blackhole.callWithdraw(vault, 1);
vm.stopPrank();
// The NFT is now owned by the blackhole
assertEq(nft.ownerOf(1), address(blackhole));
console.log("NFT is now locked in the blackhole contract with no way to transfer it out");
// There's no way to get the NFT out of the blackhole!
// If the contract doesn't have any functions to transfer NFTs,
// the NFT is permanently locked.
// In contrast, if safeTransferFrom was used, let's demonstrate:
vm.startPrank(owner);
nft.approve(address(vault), 2);
nft.transferFrom(owner, address(vault), 2);
vault.depositEgg(2, owner);
vm.stopPrank();
// Try to directly use safeTransferFrom to the blackhole
vm.prank(address(vault));
try nft.safeTransferFrom(address(vault), address(blackhole), 2) {
// If this succeeds, we'll fail the test
console.log("ERROR: safeTransferFrom should have reverted when sending to a contract without onERC721Received");
fail(); // No arguments with newer Forge versions
} catch {
// This should fail because blackhole doesn't implement onERC721Received
// The NFT should still be in the vault
assertEq(nft.ownerOf(2), address(vault));
console.log("safeTransferFrom correctly reverted when sending to NFTBlackhole");
}
// This demonstrates that:
// 1. With transferFrom, the NFT gets locked in the blackhole
// 2. With safeTransferFrom, the transfer would revert, protecting the NFT
}
}

Impact

The impact of this vulnerability is that NFTs could become permanently lost if withdrawn to contracts that:

  1. Don't implement the onERC721Received function, and

  2. Don't have any functionality to transfer NFTs out

This particularly affects:

  • Smart contract wallets that don't fully implement ERC721 receiver functionality

  • Protocol contracts that might interact with the vault

  • Integration with other DeFi or NFT systems
    While this doesn't affect transfers to regular externally owned accounts (EOAs), it presents a significant risk for contract interactions, which are increasingly common in the Web3 ecosystem.

Tools Used

  • Manual code review

  • Foundry for creating and running proof of concept tests

Recommendations

Replace the transferFrom call with safeTransferFrom in the withdrawEgg function.

Additionally, ensure the EggVault contract implements the ERC721Holder interface to properly handle incoming NFTs

Updates

Lead Judging Commences

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

Give us feedback!