Remediations for EIP-1283 reentrancy bug

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.

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

2 Likes

I agree with @ajsutton

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.

  1. Cost could be 2300 gas and 2100 gas refund instead of just cost 200 gas
2 Likes

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.

4 Likes

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.

1 Like

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.

6 Likes

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.

1 Like

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:

1 Like

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.

2 Likes

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.

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.

Another solution would keep the old mechanics until the last step of transaction where the amount will be refunded. This means that the old sstore gas mechanics dictates how much gas must be available before and during the transaction, but the new mechanics determine how much will be available after the transaction.

Another solution would keep the old mechanics until the last step of transaction where the amount will be refunded. This means that the old sstore gas mechanics dictates how much gas must be available before and during the transaction, but the new mechanics determine how much will be available after the transaction.

This is basically option 5.

1 Like

We should also add here proposal #0, which is to drop EIP-1283 as suggested by EIP-1283 Incident Report.

For simplicity, security and a more generic past/future friendly framework, we should consider both proposals #0 (drop EIP-1283) and #6 (EVM versioning based opcode/gas/behaviour updates). Once we have #6, we can reconsider EIP-1283 or any of its variants because the impact will be limited to contracts deployed with that latest version.

Or, if we are not too concerned about using up one more opcode, we could introduce EIP-1283 as SSTORE-CHEAPER (as @lrettig suggested) for now - proposal #7 which needs to be added to the above list. We will have to be judicious about opcode allocation going forward but this will avoid having to design an EVM versioning system and manage complex deployment/interaction scenarios. And this might suffice if the current EVM will eventually be replaced (before the opcodes run out) by an EWasm engine in Serenity Phase 2.

1 Like

Consider a Factory creating new Contract for every new user (like a Multisig). It could be a hardcopy or just a delegate wrapper. How versioning should work in this case? Looks like it should stick to Factory’s EVM version, even the newer EVM was introduced.

Should we give a possibility to “upgrade” the EVM version?

Yeah, that is a great edge case you have noticed. Theoretically, there should be some way to re-deploy contracts and upgrade them. And the current upgradeability scheme doesn’t seem to be well suited for it, so it needs to be though through.

Any child contract that is deployed by a Factory should theoretically share the same EVM version because otherwise it would be the same scenario as with this Constantinople situation: you have a vulnerable contract and you can’t do anything about it.

yeah, one of the advantages to the proposal #6 is that it allows more aggressive improvements to the EVM in a safer way.

I support #5, or #0, dropping the EIP.
For better or for worse, gas behaviour is a part of EVM semantics, and any direct change in gas costs will have possibility of changing the behaviour of already deployed contracts. The only way we can reduce the cost of transactions without changing their semantics is by putting the discount into the refund bucket.

Making changes to account for the gas stipend arbitrary number of 2300 would imply that contracts written with call.gas(2031).value(x) would now have changed behaviour. This is why I do not support proposal #3 or #1.
And as I final note, I don’t think changes in the EVM should not take into account how solidity writes contracts.

1 Like