Remediations for EIP-1283 reentrancy bug

I find this a really interesting point to discuss on the “ethical” side of forks.

The fork is now delayed because this introduces reentrancies into contracts and some of them have been found. The main reason I think for the delay is because most smart contract writers have assumed that send/transfer guards against reentrancies while actually this is not a feature of them: it is just that on the current network it is indeed not possible. Because most developers were “educated” this way we can assume there are “many” contracts on the network which can now be attacked and therefore we delay.

But this poses the question. What if there is a contract with 1 ether in it. The only way for the owner to withdraw this Ether is to self-call into the contract and run into an INVALID opcode (which is not the designated INVALID opcode but just a random one which is currently not assigned (e.g. CREATE2 opcode pre-constantinople)).

If we now fork we break how this contract works. More fundamentally: the state change of a single TX pre-fork is now different than the state change of the same TX after-fork (and of course this can fundamentally be expanded to multi-tx state changes). Do we now move on with the fork?

I think everyone here would say “yes, of course, because this contract looks like it wants to prevent a fork to use this specific opcode which was INVALID before but we now want to use it for this EIP”. Or: “yes, this is the only contract affected”. Or: “there only is one ether in it”.

But is this ethical? What if there was 1 million Ether in it? How about 10 million? What if there are 10 contracts to be found? What about 100? What if 1% of all contracts are affected?

Can someone take the network hostage (against forks) by creating a contract which opposes a certain fork?

Examples are hence the above: can only withdraw Ether if the opcode is INVALID. You can expand this to ALL opcodes which are currently INVALID - if these are VALID opcodes (you would need some tricks here to make this contract of course because of stack management - but it is hypothetical for now) then this Ether is locked.

Or another example raised by @sorpaas the assertion that some opcode uses certain GAS. Could I have taken the network hostage by assering that extcodesize uses 20 gas (pre Tangerine Whistle) instead of 700?

These are nitpicky things to think about but I think we need to be more clear about what kind of changes we can expect in the future and what “immutable” features Ethereum has. Will gas stipend ever change? Etc.

We have changed semantics before. I think the general way of thinking has been like this…

  • Has the contract developer, who’s contract semantics may change, done a reasonable thing, or just plain stupid?

For example, we haven’t been too concerned with changing some gascosts, simply because ith has been a best practice for years not to hardcode expectations on gascosts.
Also, in the case of invalid opcodes, could a developer just make up an opcode and hope that it is unallocated forever? Why would someone make that assumption? Additionally, he’d have to code that up in assembly, since a soldity dev can’t add his own opcodes.

In the current case, it has been – if not best practice – at least commonly agreed that a solidity send and transfer cannot lead to state changing re-entrancy.

1 Like

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.