Remediations for EIP-1283 reentrancy bug

I mean removing the then redundant 2300 gas cap on send and transfer in the case that we went with 2.

I wouldn’t call that bloated, it’s a useful call type. But I’ll happily leave that decision to others.

This would be a change in solidity? We can’t change contracts which are on the chain already, so we cannot simply change that any contract which forwards 2300 gas now suddenly forwards less.

In solidity, I think the reasonable change would be to assume that CALL always costs at least 700 gas and then hence only forward 700 gas. This can only lead to problems if the fallback function is so deep in the contract (too much solidity functions are around) such that the 700 gas is already used before it reaches the fallback function. Of course this can be optimized via a check that if CALLDATALENGTH==0 it immediately jumps to the fallback but this adds a slight amount of overhead gas before you get into any function block.

I think this is a bad idea both because we can’t guarantee we’ll identify all vulnerable contracts, and because we shouldn’t violate this invariant retrospectively.

FWIW - delegatecall is a different call type, but results in a regular call context (it executes as normal). Likewise for callcode. We wouldn’t need a new opcode for this, either.

I wasn’t aware Solidity does this. The call stipend is an EVM feature; Solidity could equally set the gas to 0 with the same effect.

In my opinion, the EIP-1283 does not need to be remediated, but rather removed all together. I do understand its intent (making inter-frame communications cheaper), and I proposed an alternative which is cheaper, slightly more powerful, and without the semantic changes to the existing opcodes:

Looking at the history EIP-1283, we see that it has been EIP-1087, formulated in a way which was harder to implement in Parity than in Geth. Then, it was reformulated into EIP-1283, to make it more abstract and less implementation-dependent. However, the semantic complexity was still there, and that caused a consensus issue on Ropsten, which delayed Constantinople once. And now it delays Constantinople once again, due to unintended consequences. Both of these incident can be traced to two things this EIP is doing that were not done before:

  1. Reducing refund counter (prior to that, refund counter was only increased). That was basis of the Ropsten bug
  2. Reducing the gas cost on an action. In all other hard forks, the gas cost of operations were only increased.

There is another potential impact of this EIP, which has not been appreciated yet. It will make harder for State Rent group to figure out alternatives to the State Rent, because the edge cases introduced by this EIP will make analysis harder.

My suggestion is to simply exclude this EIP from Constantinople. If the inter-frame communication is still in demand, it can be introduced by a more specialised change, rather than piggy-backing on the existing resource (contract storage). But honestly, I think this particular cost reduction is not a very important feature, and app developers can definitely live without it, while we are concentrating our efforts on Ethereum 1x and scalability improvements instead.

7 Likes

I agree with @Arachnid. Besides doing a deep chain analysis which will take very long to “look” correct it will be extremely hard to prove that there are a low amount of contracts susceptible. Keep in mind that simply looking for “drain” attacks are just part of the scope. It is also possible to do funky things to the contract by writing to some storage slots which messes up the logic of the contract. I think this not doable, but I have no experience with static analysis so that might just be my inexperience.

No this does not work, because the idea behind transfer and send is that you are also allowed to send Ether to contracts which have a payable fallback. These hence execute (given a solidity-compiled contract) the code to select a function: if none is found, it checks if there is a (payable) fallback. (Hence forwarding 0 gas will immediately result in an out of gas error).

Besides all this I agree with @AlexeyAkhunov to remove it altogether. I think in most cases writing to the same storage slot in a contract twice should be removed at compile time. At least in transactions which only happen during a single call this can be removed at compile-time. There might be some cases that this is harder if there are calls to other contracts (which call back into the current contract), but I think with some tricks this might also be possible. Hence the only case where this EIP is applicable is where a contract calls another one, which calls back into the current one. In all other cases more than single-writes to the same storage slot can be removed at compile time.

1 Like

IF we can gain the necessary confidence that this bug isn’t exploitable against any active contracts in the wild, why is maintaining this invariant necessary? Not only was the invariant only implied, not explicitly stated, but if no one is depending on it what do we gain by maintaining it?

2 Likes

Unfortunately, it cannot be removed at compile time, IMO. In order to do that, there would need to be an alternative resource in EVM that allows retaining information between the call frames. Currently, only storage is retained between the frames, since fresh memory is given to each frame, and the stacks are segregated too.

2 Likes

Yes, this is why I noted that you cannot remove it if there is a call to another contract which calls back into the current one. But you are right, it is more specific: it there are multiple call frames on the current contract then it is not possible to remove at compile time. However, I think there might be some kind of trick via the returnvalue of a call frame.

I’m generally against asserting that we will never decrease gas costs. In order for gas accounting to function appropriately we need the power/authority to be able to tune them up/down as hardware/software changes with time. While I can appreciate that having this ability makes things harder, I believe it is necessary for keeping Ethereum competitive.

IIUC, it sounds like you are sort of arguing that Ethereum 1.x is in maintenance mode and we basically shouldn’t be doing any active development on it that isn’t necessary for Ethereum 2.0? Any changes to the EVM should be proposed targeting Ethereum 2.0 instead?

I am not saying we should not decrease costs of operations. I am pointing out that there was complexity in EIP which was not on the plain sight. And it should have been given more weight when discussing the alternatives.

Yes. It has already been proposed by Vitalik at DevCon3, October 2017, in his “Modest Proposal for 2.0”. He suggested to keep Ethereum 1.0 “safe and conservative”. Of course, Ethereum 1x initiative kind of goes opposite to that, but only because we believe that there are SOME changes necessary to keep Ethereum 1.0 alive.

1 Like

You misunderstand; the EVM itself enforces the gas stipend. If you make a call to another contract that sends value with 0 gas, the recipient contract gets 2300 gas regardless.

That’s the whole point of the EIP - to make the use of temporary storage for cross-contract calls practical. Examples include mutexes and setting temporary token allowances.

It was explicitly designed in as an invariant, because it’s useful to be able to reason straightforwardly about ‘value sends’ while still providing the target contract with the opportunity to execute a little code. I don’t think we should throw that out, even in the (hypothetical and very unlikely) situation that we can without breaking anything already deployed.

2 Likes

This is how I understood it but I wasn’t aware that this was done by the EVM. Is there an issue with removing this gas stipend in the case that send and transfer use the new call context you described for 2.?

I agree that an invariant shouldn’t be changed.

Removing the stipend would break existing code that relies on being able to log an event when it’s sent funds, if it’s called by an existing contract.

1 Like

I did not know this and assumed you could always forward 0 gas, making transfers to any existing contract impossible.

I see, this is a good use case.

But in that case, I actually think that reducing the stipend is also an interesting option. I misunderstood that the stipend was not the same as the forwarded gas in a call (because solidity forwards this exact amount). Why is this stipend there in the first place? What is the reason for this? Why must we not explicitly forward an amount of gas and why is there a minimum of 2300?

The stipend exists to ensure that a contract that’s sent ether always has the opportunity to log an event to record that fact.

1 Like

Couldn’t you have it forward as much gas as is required? I’m not understanding why removing this would default it back to 0.

Proper understanding the stipend requires a bit of history.
Stipend to contracts was in EVM from the beginning. But, unfortunately, the case where call value was “0” was forgotten (or rather it was thought that people won’t do it), so there is no stipend in this case. That is why, sending 0 wei to a contract would fail some time in the past, whereas sending non-zero amount would succeed.

Solidity made a work around to fix this - it now generates the code that checks that if the target of the CALL is a contract, and the Call Value is 0, then it adds 2300 to gas given to the call. This part is not in the EVM, it is a patch to fix the usability issue. The more fundamental fix would be to change EVM, but it was never done

5 Likes

This is starting to make a little bit more sense. However, I still do not understand that there always is a minimum of 2300 gas forwarded (and via above text this is apparently the minimum if the endowment > 0). What if I want to transfer Ether but want to explicitly revoke that anything is logged (so gas forwarded is now less than 375). I don’t see why this always has to be minimally 2300.

So this is also the reason why a lot of (older) contracts always send 1 wei with their CALLs to other contracts?

At present, any time you send value with 0 gas, the recipient gets 2300 gas instead. If we removed the stipend from the EVM, in this event the recipient would get 0 gas and immediately fail. This would break a lot of existing code, as well as breaking the current assumption that you can always at least log an event when sent ether.

That’s not a mistake - the intention is just to provide a stipend when necessary to log receiving ether. There’s no reason to log receiving 0 ether.

To be fair, adding new opcodes for transient storage has the exact same issue, just it’s harder to be exploited. Although transient, it’s still a (temporary) state change and can affect things similarly.