Skip to content

Comments

fix: prevent eth sendtransaction#725

Merged
tynes merged 4 commits intov0.3.0-rcfrom
fix/prevent-eth-sendtransaction
May 3, 2021
Merged

fix: prevent eth sendtransaction#725
tynes merged 4 commits intov0.3.0-rcfrom
fix/prevent-eth-sendtransaction

Conversation

@tynes
Copy link
Contributor

@tynes tynes commented May 3, 2021

Description
This PR blocks certain endpoints from being available for users based on the environment variable USING_OVM. We need to turn this env var into an actual flag and have it select a consensus.Engine, but will leave that for future work. The blocked endpoints are:

  • eth_fillTransaction
  • eth_sign
  • eth_sendTransaction
  • eth_signTransaction

It also adds in the txmeta.rawTransaction field in an additional place where transactions are serialized based on user input. This code path should no longer be reachable because it is behind eth_sendTransaction.

@changeset-bot
Copy link

changeset-bot bot commented May 3, 2021 •
edited
Loading

🦋 Changeset detected

Latest commit: 0c22e77

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@eth-optimism/l2geth Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tynes tynes requested a review from snario May 3, 2021 00:51
@tynes tynes changed the title Fix/prevent eth sendtransaction fix: prevent eth sendtransaction May 3, 2021
snario
snario reviewed May 3, 2021
View reviewed changes
l2geth/internal/ethapi/api.go Outdated
Copy link
Contributor

@snario snario May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have some kind of prefix e.g., "OVMError: Cannot fill transaction" or in some other way indicate this error is thrown to indicate it is due to the vm being wrong

Copy link
Contributor

@gakonst gakonst May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO just create a global error called: var errOvmNotSupported = errors.New("OVMError: Endpoint disabled"), maybe wrapping it with some additional context per method, although not necessary.

@tynes tynes force-pushed the fix/prevent-eth-sendtransaction branch from ed27d34 to a6be210 Compare May 3, 2021 02:50
@gakonst
Copy link
Contributor

gakonst commented May 3, 2021

Should this PR not be against master?

@smartcontracts
Copy link
Contributor

smartcontracts commented May 3, 2021 •
edited
Loading

Intuitively it seems like we should be able to block these endpoints via some sort of configuration instead of by modifying geth. Is that not the case?

@gakonst
Copy link
Contributor

gakonst commented May 3, 2021 •
edited
Loading

@smartcontracts no, you can disable entire namespaces in geth, but cannot block off specific methods inside a namespace.

gakonst
gakonst suggested changes May 3, 2021
View reviewed changes
l2geth/internal/ethapi/api.go Outdated
Copy link
Contributor

@gakonst gakonst May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO just create a global error called: var errOvmNotSupported = errors.New("OVMError: Endpoint disabled"), maybe wrapping it with some additional context per method, although not necessary.

@tynes tynes force-pushed the fix/prevent-eth-sendtransaction branch from a6be210 to 0c22e77 Compare May 3, 2021 19:20
@tynes
Copy link
Contributor Author

tynes commented May 3, 2021

Intuitively it seems like we should be able to block these endpoints via some sort of configuration instead of by modifying geth. Is that not the case?

Its possible to run a proxy in front of geth that blocks certain RPC methods

@tynes tynes merged commit 09d6827 into v0.3.0-rc May 3, 2021
@tynes tynes deleted the fix/prevent-eth-sendtransaction branch May 3, 2021 20:36
tynes added a commit that referenced this pull request May 10, 2021
@karlfloersch @tynes
@smartcontracts @ben-chain @gakonst @maurelian @K-Ho
feat: v0.3.0 release candidate (#603)
b799caa 
* feat: Attempt to decode txs as RLP first (#563)

Co-authored-by: smartcontracts <smartcontracts@doge.org>

* l2geth: remove eth_sendRawEthSignTransaction endpoint (#589)

* feat[contracts]: Use standard RLP transaction format (#566)

* feat[contracts]: Use standard RLP transaction format

* fix[l2geth]: Encode transaction as RLP

* fix: Correct gas estimation in integration tests

* fix: Correct gas estimation in integration tests

* Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol

Co-authored-by: ben-chain <ben@pseudonym.party>

* fix[contracts]: Use isCreate instead of checking target address

* fix[contracts]: Minor optimization in SequencerEntrypoint

* fix[contracts]: Pass max gas to contract call in EOA contract

Co-authored-by: ben-chain <ben@pseudonym.party>

* feat[contracts]: Make ProxyEOA compatible with eip1967 (#592)

* feat[contracts]: Make ProxyEOA compatible with eip1967

* fix[contracts]: Fix bug introduced by indirect constant

* chore[contracts]: Add changeset

* Update .changeset/old-cycles-invite.md

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

* l2geth: remove ovmsigner (#591)

* l2geth: remove ovmsigner

Also reduce the diff

Co-authored-by: smartcontracts

* l2geth: add changeset

* l2geth: set rlp encoded tx in txmeta in RPC layer (#644)

* l2geth: set rlp encoded tx in txmeta in RPC layer

* l2geth: remove extra setter of txmeta

* chore: add changeset

* feat: Have ExecutionManager pass data upwards (#643)

* feat[contracts]: Make ExecutionManager return data

* fix[l2geth]: fix linting error

* fix[contracts]: Fix build error

* fix[contracts]: fix failing unit tests

* Add changeset

Co-authored-by: Karl Floersch <karl@karlfloersch.com>

* rpc: only allow txs with no calldata when there is value (#645)

* l2geth: api checks for 0 value

* chore: add changeset

* l2geth: remove check for specific gasprice

* feat[contracts]: Add value transfer support to ECDSAContractAccount (#619)

* feat[contracts]: Use standard RLP transaction format (#566)

* feat[contracts]: Use standard RLP transaction format

* fix[l2geth]: Encode transaction as RLP

* fix: Correct gas estimation in integration tests

* fix: Correct gas estimation in integration tests

* Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol

Co-authored-by: ben-chain <ben@pseudonym.party>

* fix[contracts]: Use isCreate instead of checking target address

* fix[contracts]: Minor optimization in SequencerEntrypoint

* fix[contracts]: Pass max gas to contract call in EOA contract

Co-authored-by: ben-chain <ben@pseudonym.party>

* feat[contracts]: Add value transfer to contract account

* fix[contracts]: Tweak transfer logic and add tests

* fix[geth]: Remove logic that rejects value gt 0 txs

* fix: nonce issue in rpc tests

* fix: use correct wallet in rpc value tests

* Update rpc.spec.ts

* cleanup: remove double definition

* chore: add changeset

* chore: add changeset

* tests: delete dead test

* l2geth: log the tx value

* l2geth: pass through zero value at top level

* test: receipt passes

* test: more specifically set balance

Co-authored-by: ben-chain <ben@pseudonym.party>
Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>

* dtl: remove legacy encoding (#618)

* dtl: remove legacy decoding

* tests: remove dead test

* chore: add changeset

* Add Goerli v3 deployment (#651)

* Add Goerli v3 deployment

* Add Goerli v3 to README

* dtlL fix syncing off by one (#687)

* dtl: syncing off by one error

* chore: add changeset

* dtl: index the value field (#686)

* chore: add changeset

* chore: add changeset

* dtl: pass through value field

* core-utils: update and test toRpcString

* lint: fix

* l2geth: parse value fields

* chore: add changeset

* rpc: gas fixes (#695)

* l2geth: prevent fees lower than 21000

* l2geth: remove old check for too high tx gaslimit

* tests: update to use new min gas estimated value

* chore: add changeset

* test: update expected values

* test: remove dead test

* examples: fix waffle example + gas changes in tests (#724)

* examples: fix waffle example

* tests: update gas price in assertion

* chore: add changeset

* l2geth: estimate gas assertion in decimal

* test: use configurable key

* ops: delete extra whitespace (#731)

* fix: prevent eth sendtransaction (#725)

* api: prevent unsafe calls

* api: fill in txmeta

* chore: add changeset

* chore: add changeset

* l2geth + contracts:  standard interface for systems contracts and userland contracts (#721)

* l2geth: fix call returndata parsing

* contracts: standardize simulateMessage and run to return bytes

* chore: add changeset

* chore: add changeset

* l2geth: more simple decoding

* contracts: remove named arguments

* chore: fix linter errors

* Add contract deployment to Kovan (#715)

* fix: remove type check in rollup client (#750)

* l2geth: remove tx type check in client

* chore: add changeset

* dtl: prevent null reference in L1 handler (#757)

* dtl: prevent reference of null value

* chore: add changeset

* test: eth_call exceptions (#800)

* feat[l2geth]: Pass up contract revert reasons during DoEstimateGas (#774)

* wip: Starting work on geth revert reasons during estimate gas

fix: error in comment

fix: I got things backwards

fix: Use UnpackValues instead of Unpack

Update l2geth/accounts/abi/abi.go

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

* Add integration test for reverts

fix: build error

* chore: Add changeset

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

* chore: add changeset (#831)

* Migrate ETH between gateways (#778)

* add migrate ETH functionality

* contracts: add eth gateway docstring (#832)

Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>

Co-authored-by: smartcontracts <smartcontracts@doge.org>
Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>
Co-authored-by: smartcontracts <kelvinfichter@gmail.com>
Co-authored-by: ben-chain <ben@pseudonym.party>
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
Co-authored-by: Maurelian <maurelian@protonmail.ch>
Co-authored-by: Kevin Ho <kevinjho1996@gmail.com>
mds1 added a commit to ScopeLift/uniswap-v3-core-ovm that referenced this pull request May 11, 2021
@mds1
WIP: Add .connect() and .wait() to get back to CALL_EXCEPTION error
08d0323 
Needed to add this since ethereum-optimism/optimism#725 now blocks
using l2geth's signers. Later we'll instead modify the wallets to always .wait() by default
to clean up the diff
InoMurko pushed a commit to omgnetwork/optimism that referenced this pull request May 25, 2021
@karlfloersch @tynes
@smartcontracts @ben-chain @gakonst @maurelian @K-Ho
feat: v0.3.0 release candidate (#603)
67c5456 
* feat: Attempt to decode txs as RLP first (#563)

Co-authored-by: smartcontracts <smartcontracts@doge.org>

* l2geth: remove eth_sendRawEthSignTransaction endpoint (#589)

* feat[contracts]: Use standard RLP transaction format (#566)

* feat[contracts]: Use standard RLP transaction format

* fix[l2geth]: Encode transaction as RLP

* fix: Correct gas estimation in integration tests

* fix: Correct gas estimation in integration tests

* Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol

Co-authored-by: ben-chain <ben@pseudonym.party>

* fix[contracts]: Use isCreate instead of checking target address

* fix[contracts]: Minor optimization in SequencerEntrypoint

* fix[contracts]: Pass max gas to contract call in EOA contract

Co-authored-by: ben-chain <ben@pseudonym.party>

* feat[contracts]: Make ProxyEOA compatible with eip1967 (#592)

* feat[contracts]: Make ProxyEOA compatible with eip1967

* fix[contracts]: Fix bug introduced by indirect constant

* chore[contracts]: Add changeset

* Update .changeset/old-cycles-invite.md

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

* l2geth: remove ovmsigner (#591)

* l2geth: remove ovmsigner

Also reduce the diff

Co-authored-by: smartcontracts

* l2geth: add changeset

* l2geth: set rlp encoded tx in txmeta in RPC layer (ethereum-optimism#644)

* l2geth: set rlp encoded tx in txmeta in RPC layer

* l2geth: remove extra setter of txmeta

* chore: add changeset

* feat: Have ExecutionManager pass data upwards (ethereum-optimism#643)

* feat[contracts]: Make ExecutionManager return data

* fix[l2geth]: fix linting error

* fix[contracts]: Fix build error

* fix[contracts]: fix failing unit tests

* Add changeset

Co-authored-by: Karl Floersch <karl@karlfloersch.com>

* rpc: only allow txs with no calldata when there is value (ethereum-optimism#645)

* l2geth: api checks for 0 value

* chore: add changeset

* l2geth: remove check for specific gasprice

* feat[contracts]: Add value transfer support to ECDSAContractAccount (ethereum-optimism#619)

* feat[contracts]: Use standard RLP transaction format (#566)

* feat[contracts]: Use standard RLP transaction format

* fix[l2geth]: Encode transaction as RLP

* fix: Correct gas estimation in integration tests

* fix: Correct gas estimation in integration tests

* Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol

Co-authored-by: ben-chain <ben@pseudonym.party>

* fix[contracts]: Use isCreate instead of checking target address

* fix[contracts]: Minor optimization in SequencerEntrypoint

* fix[contracts]: Pass max gas to contract call in EOA contract

Co-authored-by: ben-chain <ben@pseudonym.party>

* feat[contracts]: Add value transfer to contract account

* fix[contracts]: Tweak transfer logic and add tests

* fix[geth]: Remove logic that rejects value gt 0 txs

* fix: nonce issue in rpc tests

* fix: use correct wallet in rpc value tests

* Update rpc.spec.ts

* cleanup: remove double definition

* chore: add changeset

* chore: add changeset

* tests: delete dead test

* l2geth: log the tx value

* l2geth: pass through zero value at top level

* test: receipt passes

* test: more specifically set balance

Co-authored-by: ben-chain <ben@pseudonym.party>
Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>

* dtl: remove legacy encoding (ethereum-optimism#618)

* dtl: remove legacy decoding

* tests: remove dead test

* chore: add changeset

* Add Goerli v3 deployment (ethereum-optimism#651)

* Add Goerli v3 deployment

* Add Goerli v3 to README

* dtlL fix syncing off by one (ethereum-optimism#687)

* dtl: syncing off by one error

* chore: add changeset

* dtl: index the value field (ethereum-optimism#686)

* chore: add changeset

* chore: add changeset

* dtl: pass through value field

* core-utils: update and test toRpcString

* lint: fix

* l2geth: parse value fields

* chore: add changeset

* rpc: gas fixes (ethereum-optimism#695)

* l2geth: prevent fees lower than 21000

* l2geth: remove old check for too high tx gaslimit

* tests: update to use new min gas estimated value

* chore: add changeset

* test: update expected values

* test: remove dead test

* examples: fix waffle example + gas changes in tests (ethereum-optimism#724)

* examples: fix waffle example

* tests: update gas price in assertion

* chore: add changeset

* l2geth: estimate gas assertion in decimal

* test: use configurable key

* ops: delete extra whitespace (ethereum-optimism#731)

* fix: prevent eth sendtransaction (ethereum-optimism#725)

* api: prevent unsafe calls

* api: fill in txmeta

* chore: add changeset

* chore: add changeset

* l2geth + contracts:  standard interface for systems contracts and userland contracts (ethereum-optimism#721)

* l2geth: fix call returndata parsing

* contracts: standardize simulateMessage and run to return bytes

* chore: add changeset

* chore: add changeset

* l2geth: more simple decoding

* contracts: remove named arguments

* chore: fix linter errors

* Add contract deployment to Kovan (ethereum-optimism#715)

* fix: remove type check in rollup client (ethereum-optimism#750)

* l2geth: remove tx type check in client

* chore: add changeset

* dtl: prevent null reference in L1 handler (ethereum-optimism#757)

* dtl: prevent reference of null value

* chore: add changeset

* test: eth_call exceptions (ethereum-optimism#800)

* feat[l2geth]: Pass up contract revert reasons during DoEstimateGas (ethereum-optimism#774)

* wip: Starting work on geth revert reasons during estimate gas

fix: error in comment

fix: I got things backwards

fix: Use UnpackValues instead of Unpack

Update l2geth/accounts/abi/abi.go

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

* Add integration test for reverts

fix: build error

* chore: Add changeset

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

* chore: add changeset (ethereum-optimism#831)

* Migrate ETH between gateways (ethereum-optimism#778)

* add migrate ETH functionality

* contracts: add eth gateway docstring (ethereum-optimism#832)

Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>

Co-authored-by: smartcontracts <smartcontracts@doge.org>
Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>
Co-authored-by: smartcontracts <kelvinfichter@gmail.com>
Co-authored-by: ben-chain <ben@pseudonym.party>
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
Co-authored-by: Maurelian <maurelian@protonmail.ch>
Co-authored-by: Kevin Ho <kevinjho1996@gmail.com>
OptimismBot pushed a commit that referenced this pull request Dec 8, 2025
@0xiamflux
fix: add extra instruction overrides for useCustomGasToken on OPCMv2 (#…
d4ed6d1 
…725)
theochap pushed a commit that referenced this pull request Dec 10, 2025
@refcell
cleans up kona-mpt deps (#725)
8513e20
github-merge-queue bot pushed a commit that referenced this pull request Dec 11, 2025
@0xniha @0xiamflux
@janjakubnanista @scharissis @geoknee @aliersh @smartcontracts
feat: add cgt to opcmv2 (#18455)
adf9b32 
* feat: add cgt to opcmv2

* feat: add cgt to opcmv2

* fix: change semver

* feat: add cgt to opcmv2

* feat: add cgt to opcmv2

* fix: change semver

* refactor: change disputeGameConfigs order in _loadFullConfig

* fix: comment slash rules

* feat: add opcmv2-cgt matrix in CI

* fix: enable cgt overrides on OPCMv2 (#724)

* fix: check CGT is not enabled before enabling it

* fix: enable CGT overrides

* fix: add extra instruction overrides for useCustomGasToken on OPCMv2 (#725)

* chore: link TODO to issue (#727)

* fix: opcm2 revert upgrade cgt (#728)

* fix: add check enabled CGT during upgrades

* test: set CGT false by default during forked tests

* chore: expand comment on OPCMv2 regarding CGT

* chore: pre-pr ready

* refactor: remove unnecessary check for CGT feature (#735)

* fix: cgt upgrade in opcm v2 and semver (#738)

* fix: Full checkout for FFI build (#18527)

* chore(op-acceptance-tests): delete very old logs (#18529)

* chore(op-acceptance-tests): op-acceptor v3.8.0 (#18530)

* jovian: remove feature toggles (#17978)

* jovian: remove feature toggles

scope is now fixed

* Updated op-geth to v1.101604.0-synctest.0.0.20251120150812-e50f80a16afc

* lint

* just update-op-geth f48f382

* Use Jovian in test error message

* just update-op-geth ba6bdcfef42341fe2b5ce124c31ff2d6b264e9e4

* chore(ai-eng): add ReinitializableBase test to exclusion list (#18531)

* feat: have OPCM upgrade allowances be upgrade specific (#18462)

* feat: have OPCM upgrade allowances be upgrade specific

Updates the OPCMv2 check for allowed extra instructions to be
specific to releases. When release versions are bumped, the
allowances become automatically invalid and would reveal
anywhere in the codebase where the allowance is being used.

* fix: semver lock

* fix: emit instruction key in error

* fix: better semantics

* docs: proper OPCM versioning policy

* fix: final version tweaks

* fix(op-acceptance-test): flake-shake; empty slack notifications. (#18542)

* feat: add cgt to opcmv2

* feat: add cgt to opcmv2

* fix: change semver

* feat: add cgt to opcmv2

* feat: add cgt to opcmv2

* fix: change semver

* refactor: change disputeGameConfigs order in _loadFullConfig

* fix: enable cgt overrides on OPCMv2 (#724)

* fix: check CGT is not enabled before enabling it

* fix: enable CGT overrides

* fix: opcm2 revert upgrade cgt (#728)

* fix: add check enabled CGT during upgrades

* test: set CGT false by default during forked tests

* chore: expand comment on OPCMv2 regarding CGT

* fix: cgt upgrade in opcm v2 and semver (#738)

* fix: merge conflicts

* fix: pre-pr

* fix: add missing isMatchingInstruction for cgt in false

* refactor: opcm2 extra instruction keymatch (#747)

* refactor: add helper function to match instruction by key only

* chore: pre-pr ready

* test: refactor key matching test

* test: add test suite for IsMatchingKey helper function

* chore: pre-pr ready

* fix: opcmv2 semver

---------

Co-authored-by: Flux <175354924+0xiamflux@users.noreply.github.com>
Co-authored-by: Ján Jakub Naništa <jan.jakub.nanista@gmail.com>
Co-authored-by: Stefano Charissis <stefano@oplabs.co>
Co-authored-by: George Knee <georgeknee@googlemail.com>
Co-authored-by: Ariel Diaz <65925295+aliersh@users.noreply.github.com>
Co-authored-by: smartcontracts <14298799+smartcontracts@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@karlfloersch karlfloersch Awaiting requested review from karlfloersch karlfloersch is a code owner

@smartcontracts smartcontracts Awaiting requested review from smartcontracts smartcontracts is a code owner

2 more reviewers

@gakonst gakonst gakonst requested changes

@snario snario snario approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tynes @gakonst @smartcontracts @snario