Remediations for EIP-1283 reentrancy bug

Well, not quite. They know that either

  1. If the tx goes OOG or error, they’ll get the full gas without doing all the corresponding work
  2. If the tx exit’s early, they’ll get the gas corresponding to that amount of work done. Since gasUsed is lower than gas, only gasUsed will count towards the block gas used, so they can fit another tx in with the gas that was not used.
  3. If there’s a refund, at least they get paid for half of the work done

I thing bullet three is quite important. Essentially, why would a miner ever choose to include a tx which they get nothing in transaction fees for (in the extreme that there was no limit on the refund)

I’m not saying that the whole thing is bad, just that the comment about removing the refund cap is not a good idea.

EDIT: another example from a malicious miner point of view:

I could make a tx which first burns throough 2M gas in a spin-loop or whatever, then clear some storage that I have previously filled (or gastoken), earn 2M of refunds. After tx, only 1 gas was consumed. So I can fill a block with 8M of those transactions (well, maybe keep it a bit lower than that :slight_smile: ), and mine that block. It would grind everyone to a halt, essentially allowing me to mine blocks with unlimited gasLimit amount of work in it, but still within the gasLimit.

2 Likes

If #7 is the actual intended behavior for #5, I would recommend changing the proposal to state that the gas cost is not 5,000 gas, but the original gas cost pre-EIP.

A misunderstanding of EIP 1283 was already the root cause of the Ropsten fork, so I think it’s crucial to be as precise as possible, to remove any assumption on how the reader will understand the proposal. That would help the correct review from third-party.

Has someone already proposed to create a new stack for every external function call,
by doing so, we could prevent called functions from changing the state of the calling function.
Or we could use a shadow stack that can not be written by external functions and compare it to the stack after the external function was completed.

Edit: Nvm, I just realized I did not understand the attack correctly.
I thought it was about smashing the existing stack

Nitpick, it actually wasn’t. An early theory was that there was a misunderstanding whether it was per-context or per-tx, but that wasn’t the case. The ropsten fork came because of an erroneous assumption in the implementation details, caused by having per-context refund counters instead of global refund counter. The assumption was that they could not go negative, which is only true for the global one. It’s true that EIP did not clarify this, but assumed the use of a global refund counter - a fair assumption imo, since YP only defines it as such.

@MariusVanDerWijden I think you’ll have to flesh out some more details, I don’t follow. There’s already a new stack for every external function call.

3 Likes

Could you please elaborate on what the semantic change in this context means?
Also, would you recommend that SSTORE revert if less than 5000 gas remains, and if not, why?

Regarding the initial choice of 2300 as minimal remaining gas as opposed to 5000. The gas cost of opcodes are not invariant and contract developers are warned about it, so providing 4999 gas is a logical mistake in my opinion, and I doubt that developers who had made it should be protected by the EVM, even if there is an instance of such contract on the mainnet. Relying on a gas stipend to be lower than storage write cost, on the other hand, seemed to be the original intention since the very beginning.

Trying to provide a draft what #5 and #7 would look like.

#5:

    If current value equals new value (this is a no-op), 200 gas is deducted.
    If current value does not equal new value
        If original value equals current value (this storage slot has not been changed by the current execution context)
            If original value is 0, 20000 gas is deducted.
            Otherwise, 5000 gas is deducted. If new value is 0, add 15000 gas to refund counter.
        If original value does not equal current value (this storage slot is dirty). Apply all of the following clauses.
            Deduct 5000 gas, and add 4800 gas to refund counter.
            If original value is not 0
                If current value is 0 (also means that new value is not 0), remove 15000 gas from refund counter. We can prove that refund counter will never go below 0.
                If new value is 0 (also means that current value is not 0), add 15000 gas to refund counter.
            If original value equals new value (this storage slot is reset)
                If original value is 0, add 19800 gas to refund counter.
                Otherwise, add 4800 gas to refund counter.

#7:

    If current value equals new value (this is a no-op).
        If current value is 0, and new value is not 0, deduct 20000 gas, and add 19800 gas to refund counter. Otherwise, deduct 5000 gas, and add 4800 gas to refund counter.
    If current value does not equal new value
        If original value equals current value (this storage slot has not been changed by the current execution context)
            If original value is 0, 20000 gas is deducted.
            Otherwise, 5000 gas is deducted. If new value is 0, add 15000 gas to refund counter.
        If original value does not equal current value (this storage slot is dirty). Apply all of the following clauses.
            If current value is 0, and new value is not 0, deduct 20000 gas, and add 19800 gas to refund counter. Otherwise, deduct 5000 gas, and add 4800 gas to refund counter.
            If original value is not 0
                If current value is 0 (also means that new value is not 0), remove 15000 gas from refund counter. We can prove that refund counter will never go below 0.
                If new value is 0 (also means that current value is not 0), add 15000 gas to refund counter.
            If original value equals new value (this storage slot is reset)
                If original value is 0, add 19800 gas to refund counter.
                Otherwise, add 4800 gas to refund counter.

Both look like fine fix. But still, compared with #1, I have a slight feeling that this tries to preserve an invariant that we really didn’t have. This is an good thing to discuss whether we should preserve those as it’s about immutability. However, I want to note that any hard fork related to EVM would inevitably break at least those contracts that is designed specifically to be broken by it. (For example, any new opcode breaks a contract that just have this opcode byte, any gas cost change prevents an assertion that GAS must equal to a specific value.)

2 Likes

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