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 9 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.

Give us feedback!