First Flight #17: Dussehra
First Flight #17
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Address 0x0000000000000000000000000000000000000000 can be selected as Ram

waydou

Summary

Address 0x0000000000000000000000000000000000000000 can be selected as Ram

Vulnerability Details

The problem relies on the conditions of tokenIds in the ChoosingRam::IncreaseincreaseValuesOfParticipants where we check if the two arguments are striclty greater than ramNFT.tokenCounter()

if (tokenIdOfChallenger > ramNFT.tokenCounter()) {
revert ChoosingRam__InvalidTokenIdOfChallenger();
}
if (tokenIdOfAnyPerticipent > ramNFT.tokenCounter()) {
revert ChoosingRam__InvalidTokenIdOfPerticipent();
}

However the incrementation in RamNFT::mintRamNFT is done using uint256 newTokenId = tokenCounter++; the post-increment operator (tokenCounter++) returns the current value of tokenCounter before incrementing it. Therefore, if tokenCounter is initially 0, newTokenId will also be 0 before tokenCounter is incremented. and this means that at anytime the nft with tokenID tokenCounter isn't minted yet and not assigned to anyone.

Impact

An attacker or an ignorant user could spam ChoosingRam::IncreaseincreaseValuesOfParticipants. with tokenIdOfAnyPerticipent equal to tokenCounter. This could result in the NFT associated with tokenCounter (which is unminted and thus assigned to the zero address) being selected as NFT of the Ram. Consequently, the protocol would be broken as half of the funds would not go to any participant but remain unassigned.

function test_TokenCounter() public participants{
// there are two participants owning nfts with tokenID 0 and 1
​
console.log("tokencounter :",ramNFT.tokenCounter()); // tokencounter : 2
console.log("characteristics of tokenID 2:",ramNFT.getCharacteristics(2).ram); // 0x0000000000000000000000000000000000000000
​
vm.startPrank(player2);
choosingRam.increaseValuesOfParticipants(1, 2);
choosingRam.increaseValuesOfParticipants(1, 2);
choosingRam.increaseValuesOfParticipants(1, 2);
choosingRam.increaseValuesOfParticipants(1, 2);
choosingRam.increaseValuesOfParticipants(1, 2);
vm.stopPrank();
console.log("characteristics isJitaKrodhah of tokenID 2:",ramNFT.getCharacteristics(2).isJitaKrodhah); // true
console.log("characteristics isDhyutimaan of tokenID 2:",ramNFT.getCharacteristics(2).isDhyutimaan); // true
console.log("characteristics isVidvaan of tokenID 2:",ramNFT.getCharacteristics(2).isVidvaan); // true
console.log("characteristics isAatmavan of tokenID 2:",ramNFT.getCharacteristics(2).isAatmavan); // true
console.log("characteristics isSatyavaakyah of tokenID 2:",ramNFT.getCharacteristics(2).isSatyavaakyah); // true
assertEq(ramNFT.getCharacteristics(2).ram, address(0));
}

Tools Used

Foundry

Recommendations

Ensure that the checks in ChoosingRam::increaseValuesOfParticipants validate that token IDs are not only within bounds but are also minted and assigned to a valid address. In general NFTs can be burned and hence a good practice is to check that they cannot be used in the selection process.
The following mitigation addresses both issues:

if (tokenIdOfChallenger >= ramNFT.tokenCounter() || ramNFT.ownerOf(tokenIdOfChallenger) == address(0)) {
revert ChoosingRam__InvalidTokenIdOfChallenger();
}
if (tokenIdOfAnyParticipant >= ramNFT.tokenCounter() || ramNFT.ownerOf(tokenIdOfAnyParticipant) == address(0)) {
revert ChoosingRam__InvalidTokenIdOfParticipant();
}
Updates

Lead Judging Commences

bube Lead Judge
over 1 year ago
bube Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

The token counter check is incorrect

Support

FAQs

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