[H-3] Rarity for Every Snek is Set to 33 Instead of the Suggested Rarity, Causing the Rare and Legendary NFTs to be as Common as Common Ones
Description: The rarity of the NFTs is set using the random word in the snek_raffle::fulfillRandomWords function, but the value is set by modding the random word by 3 instead of 100. This results in a rarity distribution that does not reflect the intended rarity levels, making rare and legendary NFTs as common as common ones.
@internal
def fulfillRandomWords(request_id: uint256, random_words: uint256[MAX_ARRAY_SIZE]):
index_of_winner: uint256 = random_words[0] % len(self.players)
recent_winner: address = self.players[index_of_winner]
self.recent_winner = recent_winner
self.players = []
self.raffle_state = RaffleState.OPEN
self.last_timestamp = block.timestamp
@> rarity: uint256 = random_words[0] % 3
self.tokenIdToRarity[ERC721._total_supply()] = rarity
log WinnerPicked(recent_winner)
ERC721._mint(recent_winner, ERC721._total_supply())
send(recent_winner, self.balance)
Impact: This issue significantly undermines the rarity factor, making the NFTs less valuable and less desirable to collectors.
Proof of Concept:
First to proove this concept we must change chainlinkvrfMock.vy to actually produce a random number:
Change the chainlinkvrfMock.vy so it returns the block.timestamp as random number
@external
def fulfillRandomWords(requestId: uint256, consumer: address):
"""Returns an array of random numbers. In this mock contract, we ignore the requestId and consumer.
Args:
requestId (uint256): The request Id number
consumer (address): The consumer address to
"""
# Default to 77 as a mocking example
- words: uint256[MAX_ARRAY_SIZE] = [77]
+ words: uint256[MAX_ARRAY_SIZE] = [block.timestamp]
self.fulfillRandomWordsWithOverride(requestId, consumer, words)
Add the following Script to the test suit snek_raffle_test.py:
Note that the random library is used here to move the time so the vrfcoordintorMock actually produce a randomNumber:
import random
from collections import Counter
def test_rarity(
raffle_boa_entered, vrf_coordinator_boa, entrance_fee
):
rarities = []
for j in range(100):
additional_entrants = 10
for i in range(additional_entrants):
player = boa.env.generate_address(i)
boa.env.set_balance(player, STARTING_BALANCE)
with boa.env.prank(player):
raffle_boa_entered.enter_raffle(value=entrance_fee)
starting_balance = boa.env.get_balance(USER)
rand = int(random.randrange(1,100))
boa.env.time_travel(seconds=INTERVAL +1+rand)
raffle_boa_entered.request_raffle_winner()
vrf_coordinator_boa.fulfillRandomWords(0, raffle_boa_entered.address)
recent_winner = raffle_boa_entered.get_recent_winner()
winner_balance = boa.env.get_balance(recent_winner)
prize = entrance_fee * (additional_entrants + 1)
rarities.append(raffle_boa_entered.get_token_Rarity(j))
item_counts = Counter(rarities)
print(item_counts)
assert(rarities.count(2) >= 5)
The printed output will be something like this:
tests/snek_raffle_test.py ..Counter({2: 47, 0: 30, 1: 23})
which indicates that more than 5 percent of the NFTs are actully rare.
Recommended Mitigation: To address this issue, the rarity determination should be adjusted to reflect a more balanced distribution. This can be achieved by modding the random word by 100 instead of 3, allowing for a more granular distribution of rarities. Additionally, the rarity distribution should be clearly defined and implemented in the tokenURI function to ensure that each rarity category is represented accurately.
Here's how the fulfillRandomWords function can be revised to implement the recommended mitigation:
@internal
def fulfillRandomWords(request_id: uint256, random_words: uint256[MAX_ARRAY_SIZE]):
index_of_winner: uint256 = random_words[0] % len(self.players)
recent_winner: address = self.players[index_of_winner]
self.recent_winner = recent_winner
self.players = []
self.raffle_state = RaffleState.OPEN
self.last_timestamp = block.timestamp
- rarity: uint256 = random_words[0] % 3
+ rarity: uint256 = random_words[0] % 100
self.tokenIdToRarity[ERC721._total_supply()] = rarity
log WinnerPicked(recent_winner)
ERC721._mint(recent_winner, ERC721._total_supply())
#q misshandling of eth? pull over push
send(recent_winner, self.balance)
And the tokenURI function should be updated to reflect the new rarity distribution:
@external
@view
def tokenURI(token_id: uint256) -> String[53]:
rarity: uint256 = self.tokenIdToRarity[token_id]
+ if rarity < LEGEND_RARITY:
+ return self.rarityToTokenURI[2]
+ elif rarity < RARE_RARITY:
+ return self.rarityToTokenURI[1]
+ else:
+ return self.rarityToTokenURI[0]
- return self.rarityToTokenURI[rarity]
This approach ensures that the rarity distribution is more balanced, with each rarity category having a distinct chance of being selected. This will make the NFTs more valuable and desirable to collectors, addressing the issue of rare and legendary NFTs being as common as common ones.
