Remediations for EIP-1283 reentrancy bug

bradleat · January 17, 2019, 7:24am

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.

Arachnid · January 17, 2019, 7:59am

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.

rajeevgopalakrishna · January 17, 2019, 9:35am

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.

Ethernian · January 17, 2019, 12:31pm

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?

mandrigin · January 17, 2019, 1:59pm

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.

mandrigin · January 17, 2019, 2:05pm

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

MrChico · January 17, 2019, 4:55pm

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.

cheeselord1 · January 17, 2019, 5:42pm

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.

montyly · January 17, 2019, 11:31pm

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

MrChico · January 18, 2019, 1:15am

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.

Arachnid · January 18, 2019, 2:12am

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.

holiman · January 18, 2019, 7:46am

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.

MrChico · January 18, 2019, 9:28am

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?

equivrel · January 18, 2019, 9:49am

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.

holiman · January 18, 2019, 9:52am

Well, not quite. They know that either

If the tx goes OOG or error, they’ll get the full gas without doing all the corresponding work
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.
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 ), 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.

montyly · January 18, 2019, 9:57am

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.

MariusVanDerWijden · January 18, 2019, 10:02am

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

holiman · January 18, 2019, 10:02am

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.

alex-forshtat-tbk · January 18, 2019, 12:02pm

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.

sorpaas · January 18, 2019, 1:24pm

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