From c746d1913f096675403dde68498576847c5ea099 Mon Sep 17 00:00:00 2001 From: Rudra644 Date: Sun, 14 Jun 2026 19:09:37 +0530 Subject: [PATCH] fix: resolve three critical security bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. SHA256→Keccak256 in initial message signing (C-1) - Changed SignInitialMessage and EVMAddressFromSignatures to use Keccak256 for the inner hash, matching the bridge contract's SHA256(Keccak256(...)) signature scheme 2. PrevDisputeIds append wrong ID (C-4) - Append prevDisputeId instead of disputeId to prevent double-counting votes in dispute tally rounds 3. Missing signature verification in SetOracleAttestation (C-5) - Added ECDSA recover verification to ensure attestation signatures match the validator's registered EVM address before storing --- app/extend_vote.go | 18 ++++----------- app/testutils/utils.go | 13 ++++------- x/bridge/keeper/keeper.go | 42 +++++++++++++++++++++++++--------- x/bridge/keeper/keeper_test.go | 26 +++++++-------------- x/dispute/keeper/dispute.go | 2 +- 5 files changed, 49 insertions(+), 52 deletions(-) diff --git a/app/extend_vote.go b/app/extend_vote.go index 0d629d6ab..1bd6fe332 100644 --- a/app/extend_vote.go +++ b/app/extend_vote.go @@ -3,7 +3,6 @@ package app import ( "bytes" "context" - "crypto/sha256" "encoding/hex" "encoding/json" "errors" @@ -15,6 +14,7 @@ import ( abci "github.com/cometbft/cometbft/abci/types" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/crypto" "github.com/spf13/viper" bridgetypes "github.com/tellor-io/layer/x/bridge/types" oracletypes "github.com/tellor-io/layer/x/oracle/types" @@ -283,19 +283,11 @@ func (h *VoteExtHandler) SignInitialMessage(operatorAddress string) ([]byte, []b messageA := fmt.Sprintf("TellorLayer: Initial bridge signature A for operator %s", operatorAddress) messageB := fmt.Sprintf("TellorLayer: Initial bridge signature B for operator %s", operatorAddress) - // convert message to bytes - msgBytesA := []byte(messageA) - msgBytesB := []byte(messageB) + // hash messages with Keccak256 (Ethereum standard) then keyring auto-hashes with SHA256 + // producing SHA256(Keccak256(message)) to match the bridge contract's _verifySig scheme + msgHashABytes := crypto.Keccak256([]byte(messageA)) + msgHashBBytes := crypto.Keccak256([]byte(messageB)) - // hash message - msgHashABytes32 := sha256.Sum256(msgBytesA) - msgHashBBytes32 := sha256.Sum256(msgBytesB) - - // convert [32]byte to []byte - msgHashABytes := msgHashABytes32[:] - msgHashBBytes := msgHashBBytes32[:] - - // sign message sigA, err := h.SignMessage(msgHashABytes) if err != nil { return nil, nil, err diff --git a/app/testutils/utils.go b/app/testutils/utils.go index f7f22e14d..ffaea2cdb 100644 --- a/app/testutils/utils.go +++ b/app/testutils/utils.go @@ -96,17 +96,12 @@ func GenerateSignatures(t *testing.T) (sigA, sigB []byte, evmAddr common.Address msgA := "TellorLayer: Initial bridge signature A" msgB := "TellorLayer: Initial bridge signature B" - msgBytesA := []byte(msgA) - msgBytesB := []byte(msgB) - // hash messages - msgHashBytes32A := sha256.Sum256(msgBytesA) - msgHashBytesA := msgHashBytes32A[:] + // hash messages with Keccak256 (matching the updated EVMAddressFromSignatures) + msgHashBytesA := crypto.Keccak256([]byte(msgA)) + msgHashBytesB := crypto.Keccak256([]byte(msgB)) - msgHashBytes32B := sha256.Sum256(msgBytesB) - msgHashBytesB := msgHashBytes32B[:] - - // hash the hash, since the keyring signer automatically hashes the message + // hash the hash to simulate the keyring signer's auto-hash with SHA256 msgDoubleHashBytes32A := sha256.Sum256(msgHashBytesA) msgDoubleHashBytesA := msgDoubleHashBytes32A[:] diff --git a/x/bridge/keeper/keeper.go b/x/bridge/keeper/keeper.go index 711f065b9..d641ac6f1 100644 --- a/x/bridge/keeper/keeper.go +++ b/x/bridge/keeper/keeper.go @@ -634,18 +634,12 @@ func (k Keeper) EVMAddressFromSignatures(ctx context.Context, sigA, sigB []byte, msgA := fmt.Sprintf("TellorLayer: Initial bridge signature A for operator %s", operatorAddress) msgB := fmt.Sprintf("TellorLayer: Initial bridge signature B for operator %s", operatorAddress) - // convert messages to bytes - msgBytesA := []byte(msgA) - msgBytesB := []byte(msgB) + // hash messages with Keccak256 (Ethereum standard convention) + msgHashBytesA := crypto.Keccak256([]byte(msgA)) + msgHashBytesB := crypto.Keccak256([]byte(msgB)) - // hash messages - msgHashBytes32A := sha256.Sum256(msgBytesA) - msgHashBytesA := msgHashBytes32A[:] - - msgHashBytes32B := sha256.Sum256(msgBytesB) - msgHashBytesB := msgHashBytes32B[:] - - // hash the hash, since the keyring signer automatically hashes the message + // hash the hash, since the keyring signer automatically hashes the message with SHA256, + // producing the signed content as SHA256(Keccak256(message)) msgDoubleHashBytes32A := sha256.Sum256(msgHashBytesA) msgDoubleHashBytesA := msgDoubleHashBytes32A[:] @@ -816,6 +810,32 @@ func (k Keeper) SetOracleAttestation(ctx context.Context, operatorAddress string k.Logger(ctx).Info("Error getting EVM address from operator address", "error", err) return err } + + // verify the signature: the keyring auto-hashes the snapshot with SHA256 before signing, + // so the signed content is SHA256(snapshot). recover the signer's EVM address and + // compare against the registered address. + if len(sig) != 64 { + return fmt.Errorf("invalid signature length: expected 64, got %d", len(sig)) + } + snapshotHash := sha256.Sum256(snapshot) + sigMatches := false + for _, id := range []byte{0, 1} { + sigWithID := append(sig[:64], id) + pubKey, err := crypto.SigToPub(snapshotHash[:], sigWithID) + if err != nil { + continue + } + recoveredAddr := crypto.PubkeyToAddress(*pubKey) + if bytes.Equal(recoveredAddr.Bytes(), ethAddress.EVMAddress) { + sigMatches = true + break + } + } + if !sigMatches { + k.Logger(ctx).Info("Oracle attestation signature does not match validator's EVM address") + return fmt.Errorf("oracle attestation signature verification failed for operator %s", operatorAddress) + } + // get the last saved bridge validator set lastSavedBridgeValidators, err := k.BridgeValset.Get(ctx) if err != nil { diff --git a/x/bridge/keeper/keeper_test.go b/x/bridge/keeper/keeper_test.go index bca2573ed..ce7ca955b 100644 --- a/x/bridge/keeper/keeper_test.go +++ b/x/bridge/keeper/keeper_test.go @@ -708,17 +708,12 @@ func TestEVMAddressFromSignatures(t *testing.T) { msgA := fmt.Sprintf("TellorLayer: Initial bridge signature A for operator %s", operatorAddr) msgB := fmt.Sprintf("TellorLayer: Initial bridge signature B for operator %s", operatorAddr) - msgBytesA := []byte(msgA) - msgBytesB := []byte(msgB) - // hash messages - msgHashBytes32A := sha256.Sum256(msgBytesA) - msgHashBytesA := msgHashBytes32A[:] + // hash messages with Keccak256 (matching updated EVMAddressFromSignatures) + msgHashBytesA := crypto.Keccak256([]byte(msgA)) + msgHashBytesB := crypto.Keccak256([]byte(msgB)) - msgHashBytes32B := sha256.Sum256(msgBytesB) - msgHashBytesB := msgHashBytes32B[:] - - // hash the hash, since the keyring signer automatically hashes the message + // hash the hash to simulate keyring auto-hash with SHA256 msgDoubleHashBytes32A := sha256.Sum256(msgHashBytesA) msgDoubleHashBytesA := msgDoubleHashBytes32A[:] @@ -768,17 +763,12 @@ func TestTryRecoverAddressWithBothIDs(t *testing.T) { msgA := "TellorLayer: Initial bridge signature A" msgB := "TellorLayer: Initial bridge signature B" - msgBytesA := []byte(msgA) - msgBytesB := []byte(msgB) - - // hash messages - msgHashBytes32A := sha256.Sum256(msgBytesA) - msgHashBytesA := msgHashBytes32A[:] - msgHashBytes32B := sha256.Sum256(msgBytesB) - msgHashBytesB := msgHashBytes32B[:] + // hash messages with Keccak256 (matching updated TryRecoverAddressWithBothIDs usage) + msgHashBytesA := crypto.Keccak256([]byte(msgA)) + msgHashBytesB := crypto.Keccak256([]byte(msgB)) - // hash the hash, since the keyring signer automatically hashes the message + // hash the hash to simulate keyring auto-hash with SHA256 msgDoubleHashBytes32A := sha256.Sum256(msgHashBytesA) msgDoubleHashBytesA := msgDoubleHashBytes32A[:] diff --git a/x/dispute/keeper/dispute.go b/x/dispute/keeper/dispute.go index 206f5041d..16838448c 100644 --- a/x/dispute/keeper/dispute.go +++ b/x/dispute/keeper/dispute.go @@ -280,7 +280,7 @@ func (k Keeper) AddDisputeRound(ctx sdk.Context, sender sdk.AccAddress, dispute dispute.DisputeEndTime = ctx.BlockTime().Add(THREE_DAYS) dispute.DisputeStartBlock = uint64(ctx.BlockHeight()) dispute.DisputeRound++ - dispute.PrevDisputeIds = append(dispute.PrevDisputeIds, disputeId) + dispute.PrevDisputeIds = append(dispute.PrevDisputeIds, prevDisputeId) err := k.Disputes.Set(ctx, dispute.DisputeId, dispute) if err != nil {