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

Vault owner can change NFT contract address leading to players that can't deposit or withdraw their eggs.

skaltso

Description:
In EggVault::setEggNFT the vault owner can set a new eggNFT contract address. Players that still have egg NFTs in the old eggNFT contract won't be able to deposit them through neither EggVault::depositEgg nor EggHuntGame::depositEggToVault or withdraw their egg NFT(s) throught EggVault::withdrawEgg.

Impact:
Players won't be able to deposit or withdraw their egg NFT(s). The deposited egg NFT(s) could be stuck until the owner changes the eggNFT address back the old eggNFT contract.

Proof of Concept:

I. User can't deposit an egg

  1. User gets an egg from the game.

  2. The vault owner changes the eggNFT address.

  3. User tries to deposit the egg, but can't.

II. User can't withdraw an egg

  1. User gets an egg and deposits it into the vault.

  2. The vault owner changes the eggNFT address.

  3. User tries to withdraw his egg NFT, but can't.

Proof of Code:
Set up a new EggstravaganzaNFT contract and place the following tests in EggHuntGameTest.t.sol

function testCantDepositEggIfNFTContractResetInVault() public {
vm.prank(address(game));
nft.mintEgg(alice, 20);
assertEq(nft.ownerOf(20), alice);
​
vm.prank(alice);
nft.approve(address(game), 20);
​
vm.prank(owner);
vault.setEggNFT(address(newNFT));
​
vm.prank(alice);
vm.expectRevert(abi.encodeWithSignature("ERC721NonexistentToken(uint256)", 20));
game.depositEggToVault(20);
​
vm.prank(alice);
nft.transferFrom(alice, address(vault), 20);
vm.prank(alice);
vm.expectRevert(abi.encodeWithSignature("ERC721NonexistentToken(uint256)", 20));
vault.depositEgg(20, alice);
}
​
function testCantWithdrawEggIfNFTContractResetInVault() public {
testDepositEggToVault();
​
vm.prank(owner);
vault.setEggNFT(address(newNFT));
​
vm.prank(alice);
vm.expectRevert(abi.encodeWithSignature("ERC721NonexistentToken(uint256)", 20));
vault.withdrawEgg(20);
}

Recommended Mitigation:
There are several ways to handle this.1

  1. Have a storage variable that keeps track of the number of deposited eggs and EggVault::setEggNFT only allows to change the eggNFT address if that variable is 0. Note that a malicious user might never withdraw their egg and the eggNFT address wouldn't be able to change.

  2. Store NFT addresses in a mapping and only add to the mapping in EggVault::setEggNFT for backwards compatibility. In the EggVault::depositEgg and EggVault::withdrawEgg have the NFT contract address as a parameter. That way when users tries to deposit or withdraw their egg NFT, they vault will deposit or transfer it from the correct eggNFT.

...
mapping(address => bool) public eggContracts;
mapping(address => mapping(uint256 => bool)) public storedEggs;
mapping(address => mapping(uint256 => address)) public eggDepositors;
...
function addEggNFT(address _eggNFTAddress) external onlyOwner {
require(_eggNFTAddress != address(0), "Invalid NFT address");
eggContracts[_eggNFTAddress] = true;
}
...
function depositEgg(uint256 tokenId, address depositor, address eggNFT) ublic {
require(eggContracts[eggNFT], "NFT not supported");
require(EggstravaganzaNFT(eggNFT).ownerOf(tokenId) == address(this), "NFT not transferred to vault");
require(!storedEggs[eggNFT][tokenId], "Egg already deposited");
storedEggs[eggNFT][tokenId] = true;
eggDepositors[eggNFT][tokenId] = depositor;
}
...
function withdrawEgg(uint256 tokenId, address eggNFT) public {
require(eggContracts[eggNFT], "NFT not supported");
require(storedEggs[eggNFT][tokenId], "Egg not in vault");
require(eggDepositors[eggNFT][tokenId] == msg.sender, "Not the original depositor");
​
storedEggs[eggNFT][tokenId] = false;
delete eggDepositors[eggNFT][tokenId];
​
EggstravaganzaNFT(eggNFT).transferFrom(address(this), msg.sender, tokenId);
}

  1. Option #2 but use openzeppelin's access control to check for supported NFTs.

Updates

Lead Judging Commences

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

State corruption

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

Appeal created

mishoko Auditor
7 months ago
m3dython Lead Judge
7 months ago
farismaulana Auditor
7 months ago
mishoko Auditor
7 months ago
m3dython Lead Judge
7 months ago
w33ked Auditor
7 months ago
m3dython Lead Judge
7 months ago
m3dython Lead Judge 7 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.