First Flight #2: Puppy Raffle
First Flight #2
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

winnerIndex will be same given fixed number of players and msg.sender bcz on-chain data generates a constant number everytime

anjalit

Summary

selectWinner using hash of on-chain data for generating random number to calculate winnerIndex generates same number everytime thus allowing same winner to mint multiple times.

Vulnerability Detail

selectWinner calculates winnerIndex using on-chain data block.timestamp ,block.difficulty and msg.sender along with length of players array. if we keep msg.sender and length same, winnerIndex will be same .

Doing some modifications in selectWinner such that it returns winnerIndex and the number caluclated(aka. randomNumber).

function selectWinner() external returns (uint256, uint256) {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
​
uint256 randomNumber = uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty)));
address winner = players[winnerIndex];
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);
​
uint256 tokenId = totalSupply();
​
// We use a different RNG calculate from the winnerIndex to determine rarity
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;
}
​
delete players;
raffleStartTime = block.timestamp;
previousWinner = winner;
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
_safeMint(winner, tokenId);
​
return (winnerIndex, randomNumber);
}

we can write a foundry test to check the randomness

  • suppose five players entered in the raffle and playerOne being the one to call selectWinner

modifier playersEntered() {
address[] memory players = new address[](5);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
players[4] = playerFive;
puppyRaffle.enterRaffle{value: entranceFee * 5}(players);
_;
}
​
function test_WinnerSameForSameNumberOfAddresses() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
vm.prank(playerOne);
​
(uint256 winnerIndex, uint256 randomNumber) = puppyRaffle.selectWinner();
console.log(winnerIndex);
console.log(randomNumber);
}
​
/* Logs:
2
84229153075551831252774086036520589045088249913386042721670938949046160372607
*/
​

  • if we change the order of players but keep the player at winnerIndex same as before and check the logs again

modifier playersEntered() {
address[] memory players = new address[](5);
players[0] = playerTwo;
players[1] = playerOne;
players[2] = playerThree;
players[3] = playerFive;
players[4] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 5}(players);
_;
}
​
function test_WinnerSameForSameNumberOfAddresses() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
vm.prank(playerOne);
​
(uint256 winnerIndex, uint256 randomNumber) = puppyRaffle.selectWinner();
console.log(winnerIndex);
console.log(randomNumber);
}
​
/*Logs:
2
84229153075551831252774086036520589045088249913386042721670938949046160372607
*/

it's clearly visible that the winnerIndex was same in both cases and the reason behind is randomNumber being same due to which playerThree was able to mint nft twice plus get the prize twice as well which is unfair to rest of the players.

Impact

Due to source of randomness not working properly a player can mint nft multiple times and get the rewards which is unfair to rest players and doesn't stand out on the behavior selectWinner was supposed to have

Tool Used

Foundry

Recommendation

Consider to not use on-chain data as the source of randomness to calculate winnerIndex.

One way of achieving this is to use chainlink VRFs as source to generte random number and Chainlink Automation to draw winner using that random number once the time interval has passed.

Updates

Lead Judging Commences

Hamiltonite Lead Judge about 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

weak-randomness

Root cause: bad RNG Impact: manipulate winner

Support

FAQs

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

Give us feedback!