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

Unsafe Transfer of NFT from Minter to The Vault Contract

SashaFlores

Summary

The depositEggToVault function in the EggHuntGame contract uses the transferFrom method instead of safeTransferFrom to transfer the ERC721 token from the minter address to the EggVault contract.

Vulnerability Details

Vulnerability Exists in EggHuntGame contract in line 87 of depositEggToVault function: https://github.com/CodeHawks-Contests/2025-04-eggstravaganza/blob/main/src/EggHuntGame.sol#L87

  • Using transferFrom bypasses the necessary checks to ensure the recipient contract EggVault is capable of safely accepting the ERC721 token.

  • The transfer will succeed without any checks, which will result in losing of the NFT without any proper handling or logging.

  • Using transferFrom only verifies that the current owner is from address and to isn't the zero address.

function transferFrom(address from, address to, uint256 tokenId) public virtual {
if (to == address(0)) {
revert ERC721InvalidReceiver(address(0));
}
// Setting an "auth" arguments enables the `_isAuthorized` check which verifies that the token exists
// (from != 0). Therefore, it is not needed to verify that the return value is not 0 here.
address previousOwner = _update(to, tokenId, _msgSender());
if (previousOwner != from) {
revert ERC721IncorrectOwner(from, tokenId, previousOwner);
}
}

  • While on the other hand, safeTransferFrom implements the saftey check if the to is a smart contract, it correctly returns the acceptance magic value to accept, if not the transfer will revert.

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

Tools Used

Manual and Visual Code Inspection

2 Forge Test Cases

Proof of Concept

  • First Test Case: Run Regular Test With Existing Code Flaw:

function testDepositEggToVault() public {
// Mint an egg to alice via the game contract.
vm.prank(address(game));
nft.mintEgg(alice, 20);
assertEq(nft.ownerOf(20), alice);
​
// Alice approves the game to transfer her NFT.
vm.prank(alice);
nft.approve(address(game), 20);
​
// Alice deposits her egg into the vault via the game contract.
vm.prank(alice);
game.depositEggToVault(20);
// The NFT should now be owned by the vault.
assertEq(nft.ownerOf(20), address(vault));
// Vault records should indicate the egg is deposited by alice.
assertTrue(vault.isEggDeposited(20));
assertEq(vault.eggDepositors(20), alice);
}

By Running forge test -vvvv the test passed with all logs of transfer emitted even in unsafe transfer to vault

[PASS] testDepositEggToVault() (gas: 180686)
Traces:
[220486] EggGameTest::testDepositEggToVault()
├─ [0] VM::prank(EggHuntGame: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a])
│ └─ ← [Return]
├─ [72259] EggstravaganzaNFT::mintEgg(ECRecover: [0x0000000000000000000000000000000000000001], 20)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: ECRecover: [0x0000000000000000000000000000000000000001], tokenId: 20)
│ └─ ← [Return] true
├─ [1071] EggstravaganzaNFT::ownerOf(20) [staticcall]
│ └─ ← [Return] ECRecover: [0x0000000000000000000000000000000000000001]
├─ [0] VM::assertEq(ECRecover: [0x0000000000000000000000000000000000000001], ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
│ └─ ← [Return]
├─ [0] VM::prank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [25506] EggstravaganzaNFT::approve(EggHuntGame: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 20)
│ ├─ emit Approval(owner: ECRecover: [0x0000000000000000000000000000000000000001], approved: EggHuntGame: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], tokenId: 20)
│ └─ ← [Stop]
├─ [0] VM::prank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [91155] EggHuntGame::depositEggToVault(20)
│ ├─ [1071] EggstravaganzaNFT::ownerOf(20) [staticcall]
│ │ └─ ← [Return] ECRecover: [0x0000000000000000000000000000000000000001]
│ ├─ [29484] EggstravaganzaNFT::transferFrom(ECRecover: [0x0000000000000000000000000000000000000001], EggVault: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 20)
│ │ ├─ emit Transfer(from: ECRecover: [0x0000000000000000000000000000000000000001], to: EggVault: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], tokenId: 20)
│ │ └─ ← [Stop]
│ ├─ [50855] EggVault::depositEgg(20, ECRecover: [0x0000000000000000000000000000000000000001])
│ │ ├─ [1071] EggstravaganzaNFT::ownerOf(20) [staticcall]
│ │ │ └─ ← [Return] EggVault: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]
│ │ ├─ emit EggDeposited(depositor: ECRecover: [0x0000000000000000000000000000000000000001], tokenId: 20)
│ │ └─ ← [Stop]
│ └─ ← [Stop]
├─ [1071] EggstravaganzaNFT::ownerOf(20) [staticcall]
│ └─ ← [Return] EggVault: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]
├─ [0] VM::assertEq(EggVault: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], EggVault: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
│ └─ ← [Return]
├─ [855] EggVault::isEggDeposited(20) [staticcall]
│ └─ ← [Return] true
├─ [0] VM::assertTrue(true) [staticcall]
│ └─ ← [Return]
├─ [871] EggVault::eggDepositors(20) [staticcall]
│ └─ ← [Return] ECRecover: [0x0000000000000000000000000000000000000001]
├─ [0] VM::assertEq(ECRecover: [0x0000000000000000000000000000000000000001], ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
│ └─ ← [Return]
└─ ← [Stop]

  • Second Test Case: Run a Safe Test With safeTransferFrom function:

function test_Revert_DepositEggToVault_safeTransfer() public {
// Mint an egg to Alice
vm.prank(address(game));
nft.mintEgg(alice, 20);
​
// Ensure the NFT is owned by Alice
assertEq(nft.ownerOf(20), alice);
​
// Attempt to transfer the NFT using safeTransferFrom (should fail)
vm.prank(alice);
vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721InvalidReceiver.selector, address(vault)));
nft.safeTransferFrom(alice, address(vault), 20);
}

The Test Passed as Expected because EggVault is Invalid Receiver with Error ERC721InvalidReceiver

[PASS] test_Revert_DepositEggToVault_safeTransfer() (gas: 106842)
Traces:
[126742] EggGameTest::test_Revert_DepositEggToVault_safeTransfer()
├─ [0] VM::prank(EggHuntGame: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a])
│ └─ ← [Return]
├─ [72259] EggstravaganzaNFT::mintEgg(ECRecover: [0x0000000000000000000000000000000000000001], 20)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: ECRecover: [0x0000000000000000000000000000000000000001], tokenId: 20)
│ └─ ← [Return] true
├─ [1071] EggstravaganzaNFT::ownerOf(20) [staticcall]
│ └─ ← [Return] ECRecover: [0x0000000000000000000000000000000000000001]
├─ [0] VM::assertEq(ECRecover: [0x0000000000000000000000000000000000000001], ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
│ └─ ← [Return]
├─ [0] VM::prank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [0] VM::expectRevert(custom error 0xf28dceb3: 0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002464a0ae920000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b00000000000000000000000000000000000000000000000000000000)
│ └─ ← [Return]
├─ [33197] EggstravaganzaNFT::safeTransferFrom(ECRecover: [0x0000000000000000000000000000000000000001], EggVault: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 20)
│ ├─ emit Transfer(from: ECRecover: [0x0000000000000000000000000000000000000001], to: EggVault: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], tokenId: 20)
│ ├─ [212] EggVault::onERC721Received(ECRecover: [0x0000000000000000000000000000000000000001], ECRecover: [0x0000000000000000000000000000000000000001], 20, 0x)
│ │ └─ ← [Revert] EvmError: Revert
│ └─ ← [Revert] ERC721InvalidReceiver(0x2e234DAe75C793f67A35089C9d99245E1C58470b)
└─ ← [Stop]

Impact

  • High Severity: This issue is considered high severity because it directly affects the NFT (egg) asset of users. While the player's funds are not at immediate risk, the player's NFT will be lost if the EggVault is not configured to accept ERC721 tokens properly.

  • Broken Game Logic: This issue may lead to poor user experience, as players may unknowingly deposit their eggs into a vault that cannot handle them correctly.

  • Certain Loss of NFTs: In the worst case, because the vault does not implement the appropriate ERC721 receiving logic, NFTs will be transferred to an address where they are inaccessible.

Recommendations

To ensure safe transfer and avoid the risk of losing NFTs, it is recommended to use safeTransferFrom instead of transferFrom:

/// @notice Allows a player to deposit their egg NFT into the Egg Vault.
function depositEggToVault(uint256 tokenId) external {
require(eggNFT.ownerOf(tokenId) == msg.sender, "Not owner of this egg");
// The player must first approve the transfer on the NFT contract.
eggNFT.safeTransferFrom(msg.sender, address(eggVault), tokenId);
eggVault.depositEgg(tokenId, msg.sender);
}
Updates

Lead Judging Commences

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