#37352 [BC-Insight] Missing Liveness Check in `collectTableNodes()`

Submitted on Dec 2nd 2024 at 17:42:35 UTC by @CertiK for Attackathon | Ethereum Protocol

• Report ID: #37352

• Report Type: Blockchain/DLT

• Report severity: Insight

• Target: https://github.com/ledgerwatch/erigon

• Impacts:

  • (Specifications) A bug in specifications with no direct impact on client implementations

Description

Brief/Intro

An issue that the node misses liveness check will be added to the node table with collectTableNodes() was identified in the Ethereum client Erigon ( https://github.com/erigontech/erigon ).

Vulnerability Details

Affected Codebase: https://github.com/erigontech/erigon/tree/v2.61.0-beta1

The function collectTableNodes() is intended to collect all the nodes for the FindNode result given a specified distance:

https://github.com/erigontech/erigon/blob/v2.60.10/accounts/abi/type.go#L158

func (t *UDPv5) collectTableNodes(rip net.IP, distances []uint, limit int) []*enode.Node {
	nodes := make([]*enode.Node, 0, len(distances))
	var processed = make(map[uint]struct{})
	for _, dist := range distances {
		// Reject duplicate / invalid distances.
		_, seen := processed[dist]
		if seen || dist > 256 {
			continue
		}


		// Get the nodes.
		var bn []*enode.Node
		if dist == 0 {
			bn = []*enode.Node{t.Self()}
		} else if dist <= 256 {
			t.tab.mutex.Lock()
			bn = unwrapNodes(t.tab.bucketAtDistance(int(dist)).entries)
			t.tab.mutex.Unlock()
		}
		processed[dist] = struct{}{}


		// Apply some pre-checks to avoid sending invalid nodes.
		for _, n := range bn {
			// TODO livenessChecks > 1
			if netutil.CheckRelayIP(rip, n.IP()) != nil {
				continue
			}
			nodes = append(nodes, n)
			if len(nodes) >= limit {
				return nodes
			}
		}
	}
	return nodes
}

However, it misses the liveness chen when collecting the nodes into the table, which is also mentioned as the TODO: (https://github.com/erigontech/erigon/blob/v2.61.0-beta1/p2p/discover/v5_udp.go#L839 ）:

In this case, the node that has not been checked liveness will also be included in the table.

It is worth noted a similar issue has been fixed in go-ethereum: https://github.com/ethereum/go-ethereum/pull/28686

Impact Details

.Nodes with no liveness check will be included in the node table.

References

• https://github.com/erigontech/erigon/tree/v2.61.0-beta1

• https://github.com/ethereum/go-ethereum/pull/28686

Proof of Concept

Proof of Concept

Here we provide the following test case to show that nodes with no liveness check will be collected.

The default livenessChecks in function wrapNode() is zero, so there is no liveness check, which is used to mimic the nodes without liveness checks.

package discover


import (
   "bytes"
   "context"
   "crypto/ecdsa"
   "encoding/binary"
   "errors"
   "fmt"
   "net"
   "reflect"
   "runtime"
   "testing"
   "time"


   "github.com/ledgerwatch/erigon/turbo/testlog"
   "github.com/ledgerwatch/log/v3"


   "github.com/ledgerwatch/erigon/p2p/discover/v5wire"
   "github.com/ledgerwatch/erigon/p2p/enode"
   "github.com/ledgerwatch/erigon/p2p/enr"
   "github.com/ledgerwatch/erigon/rlp"
)



func wrapNode(n *enode.Node) *node {
   return &node{Node: *n}
}


func wrapNodes(ns []*enode.Node) []*node {
   result := make([]*node, len(ns))
   for i, n := range ns {
      result[i] = wrapNode(n)
   }
   return result
}

////////////unit test//////////

func TestCollectTableNodes(t *testing.T) {
   logger := log.New()
   test := newUDPV5Test(t, logger)
   t.Cleanup(test.close)

   nodes253 := nodesAtDistance(test.table.self().ID(), 253, 5)
   fillTable(test.table, wrapNodes(nodes253))

   rip := new(net.IP)
   distances := []uint{253}
   limit := 256

   nodes := test.udp.collectTableNodes(*rip, distances, limit)

   fmt.Printf("The collected nodes are: %v\n", nodes)
   fmt.Printf("Number of the collected nodes is: %d\n", len(nodes))

}

As the test result shows, all nodes without liveness check are also collected into the table:

=== RUN   TestCollectTableNodes
The collected nodes are: [enr:-DyAgIJpZIRudWxsgmlwhAEAAgGIbnVsbGFkZHKgN_Nh_UDMNfBEwoyLNAHxti9lG9dB0WZUoYuTrh3Fikg enr:-DyAgIJpZIRudWxsgmlwhAIAAgKIbnVsbGFkZHKgN8qNUV_UmPwblUu29QgouNp_lYSdQkXJ7WowYEHpX_0 enr:-DyAgIJpZIRudWxsgmlwhAMAAgOIbnVsbGFkZHKgN6JjlG7eFq-HMOlKgjkhSh0dzSAeuFeUfcTQSwF6qrQ enr:-DyAgIJpZIRudWxsgmlwhAQAAgSIbnVsbGFkZHKgN3xzWvF5XH-wIVucytScYrWLImpix0rdD5oFVO2Ytrc enr:-DyAgIJpZIRudWxsgmlwhAUAAgWIbnVsbGFkZHKgN5LIK4BmeON56kQ4ocADZHbe9pKkBfoOLctBpfgBpWM]
Number of the collected nodes is: 5
--- PASS: TestCollectTableNodes (0.04s)
PASS