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

`selectWinner` can be called even if players are less than 4, owner loose funds

0xaraj

Summary

PuppyRaffle::selectWinner can be called even if we don't have 4 or more players, if protocol have enough balance. Because of PuppyRaffle::refund function implementation and wrong counting of players length.

Vulnerability Details

Let's take an example there are 4 players entered the raffle with 0.01 eth as fee(total 0.04 eth). Now 1 of them refunded, so ideally PuppyRaffle::selectWinner should not work because there are only 3 players, but it will work if we have extra eth in the contract balance to cover the fee of that left player. All this is happening because when we are refunding, we are only making refunder address(0) that is not decreasing the length of players array because address(0) is also counting as we can't remove a particular index element from an array.

How will protocol have extra eth to cover the fee of left player so protocol is collecting fee from every raffle round and if owner don't withdraw its eth then a point will come when there will be enough eth to cover the fee of that left player.

There will be two thing while selecting winner

  1. if left player address got selected(it will because address(0) is still there and we are counting winnerIndex using players.length) then tx. will revert because you can't mint nft on zero address.

  2. if a real player got selected(from those 3 players) then winner will be able to mint nft and it will get reward based on 4 players not on 3 players and that will leads to loss of funds to owner because it was his funds.

Impact

Loss of funds and wrong winner calculation.

//Here is Proof of concept
For extra eth I've made a deposit function in PuppyRaffle because if not then I've to run so many raffle to get enough eth as fee to cover the fee of left player.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
pragma experimental ABIEncoderV2;
​
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract PuppyRaffleTest is Test {
PuppyRaffle puppyRaffle;
uint256 entranceFee = 10000000000000000;
address playerOne = address(1);
address playerTwo = address(2);
address playerThree = address(3);
address playerFour = address(4);
address feeAddress = address(99);
address deployer = address(100);
uint256 duration = 1 days;
​
function setUp() public {
puppyRaffle = new PuppyRaffle(
entranceFee,
feeAddress,
duration
);
vm.deal(deployer, 100e18);
}
​
function testWinnerSelectedEvenAfterLessThanFourPlayer() external {
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
​
// 4 players are entering and aslo depositing 0.1 eth
vm.startPrank(deployer);
uint256 amountToCoverFee = 100000000000000000; //0.1 eth
puppyRaffle.deposite{value: amountToCoverFee}();
puppyRaffle.enterRaffle{value: entranceFee * players.length}(players);
console.log(address(puppyRaffle).balance);
​
// player index 0,1,3 got refunded
vm.startPrank(playerOne);
puppyRaffle.refund(0);
vm.startPrank(playerTwo);
puppyRaffle.refund(1);
vm.startPrank(playerFour);
puppyRaffle.refund(3);
console.log(address(puppyRaffle).balance);
​
//increasing the duration and calling the selectWinner
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
vm.startPrank(deployer);
puppyRaffle.selectWinner();
​
assert(puppyRaffle.previousWinner() != address(0));
}
}

Tools Used

Manual Review, Foundry

Recommendations

Use a counter to count the number of players instead of using players.length and that will help you to reduce the counter when player is getting refund because you can't remove a particular index element from array.

+ uint256 counter; // make this as state variable
+ counter++; // use this in `PuppyRaffle::enterRaffle` when you are pushing the address in players array
+ counter--; // use this in `PuppyRaffle::refund`
- uint256 totalAmountCollected = players.length * entranceFee;
+ uint256 totalAmountCollected = counter * entranceFee;
- require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
+ require(counter >= 4, "PuppyRaffle: Need at least 4 players");
Updates

Lead Judging Commences

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

refund-doesnt-reduce-players-array-size-causing-protocol-to-freeze

zero address can win the raffle

Funds are locked to no one. If someone gets the refund issue, they also got this issue. IMPACT: High Likelihood: High

Support

FAQs

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

Give us feedback!