0

I'm not ure why I keep getting a potential reentrancy attack vulnerability about the "publishEtherBox" function. The contract code is not complete but it should be enough to understand. Is it a bad syntax issue or maybe else?

contract {
...
  struct EtherBox {
    bytes32 label;
    address owner;
    string ownerUrl;
    uint256 expiration;
  }

  mapping (address => bytes32) public nicknames;
  mapping (address => address[]) public ownerToEtherBoxes;
  mapping (address => EtherBox) public etherBoxes;
  mapping (address => uint256) etherBoxesNonce;
...
  function publishEtherBox (bytes32 _label, string _ownerUrl, uint _lifespan) external onlyWhenRunning() payable {
      require(ownerToEtherBoxes[msg.sender].length < 10);
      assert(bytes(_ownerUrl).length <= 200);
      address etherBoxAddress = address(keccak256(msg.sender, etherBoxesNonce[msg.sender]++));
      ownerToEtherBoxes[msg.sender].push(etherBoxAddress);
      etherBoxes[etherBoxAddress] = EtherBox({
        label: _label,
        owner: msg.sender,
        ownerUrl: _ownerUrl,
        expiration: now + _lifespan
      });
      if(msg.value > 0){
        owner.transfer(msg.value);
      }
      emit EtherBoxPublished(msg.sender, nicknames[msg.sender], etherBoxAddress, _label, now);
  }
...
}
0

There's no real issue here, but if you want to silence the compiler warning, you can make the .transfer() call the last thing in the function:

function publishEtherBox(...) ... {
    ...
    emit EtherBoxPublished(msg.sender, nicknames[msg.sender], etherBoxAddress, _label, now);
    if (msg.value > 0) {
        owner.transfer(msg.value);
    }
}
  • cool! what happens if the transfer function fails, will the event be reverted too? – user38075 Jun 3 '18 at 21:28
  • Yes. Transactions are just that; if they fail, they have no effect (except consuming the gas used). – user19510 Jun 3 '18 at 21:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy