Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EIP-1706 Discussion #1

Closed
forshtat opened this issue Jan 17, 2019 · 8 comments
Closed

EIP-1706 Discussion #1

forshtat opened this issue Jan 17, 2019 · 8 comments

Comments

@forshtat
Copy link
Owner

No description provided.

@wighawag
Copy link

wighawag commented May 24, 2019

In the backward compatibility section it is mentioned:

Gas estimation should account for this requirement.

It should be more precise as with such proposal a simple gas estimate call (with a big enough amount of gas provided) could return a too low amount of gas to pass the check gasleft()>2300 failing to provide a true estimate of the gas required to be given.

As such existing application that relies on eth_estimateGas could be affected. If they execute the transaction with the result of eth_estimateGascall they might not give enough gas for the call to succeed due to the new 2300 rules.

We could modify the logic behind eth_estimateGas to add an extra 2300 to the result (when SSTORE is used and decrease the extra for every gas used afterward until 0) to keep backward compatibility.

@wighawag
Copy link

Created an EIP for the general problem, Your proposal could use the same mechanism (minimalGas tracking) : ethereum#2075

@wighawag
Copy link

Sorry false alert, the issues was discovered in ganache and I assumed it was the case everywhere.

Geth and parity use binary search to find the minimum gas amount and will work with 1706 keeping backward compatibility

@ritzdorf
Copy link

What I don't understand is, why the value was chosen to be 2300.

This contract was not vulnerable to reentrancy before the EIP, but would be vulnerable afterwards:

contract C {
    bool paid = false;

    function vuln(address t) external {
        require(!paid);
        t.call.gas(5000).value(100)("");
        paid = true; 
    }
}

Why did you pick 2300 as the boundary to raise an exception?

@forshtat
Copy link
Owner Author

Well, there is no strong reason to pick '2300' over '5000', but the 'call stipend' constant of 2300 is present in the Yellow Paper:

Gcallstipend -2300 - A stipend for the called contract subtracted from Gcallvalue for a non-zero value transfer.

I get it this value was widely assumed to not be enough to change contract's state and is not related to actual storage write gas costs. And it was automatically hard-coded in many deployed contracts.

'5000', on the other hand, comes from the cost of the SSTORE operation, and had to be given explicitly as a gas limit, so relying on it to protect your contract from reentrancy does seem like a mistake.

So, introducing '5000' as a limit value seemed more like a legacy logic backward-compatible patch to me, while requiring the call to have more than Gcallstipend left to modify storage seemed like a right thing to do initially.

@ritzdorf
Copy link

A quick scan of this repo (which is not up-to-date): https://github.com/thec00n/etherscan_verified_contracts find 782 cases of explicit .gas() usage, including examples like this:

https://etherscan.io/address/0x8593F6028b5B6c4F7899f9cf2e0bA2750b7f6Ee2#contracts
=> See the comment in line 1035

In that sense, I would like to challenge your assumptions.

@forshtat
Copy link
Owner Author

Well, it actually makes sense that someone would depend on explicitly setting gas to 4999. If there will be a consensus that this is the right thing to do, I will update the EIP. But in case this becomes a contentious issue, maybe it will make sense for it to have a different issue number?

@chfast
Copy link

chfast commented Jul 5, 2019

The security issue this EIP ties do mitigate is the case when a call starts with 5000/2300 gas and it is assumed that no state changes are able to be performed in such call.
The EIP-1283 breaks this invariant.

However, the scope of this EIP is bigger than required. It will also affect cases like:

  • start a call with 6000 gas,
  • spend 4000 gas on something,
  • make SSTORE - not allowed because you have 2000 gas left.

This adds a "dynamic" effect that SSTORE depends on a parameter (current "gas left") that is not easy to estimate by developers.

Alternative solution

When a call starts with less than 5000/2300 gas, enable "semi-static" execution mode where SSTORE is not allowed.

The "semi-static" mode could also include restrict CALLs as in static mode.

Including LOGs can also be considered (that would enable full static mode), but the main reason for the stipend was to provide minimum gas allowing the callee to make a LOG. This is one of the features probably nobody ever used, but we now have a big mess around it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants