Bad RPC provider data can (and did) take down sequencer

Summary

Batchers retrieve the suggested gas tip from the RPC provider by calling eth_maxPriorityFeePerGas, but if RPC returns a nil value, the resulting panic error can take down the Sequencer. This, in fact, happened during the contest.

Vulnerability Detail

When the batcher puts together a new block, it follows the following process:

The loop() function creates new L2 blocks by loading in and sending transactions.
For each tx, it calls SendTransaction() which calls CraftTx() to put together the full transaction data (chain ID, nonce, gas, etc).
At one point, CraftTx() calls calcGasTipAndFeeCap(), which queries the RPC provider by calling eth_maxPriorityFeePerGas. This information is used to fill the GasTipCap field of the transaction.

However, the eth_maxPriorityFeePerGas method is not supported by all RPC providers, and can therefore return nil.

If this happens, then when gasTipCap is passed to following function, it will panic:

gasFeeCap = txmgr.CalcGasFeeCap(head.BaseFee, gasTipCap)

func CalcGasFeeCap(baseFee, gasTipCap *big.Int) *big.Int {
    return new(big.Int).Add(
        gasTipCap,
        new(big.Int).Mul(baseFee, big.NewInt(2)),
    )
}

The unhandled panic in CalcGasFeeCap will take down the Sequencer.

Impact

If the Sequencer tries to create a transaction and pings an RPC endpoint that doesn't support eth_maxPriorityFeePerGas (or is temporarily unable to return a valid value), the Sequencer will panic and be taken down.

This happened on Jan 25 and took down the Goerli Sequencer for over 3 hours.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/op-batcher/batcher/txmgr.go#L73-L90

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/bss-core/txmgr/txmgr.go#L335-L340

Tool used

Manual Review

Recommendation

We see that the fix performed by the Optimism team on Jan 25th was to add a check in calcGasTipAndFeeCap() to ensure that eth_maxPriorityFeePerGas returns a non-nil value. Otherwise, use a default value.

We would like to note that this is solving only one instance of the dangerous pattern that exists in SuggestGasTipCap() (as well as SuggestGasPrice(), which follows the same pattern).

In both of these functions, when values are returned from the RPC, if a nil value is returned without an error, we will end up with a return value of nil, nil and this will not be caught by the current error checking.

Assuming you don't want to add more code to the diff for op-geth, consider doing a careful review of all places in op-node where these two functions are referenced, and adding a similar check to ensure nil return values are caught.

sherlock-audit / 2023-01-optimism-judging

obront - Bad RPC provider data can (and did) take down sequencer #274