[M-1] high gas cost in PuppyRaffle::enterRaffle leads to the potential Denial of service attack(DOS)
Description
In line Line: 87 , the attack using DOS can be done.due to grow in size of the player array , gas cost increases as the result it will lead to revert the transcation in any case by Organizer.This attack is known as DOS.
@> 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");
}
}
Imapct
The for loop used in PuppyRaffle::entryRaffle for checking the duplicate addresses present in the PuppyRaffle::players array.
As the looping depends on the size of the PuppyRaffle::players array, the gas Cost increases.which can even leads to the DOS attack,
where the transaction get into error out of gas.An attacker can purposefully "stuff" the array to prevent others from entering, potentially rigging the raffle.
Proof of Concept
demonstrated the test on the basis of two senarios.
when the Organizer pass PuppyRaffle::players array with 400 players in PuppyRaffle::enterRaffle
function test_for_dos() public {
address[] memory players = new address[](400);
for (uint256 i = 0; i < 400; i++) {
players[i] = address(uint160(i));
}
uint256 gasAtStart = gasleft();
puppyRaffle.enterRaffle{value: entranceFee * players.length}(players);
uint256 gasAtEnd = gasleft();
console.log("gas at start is : ", gasAtStart);
console.log("gas at end left is :", gasAtEnd);
}
Ran 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] test_for_dos() (gas: 542396706)
Logs:
gas at start is : 1073648110
gas at end left is : 531326975
when the Organizer pass PuppyRaffle::players array with 695 players in PuppyRaffle::enterRaffle (Crossing the Max Limit of players)
function test_for_dos() public {
address[] memory players = new address[](695);
for(uint256 i = 0; i < 695;i++){
players[i] = address(uint160(i));
}
uint256 gasAtStart = gasleft();
puppyRaffle.enterRaffle{value: entranceFee * players.length}(players);
uint256 gasAtEnd = gasleft();
console.log("The value of gas is : ", gasAtStart);
console.log("The value of gas is : ", gasAtEnd);
assertEq(gasAtEnd , gasAtStart);
}
[FAIL: EvmError: Revert] test_for_dos() (gas: 1056949180)
Traces:
[1056949180] PuppyRaffleTest::test_for_dos()
|- PuppyRaffle::enterRaffle{value: 695000000000000000000}([0x0000000000000000000000000000000000000000, ..., 0x00000000000000000000000000000000000002b6])
| \- <- [OutOfGas] EvmError: OutOfGas
\- <- [Revert] EvmError: Revert
Recommended Mitagation
For Reducing the Risk of DOS there are two things to reduce Mitigate the Attack
Bound the number of player to enter in game in one slot by user throguh PuppyRaffle::enterRaffle.
insted of inner loop use mapping because the time complexity of using for loop to find the duplicate is O(n) and the mapping is
O(1) with if condition.
example:
+ mapping(address => bool) public players;
function enterRaffle(address[] memory newPlayers) public payable {
- require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
+ require(newPlayers.length <= 10 , "Max slot reached");
+ 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
- // @audit DOS attack is possible over here
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");
- }
+ require(!players[newPlayers[i]],"Duplicate Player")
+ players[newPlayers[i]] = true;
+ players.push(newPlayers[i]);
+ }
emit RaffleEnter(newPlayers);
}
