# CodeHawk Puppy Raffle Smart Contract Audit Reports

Resource:
- [Link](https://www.codehawks.com/contests/clo383y5c000jjx087qrkbrj8)
- [Repo](https://github.com/Cyfrin/2023-10-Puppy-Raffle)

# Reports

## Too old version of solidity

Consider using fixed version of "updated" Solidity version

## [S-01] Denial of Service if players array goes too long during duplicate check.

### Severity: High

### Description
In `PuppyRaffle::enterRaffle`

Attacker can brute force the players array by sending lots player address

By running the following test case, the first 100 and 200 people has to pay huge different amount of gas.

Line 82

players.push(newPlayers[i]);

// 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");
    }
}

### Proof of Concept

Put following function in `test/PuppyRaffleTest.t.sol`

// forge test --mt testCanDOSEnterRaffle -vvv
function testCanDOSEnterRaffle() public {
    // Foundry lets us set a gas price
    vm.txGasPrice(1);
    uint playersNum = 100;
    address[] memory players = new address[](100);
    for (uint i = 1; i < 100; i++) {
        players[i] = address(i);
    }
    uint gasStart = gasleft();
    puppyRaffle.enterRaffle{value: entranceFee * players.length}(players);
    uint gasEnd = gasleft();
    uint256 gasUsedFirst = (gasStart - gasEnd) * tx.gasprice;
    console.log("Gas cost of the first 100 players: ", gasUsedFirst);

    // Creats another array of 100 players
    address[] memory playersTwo = new address[](playersNum);
    for (uint256 i = 0; i < playersTwo.length; i++) {
        playersTwo[i] = address(i + playersNum);
    }

    // Gas calculations for second 100 players
    uint256 gasStartTwo = gasleft();
    puppyRaffle.enterRaffle{value: entranceFee * players.length}(
        playersTwo
    );
    uint256 gasEndTwo = gasleft();
    uint256 gasUsedSecond = (gasStartTwo - gasEndTwo) * tx.gasprice;
    console.log("Gas cost of the second 100 players: ", gasUsedSecond);

    assert(gasUsedFirst < gasUsedSecond);
}

Output log

Logs:
  Gas cost of the first 100 players:  6252048
  Gas cost of the second 100 players:  18068138

### Mitigation

1. Consider allowing duplicates. User always can create a new wallet and participate in the game

2. Consider using mapping to check duplicates. Assign each wallet an UUID

+    mapping(address => uint256) public addressToRaffleId;
+    uint256 public raffleId = 0;
.
.
.
function enterRaffle(address[] memory newPlayers) public payable {
    require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
    for (uint256 i = 0; i < newPlayers.length; i++) {
        players.push(newPlayers[i]);
+            addressToRaffleId[newPlayers[i]] = raffleId;
    }

-        // Check for duplicates
+       // Check for duplicates only from the new players
+       for (uint256 i = 0; i < newPlayers.length; i++) {
+          require(addressToRaffleId[newPlayers[i]] != raffleId, "PuppyRaffle: Duplicate player");
+       }
-        for (uint256 i = 0; i < players.length; i++) {
-            for (uint256 j = i + 1; j < players.length; j++) {
-                require(players[i] != players[j], "PuppyRaffle: Duplicate player");
-            }
-        }
    emit RaffleEnter(newPlayers);
}
.
.
.
function selectWinner() external {
+       raffleId = raffleId + 1;
    require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");

## Potential Reentrancy

Did not follow the check effect action pattern

Effect after action is not recommand

Line 101

payable(msg.sender).sendValue(entranceFee);

## Incorrect implementation of getActivePlayerIndex

Incorrect return 0 if not found, it will equal to the first player index

Line 111

for (uint256 i = 0; i < players.length; i++) {
    if (players[i] == player) {
        return i;
    }
}
return 0;

## Not truly random winner

It is recommand to use oracle to get a true random value, attacker can pre-cauculate the winnderIndex and try to squeeze in it

Line 129

uint256 winnerIndex = uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;

## Arithmetic Overflow

Line 162

pragma solidity ^0.7.6;
...
uint64 public totalFees = 0;

function selectWinner() external 
    ...
    totalFees = totalFees + uint64(fee);

The max number of uint64 is `18,446,744,073,709,551,615`

So if the `totalFees` is larger then 18.446 ETH, it will overflowed and becomes a really small `totallFees` number

### Remeidation
- Upgrade to newer solidity version
- Use bigger uint like uint256 instead of uint64

## Potential destroy the feewithdrawal function

If using `selfDestruct()` can always makes this statement not fulfilled

Also, this should comes with `onlyOwner`, only owner can call this function

Line 157, 158

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

Considering change to

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