Eggstravaganza

Description

The depositEgg function currently allows the caller to arbitrarily specify the depositor address. There is no check to prevent this address from being the zero address (address(0)).

A malicious actor can exploit this by setting the depositor to address(0) when calling depositEgg. This action renders the egg permanently unwithdrawable since the zero address has no private key and cannot initiate a transaction. As a result, the egg remains locked inside the vault with no possible way to recover it.

This creates a griefing vector where eggs can be intentionally bricked, impacting the utility and value of the NFTs within the context of the game.

Impact

Medium

While this issue does not result in immediate theft or unauthorized access, it permanently disables the egg and denies the rightful owner access. In the context of the EggHunt game, where eggs may represent progress, rewards, or participation in events, bricking them can ruin the experience for players or disrupt the game logic if not handled defensively.

This could be abused for trolling, sabotage during competitions, or to inflate egg scarcity by deliberately removing them from circulation.

That said, it's less severe than direct theft since the attacker doesn't benefit, and the harm is denial rather than unauthorized gain.

Proof of Concept (PoC)

Scenario:

1. A user (malicious or naive) transfers an egg NFT to the vault.

2. They immediately call depositEgg(tokenId, address(0)).

3. The vault records the depositor as address(0).

4. Since withdrawEgg() checks eggDepositors[tokenId] == msg.sender, and no one can be address(0), the egg becomes permanently stuck in the vault.

There is no mechanism to recover or reassign the depositor after this point.

Recommended Mitigation

Add a validation check in the depositEgg function (if it remains a public function) to ensure the depositor is not the zero address:

However, a more robust fix is to eliminate the ability to specify the depositor altogether. Whether using msg.sender or onERC721Received, both alternatives naturally prevent this issue since:

• msg.sender will never be address(0).

• from in the ERC721 safeTransferFrom context will never be address(0).

This makes the attack vector unrepresentable under both safer designs.