Remediations for EIP-1283 reentrancy bug

hardfork
security
eip-1283

#62

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.


#63

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.


#64

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.


#65

This makes my point - even in such a “simple” change, changing the way an existing opcode works is fraught with difficulty.

Personally, I think that an opcode should largely continue functioning as originally released - as has been said already, developers will devise all sorts of creative solutions, and unless they tell us what they all are, changing the operating characteristics of an opcode risks breaking things.

If there are such changes to an opcode, I certainly think it is worth making it into a larger package of improvements and creating a new opcode.


#66

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.


#67

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?


#68

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.


#69

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


#70

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.


#71

I think #5 is the cleanest way of fixing the issue. It maintains nearly all the benefits of EIP-1283 (lower gas usage for certain contracts). Also should not break any existing functionality since base SSTORE cost is still 5000 and any discount is handled in refunds.

Though if #5 involves a significant delay to getting Constantinople out the door, would prefer to just drop the EIP.


#72

Hello, Trail of Bits has reviewed the six proposals from the Ethereum Magicians forum for resolving the security issues with EIP-1283. All of the six proposals are incomplete or introduce undesirable changes in existing contracts. We recommend selecting one of the following paths forward:

  • Abandon EIP-1283 and avoid implementing it at all.
  • Implement our own proposal #7 to address the security risks of EIP-1283.

We have detailed proposal #7 in Appendix C of our EIP-1283 report document. You can find the latest copy (1.2) of it here: https://github.com/trailofbits/publications/blob/master/reviews/EIP-1283.pdf


#73

What ToB outlines as prop #7 is how I interpreted that #5 would work. Of course the gas discount should be given in the refund in all cases, not just the 5000 dirty store case.


#74

Can you please elaborate? Dismissal without discussion is not very useful.

Edit: Sorry, I see they are discussed in the new version of the report. Addressing these:

I believe this is #5.


#75

So do I, more or less at least. However, there’s one additional thing in their #7.

Currently, a refund cannot exceed more than half of the gas used per transaction. We may want these proposed refunds to bypass this upper bound in order to fully take advantage of indended deductions.

IMO, this would be a DoS vector. That would mean that a user can submit a tx with a lot of gas, which looks juicy (maybe even with a high gasPrice). After executing the transaction, however, the actual gas usage would be way below the actual corresponding work done by the miner (and all nodes processing the tx). The gas refund is really refunded on the expense of the miner.

It becomes a mismatch between work performed and work paid for, which is inherently dangerous.

Additionally, there’s’ a high likelihood that such a tx would never be included in a block, since it doesn’t actually pay for itself properly. So these transactions could easily slosh around in pools on the network and cause problems.


#76

IMO, this would be a DoS vector. That would mean that a user can submit a tx with a lot of gas , which looks juicy (maybe even with a high gasPrice ). After executing the transaction, however, the actual gas usage would be way below the actual corresponding work done by the miner (and all nodes processing the tx). The gas refund is really refunded on the expense of the miner.

I don’t think this is a valid argument. It is already the case that miners do not know beforehand how much gas will actually end up being used in a tx. And refunds should only be given out when there actually is less work to be done, isn’t this the motivation for the discount in the first place?


#77

I agree. I think that moving a gas discount from run-time to refund is basically a no-op from the miner point of view, since you could achieve the same effect you are describing by just submitting transactions with a high gas limit that don’t actually consume much gas. On the other hand, this is semantically preferable since you can play around with the refund without affecting run-time semantics of the contract, which is exactly what we want to avoid as much as possible if we want contracts to be truly “immutable”.

I also agree that ToB’s #7 is probably how most people expected #5 to work.


#78

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.


#79

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.


#80

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


#81

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.