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

Redundant tokenId Parameter in mintEgg Function

mishoko

Summary

The token ID management in the project is split between different contracts, with eggCounter in the EggHuntGame contract and totalSupply in the EggstravaganzaNFT contract. This redundant state tracking creates unnecessary complexity, increases gas costs, and introduces potential risks of state inconsistency. Additionally, the mintEgg function requiring a tokenId parameter from the caller is redundant when the NFT contract could generate and track token IDs internally.

Vulnerability Details

The project implements token ID management in multiple locations:

// In EggHuntGame.sol
uint256 public eggCounter;
​
function searchForEgg() external {
// ...
if (random < eggFindThreshold) {
eggCounter++;
eggsFound[msg.sender] += 1;
eggNFT.mintEgg(msg.sender, eggCounter);
// ...
}
}
// In EggstravaganzaNFT.sol
uint256 public totalSupply;
​
function mintEgg(address to, uint256 tokenId) external returns (bool) {
require(msg.sender == gameContract, "Unauthorized minter");
_mint(to, tokenId);
totalSupply += 1;
return true;
}

This dual-tracking approach creates several issues:

  1. Redundancy: Both contracts track essentially the same information (the total number of NFTs minted).

  2. Increased Attack Surface: Managing token IDs in multiple contracts creates more opportunities for errors or inconsistencies.

  3. State Synchronization Risk: The two counters could potentially get out of sync, especially if the gameContract address is ever updated or if direct minting capabilities are added in the future.

  4. Gas Inefficiency: Each mint operation updates storage in multiple contracts, consuming more gas than necessary.

Impact

The impact of this redundancy is low to medium:

  1. Gas Costs: Higher gas costs for minting operations due to multiple state updates.

  2. Potential for Inconsistency: If minting occurs through alternate paths or if the game contract is updated, the counters could become inconsistent.

  3. Code Complexity: The split responsibility makes the code harder to maintain and understand.

  4. Centralization of Logic: The NFT contract depends on external logic for ID generation, reducing its reusability.

While this issue doesn't directly lead to fund loss, it represents a design flaw that increases complexity and gas costs while potentially introducing risks for future development.

PoC

This vulnerability can be demonstrated by tracking the state changes during a typical minting operation

Each egg minting requires two separate storage updates to maintain synchronization between the contracts.

Tools Used

  • Manual code review

Recommendations

Centralize token ID management in the NFT contract where it logically belongs:

// In EggstravaganzaNFT.sol
uint256 private _nextTokenId = 1;
​
function mintEgg(address to) external returns (uint256) {
require(msg.sender == gameContract, "Unauthorized minter");
uint256 tokenId = _nextTokenId++;
_mint(to, tokenId);
return tokenId;
}
​
function totalSupply() external view returns (uint256) {
return _nextTokenId - 1;
}

or even use internal ERC721 totatlSupply

// In EggHuntGame.sol - updated searchForEgg function
function searchForEgg() external {
// ...
if (random < eggFindThreshold) {
uint256 tokenId = eggNFT.mintEgg(msg.sender);
eggsFound[msg.sender] += 1;
emit EggFound(msg.sender, tokenId, eggsFound[msg.sender]);
}
}

This approach offers several advantages:

Single Source of Truth: Only one contract maintains token ID information.

Reduced Attack Surface: No possibility of counter misalignment between contracts.

Updates

Lead Judging Commences

m3dython Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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