Core devs discussion about a vulnerability in Constantinople, delay of the upgrade

hardfork
core-devs
eip-1283

#1

This criticial discussion began at this point on the AllCoreDevs gitter channel: https://gitter.im/ethereum/AllCoreDevs?at=5c3e0fb51cb70a372ae2c1f7

Martin Holst Swende @holiman 08:52
Please read: https://medium.com/chainsecurity/constantinople-enables-new-reentrancy-attack-ace4088297d9 @/all
We need a quick decision on potential consequences and how to move forward. We have about 37 hours left until the fork happens

Hudson Jameson @Souptacular 08:57
:frowning:

Martin Holst Swende @holiman 08:59
The example is a bit contrived, would be intersting to know if there’s a more common pattern where this could crop up

Eth-Gitter-Bridge @Eth-Gitter-Bridge 09:01
[W Dimitry] Why token transfer logic suddenly depends on sstore gas price?

Alex Beregszaszi @axic 09:01
The brief explanation is that a CALL made to transfer funds, even if no additional gas is specified, will give 2300 gas to the callee. Solidity’s .transfer() function does this and provides no option to the user to override that.
This has not been an issue previously, because even though with 2300 gas one could make a CALL from the callee, there was no way to change a storage entry when SSTORE costs 5000 gas.
After Constantinople the cheapest SSTORE case is 200 gas.

Danny Ryan @djrtwo 09:01
Is it a common pattern to have the security of a transfer premised upon not having enough gas to perform a write?

Martin Holst Swende @holiman 09:01
So a prerequisite is that the attacker managed to get the vulnerable contract to first make the slot in question dirty before the re-entrancy happens

Dean Eigenmann @decanus 09:03

Is it a common pattern to have the security of a transfer premised upon not having enough gas to perform a write?

I’d say a significant amount of devs operate under that assumption

… and later on during a live call on Zoom …

Jason Carver @carver 11:51
I am hearing postponement as the rough consensus on the Zoom call, too. People are starting to evaluate the risks of postponement, and how to mitigate them.

Nick Johnson @Arachnid 11:51
I agree that that should be a showstopper. Not introducing new bugs into previously secure contracts should be right at the top of our priority list.

… with some notes from the Zoom call …

Taylor Monahan @tayvano 12:20
Moving information from ZOOM chat to here for posterity

From Yoav Weiss to Everyone: (12:15 PM)
Since we’re delaying the fork and have time to fix it now, I’d like to re-propose the fix I brought up earlier: Accept the EIP to lower the SSTORE fee, but add a condition that reverts storage access if gasleft < 2300. This preserves the original intention of the EIP (lowering the fees) while preventing exploitation. It is unlikely that a legit transaction will need to access storage while gasleft < 2300.Y
So when we apply the fork again, maybe we could do that and keep it as close as possible to the original fork.

From Nick Johnson to Everyone: (12:18 PM)
Yoav, that’s an excellent idea for a fix.

Discussion is ongoing https://gitter.im/ethereum/AllCoreDevs?at=5c3e40c0cb47ec30004ba6e7.


Immutables, invariants, and upgradability
#2

Updates.


#3

Thanks for sharing! It’s pretty scary how late this came to light but those conversations are mostly reassuring.

I am against postponing, but I think that is mostly because I have always been incredibly salty to anyone that relied on gas stipends for security in the past and this vindicates all of my ranting.

I kind of agree with you about relying on weird behavior for security :stuck_out_tongue:

A lot of people, me included, originally learned how to write contracts through a combination of the docs, word of mouth and random internet articles, I often heard that these calls were safe against reentrancies. It’s not easy to split the “weird behaviour” that shouldn’t be relied on from the rest.

Not introducing new bugs into previously secure contracts should be right at the top of our priority list.

This seems like a no brainer to me. However some people in that chat seem to be of the opinion that it’s fine because it “only” punishes contract developers(and their users) that weren’t following recommended best practices.

If the above is a path people want to walk down, I’m going to demand some better resources. Setting the precedent that a previously 100% secure contract can be ruined because the code was not deemed good enough really rubs me the wrong way.


#4

In most articles about solidity you learn that send or transfer guards against reentrancy attacks. However, developers should still incorporate check, effects, interaction of course. In an audit when you see that technically the contract is susceptible to a reentrancy attack I think what often happens is that a note is made: “the check, effects, interaction pattern is not incorporated, but this does not matter as these type of calls guard against reentrancy”. The developer now knows that in the future they should watch out for this (when they do an explicit call and not a send or transfer), but there is not an alarm ringing that this should be fixed right now because in a hard fork later on it suddenly can be susceptible to a reentrancy!

Imagine that you are a developer who writes such contract. They are lazy (which is of course not good) and have read the note on this audit but think “nah, this is fine, we won’t change anything”. Now the hard fork happens and suddenly the contract is drained because of the hard fork. This will anger a lot of developers and will greatly reduce trust into the Ethereum network.

The main points we need to take away from this is that we need to get much better documentation (also at a low level so we can at least be assured that things who work in a certain way right now also work like this in the future) and that we need to decide what changes of smart contract execution is allowed and what not. Any change in the EVM can change to unsuspected edge cases, such as contracts depending on that extcodesize uses the amount of gas it had before the fork where it was raised to 700 - these types of contracts can suddenly break due to the fork.


#5

Update: there is now a post on this Forum about Remediations for EIP-1283 reentrancy bug


#6

I mean, I’m +1000 on this comment :point_up:

<rant>FWIW, I fully sympathize. Such sharp corners / “weird behaviour” should not leak out to app developers. You shouldn’t have to worry about this stuff at all, and it’s entirely possible to design a system that prevents such things from occurring. The tradeoffs of the current system is not safe-by-default, and provides few guard rails. I think that it’s salvageable, and that as a community we could do much more.</rant>