Summary
The selectWinner() function uses weak randomness to determine the winnerIndex. Additionally, it has no access control, allowing anyone to call it and attempt to manipulate the raffle.
Vulnerability Details
A malicious actor can call the selectWinner() function only if one of their entries is the winner, therefore manipulating the raffle.
function selectWinner() external {
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;
@> 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();
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);
}
Impact
Change the code in PuppyRaffleTest.t.sol and run forge test --mt testWeakRandomness to see that anyone can call the selectWinner() function only if they will be selected as the winner.
+ function _getRandomNumber() internal view returns (uint256) {
+ uint256 randomNumber = uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % 4;
+ return randomNumber;
+ }
+ function testWeakRandomness() public playersEntered {
+ vm.prank(playerOne);
+ for (uint256 i = 0; i < 500; i++) {
+ vm.warp(block.timestamp + duration + i);
+ vm.roll(block.number + i);
+ if (_getRandomNumber() == 0) {
+ puppyRaffle.selectWinner();
+ break;
+ }
+ }
+ assertEq(playerOne, puppyRaffle.previousWinner());
+ }
Tools Used
Recommendations
Best practice is to use an oracle to determine the winnerIndex, so that it cannot be manipulated by anyone including the owner. To prevent users from manipulating the raffle, add access control to restrict access to the owner.
- function selectWinner() external {
+ function selectWinner() external onlyOwner {
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;
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);
}
