Remediations for EIP-1283 reentrancy bug

hardfork
security
eip-1283

#42

It makes the call with 2800 gas.


#43

Yes. I encountered this during Gnosis Safe audit (I looked through byte code, rather than Solidity text). Just tried this code:

pragma solidity ^0.5.0;
contract Sender {
        function send(address payable target, uint256 amount) public {
                target.transfer(amount);
        }
}

If you perform

solc --asm Send.sol

look for the string “8fc” (which is hex for 2300), and you will see the code that adds the stipend if amount is 0:

        /* "Send.sol":108:131  target.transfer(amount) */
      0x8fc
        /* "Send.sol":124:130  amount */
      dup3
        /* "Send.sol":108:131  target.transfer(amount) */
      swap1
      dup2
      iszero
      mul
      swap1

#44

I agree - that’s why I personally favor options 1 or 2, because they make the stipend invariant more explicit.

Personally I think that’s quite a messy remediation - it adds a new opcode that exists solely to work around a bug elsewhere.

I see, it only adds to the amount if the value is 0. I have to say, this is pretty damn weird. Why did the Solidity authors feel it important to allow for calls with no value to log things?

Edit: Never mind, I get it. It’s to work around the case of x.transfer(y) where y happens to be 0 immediately failing due to out-of-gas.


#45

It has been introduced because lot of people complained about failing “send” to contracts if the value is 0. So they would either ask everybody to enclose their “send” into

if amount > 0 {
   target.send(amount)
}

Or make it for them, to stop them complaining :slight_smile:


#46

In general, and especially in light of Eth2 and Ewasm, I am not a fan of changing existing EVM semantics (introducing “breaking changes”) at this stage. Introducing a new opcode, instead of changing the behavior of an existing one, could have prevented this issue. I created a new thread to discuss this:


#47

No need for that.

Have another fork, “Constantinople Again”, that disables EIP-1283, and roll it out on testnets at first opportunity.

Then have another fork, “Constantinople Finally”, that re-enables EIP-1283, and enables an EIP that makes the “no SSTORE if below 2300 gas left” invariant explicit.

On main-net, roll out all three forks at the same block.

If testnets are not deemed that important, can also combine “Again” and “Finally” into one.


#48

For the record, this is more or less what @karalabe proposed, except as two forks rather than three.


#49

Here’s a snippet from py-evm:

child_msg_gas = gas + (constants.GAS_CALLSTIPEND if value else 0)

(gas and value are arguments to CALL.)


As shown in this discussion, reasoning gets a little more complicated when taking Solidity’s workaround to enable fingering into account:

// Provide the gas stipend manually at first because we may send zero ether.
// Will be zeroed if we send more than zero ether.
m_context << u256(eth::GasCosts::callStipend);
...
// gas <- gas * !value
m_context << Instruction::SWAP1 << Instruction::DUP2;
m_context << Instruction::ISZERO << Instruction::MUL << Instruction::SWAP1;

These lines are between ~2 and ~4 years old.


#50

For ref: implementing commit, PR, and issue. Via the latter - people discussing the workaround. That links to a blog post, which should help find discussions where “a lot of people complained” (by reference search).

(This is getting awfully OT, sorry.)


#51

I would say it’s actually a very clean remediation - instead of redefining an existing opcode, our set of desired properties is encoded into a new one.


#52

I agree with @ajsutton


#53

The only change to the existing opcode, though, is that it costs less gas in certain situations. If we weren’t aware that could introduce a vulnerability, I don’t think we’d even be considering it. And if we start introducing a new opcode for every gas cost change, we’ll run out of opcodes very quickly.


#54
  1. Cost could be 2300 gas and 2100 gas refund instead of just cost 200 gas

#55

One more (much technically challenging) solution would be to assign EVM version and gas prices to contracts at deployment. That means, that smart contract that is deployed before the hard fork is always executed with old gas prices (and old features of EVM).

So, we will have EVM0 (pre Constantinople) and EVM1 (Constantinople). When a new contract (running EVM1) calls anything that is deployed before that, EVM1 communicates to EVM0, and the old contract will use old gas prices and old assumptions will stay the same. This communication isn’t trivial, but since contracts have very specific interfaces it is not impossible.

Cons:

  • more complicated codebase and testing;
  • more complicated contract interaction;
  • bloating codebase with any hardforks;

Pros:

  • contracts that are already deployed aways will stay the same and behave the same;
  • incentive for those who can to upgrade their contracts to the new version because cheaper gas, etc.

I still think that this might solve the whole class of problems like that and might be worth it in the long run because the contracts behaviour would be truly immutable.


Immutables, invariants, and upgradability
#56

From Dan Guido (@dguido, Trail of Bits) via gitter:

Hello all, we teamed up with ChainSecurity and Eveem to investigate the ramifications of EIP-1283 over the last 24 hours. We have collected our findings and recommendations into a document which you can find here: https://github.com/trailofbits/publications/blob/master/reviews/EIP-1283.pdf

You will want to note a few key takeaways: We strongly recommend against adopting EIP-1283. If you must implement EIP-1283, then you should ensure that dirty storage is tracked per call, not per transaction. This will ensure that any call causing reentrancy will not be given a gas discount.

You will also note that we did find a few contracts that became vulnerable due to EIP-1283, however, our review was NOT exhaustive. All three of the analysis techniques that we used – ChainSecurity, Trail of Bits, and Eveem – have limitations.

I expect that we’ll update the doc again tomorrow with some minor additional notes, recommendations, and findings.
This was a race to the finish line, and I think we all would have wanted to do more.


#57

This is a reasonable idea - I’ll add it to the list. The main barrier is that it will require either a new consensus field for accounts, or some other means of communicating EVM versioning.

It’s worth noting that this doesn’t require two entirely separate EVMs, just some context that gets passed around for the current execution environment. Nodes already need most of this functionality to handle previous hard forks that have changed execution rules.

One way to handle this would be to introduce a new opcode, along these lines:

VERSION: Pops one element from the stack and changes the execution environment to the specified version. Clears stack and local memory before handing control to the new version, which begins executing at the next PC value.

Each new contract would then start with a prologue along the lines of PUSH 1 VERSION to enable the new EVM. This avoids the need to introduce new consensus data structures.

This can even be used for a transition to Web Assembly; contracts would just start with a prologue that switches the execution environment to EWASM.

Alternately, this could be a pseudo-opcode that’s only valid at the start of a contract, for simplicity reasons.


Immutables, invariants, and upgradability
#58

Very well articulated. IMO, this is very similar to challenges faced by microprocessor companies when introducing or modifying architectural features. The underlying micro-architecture can/will change to improve performance/power but existing architectural interfaces/semantics will remain the same or are enhanced with new features via new opcodes.

Backward-compatibility and Interoperability are social contracts with developers/users. This may be viewed as legacy baggage but guarantees that code running on one processor will have the same behaviour on any future processor.


Immutables, invariants, and upgradability
#59

Gas usage is exposed to the contract and therefore could have been used to encode certain semantics in it. So changing it in either direction could break existing contracts. It might have been explicitly stated that this could change in future and hence do not rely on it; but if it has been exposed to the developer then there’s no guarantee on how creatively it may have been used. :slight_smile:


#60

Versioning might get complicated but don’t see how we can avoid it. Either we version the opcodes i.e. SSTORE, SSTORE2 (similar to CREATE, CREATE2) and run the risk of exhausting one-byte opcodes forcing us to go multi-byte and variable-length opcodes, or we version the EVM semantics. EVM versioning gives the most flexibility IMO.


Immutables, invariants, and upgradability
#61

It is, as many have pointed out, implied functionality. Saying that it was explicitly designed as an invariant is misleading. There was always an expectation that storage would change because of how economically significant it is and due to the complexity of the EVM’s stack-based execution model. This particular feature was added when the idea of logging was added, and yes, it was added to solve the simple problem “if I receive ether to my code account, how will I know?”. The invariant, if there is one, is: there is enough gas to emit a log when a code account receives ether. The invariant was never that a called code account cannot call another method that modifies storage. Just because the behaviour is default, does not mean that it is an invariant.