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

Missing approval handling in depositEggToVault function

mishoko

Summary

The depositEggToVault function in the EggHuntGame contract attempts to transfer NFTs from users to the vault without checking if the contract has the required approval. This causes transactions to revert when users call the function without previously approving the game contract, making the core deposit functionality unusable for most users.

Vulnerability Details

In the EggHuntGame contract, the depositEggToVault function tries to transfer an NFT from the user to the 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.transferFrom(msg.sender, address(eggVault), tokenId);
eggVault.depositEgg(tokenId, msg.sender);
}

The code correctly checks that the caller owns the NFT but doesn't verify that they've approved the contract to transfer it. There's even a comment noting "The player must first approve the transfer on the NFT contract", but the code doesn't handle this requirement or provide any helpful error messages to users.

When a user calls this function without first approving the game contract to transfer their NFT, the transaction will revert with an error like ERC721InsufficientApproval, which is confusing for users who expect the game to handle the deposit process seamlessly.

Impact

This vulnerability breaks a core piece of functionality in the game:

  1. Users cannot deposit eggs to the vault using the provided function unless they manually approve the contract first

  2. This creates a poor user experience and may lead to confusion and support issues

  3. Players who don't understand the need for approval may think the game is broken

  4. The issue affects 100% of deposit attempts where approval hasn't been granted

While this doesn't directly lead to fund loss, it severely impacts the usability of a core game function, potentially leading to player abandonment and rendering the vault feature unusable for most users.

Tools Used

  • Manual code review

  • Foundry for testing contract interactions

Recommendations

Option 1: Add approval checking and guidance:

function depositEggToVault(uint256 tokenId) external {
require(eggNFT.ownerOf(tokenId) == msg.sender, "Not owner of this egg");
// Check if this contract has approval and if not, inform the user
if (!eggNFT.isApprovedForAll(msg.sender, address(this)) &&
eggNFT.getApproved(tokenId) != address(this)) {
revert("Please approve the game contract to transfer your egg first");
}
// The transfer can proceed if approved
eggNFT.transferFrom(msg.sender, address(eggVault), tokenId);
eggVault.depositEgg(tokenId, msg.sender);
}

Option 2: Create a two-step process with clearer UI guidancetep 1: Frontend should call this first, then call depositEggToVault after approval

Step 1: Frontend should call this first, then call depositEggToVault after approval
Step 2: After approval is granted -> depositEggToVault => The player must first approve the transfer on the NFT contract

Updates

Lead Judging Commences

m3dython Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.