Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

Koolex - LES (Light Ethereum Subprotocol) doesn't forward the transaction to the sequencer #175

Open
github-actions bot opened this issue Feb 20, 2023 · 5 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Medium A valid Medium severity issue Reward A payout will be made for this issue

Comments

@github-actions
Copy link

Koolex

medium

LES (Light Ethereum Subprotocol) doesn't forward the transaction to the sequencer

Summary

LES (Light Ethereum Subprotocol) doesn't forward the transaction to the sequencer when receiving it over RPC.

Vulnerability Detail

When a user submits a transaction to op-geth node (validator/verfier mode), the node sends the transaction to the sequencer, if no error, it adds it to the tx pool.

func (b *EthAPIBackend) SendTx(ctx context.Context, tx *types.Transaction) error {
	if b.eth.seqRPCService != nil {
		data, err := tx.MarshalBinary()
		if err != nil {
			return err
		}
		if err := b.eth.seqRPCService.CallContext(ctx, nil, "eth_sendRawTransaction", hexutil.Encode(data)); err != nil {
			return err
		}
	}
	return b.eth.txPool.AddLocal(tx)
}

https://github.com/ethereum-optimism/op-geth/blob/optimism-history/eth/api_backend.go#L253-L264

However, when LES, It only adds the transaction to the tx pool.

func (b *LesApiBackend) SendTx(ctx context.Context, signedTx *types.Transaction) error {
	return b.eth.txPool.Add(ctx, signedTx)
}

https://github.com/ethereum-optimism/op-geth/blob/optimism-history/les/api_backend.go#L193-L195

Note: Sequencer http flag is configured only if we're running in verifier mode.

Impact

  • Transction isn't sent to the sequencer and will never be processed (submitted to L1).
  • Inconsistency among op-geth nodes validators/verifiers and the sequencer.
  • Additionally, from UX perspective, it is misleading as the user would think the transaction was submitted "successfully".

Code Snippet

Check above.

Tool used

Manual Review

Recommendation

Match this RPC change in the LES RPC. As it seems to be overlooked.

Ref:
https://op-geth.optimism.io/

@rcstanciu
Copy link
Contributor

Comment from Optimism


Description: LES doesn't forward transactions to the sequencer

Reason: We don't support LES, so this is out-of-scope.

Action: No action.

@sherlock-admin sherlock-admin added the Non-Reward This issue will not receive a payout label Feb 21, 2023
@koolexcrypto
Copy link

koolexcrypto commented Feb 23, 2023

Escalate for 53 USDC

I kindly ask you to reconsider the assessment due to the following reasons:

  1. In the contest description, it lists the components that are in scope:

    The key components of the system can be found in our monorepo at commit 3f4b3c3281.
    L1 Contracts
    L2 Contracts (AKA Predeploys)
    op-node
    op-geth (in its own repo)

    op-geth is listed as it is, and it is never mentioned anywhere in the docs that LES is out of scope. As a participant, I spent quite some time to look for bugs in LES. otherwise, I would have shifted my focus on some other parts of the component considering the short time of the contest. On a personal level, it feels a bit unfair.

  2. If you go over LES directory in the source code. You could see the code was adapted to match Optimism specs. This actually gave me a confirmation that it is in scope. Here are some examples:

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Feb 23, 2023

Escalate for 53 USDC

I kindly ask you to reconsider the assessment due to the following reasons:

  1. In the contest description, it lists the components that are in scope:

The key components of the system can be found in our monorepo at commit 3f4b3c3281.
L1 Contracts
L2 Contracts (AKA Predeploys)
op-node
op-geth (in its own repo)

op-geth is listed as it is, and it is never mentioned anywhere in the docs that LES is out of scope. As a participant, I spent quite some time to look for bugs in LES. otherwise, I would have shifted my focus on some other parts of the component considering the short time of the contest. On a personal level, it feels a bit unfair.

  1. If you go over LES directory in the source code. You could see the code was adapted to match Optimism specs. This actually gave me a confirmation that it is in scope. Here are some examples:

You've created a valid escalation for 53 USDC!

To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label Feb 23, 2023
@Evert0x
Copy link
Contributor

Evert0x commented Mar 2, 2023

Escalation accepted.

Labeling as medium severity.

This was an oversight on Optimism's part and there are markers that would suggest it should be in scope.

@Evert0x Evert0x reopened this Mar 2, 2023
@sherlock-admin
Copy link
Contributor

Escalation accepted.

Labeling as medium severity.

This was an oversight on Optimism's part and there are markers that would suggest it should be in scope.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

@sherlock-admin sherlock-admin added Escalation Resolved This issue's escalations have been approved/rejected and removed Escalated This issue contains a pending escalation labels Mar 2, 2023
@Evert0x Evert0x added the Medium A valid Medium severity issue label Mar 2, 2023
@rcstanciu rcstanciu added Reward A payout will be made for this issue and removed Non-Reward This issue will not receive a payout labels Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Escalation Resolved This issue's escalations have been approved/rejected Medium A valid Medium severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

4 participants