Remediations for EIP-1283 reentrancy bug

In the case of net gas metering, the mismatch is in the other direction: contracts are paying far more gas than is required for SSTOREs that don’t result in any change on disk. Refunds would simply level that out, so inn t his case I believe it would be safe to have an uncapped refund counter.

I don’t believe anyone is suggesting removing the refund cap as a whole - only that refunds due to the gas savings from 1283 should be unlimited.

Ah, yeah maybe that was the intention. While that does get around the stuff I wrote, it’s not exactly straightforward either. Instead of keeping a global refund counter, where we eventually just cap and apply, we would instead have to have two separate refund counters, where one is uncapped, and the other capped.

Yeah, that’s the reason why from this thought experiment I would have a slight preference towards #1, compared with #5 or #7.

Not sure if there is already one but wouldn’t it be useful to have a “contract developer’s manual” (similar to “software developer’s manual” for microprocessors) whose instruction set reference explains in detail the operation of every opcode and what assumptions developers may make or not? A more descriptive and less formal version of YP’s Appendix H.2 perhaps.

1 Like

We absolutely need this. I have not been able to find an actual list of all Ethereum opcodes around (which makes me question how clients are developed in the first place - there must be some centralized place where developers of these clients find an overview of what their clients should do). For example this evm-opcode list of Trail of Bits shows opcodes which either have no description or are not in the Yellow Paper (I have not checked if TXEXECGAS actually exists or not (EDIT: does not exist - appears to be tx.gas instead of msg.gas proposed in some non-accepted EIP)). If there is a place where this is all documented, please let me know.

IMO, this documentation should be either the Yellow Paper edited or the Jello Paper (the “readable” Yellow Paper) edited in such way that it provides an overview of what EVM features are and what a developer can expect to be immutable. Examples are that semantics of non-invalid opcodes do not change, with exception to gas usage. Another thing which should be in there is what developers can expect the gas stipend to be immutable or not.

3 Likes

I disagree for reasons I explained here:

Intent is subjective and hard to establish and I don’t think we core developers should be in the business of trying to establish intent. Maybe someone wrote something a certain way on purpose. Maybe they made a mistake.

In any case, this is a social question, not a technical question. The underlying question here is, what social contract do contract developers have with Ethereum, explicit or implied?

I’m confused. In the first part, you’re saying that we should not be the judge of contract developer (that is, we don’t have a saying in whether a contract is doing a reasonable thing, or just “plain stupid”). Thus we should make sure all upgrades to be backwards-compatible. However, one of the solutions you give is that we make changes by introducing new opcodes.

Doesn’t new opcodes break backwards-compatibility for those contracts contained that new opcodes, if we don’t consider contract developers’ intent?

This is an extreme case. A contract should not be using an unused opcode. IMHO that should be a VM error (though I think in EVM it may be treated as a noop). Anyway we can get around this too with VM versioning. See Immutables, invariants, and upgradability.

Following what you wrote above, if we don’t consider contract developers’ intent, I disagree that this would be an “extreme” case compared with other cases you provided. We can well have developers who used customized compilers before we got designated invalid instruction, and that compiler happened to use another opcode for out-of-gas revert (because let’s face it, 0xfe isn’t that obvious, not like 0xff or 0x00, both of which have been used for other proposes). For the same reason, I argue changing invalid opcode from out-of-gas revert to noop would be a bad idea.

I do agree with you that account versioning (given it’s well designed) would solve the problem you wrote, but I think it’s a logical error if we use new opcodes to solve the problem you wrote.

I agree that this should not be changed now, but I consider this a design flaw in EVM. There should be a single “correct” opcode for OOG revert, and all other unimplemented opcodes should cause a VM error.

When there are fatal, low-level flaws in your VM design (and EVM has others), the only option may be to start from scratch–in our case, with empty shards in Eth2 and/or with Ewasm.

This is in the yellow paper - albeit, it’s not very readable.

2 Likes

If it helps the conversation/decision any, we’ve released an experimental version of ganache-cli which allows the user to set a flag to disable EIP-1283. Installation instructions are available at our release page here: https://github.com/trufflesuite/ganache-cli/releases/tag/v6.4.0-eip1283.0

tl;dr: npm install -g ganache-cli@eip1283

2 Likes

It seems that on today’s ACD call (https://www.youtube.com/watch?v=41kiRf1E-jI) there was some agreement to consider EIP-1283 again for the next update (Istanbul). It is considered together with http://eips.ethereum.org/EIPS/eip-1706 (or an alternative solution proposed by @chfast accomplishing the same fix). This has been documented in EIP-2200.

I am still worried there might be another bug in EIP-1283, because it decreases costs greatly. And we don’t seem to have another “good name” for a new Petersburg update.

@sorpaas how can we ensure that more testing and review before Istanbul would be able to spot issues?

@holiman what would be the best way to look for issues (e.g. parts of EVM affected, other potential cases which could affect already deployed contracts)? Would an “audit” useful in any way? Would an implementation in KEVM help detecting any potential issues?

We have many other gas reduction EIPs this time. All of them will break backward compatibility in some ways. I wouldn’t really think 1283 is any more dangerous compared with them.

As for our past experience, if we have issues, it is in most cases not about the EIP itself, but about how it interacts with existing contracts. What we can do is to reach out to developers in the ecosystem for feedbacks, as much as we can, to ensure the vast majority of the contracts do not have issues. That’d actually be enough.

But of course, for all those gas reduction EIPs, we can just use account versioning to avoid any potential backward compatibility problems. But we did not have agreement on this point yet (not only 1283, but also several other EIPs).

This is true. I’m aware that 1884 and 2045 propose changes/reductions. Is there any other left in Istanbul proposing reductions?

However, only 1283 reduces the cost of SSTORE, hence it is the only one having a potential effect cross call frames.

Do you have any reasons for thinking this other than general concern?

This EIP makes mutex way cheaper which is a much better pattern for limiting reentrancy than calls with a max gas limit of 5000. Long term it feels much safer to have.

Also, no live contracts were ever found that are vulnerable to even the original issue is fixed in 1706.

My reason is that the EIP was out for many months and the issue was only discovered a few days prior to launching on mainnet. Once the risk at that time was averted, I don’t think anyone kept reviewing the change for other issues.

Now we’ll have a single month for people to focus on it again. I don’t think we can afford another case of finding an issue last minute.

I barely consider the other issue to be an issue since there wasn’t a single live contract found that it would break, but I do see your point.

Personally, I don’t think we can afford another hardfork without some form of this EIP getting through.

  1. The gas cost of multi-step token transfers is absurdly high right now, and very limiting for DAPP developers. There are a number of uniswap extension contracts I want to write that will have prohibitively high gas costs without this EIP, and totally reasonable costs with it.

  2. This EIP finally makes mutex affordable which is much better pattern for stopping re-entrancy. The only reason there was any issue is people using gas limited sends to prevent re-entrancy, which is an anti-pattern.

    Reentrancy can become almost a non-issue if this upgrade goes through and Solidity adds the nonreentrant modifier like vyper did. This is way better than the current status quo.
    https://github.com/ethereum/vyper/issues/1204

  3. It add significant scaling to Ethereum 1.0 at essentially no cost (smarter gas pricing).

In my opinion this EIP is the lowest hanging fruit in ETH 1.0 improvements today and it would be a huge mistake to kill it.

For reference, there is a new proposal coming from @AlexeyAkhunov which I’m now promoting which is something in between option 5 and 3.

Already proposed here as number 6.

I admit I might have missed some responses in this thread, but critic found for 3/5 like options are:

I don’t believe the 4% gas cost increase in worst case matches with “seriously”.

I can try to add information how much gas in advance you need to provide in transaction to the spreadsheet. But so far “much more” is not very precise figure.


Also, would be nice to put #6 and #7 in the top comment.

What I meant by ‘seriously decreasing the impact’ is that the final gas cost is less important for scaling here than the gas limit of a transaction. If I want to execute 100 SSTOREs out of which 99 are just updating the single storage address I will need at least 21000 + 100 * 5000 gas now (gas limit of 521000), with the EIP 1283 it was meant to be 21000 + 5000 + 99 * 200 (gas limit of 45800). With the 2300 + 2100 refund the gas limit required would be 21000 + 5000 + 99 * 2300 (253700). Maybe we can test the transaction gas limit against the gas used - refund?