usmannk - p2p signatures are not unique per chain

[github-actions]

usmannk

medium

p2p signatures are not unique per chain

Summary

Signed L2 blocks can be gossiped on the p2p network for any chain.

Vulnerability Detail

Signed L2 blocks are meant to use the L2 Chain ID to create p2p signatures unique to each chain.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-node/rollup/types.go#L44-L45

However, the actual signing code clobbers the chain ID section with the payload hash

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-node/p2p/signer.go#L26-L39

msgInput[32:64] is filled with the chain ID and then immediately overwritten. The intended behavior is described in a comment on line 35.

Impact

Blocks signed for one chain id can be injected into the p2p pool on another, where nodes will ingest and attempt to append them to the chain. This will generally fail due to block number or parent root hash mismatch, but causes unnecessary network load and is a DoS vector.

Code Snippet

Tool used

Manual Review

Recommendation

Fix the SigningHash function such that the payload hash is placed into the third 32 bytes of msgInput.

Duplicate of #67

[rcstanciu]

Comment from Optimism

---

Reason: The chain ID is indeed being clobbered by the payloadBytes. DDoS risk is low, however, so this is more of a low than a medium.

[UsmannK]

Escalate for 250 USDC

Optimism team acknowledged this is a bug and fixed it here: ethereum-optimism/optimism#4864

I believe that this issue could lead to consensus splits, especially with decentralized sequencers. spec-compliant clients would have a complete p2p layer split between themselves and the op-node clients. op-node clients would only gossip and accept the non-compliant blocks. spec-compliant clients would only gossip and ingest blocks where the chainid was written into the signature correctly. There would be a total p2p split between compliant and non-compliant clients for this issue.

Consensus failures are valid medium severity issues:

- Causing a consensus failure
- Explanation: There is one sequencer op-node submitting transaction batches to L1, but many verifier op-nodes will read these batches and check the results of its execution. The sequencer and verifiers must remain in consensus, even in the event of an L1 reorg.

Also as a general security issue, It seems somewhat serious that blocks with the wrong chain id are marked valid on mainnet and are gossiped + processed. It breaks quite a few assumptions that were made in the p2p/block processing system. While there are only consensus impacts currently, allowing blocks to be replayed cross-chain opens up the potential for other nasty and unexpected bugs in the future, as developers would not anticipate cross-chain replay to be a possibility.

In any case, given the team's comment "this is more of a low than a medium.", this issue should at least be a rewarded low due to breaking specification.

[proof of minimum Low] Link to the broken specification here: https://github.com/ethereum-optimism/optimism/blob/develop/specs/rollup-node-p2p.md#block-signatures

Please note that #67 is a dup of this issue, but does not address the consensus issue in their report -- only the specification-breaking issue, so that report may be Low.

[sherlock-admin]

Escalate for 250 USDC

Optimism team acknowledged this is a bug and fixed it here: ethereum-optimism/optimism#4864

I believe that this issue could lead to consensus splits, especially with decentralized sequencers. spec-compliant clients would have a complete p2p layer split between themselves and the op-node clients. op-node clients would only gossip and accept the non-compliant blocks. spec-compliant clients would only gossip and ingest blocks where the chainid was written into the signature correctly. There would be a total p2p split between compliant and non-compliant clients for this issue.

Consensus failures are valid medium severity issues:

- Causing a consensus failure
- Explanation: There is one sequencer op-node submitting transaction batches to L1, but many verifier op-nodes will read these batches and check the results of its execution. The sequencer and verifiers must remain in consensus, even in the event of an L1 reorg.

Also as a general security issue, It seems somewhat serious that blocks with the wrong chain id are marked valid on mainnet and are gossiped + processed. It breaks quite a few assumptions that were made in the p2p/block processing system. While there are only consensus impacts currently, allowing blocks to be replayed cross-chain opens up the potential for other nasty and unexpected bugs in the future, as developers would not anticipate cross-chain replay to be a possibility.

In any case, given the team's comment "this is more of a low than a medium.", this issue should at least be a rewarded low due to breaking specification.

[proof of minimum Low] Link to the broken specification here: https://github.com/ethereum-optimism/optimism/blob/develop/specs/rollup-node-p2p.md#block-signatures

Please note that #67 is a dup of this issue, but does not address the consensus issue in their report -- only the specification-breaking issue, so that report may be Low.

You've created a valid escalation for 250 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.

[Evert0x]

Escalation accepted.

Same escalation from #67 applies, also marking as duplicate of #67

[sherlock-admin]

Escalation accepted.

Same escalation from #67 applies, also marking as duplicate of #67

This issue's escalations have been accepted!

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