obront - Batcher frames are incorrectly decoded leading to consensus split

[github-actions]

obront

medium

Batcher frames are incorrectly decoded leading to consensus split

Summary

There is an error in the implementation of how frames are decoded, which will allow invalid frames to be accepted. This can allow a malicious sequencer to cause a consensus split between op-node implementations and the (correct) reference implementation.

Vulnerability Detail

Optimism implements a highly efficient derivation scheme based on compression of L2 data that is sent to an L1 address. The spec clearly defines how this works. Channels are split into frames, each one encoded as defined here. Frames will be aggregated by the derivation driver.

Docs state that:

All data in a frame is fixed-size, except the frame_data. The fixed overhead is 16 + 2 + 4 + 1 = 23 bytes. Fixed-size frame metadata avoids a circular dependency with the target total data length, to simplify packing of frames with varying content length.

Specifically:

is_last is a single byte with a value of 1 if the frame is the last in the channel, 0 if there are frames in the channel. Any other value makes the frame invalid (it must be ignored by the rollup node).

Clearly, is_last is mandatory as per the specs. However, if we look at the code it will accept a frame even if is_last is not supplied.

Decoding of the frame is done in frame.go, in the UnmarshalBinary function. After reading the frame data, only the last byte remains.

if isLastByte, err := r.ReadByte(); err != nil && err != io.EOF {
	return fmt.Errorf("error reading final byte: %w", err)
} else if isLastByte == 0 {
	f.IsLast = false
	return err
} else if isLastByte == 1 {
	f.IsLast = true
	return err
} else {
	return errors.New("invalid byte as is_last")
}

If the ByteReader object is empty, reading the next byte will return an EOF and the error clause is skipped. The result of ReadByte when an error occurs is undefined, however in all Go setups we've tested isLastByte is zero. This means it sets f.IsLast = false and returns the EOF.

Back in ParseFrames which calls UnmarshalBinary, the EOF is ignored and the frame is accepted:

for buf.Len() > 0 {
	var f Frame
	if err := (&f).UnmarshalBinary(buf); err != io.EOF && err != nil {
		return nil, err
	}
	frames = append(frames, f)
}

So, it is demonstrated that an invalid frame is accepted by the Optimism implementation, provided the frame is the last one in the frames buffer. The impact is that a malicious sequencer can cause a consensus split between correct implementations and the reference implementation. It has been defined by the rules as Medium severity:

- 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.

All that is needed is to send in different frame packages two frames of a channel, omit the is_last byte in the first frame and make sure it is the last frame in the package.

Impact

Malicious sequencer can easily cause a consensus split by taking advantage of the incorrect frame reading logic in op-node.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/407f97b9d13448b766624995ec824d3059d4d4f6/op-node/rollup/derive/frame.go#L93

Tool used

Manual Review

Recommendation

In UnmarshalBinary, return a non-EOF error when is_last byte does not exist.

[rcstanciu]

Comment from Optimism

---

Description: A malicious or buggy batcher could submit a frame that the op-node would accept that a spec compliant node would not due to a bug in our parsing code.

Reason: This would cause a consensus failure between different clients.

Action: We need to fix this code.

[zobront]

Escalate for 250 USDC

This submission concerns a specific consensus failure due to parsing of batcher frames.

It has been wrongly duped to #53, which concerns a contract level issue with posted output roots for withdrawals.

Optimism has acknowledged the validity of the issue and this is certainly just a misclick, as the issues are entirely unrelated.

[sherlock-admin]

Escalate for 250 USDC

This submission concerns a specific consensus failure due to parsing of batcher frames.

It has been wrongly duped to #53, which concerns a contract level issue with posted output roots for withdrawals.

Optimism has acknowledged the validity of the issue and this is certainly just a misclick, as the issues are entirely unrelated.

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 and resolving incorrect duplication state

[sherlock-admin]

Escalation accepted and resolving incorrect duplication state

This issue's escalations have been accepted!

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