Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid chainid underflow when v = 0 #14

Merged
merged 1 commit into from
Mar 24, 2023

Conversation

pcw109550
Copy link
Member

For prebedrock transactions, some txs have r = 0, s = 0, v = 0.
When apis(ots_getBlockTransactions, eth_getTransactionByHash etc) tries to recover LegacyTx's chainid by using v = 0, It calls DeriveChainId. Below is the method implementation.

// deriveChainID derives the chain id from the given v parameter
func DeriveChainId(v *uint256.Int) *uint256.Int {
	if v.IsUint64() {
		v := v.Uint64()
		if v == 27 || v == 28 {
			return new(uint256.Int)
		}
		return new(uint256.Int).SetUint64((v - 35) / 2)
	}
	r := new(uint256.Int).Sub(v, u256.Num35)
	return r.Div(r, u256.Num2)
}

So when v = 0, return value will be (v - 35) / 2 = (0 - 35) / 2 = -18, so underflow occurs. When casted to uint64, it results in value 0x7fffffffffffffee.

This bug is observed by inspecting; optimism's RPC endpoint: https://goerli.optimism.io, testinprod's RPC endpoint, and even at alchemy. Example payload:

{
	"jsonrpc":"2.0",
	"method":"eth_getTransactionByHash",
	"params":[
		"0x7334ddc1f6beaf66892c25cffdecec275cdfabaf4def047f0c3ce20e6f6483e8"
	],
	"id":1
}

Response:

{
    "jsonrpc": "2.0",
    "result": {
        "blockHash": "0x15d55041e8f7b0d1f303b6d4cefe2d2efc257d67acd9f17307261a8f7d786e0e",
        "blockNumber": "0x1",
        "chainId": "0x7fffffffffffffee",
        "from": "0x0000000000000000000000000000000000000000",
        "gas": "0x30d40",
        "gasPrice": "0x0",
        "hash": "0x7334ddc1f6beaf66892c25cffdecec275cdfabaf4def047f0c3ce20e6f6483e8",
        "input": "0xcbd4ece90000000000000000000000004200000000000000000000000000000000000010000000000000000000000000636af16bf2f682dd3109e60102b8e1a089fedaa80000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000e4662a633a0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000deaddeaddeaddeaddeaddeaddeaddeaddead00000000000000000000000000003a605b442055df2898e18cf518feb2e2a6bd0d310000000000000000000000003a605b442055df2898e18cf518feb2e2a6bd0d310000000000000000000000000000000000000000000000000429d069189e000000000000000000000000000000000000000000000000000000000000000000c0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
        "nonce": "0x0",
        "r": "0x0",
        "s": "0x0",
        "to": "0x4200000000000000000000000000000000000007",
        "transactionIndex": "0x0",
        "type": "0x0",
        "v": "0x0",
        "value": "0x0"
    },
    "id": 1
}

l2geth also has the identical bug, but its RPC handler implementation is different, and does not include buggy chainId to its RPC response.

This PR is a temp fix. When v = 0, do not include chainId in response. Better to not include then returning wrong result.

Also, this bug broke otterscan's frontend: ots_getBlockTransactions returned response containing chainId as 0x7fffffffffffffee. Frontend ethers.js BigNumber library overflowed, displaying incorrect results for /block/{block_number}/tx endpoint of otterscan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants