0

I'm trying to exploit the following contract (based on the this thread: https://twitter.com/calyptus_web3/status/1664164696809783296) through reentrency:

contract Reentrency1 {
mapping (address => uint) public balance;

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

function withdraw(uint amount) public {
    require(balance[msg.sender] >= amount, "not enough balance");
    balance[msg.sender] -= amount;
    payable(msg.sender).transfer(amount);
}

}

Now this is my attack contract:

contract Reentrency1Hack {
address public victim;

event fallbackExecuted(uint amount);

function deposit(address _victim) public payable {
    victim = _victim;
    Reentrency1(_victim).deposit{value: 1 ether}();
}

function hack(address _victim) public payable {
    victim = _victim;
    Reentrency1(_victim).withdraw(1 ether);
}

function withdraw() public {
    payable(msg.sender).transfer(address(this).balance);
}

fallback() external payable {
    Reentrency1(victim).deposit();
    
    emit fallbackExecuted(msg.value);
}

}

The idea is that the msg.value upon executing the deposit() function from the follback() function would be still contain 1 ether and hence the balance would be updated.

The hack() function reverts, and it reverts really late - when the control is given back already to the hack() function after withdraw() is complete.

Doing this on Remix and solidity 0.8.13

Could you please help me what could be the issue here?

1
  • it looks like I hit the 2300 gas limit of transfer(), when switched to call and gave it more gas it worked. Jun 5, 2023 at 11:29

1 Answer 1

0

The idea is that the msg.value upon executing the deposit() function from the follback() function would be still contain 1 ether and hence the balance would be updated.

It doesn't work that way, msg.value is specific to each function call, as is msg.sender, so in the call from fallback() to deposit() it will be 0.

As to the revert, it must be gas. The transfer() in withdraw() sends a fixed amount of 2300 gas to fallback(), wich is not enough to call deposit() and throw the event. Change it to a call and it will pass:

//payable(msg.sender).transfer(amount);
msg.sender.call{value:amount}("");

In fact, the attack should never be possible with transfer() or send(), because of the fixed gas limit. The function is already using the Checks-Effects-Interactions pattern, so changing it to call() should be safe.

3
  • Thanks, that makes sense to me too, that the Checks-Effects-Interaction pattern prevents the reentrancy despite not having the dependency lock. It looks like then everyone on that twitter thread above are wrong and the contract is not really vulnerable, what do you think? Jun 6, 2023 at 14:51
  • At first glance it might seem so, but the tweet did not refer to a reentrancy issue, but a contract that is already secure but can be optimised.
    – ceseshi
    Jun 6, 2023 at 15:52
  • you might be right, I got misled by the comments, reentrancy was mentioned there a few times. Jun 6, 2023 at 20:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.