Remediations for EIP-1283 reentrancy bug

[Arachnid]

@MicahZoltu

Can you clarify, what happens if someone issues a CALL with value and specifies 500 gas? Does the EVM increase the gas provided to 2300, or does it make the call with 500 gas?

It makes the call with 2800 gas.

[AlexeyAkhunov]

@Arachnid

Are you sure about this? I just tested 0.4.24, and it makes the call with a gas value of 0.

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

[Arachnid]

@jochem-brouwer

This whole stipend makes EIPs harder to review because we now always have to check if you can’t do any fancy tricks with the 2300 - 700 (700 for CALL) gas via reentrancy.

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

@jochem-brouwer

What if we do the same thing for EIP 1283 but now use a new opcode which does exactly the same as SSTORE but also implements EIP 1283 on dirty storage. (This might be an EIP which has been proposed before, sorry if that is the case).

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

@AlexeyAkhunov

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

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.

[AlexeyAkhunov]

@Arachnid

Why did the Solidity authors feel it important to allow for calls with no value to log things?

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

[lrettig]

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:

Immutables, invariants, and upgradability

One of the critical meta-questions raised by Remediations for EIP-1283 reentrancy bug and the delay of the Constantinople upgrade is: Precisely what on Ethereum is immutable and what behavior should be considered invariant? Since irregular state transitions are outside the scope of this conversation, for sake of argument let’s all agree that code and data (storage) are immutable. However, we’re left with the challenge that EVM semantics can and do change during a hard fork, the most germane ex…

[veox]

@ajsutton

For any potential solution we’ll need to remember that EIP-1283 has already rolled out on a number of test networks. As such, we’ll need a way to not break them - most likely by having clients support Constantinople-with-EIP-1283 and Constantinople-as-deployed-on-Mainnet.

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.

[lrettig]

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

[veox]

@MicahZoltu

what happens if someone issues a CALL with value and specifies 500 gas?

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.

[veox]

@AlexeyAkhunov

It has been introduced because lot of people complained about failing “send” to contracts if the value is 0.

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.)

[naikmyeong]

I agree with @ajsutton

[Arachnid]

@consciousEntity

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.

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.

[k06a]

6. Cost could be 2300 gas and 2100 gas refund instead of just cost 200 gas

[mandrigin]

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.

[benjamincburns]

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: publications/reviews/EIP-1283.pdf at master · trailofbits/publications · GitHub

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.

[Arachnid]

@mandrigin

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.

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.

[rajeevgopalakrishna]

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.

[rajeevgopalakrishna]

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.

[rajeevgopalakrishna]

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.

[subtly]

@Arachnid

It was explicitly designed in as an invariant, because it’s useful to be able to reason straightforwardly about ‘value sends’ while still providing the target contract with the opportunity to execute a little code. I don’t think we should throw that out, even in the (hypothetical and very unlikely) situation that we can without breaking anything already deployed.

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.

[subtly]

Although I agree with @AlexeyAkhunov, I think the best and safest option is 5: to require 5000 gas and refund 4800. This keeps the status quo (safe default) while also enabling all of the benefits of the EIP.