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

`EggVault::setEggNFT` causes players to be unable to deposit eggs into vault through `EggHuntGame::depositEggToVault`

37h3rn17y2

Summary

Game owner calling EggVault::setEggNFT to set new egg NFT address will overwrite EggVault::eggNFT with the new egg NFT address. Function call EggHuntGame::depositEggToVault will pass execution flow to EggVault::depositEgg which evaluates eggNFT.ownerOf on the new egg NFT instead of the old egg NFT, which does not exist and will revert with ERC721NonexistentToken(<tokenId>). Hence, players are unable to deposit eggs into the vault through EggHuntGame::depositEggToVault, breaking core protocol functionality.

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, any function calls involving EggVault::eggNFT will be executed in the context of the new egg NFT instead of the old egg NFT.

In particular, when a player attempts to deposit an egg into the vault by calling EggHuntGame::depositEggToVault, the execution flow is passed to EggVault::depositEgg on #L88. Subsequently, the require statement on EggHuntGame::depositEggToVault#L88 will attempt to evaluate eggNFT.ownerOf on the new egg NFT instead of the old egg NFT. However, the NFT does not exist, hence, the call will revert with ERC721NonexistentToken(<tokenId>).

EggHuntGame::depositEggToVault#L88

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

`EggVault::depositEgg#L30

function depositEgg(uint256 tokenId, address depositor) public {
@> require(eggNFT.ownerOf(tokenId) == address(this), "NFT not transferred to vault");
require(!storedEggs[tokenId], "Egg already deposited");
storedEggs[tokenId] = true;
eggDepositors[tokenId] = depositor;
emit EggDeposited(depositor, tokenId);
}

Impact

Impact: Medium, players will to be unable to deposit eggs into vault through EggHuntGame::depositEggToVault, disrupting the core protocol functionality
Likelihood: Low, game owner is trusted
Severity: Medium

PoC

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

forge test --mt testSetEggNFTBreaksDepositToVault

function testSetEggNFTBreaksDepositToVault() public {
// 1. Owner sets new Egg NFT on EggVault
vm.startPrank(owner);
EggstravaganzaNFT newNft = new EggstravaganzaNFT("NewEggstravaganza", "NewEGG");
vault.setEggNFT(address(newNft));
vm.stopPrank();
​
// 2. Bob finds an egg
// simulated here by EggHuntGame awarding Bob an egg
uint256 winningEggTokenId = 1;
vm.prank(address(game));
nft.mintEgg(bob, winningEggTokenId);
​
// 3. Bob cannot deposit egg into vault
vm.startPrank(bob);
nft.approve(address(game), winningEggTokenId);
vm.expectRevert();
game.depositEggToVault(winningEggTokenId);
vm.stopPrank();
}

Tools Used

Manual review

Recommendations

Consider removing the EggVault::setEggNFT function

Updates

Lead Judging Commences

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