First Flight #2: Puppy Raffle

Summary

This vulnerability results from the improper removal of refunded players, which are assigned to address-zero to imply that the player is removed, rather than being actually removed from the players array.

Consequently, the length of the array remains unchanged. This mismanagement has a significant impact on the selectWinner function, where the contract may not have enough funds to pay the winner, ultimately causing a reversion due to insufficient funds.

Vulnerability Details

In the refund function, instead of removing them from the players array, they are reassigned to address-zero, resulting in an unchanged array length.

This leads to a flawed calculation of the totalAmountCollected that affect prize pool and fee calculation in the selectWinner function, which is based on the array length, including players who have received refunds.

As a result, the prize pool and fee are overestimated, and when a winner is selected, the PuppyRaffle contract may not be enough funds in the contract to cover the full prize, resulting in a reversion due to insufficient funds.

Impact

The contract may not possess adequate funds to fully reward the winner, leading to a reversion and potential dissatisfaction among participants.

Example Attack Scenario

Consider the following scenario:

Assuming: There are 5 active players.

• players.length is 5,

• the fund in the contract is 5 * entranceFee.

After some time 4 players make a call for refudning, resulting in 4 refunded users, and the fund in the contract is reduced to 1 * entranceFee (pay fund back at L101).
However, the players.length remains at 5, including the refunded users as a zero addresses.

• players.length is 5,

• the fund in the contract is 1 * entranceFee ((5-4) * entranceFee).

Later, when selectWinner() is invoked, the totalAmountCollected is calculated with 5 * entranceFee, the prizePool is determined as 4 * entranceFee, and the fee is calculated as 1 * entranceFee.

• uint256 totalAmountCollected = players.length * entranceFee;

• uint256 prizePool = (totalAmountCollected * 80) / 100;

• uint256 fee = (totalAmountCollected * 20) / 100;

But, the actual fund in the contract is only 1 * entranceFee, which is less than the calculated prizePool.

As a result, when the contract attempts to send the prizePool to the winner, a reversion occurs at line 151 due to insufficient funds.

Tools Used

VS Code: Manual

Recommendations

Implement a mechanism to correctly remove players who have been refunded from the players array, ensuring that the array accurately represents active participants.

Here's an example reffernce logic of removing array element:

https://solidity-by-example.org/array/

Section: Examples of removing array element