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

`EggVault::setEggNFT` causes existing egg in `EggVault` to be locked and unable to be withdrawn

37h3rn17y2

Summary

Game owner calling EggVault::setEggNFT to set new egg NFT address will overwrite EggVault::engNFT with the new egg NFT address. When players with existing eggs in the EggVault calls EggVault::withdrawEgg, the function attempts to withdraw the new egg NFTs (which does not exist) instead of the old egg NFT, and will revert with ERC721NonexistentToken(<tokenId>). Hence, players with existing eggs in the EggVault will be unable to withdraw thier eggs.

Vulnerability Details

After game owner calls EggVault::setEggNFT to set new egg NFT address, the NFT address state variable EggVault::eggNFT will be overwritten with the new egg NFT address. As such, no functions of the old egg NFT can be called within the EggVault. Hence, all old egg NFTs held within the vault cannot be withdrawn from the EggVault.

Additionally, when a player that has existing eggs in the EggVault tries to call EggVault::withdrawEgg, the function calls eggNFT.transferFrom on #L45. This will attempt to transfer the new egg NFT to the player instead of the old egg NFT. However, the NFT does not exist, hence, the call will revert with ERC721NonexistentToken(<tokenId>)

EggVault::withdrawEgg#L45

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

Impact

Impact: High, player's eggs are locked in EggVault and unable to be withdrawn
Likelihood: Low, game owner is trusted
Severity: Medium

PoC

Place the following code into EggHuntGameTest.t.sol and run using:

forge test --mt testSetEggNFTBreaksVaultWithdrawal

function testSetEggNFTBreaksVaultWithdrawal() public {
// 1. Alice finds an egg
// simulated here by EggHuntGame awarding Alice an egg
uint256 winningEggTokenId = 1;
vm.prank(address(game));
nft.mintEgg(alice, winningEggTokenId);
​
// 2. Alice deposits egg into vault
vm.startPrank(alice);
nft.approve(address(game), winningEggTokenId);
game.depositEggToVault(winningEggTokenId);
vm.stopPrank();
​
// 3. Owner sets new Egg NFT on EggVault
vm.startPrank(owner);
EggstravaganzaNFT newNft = new EggstravaganzaNFT("NewEggstravaganza", "NewEGG");
vault.setEggNFT(address(newNft));
vm.stopPrank();
​
// 4. Alice is unable to withdraw her existing egg from vault
vm.prank(alice);
vm.expectRevert();
vault.withdrawEgg(winningEggTokenId);
}

Tools Used

Manual review

Recommendations

Consider removing the EggVault::setEggNFT function

Updates

Lead Judging Commences

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

State corruption

Changing the NFT contract address doesn't update the storedEggs and eggDepositors mappings

Support

FAQs

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