feat(contracts): native ETH value support for ovmCALL #8

ben-chain · 2021-05-27T09:47:15Z

Lots of good progress. Includes:

experimental compiler build working smoothly
all opcodes implemented
contract unit tests for most cases
basic integration test cases

Still missing:

change to OVM_ECDSAContractAccount
- dependency: fix smock
fix gas check
addition of value to simulateMessage/eth_call
ovmCREATE types with value
sad path testing for INVALID_STATE_ACCESS during fraud proof (low priority; this only JUMPs to existing code paths

ben-chain · 2021-05-27T18:45:24Z

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

@@ -13,6 +13,7 @@ import { Lib_ErrorUtils } from "../../libraries/utils/Lib_ErrorUtils.sol";
 import { iOVM_ExecutionManager } from "../../iOVM/execution/iOVM_ExecutionManager.sol";
 import { iOVM_StateManager } from "../../iOVM/execution/iOVM_StateManager.sol";
 import { iOVM_SafetyChecker } from "../../iOVM/execution/iOVM_SafetyChecker.sol";
+import { IUniswapV2ERC20 } from "../../libraries/standards/IUniswapV2ERC20.sol";


I was able to import OVM_ETH here instead, but could not access OVM_ETH.balanceOf.selector for whatever reason...

ben-chain · 2021-05-27T18:54:45Z

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

@@ -830,6 +932,7 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
        (bool success, bytes memory data) = ovmCALL(


just noticed: this could be a staticcall

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

ben-chain · 2021-05-27T19:19:34Z

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

+
+            // Since we transferred it in above and the call reverted, the transfer back should always pass.
+            // If it did not, this is an OOG, and we have to make the parent out-of-gas as well.
+            // TODO: should we also enforce there is always enough extra gas to make it past this step?  This would mean this condition is only triggered due to some critical bug in the ERC20 implementation.


After talking this through with @maurelian this definitely needs to be fixed. We need to do some sort of enforcement of gasleft beforehand (perhaps line 1112 above becomes (success, returndata) = _contract.call{gas: min(_gasLimit, gasleft() - MIN_GAS_FOR_RETURN_VALUE_BACK) }(_data);

Otherwise, this code path here potentially allows a malicious contract to out-of-gas anyone who calls it with nonzero value, even if they only give the malicious contract a small portion of their own gas.

In addition to ^^^ I'm going to give my understanding of the problem we're facing here and the proposed solution because I found it quite difficult to wrap my head around. This is based on conversation with @ben-chain (please correct me if you see any mistakes).

What we're trying to achieve here is to duplicate the behavior of EVM calls that come with attached value. Specifically, this behavior is as follows:

If making a call with a value > 0 and the call fails, then the value IS NOT transferred to the recipient.

If making a call with a value > 0 and the call succeeds, then the value IS transferred to the recipient.

If making a call with a value > 0 and the user is unable to transfer funds (e.g., because they don't have enough balance OR because they don't have enough gas to execute the payment), then the value IS NOT transferred to the recipient AND the call fails with (success=false, returndata=0x). This IS how the EVM behaves. See geth: https://github.com/ethereum/go-ethereum/blob/2dee31930c9977af2a9fcb518fb9838aa609a7cf/core/vm/evm.go#L298

Case (2) is the straightforward happy path case.
Case (3) is also pretty easy to handle by checking for failure of the initial transfer.

Case (1) is slightly harder -- you want to revert the payment if the call fails. You can either do that by reverting the call frame in which the payment was transferred or by transferring the payment back to the original sender. Reverting the call frame here IS technically an option for cases where the transfer fails for some unexpected reason but we can avoid this because OVM_ETH is trusted. The only case we really need to be careful of is if we were to run out of gas when trying to transfer the ETH back. We could get around this by adding an extra call frame (expensive) or by making sure that the user always provides enough gas to execute both transfer steps when the value is greater than 0.

ben-chain · 2021-05-27T19:27:47Z

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

@@ -1908,6 +2085,7 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
            (bool success, bytes memory returndata) = ovmCALL(
                _transaction.gasLimit,
                _transaction.entrypoint,
+                0,


can also remove this 0

on second thought: gonna leave these. Slightly more efficient and minimizes usage of an effectively deprecated code path

smartcontracts · 2021-06-01T21:27:48Z

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

        MessageContext memory nextMessageContext = messageContext;
        nextMessageContext.ovmCALLER = nextMessageContext.ovmADDRESS;
        nextMessageContext.ovmADDRESS = _address;
        nextMessageContext.isStatic = true;
+        nextMessageContext.ovmCALLVALUE = 0;


As discussed with @ben-chain, we can either attach the call value to the message context or pass it as a parameter to _callContract. Attaching the call value to the message context is probably best because it effectively is a message-specific parameter and it must be accessible for the sake of msg.value.

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

smartcontracts · 2021-06-01T23:44:31Z

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

+
+            // Since we transferred it in above and the call reverted, the transfer back should always pass.
+            // If it did not, this is an OOG, and we have to make the parent out-of-gas as well.
+            // TODO: should we also enforce there is always enough extra gas to make it past this step?  This would mean this condition is only triggered due to some critical bug in the ERC20 implementation.


In addition to ^^^ I'm going to give my understanding of the problem we're facing here and the proposed solution because I found it quite difficult to wrap my head around. This is based on conversation with @ben-chain (please correct me if you see any mistakes).

What we're trying to achieve here is to duplicate the behavior of EVM calls that come with attached value. Specifically, this behavior is as follows:

If making a call with a value > 0 and the call fails, then the value IS NOT transferred to the recipient.

If making a call with a value > 0 and the call succeeds, then the value IS transferred to the recipient.

If making a call with a value > 0 and the user is unable to transfer funds (e.g., because they don't have enough balance OR because they don't have enough gas to execute the payment), then the value IS NOT transferred to the recipient AND the call fails with (success=false, returndata=0x). This IS how the EVM behaves. See geth: https://github.com/ethereum/go-ethereum/blob/2dee31930c9977af2a9fcb518fb9838aa609a7cf/core/vm/evm.go#L298

Case (2) is the straightforward happy path case.
Case (3) is also pretty easy to handle by checking for failure of the initial transfer.

Case (1) is slightly harder -- you want to revert the payment if the call fails. You can either do that by reverting the call frame in which the payment was transferred or by transferring the payment back to the original sender. Reverting the call frame here IS technically an option for cases where the transfer fails for some unexpected reason but we can avoid this because OVM_ETH is trusted. The only case we really need to be careful of is if we were to run out of gas when trying to transfer the ETH back. We could get around this by adding an extra call frame (expensive) or by making sure that the user always provides enough gas to execute both transfer steps when the value is greater than 0.

integration-tests/contracts/ValueCallsWithCompiler.sol

ben-chain · 2021-06-02T00:25:24Z

@smartcontracts that is a great writeup, right on the money. See here for proposed fix.

packages/contracts/test/contracts/OVM/accounts/OVM_ECDSAContractAccount.spec.ts

* add ovmCALLVALUE context * add ovmBALANCE * test success and revert cases * test empty contract case * chore: lint

* fix ovmDELEGATECALL type, update tests * add ovmSELFBALANCE * fix ovmDELEGATECALL jumping to CALL * chore: lint

github-actions bot added A-contracts A-integration-tests M-contracts 2-reviewers labels May 27, 2021

ben-chain commented May 27, 2021

View reviewed changes

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol Show resolved Hide resolved

ben-chain commented May 27, 2021

View reviewed changes

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol Outdated Show resolved Hide resolved

ben-chain commented May 27, 2021

View reviewed changes

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol Show resolved Hide resolved

ben-chain commented May 27, 2021

View reviewed changes

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol Show resolved Hide resolved

ben-chain commented May 27, 2021

View reviewed changes

ben-chain force-pushed the feat/OP759/call-with-value branch from 4572dfd to ea5c006 Compare June 1, 2021 22:01

smartcontracts reviewed Jun 1, 2021

View reviewed changes

smartcontracts reviewed Jun 2, 2021

View reviewed changes

integration-tests/contracts/ValueCallsWithCompiler.sol Outdated Show resolved Hide resolved

ben-chain force-pushed the feat/OP759/call-with-value branch from 34b479f to 7a59b2b Compare June 2, 2021 00:29

smartcontracts reviewed Jun 2, 2021

View reviewed changes

packages/contracts/test/contracts/OVM/accounts/OVM_ECDSAContractAccount.spec.ts Outdated Show resolved Hide resolved

ben-chain force-pushed the feat/OP759/call-with-value branch from 7a59b2b to 676ef59 Compare June 8, 2021 17:29

github-actions bot added A-geth A-ci M-batch-submitter M-core-utils M-dtl M-ops M-smock labels Jun 8, 2021

ben-chain changed the base branch from develop to regenesis/0.4.0 June 8, 2021 17:30

ben-chain added 3 commits June 8, 2021 13:40

feat(contracts): add ovmCALL-types with native value

da76d28

* add ovmCALLVALUE context * add ovmBALANCE * test success and revert cases * test empty contract case * chore: lint

test(integration-tests): ovmCALL-types with value (compiler and wrapper)

df0cfcd

* fix ovmDELEGATECALL type, update tests * add ovmSELFBALANCE * fix ovmDELEGATECALL jumping to CALL * chore: lint

fix(contracts): account for intrinsic gas of OVM_ETH sends

32da512

smartcontracts and others added 4 commits June 8, 2021 13:40

fix(contracts): merge conflict bug

9650e72

fix(contracts): update gas benchmark

8fed20a

TEMP: use experimental compile in contracts:

e8b6641

feat(contracts,l2geth): support value calls in OVM_ECDSAContractAccount

055f025

ben-chain force-pushed the feat/OP759/call-with-value branch from 676ef59 to f3d7b22 Compare June 8, 2021 17:44

github-actions bot removed M-core-utils M-dtl A-ci M-batch-submitter M-ops M-smock labels Jun 8, 2021

K-Ho and others added 7 commits June 8, 2021 13:57

feat(contracts): add funding L2 ETH step to deploy script

612916b

fix(contracts): ovmDELEGATECALL does not change message context

2157b05

feat(contracts): sending value between EOAs

7de2d4b

test(integration-tests): ovmDELEGATECALL preserves ovmCALLVALUE

ccfc715

test(integration-tests): assert ovmSELFBALANCEs correct

6f52617

test(integration-tests): intrinsic gas for eth value calls

2bd2ce5

test(integration-tests): update gas values

2dc806f

ben-chain force-pushed the feat/OP759/call-with-value branch from f3d7b22 to 2dc806f Compare June 8, 2021 17:58

ben-chain mentioned this pull request Jun 8, 2021

feat(contracts, l2geth): native ETH value support for ovmCALL ethereum-optimism/optimism#1038

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(contracts): native ETH value support for ovmCALL #8

feat(contracts): native ETH value support for ovmCALL #8

ben-chain commented May 27, 2021 •

edited

Loading

ben-chain May 27, 2021

ben-chain May 27, 2021

ben-chain May 27, 2021

smartcontracts Jun 1, 2021

ben-chain May 27, 2021

ben-chain Jun 1, 2021

smartcontracts Jun 1, 2021

smartcontracts Jun 1, 2021

ben-chain commented Jun 2, 2021 •

edited

Loading

		@@ -830,6 +932,7 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
		(bool success, bytes memory data) = ovmCALL(

feat(contracts): native ETH value support for ovmCALL #8

Are you sure you want to change the base?

feat(contracts): native ETH value support for ovmCALL #8

Conversation

ben-chain commented May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-chain commented Jun 2, 2021 • edited Loading

ben-chain commented May 27, 2021 •

edited

Loading

ben-chain commented Jun 2, 2021 •

edited

Loading