Summary
The enterRaffle() function checks for duplicates by looping through the players array. As this array gets bigger with more entrants, new entrants have to pay much more gas compared to early entrants, leading to potential Denial of Service.
Vulnerability Details
Double-looping through an array is very gas-expensive on the EVM. As the players array grows bigger, gas cost for entering the raffle may become prohibitive.
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]);
}
@>
@> 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");
@> }
@> }
emit RaffleEnter(newPlayers);
}
Impact
Change the code in PuppyRaffleTest.t.sol and run forge test --mt testGasForDuplicateCheck -vvv to see how much more gas it costs for the first 100 and the second 100 players to enter, respectively.
+ function testGasForDuplicateCheck() public {
+ vm.txGasPrice(1);
+ uint256 playersAlreadyEntered = 100;
+ address[] memory players = new address[](playersAlreadyEntered);
+ for (uint256 i = 0; i < playersAlreadyEntered; i++) {
+ players[i] = address(i);
+ }
+ uint256 gasStart = gasleft();
+ puppyRaffle.enterRaffle{value: entranceFee * playersAlreadyEntered}(players);
+ uint256 gasEnd = gasleft();
+ uint256 gasUsedFirst = (gasStart - gasEnd) * tx.gasprice;
+ console.log("gasUsedFirst", gasUsedFirst);
+ for (uint256 i = 0; i < playersAlreadyEntered; i++) {
+ players[i] = address(i + playersAlreadyEntered);
+ }
+ gasStart = gasleft();
+ puppyRaffle.enterRaffle{value: entranceFee * playersAlreadyEntered}(players);
+ gasEnd = gasleft();
+ uint256 gasUsedSecond = (gasStart - gasEnd) * tx.gasprice;
+ console.log("gasUsedSecond", gasUsedSecond);
+ }
// gasUsedFirst 6252039
// gasUsedSecond 18067741
Tools Used
Recommendations
Get rid of the duplicate check, actors can create multiple wallets anyway. Alternatively, consider using a mapping(address => bool) to check for duplicates.
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]);
}
- // 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");
- }
- }
emit RaffleEnter(newPlayers);
}
