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

wrong array manipulation in `refund()` function causing protocol to stop

Nocturnus

Summary

The intended behavior is to refund the entrance fee to the caller of the function whose address is stored in the players array. The refund() function logic facilitates the refund of the entrance fee to each player.

In addition, the selectWinner() function is designed to provide the winner with an NFT and 80% of the total collected amount, while the remaining 20% is allocated to the totalFee variable for the protocol's use.

Vulnerability Details

The refund() function employs the following array manipulation:

players[playerIndex] = address(0);

Issues arise when players are refunded. Their addresses in the players array are set to the zero address, but this doesn't affect the length of the players array. The erroneous array length is then used in the calculations of totalAmountCollected, prizePool, and totalFees, as shown in the code below:

uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);

This causes at least 2 critical issues in the protocol.

Impact

There are two critical issues that arise from the aforementioned problems:

  1. When only one player is refunded, and their index is set to the zero address, the length of the players array remains one more than the actual number of players.

Assuming there were initially four players in the raffle, and one of them is refunded, the actual number of players is three, but the length of the players array remains at four. Consequently, the totalAmountCollected will be miscalculated due to the erroneous length of the players array.

This miscalculation affects both the prize pool and the totalFees. As a result, the selectWinner() function is reverted with the message "PuppyRaffle: Failed to send the prize pool to the winner" due to insufficient funds. Additionally, the totalFees amount is impacted due to the incorrect calculation of the players array length.

The totalAmountCollected will be miscalculated due to wrong length of the players array:

uint256 totalAmountCollected = players.length * entranceFee; // 4 * 1 = 4
uint256 prizePool = (totalAmountCollected * 80) / 100; // 4 * 80 / 100 = 3.2
uint256 fee = (totalAmountCollected * 20) / 100; // 4 * 20 / 100 = 0.8
totalFees = totalFees + uint64(fee); // 0 + 0.8 = 0.8

Based on the provided details and calculations, the prize pool for the winner is expected to be 3.2 ether. However, due to the refunding of one of the players, only 3 ether remains in the contract. Consequently, when the selectWinner() function attempts to execute, it will be reverted with the message "PuppyRaffle: Failed to send prize pool to winner" due to the insufficient funds.

Furthermore, the totalFees variable is also impacted as a result of the miscalculation arising from the manipulation of the players array. This can potentially lead to further issues within the smart contract.

At this point, if a new player is added by callingenterRaffle() it will be possible to call selectWinner() and the prizepool will be send to the winner but the prizepool amount will be wrong.

uint256 totalAmountCollected = players.length * entranceFee; // 5 * 1 = 5
uint256 prizePool = (totalAmountCollected * 80) / 100; // 5 * 80 / 100 = 4
uint256 fee = (totalAmountCollected * 20) / 100; // 5 * 20 / 100 = 1
totalFees = totalFees + uint64(fee); // 0 + 1 = 1

The length of the players array is now calculated as 5 and according to the formula prizepool should be 4 ether. Since the balance of the contract is 4 there is enough ether to send to the winner.
On the other hand, the totalFees amount supposed to be 1 ether but since the whole 4 ether is sent to the winner the actual balance of the protocol is 0.

Considering the recalculated length of the players array as 5 and the corresponding prizePool calculation of 4 ether, the balance of the contract would indeed be sufficient to send to the winner in this scenario.

However, it's crucial to note that the totalFees amount is expected to be 1 ether, as determined by the calculation. Due to the entire 4 ether being sent to the winner, the actual balance of the protocol would be reduced to 0, resulting in a discrepancy between the expected totalFees and the actual balance.

  1. The second critical issue occurs when two players are refunded, and their indices are both set to the zero address, resulting in the inability for new players to enter the raffle. This is due to the prevention mechanism in the enterRaffle() function, which checks for duplicate entries in the players array. With two zero addresses present, the function reverts, blocking new entries.

The logic in the enterRaffle() function prevents new players to enter the raffle. There are 2 zero addresses in the players array. PoC in Test3

// Check for duplicates
for (uint256 i = 0; i < players.length - 1; i++) {
for (uint256 j = i + 1; j < players.length; j++) {
require(
players[i] != players[j], "PuppyRaffle: Duplicate player"
);
}
}

in the scenario where there were initially 10 or more players in the players array, the selectWinner() function could indeed be called, enabling the winner to receive the NFT and the prize pool, but with inaccurate calculations due to the previously highlighted critical issues.

While the winner may still be able to claim the NFT and the prize pool, these discrepancies can have broader implications for the fairness and transparency of the raffle system, as the actual allocation of funds may not align with the expected distribution based on the flawed calculations. Therefore, rectifying these issues is crucial to ensuring the integrity and reliability of the raffle protocol.

If initially there were 10 or more players in the players array, selectWinner() function can be called and winner can get the NFT and the prizepool although the calculations will be wrong due to detailed explanations provided in the previous critical case.

In cases where the number of initial players is less than ten and two players are refunded, the selectWinner() function reverts due to a shortage of funds compared to the protocol's balance.

Assume there were 9 players initially and 2 of them refunded. The balance of the contract will be 7 ether but the prizepool will be 7.2:

uint256 totalAmountCollected = players.length * entranceFee; // 9 * 1 = 9
uint256 prizePool = (totalAmountCollected * 80) / 100; // 9 * 80 / 100 = 7.2
uint256 fee = (totalAmountCollected * 20) / 100; // 9 * 20 / 100 = 1.1
totalFees = totalFees + uint64(fee); // 0 + 1.8 = 1.8

  • selectWinner() reverts due to above explanation.

  • withdrawFees() also reverts because:

function withdrawFees() external {
require(
address(this).balance == uint256(totalFees), // 7 != 1.8
"PuppyRaffle: There are currently players active!"
);

The only action can be to call refund() by all players.

Below are the tests used as PoCs:

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
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 = 1e18;
address playerOne = address(1);
address playerTwo = address(2);
address playerThree = address(3);
address playerFour = address(4);
address playerFive = address(5);
address playerSix = address(6);
address feeAddress = address(99);
uint256 duration = 1 days;
​
function setUp() public {
puppyRaffle = new PuppyRaffle(entranceFee, feeAddress, duration);
}
modifier playersEntered() {
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
_;
}
modifier playerEntered() {
address[] memory players = new address[](1);
players[0] = playerOne;
puppyRaffle.enterRaffle{value: entranceFee}(players);
_;
}
function testTest1() public playersEntered {
uint256 indexOfPlayer = puppyRaffle.getActivePlayerIndex(playerOne);
vm.prank(playerOne);
puppyRaffle.refund(indexOfPlayer);
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
console.log("Contract Balance Before selectWinner():",address(puppyRaffle).balance);
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
}
function testTest2() public playersEntered {
uint256 indexOfPlayer = puppyRaffle.getActivePlayerIndex(playerOne);
vm.prank(playerOne);
puppyRaffle.refund(indexOfPlayer);
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
console.log("Contract Balance Before selectWinner():",address(puppyRaffle).balance);
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}
function testTest3() public playersEntered {
uint256 indexOfPlayerOne = puppyRaffle.getActivePlayerIndex(playerOne);
vm.prank(playerOne);
puppyRaffle.refund(indexOfPlayerOne);
uint256 indexOfPlayerTwo = puppyRaffle.getActivePlayerIndex(playerTwo);
vm.prank(playerTwo);
puppyRaffle.refund(indexOfPlayerTwo);
address[] memory players = new address[](1);
players[0] = playerFive;
vm.expectRevert("PuppyRaffle: Duplicate player");
puppyRaffle.enterRaffle{value: entranceFee}(players);
console.log("Contract Balance Before selectWinner():",address(puppyRaffle).balance);
}
}

Tools Used

  • Manual code review

  • Foundry

Recommendations

Implement an if loop to verify the existence of the player in the players array before executing operations. This additional validation can help prevent issues stemming from the manipulation of the players array.

Updates

Lead Judging Commences

Hamiltonite Lead Judge almost 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.