## Description The PuppyRaffle.sol uses Solidity compiler version 0.7.6. Any Solidity version before 0.8.0 is prone to Overflow/Underflow vulnerability. Short example - a `uint8 x;` can hold 256 values (from 0 - 255). If the calculation results in `x` variable to get 260 as value, the extra part will overflow and we will end up with 5 as a result instead of the expected 260 (because 260-255 = 5). ## Vulnerability Details I have two example below to demonstrate the problem of overflow and underflow with versions before 0.8.0, and how to fix it using safemath: Without `SafeMath`: ``` function withoutSafeMath() external pure returns (uint256 fee){ uint8 totalAmountCollected = 20; fee = (totalAmountCollected * 20) / 100; return fee; } // fee: 1 // WRONG!!! ``` In the above code,`without safeMath`, 20x20 (totalAmountCollected \* 20) was 400, but 400 is beyond the limit of uint8, so after going to 255, it went back to 0 and started counting from there. So, 400-255 = 145. 145 was the result of 20x20 in this code. And after dividing it by 100, we got 1.45, which the code showed as 1. With `SafeMath`: ``` function withSafeMath() external pure returns (uint256 fee){ uint8 totalAmountCollected = 20; fee = totalAmountCollected.mul(20).div(100); return fee; } // fee: 4 // CORRECT!!!! ``` This code didnt suffer from Overflow problem. Because of the safeMath, it was able to calculate 20x20 as 400, and then divided it by 100, to get 4 as result. ## Impact Depending on the bits assigned to a variable, and depending on whether the value assigned goes above or below a certain threshold, the code could end up giving unexpected results. This unexpected OVERFLOW and UNDERFLOW will result in unexpected and wrong calculations, which in turn will result in wrong data being used and presented to the users. ## Recommendations Modify the code to include SafeMath: 1. First import SafeMath from openzeppelin: ``` import "@openzeppelin/contracts/math/SafeMath.sol"; ``` 2. then add the following line, inside PuppyRaffle Contract: ``` using SafeMath for uint256; ``` (can also add safemath for uint8, uint16, etc as per need) 3. Then modify the `require` inside `enterRaffle() function`: ```diff - require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle"); + uint256 totalEntranceFee = newPlayers.length.mul(entranceFee); + require(msg.value == totalEntranceFee, "PuppyRaffle: Must send enough to enter raffle"); ``` 3. Then modify variables (`totalAmountCollected`, `prizePool`, `fee`, and `totalFees`) inside `selectWinner()` function: ```diff - uint256 totalAmountCollected = players.length * entranceFee; + uint256 totalAmountCollected = players.length.mul(entranceFee); - uint256 prizePool = (totalAmountCollected * 80) / 100; + uint256 prizePool = totalAmountCollected.mul(80).div(100); - uint256 fee = (totalAmountCollected * 20) / 100; + uint256 fee = totalAmountCollected.mul(20).div(100); - totalFees = totalFees + uint64(fee); + totalFees = totalFees.add(fee); ``` This way, the code is now safe from Overflow/Underflow vulnerabilities.