Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Severity: low
Valid

Off-by-one rarity thresholds mint 71/25/4 instead of the documented 70/25/5 distribution

verdoolaegelouis

Root + Impact

Root cause: the rarity thresholds in PuppyRaffle::selectWinner use <= against a value drawn from % 100, which shifts every band by one.

Impact: the minted distribution is 71 / 25 / 4 instead of the documented 70 / 25 / 5, so the legendary puppy is 20% rarer than advertised and the common one is over-issued.

Description

  • The contract documents three tiers through its constants: COMMON_RARITY = 70, RARE_RARITY = 25, LEGENDARY_RARITY = 5, which are meant to be percentages summing to 100.

  • rarity is keccak256(...) % 100, so it takes the 100 values 0 through 99. Testing rarity <= 70 accepts 71 of them, not 70, and the error cascades: the rare band keeps its 25 values, and the legendary band is left with only the 4 values 96..99.

uint256 rarity = uint256(keccak256(abi.encodePacked(msg.sender, block.difficulty))) % 100; // 0..99
@> if (rarity <= COMMON_RARITY) { // 0..70 -> 71 values, not 70
tokenIdToRarity[tokenId] = COMMON_RARITY;
@> } else if (rarity <= COMMON_RARITY + RARE_RARITY) { // 71..95 -> 25 values
tokenIdToRarity[tokenId] = RARE_RARITY;
} else {
tokenIdToRarity[tokenId] = LEGENDARY_RARITY; // 96..99 -> 4 values, not 5
}

Risk

Likelihood:

  • Deterministic. It applies to every mint the contract will ever perform, with no precondition.

Impact:

  • The scarcity the NFT collection advertises is wrong at issuance, and cannot be corrected retroactively once tokens are minted.

  • Holders and secondary-market buyers price legendary puppies on a stated 5% supply that the contract will never produce.

Proof of Concept

The band sizes follow directly from the comparison operators; counting them over the full domain makes the shift explicit:

function test_rarityBandsAreOffByOne() public {
uint256 common; uint256 rare; uint256 legendary;
​
for (uint256 rarity = 0; rarity < 100; rarity++) {
if (rarity <= 70) common++;
else if (rarity <= 95) rare++;
else legendary++;
}
​
assertEq(common, 71); // documented as 70
assertEq(rare, 25); // correct
assertEq(legendary, 4); // documented as 5
}

Control — with strict comparisons the same loop reproduces the documented distribution exactly, confirming the operators are the only cause:

function test_strictComparisonsGiveTheDocumentedSplit() public {
uint256 common; uint256 rare; uint256 legendary;
​
for (uint256 rarity = 0; rarity < 100; rarity++) {
if (rarity < 70) common++;
else if (rarity < 95) rare++;
else legendary++;
}
​
assertEq(common, 70);
assertEq(rare, 25);
assertEq(legendary, 5);
}

Recommended Mitigation

Use strict comparisons so each band covers exactly its stated number of values:

- if (rarity <= COMMON_RARITY) {
+ if (rarity < COMMON_RARITY) {
tokenIdToRarity[tokenId] = COMMON_RARITY;
- } else if (rarity <= COMMON_RARITY + RARE_RARITY) {
+ } else if (rarity < COMMON_RARITY + RARE_RARITY) {
tokenIdToRarity[tokenId] = RARE_RARITY;
} else {
tokenIdToRarity[tokenId] = LEGENDARY_RARITY;
}

A test asserting that the three bands sum to 100 and match the constants would have caught this and guards against regressions.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 3 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[L-03] Participants are mislead by the rarity chances.

## Description The drop chances defined in the state variables section for the COMMON and LEGENDARY are misleading. ## Vulnerability Details The 3 rarity scores are defined as follows: ``` uint256 public constant COMMON_RARITY = 70; uint256 public constant RARE_RARITY = 25; uint256 public constant LEGENDARY_RARITY = 5; ``` This implies that out of a really big number of NFT's, 70% should be of common rarity, 25% should be of rare rarity and the last 5% should be legendary. The `selectWinners` function doesn't implement these numbers. ``` uint256 rarity = uint256(keccak256(abi.encodePacked(msg.sender, block.difficulty))) % 100; if (rarity <= COMMON_RARITY) { tokenIdToRarity[tokenId] = COMMON_RARITY; } else if (rarity <= COMMON_RARITY + RARE_RARITY) { tokenIdToRarity[tokenId] = RARE_RARITY; } else { tokenIdToRarity[tokenId] = LEGENDARY_RARITY; } ``` The `rarity` variable in the code above has a possible range of values within [0;99] (inclusive) This means that `rarity <= COMMON_RARITY` condition will apply for the interval [0:70], the `rarity <= COMMON_RARITY + RARE_RARITY` condition will apply for the [71:95] rarity and the rest of the interval [96:99] will be of `LEGENDARY_RARITY` The [0:70] interval contains 71 numbers `(70 - 0 + 1)` The [71:95] interval contains 25 numbers `(95 - 71 + 1)` The [96:99] interval contains 4 numbers `(99 - 96 + 1)` This means there is a 71% chance someone draws a COMMON NFT, 25% for a RARE NFT and 4% for a LEGENDARY NFT. ## Impact Depending on the info presented, the raffle participants might be lied with respect to the chances they have to draw a legendary NFT. ## Recommendations Drop the `=` sign from both conditions: ```diff -- if (rarity <= COMMON_RARITY) { ++ if (rarity < COMMON_RARITY) { tokenIdToRarity[tokenId] = COMMON_RARITY; -- } else if (rarity <= COMMON_RARITY + RARE_RARITY) { ++ } else if (rarity < COMMON_RARITY + RARE_RARITY) { tokenIdToRarity[tokenId] = RARE_RARITY; } else { tokenIdToRarity[tokenId] = LEGENDARY_RARITY; } ```

Support

FAQs

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

Give us feedback!