[FIXED] What causes a reentrancy error in this function?

Issue

    function recoverContribution() public payable{
        require(hasDeadlinePassed(), "deadline has not passed, contributions cannot be recovered rightnow");
        require(!(address(this).balance >= minimumTarget), "target has been met, cannot recover contributions now");
        require(contributors[msg.sender] != 0, "you have not contributed anything");
        payable(msg.sender).transfer(contributors[msg.sender]);
        contributors[msg.sender] = 0;
    }

The above function is called by a contributor to recover his/her funds incase the target has not been met and the dead line has passed.

this function gives a reentrancy error and a gas cost infinite error.

this function is extremely simple why would this function exhibit such potential errors?

Solution

Simple theory on reentrancy attacks.

When you use any transfer function on your contract, to transfer ether from your contract to someone else, if that someone else is another contract, the contract can execute logic before the transfer is finished ( check out the receive fallback function ), on this receive function, the attacker contract can call again your recoverContribution function, and keep on transferring and reentring.

Now, if you keep track of the ether AFTER the transfer, this will cause a reentrancy vulnerability because you'll update their ether balance only after the transfer has been executed, which is the case of your function, so they can empty your balance by calling recoverContribution, because your requires will pass the checks since you update their balance only after the transfer has been done.

To avoid this kind of attack, just update their balance BEFORE the transfer, this way their balance will be updated on each call, even if they are reentering from their receive function.

So basically to avoid reentrancy attacks on your function, just do:

   function recoverContribution() public payable{
    require(hasDeadlinePassed(), "deadline has not passed, contributions cannot be recovered rightnow");
    require(!(address(this).balance >= minimumTarget), "target has been met, cannot recover contributions now");
    require(contributors[msg.sender] != 0, "you have not contributed anything");

    contributors[msg.sender] = 0;
    payable(msg.sender).transfer(contributors[msg.sender]);
}

Answered By - MrFrenzoid

Answer Checked By - Clifford M. (PHPFixing Volunteer)