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

[L-2] Players' Egg NFTs Can Be Locked After Depositing in `EggVault::depositEgg`

twilly

Summary

The EggVault contract enables players to deposit and withdraw their egg NFTs into a vault. However, the depositEgg function does not verify that the depositor address matches the original depositor (msg.sender). Since the address depositor parameter is not validated, a player can deposit eggs to any address, preventing them from withdrawing their NFT later via the withdrawEgg function.

Vulnerability Details

The depositEgg function lacks address validation checks for the address depositor parameter, which can result in a player's egg NFT becoming locked in the vault.

/// @notice Records the deposit of an egg (NFT).
/// The NFT must already have been transferred to the vault.
function depositEgg(uint256 tokenId, address depositor) public {
require(eggNFT.ownerOf(tokenId) == address(this), "NFT not transferred to vault");
require(!storedEggs[tokenId], "Egg already deposited");
@> // @audit no validation check on the depositor address can prevent players from withdrawing their eggs
​
storedEggs[tokenId] = true;
eggDepositors[tokenId] = depositor;
emit EggDeposited(depositor, tokenId);
}
​

Impact

This vulnerability allows the following issues:

  • Players cannot withdraw NFTs accidentally deposited to other addresses.

  • Players may receive egg NFTs they did not earn from the hunt.

PoC

  1. Bob accidentally deposits his NFT to Alice's address

  2. Bob tries to withdraw the NFT

  3. Fails with "Not the original depositor" error

  4. Bob is unable to ever withdraw his NFT

Add the following test to EggHuntGameTest.t.sol file and run with forge test --mt testVaultEggsLockedFromOriginalDepositor.

function testVaultEggsLockedFromOriginalDepositor() public {
// Mint an egg directly to the vault (using game contract privileges).
vm.prank(address(game));
nft.mintEgg(address(vault), 10);
// Ensure the vault owns token 10.
assertEq(nft.ownerOf(10), address(vault));
​
// Bob deposits the egg into alice's vault
vm.startPrank(bob);
vault.depositEgg(10, alice);
// The egg should now be marked as deposited.
assertTrue(vault.isEggDeposited(10));
// The depositor recorded should be alice.
assertEq(vault.eggDepositors(10), alice);
​
// Bob tries to withdraw his egg NFT.
vm.expectRevert("Not the original depositor");
vault.withdrawEgg(10);
vm.stopPrank();
​
// Alice is able to withdrawal the egg NFT since she is the set depositor.
vm.prank(alice);
vault.withdrawEgg(10);
// After withdrawal, alice should be the owner again.
assertEq(nft.ownerOf(10), alice);
// The stored egg flag should be cleared.
assertFalse(vault.isEggDeposited(10));
// And the depositor mapping should be reset to the zero address.
assertEq(vault.eggDepositors(10), address(0));
}
​

Tools Used

Manual review, Foundry

Recommendations

Consider adding documentation to advise players to double check the addresses provided to the depositEgg function.

Additionally, implement a validation check to the depositEgg function, similar to the one in the withdrawEgg function:

function depositEgg(uint256 tokenId, address depositor) public {
require(eggNFT.ownerOf(tokenId) == address(this), "NFT not transferred to vault");
require(!storedEggs[tokenId], "Egg already deposited");
+ require(depositor == msg.sender, "Not the original depositor");
​
storedEggs[tokenId] = true;
eggDepositors[tokenId] = depositor;
emit EggDeposited(depositor, tokenId);
}
Updates

Lead Judging Commences

m3dython Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Unsafe direct token transfer

Users can transfer NFTs directly to the vault using standard ERC721 transferFrom(), bypassing the registration

Support

FAQs

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