search

sherlock-audit / 2023-01-optimism-judging

24 stars 10 forks source link

Koolex - Incorrectness in computing block signing hash allows cross-chain replay attacks #67

Open github-actions[bot] opened 1 year ago

github-actions[bot] commented 1 year ago

Koolex

high

Incorrectness in computing block signing hash allows cross-chain replay attacks

Summary

Incorrectness in computing block signing hash allows cross-chain replay attacks

Vulnerability Detail

The sequencer signs over a message: keccak256(domain ++ chain_id ++ payload_hash). The chain_id is included to prevent replaying the same message over another chain. However, SigningHash function fails to ensure the chain_id is included in the message.

func SigningHash(domain [32]byte, chainID *big.Int, payloadBytes []byte) (common.Hash, error) {
    var msgInput [32 + 32 + 32]byte
    // domain: first 32 bytes
    copy(msgInput[:32], domain[:])
    // chain_id: second 32 bytes
    if chainID.BitLen() > 256 {
        return common.Hash{}, errors.New("chain_id is too large")
    }
    chainID.FillBytes(msgInput[32:64])
    // payload_hash: third 32 bytes, hash of encoded payload
    copy(msgInput[32:], crypto.Keccak256(payloadBytes))

    return crypto.Keccak256Hash(msgInput[:]), nil
}

If you look at this line:

copy(msgInput[32:], crypto.Keccak256(payloadBytes))

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

It is supposed to copy the encoded payload hash to the third 32 bytes of msgInput. However, it start from 32nd byte which would overwrite the second 32 bytes allocated for the chain_id.

Impact

Any block signed by the sequencer in any chain is valid for other chains. For example, a malicious verifier can pick a message signed for a test chain and gossip it out for P2P on main chain.

Code Snippet

Please create a file gossip_crosschain_test.go under op-node/p2p directory and add the following code:

package p2p

import (
    // "encoding/binary"
    l "log"
    "testing"
    "math/big"

    "github.com/stretchr/testify/require"
    "github.com/ethereum-optimism/optimism/op-node/rollup"
)

func TestSigningHash(t *testing.T) {

    cfg1 := &rollup.Config{
        L2ChainID:         big.NewInt(100),
    }
    cfg2 := &rollup.Config{
        L2ChainID:         big.NewInt(101),
    }

    payloadBytes := []byte("arbitraryData")
    hash, err := SigningHash(SigningDomainBlocksV1,cfg1.L2ChainID,payloadBytes)
    if err != nil {
        l.Println("Error while hashing", err)
    }
    l.Println("hash : ", hash)

    hash2, err2 := SigningHash(SigningDomainBlocksV1,cfg2.L2ChainID,payloadBytes)
    if err2 != nil {
        l.Println("Error while hashing", err2)
    }
    l.Println("hash2: ", hash2)

    require.NotEqual(t, hash, hash2)
}

Run the test as follows:

go test -run=TestSigningHashCrossChain ./p2p -v

The test will fail as the two hashes match:

=== RUN   TestSigningHash
2023/01/28 15:57:41 hash :  0x35af5b09dcb95c4ebac34a932a84a70d6aa97dece830972ce9d0affb7cbaea30
2023/01/28 15:57:41 hash2:  0x35af5b09dcb95c4ebac34a932a84a70d6aa97dece830972ce9d0affb7cbaea30
    gossip_attack_test.go:35: 
            Error Trace:    /share/2023-01-optimism-koolexcrypto/optimism/op-node/p2p/gossip_attack_test.go:35
            Error:          Should not be: 0x35af5b09dcb95c4ebac34a932a84a70d6aa97dece830972ce9d0affb7cbaea30
            Test:           TestSigningHash
--- FAIL: TestSigningHash (0.00s)
FAIL
FAIL    github.com/ethereum-optimism/optimism/op-node/p2p   0.634s
FAIL

Tool used

Manual Review

Recommendation

Just replace this 

    copy(msgInput[32:], crypto.Keccak256(payloadBytes))

with 

    copy(msgInput[64:], crypto.Keccak256(payloadBytes))

Run the test above again and it should pass successfully.

rcstanciu commented 1 year ago

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.

koolexcrypto commented 1 year ago

Escalate for 150 USDC

Speaking generally, cross-chain replay was always a critical/high issue. However, in this case here, I believe it should be considered as a medium (at least) since DDoS risk can not be disregarded. Although Optimism's judge acknowledged the DDoS risk, I disagree with the low severity assessment for the following reasons:

  1. As you might agree, It is difficult to measure the extent of DDoS attack given the short time of the contest. nevertheless, the issue could cause unpredictable bugs causing serious impacts in future.

  2. Since it's planned to decentralise the sequencer ultimately by supporting multiple concurrent sequencers (check refs below), the risk is even higher. Imagine the following (note: this is very simplified just to explain the impact roughly):

    • Assume DDoS_risk is the risk level of making the network too busy. seq(x) is sequencer x. count is how many sequencers we have. impact(seq(x)) is the potential impact caused by sequencer x. T(x) is the count of blocks signed by sequencer x in other chains (e.g. Optimism Goerli). The higher the T, the higher the impact. If T = 0, there is basically no impact

      Let $impact(seq(x)) = f(T(x))$ ,then

      DDoS_risk = $\sum_{n = 1}^{count} impact(seq(n))$

As you can notice, once we have multiple sequencers, the risk gets higher.

  1. It's also worth mentioning that if it happens that a sequencer produces blocks for Optimism and a forked chain, then one can gossip (in Optimism) those blocks signed for the forked chain. Have a look at this forked chain (VMeta3 chain): https://github.com/VMETA3/vmeta3-chain/blob/master/op-node/p2p/signer.go#L25-L35 It has exactly the same code.

  2. Even though the code was audited multiple times, the issue was still overlooked since (at least) May 16, 2022. This shows somewhat the error could possibly stay there till after decentralising the sequencer. Check: https://github.com/ethereum-optimism/optimism/commits/2006a2f5ed921b1069446abbae805a037541dcab/op-node/p2p/signer.go

Moreover, as per Optimism specs:

The signature is a secp256k1 signature, and signs over a message: keccak256(domain ++ chain_id ++ payload_hash)

Still, the issue was marked as non-reward even though it goes under the "Specification errors" category described in the contest regardless of the reasoning above.

References/Links:

Please note that there is a duplicate of the issue and the escalation should be applied to both Issue #125. The issue is also escalated.

sherlock-admin commented 1 year ago

Escalate for 150 USDC

Speaking generally, cross-chain replay was always a critical/high issue. However, in this case here, I believe it should be considered as a medium (at least) since DDoS risk can not be disregarded. Although Optimism's judge acknowledged the DDoS risk, I disagree with the low severity assessment for the following reasons:

  1. As you might agree, It is difficult to measure the extent of DDoS attack given the short time of the contest. nevertheless, the issue could cause unpredictable bugs causing serious impacts in future.

  2. Since it's planned to decentralise the sequencer ultimately by supporting multiple concurrent sequencers (check refs below), the risk is even higher. Imagine the following (note: this is very simplified just to explain the impact roughly):

    • Assume DDoS_risk is the risk level of making the network too busy. seq(x) is sequencer x. count is how many sequencers we have. impact(seq(x)) is the potential impact caused by sequencer x. T(x) is the count of blocks signed by sequencer x in other chains (e.g. Optimism Goerli). The higher the T, the higher the impact. If T = 0, there is basically no impact

      Let $impact(seq(x)) = f(T(x))$ ,then

      DDoS_risk = $\sum_{n = 1}^{count} impact(seq(n))$

As you can notice, once we have multiple sequencers, the risk gets higher.

  1. It's also worth mentioning that if it happens that a sequencer produces blocks for Optimism and a forked chain, then one can gossip (in Optimism) those blocks signed for the forked chain. Have a look at this forked chain (VMeta3 chain): https://github.com/VMETA3/vmeta3-chain/blob/master/op-node/p2p/signer.go#L25-L35 It has exactly the same code.

  2. Even though the code was audited multiple times, the issue was still overlooked since (at least) May 16, 2022. This shows somewhat the error could possibly stay there till after decentralising the sequencer. Check: https://github.com/ethereum-optimism/optimism/commits/2006a2f5ed921b1069446abbae805a037541dcab/op-node/p2p/signer.go

Moreover, as per Optimism specs:

The signature is a secp256k1 signature, and signs over a message: keccak256(domain ++ chain_id ++ payload_hash)

Still, the issue was marked as non-reward even though it goes under the "Specification errors" category described in the contest regardless of the reasoning above.

References/Links:

Please note that there is a duplicate of the issue and the escalation should be applied to both Issue #125. The issue is also escalated.

You've created a valid escalation for 150 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 commented 1 year ago

Escalation accepted.

Accepting as a specification issue.

Although it's an issue that needs to be addressed, the likelihood that it will have an impact is so insignificant that it's considered a non-medium severity issue, labeling as spec issue because of section in escalation.

sherlock-admin commented 1 year ago

Escalation accepted.

Accepting as a specification issue.

Although it's an issue that needs to be addressed, the likelihood that it will have an impact is so insignificant that it's considered a non-medium severity issue, labeling as spec issue because of section in escalation.

This issue's escalations have been accepted!

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