fix: guarantee that max prefix length is < min prefix length + child …

…size (#369) * fix: guarantee that max prefix length is < min prefix length + child size * same fix for rust implementation * add test for rust * lint * Update rust/src/verify.rs * chore: rust formatting * proto: add inner op max prefix length restriction comment * test: add test for forging non existence proof * fix: test * lint * test: add unit test for new restriction * chore: update changelog --------- Co-authored-by: colin axnér <[email protected]>
cosmos · Sep 26, 2024 · 7000936 · 7000936
1 parent 22a0173
commit 7000936
Show file tree

Hide file tree

Showing 9 changed files with 175 additions and 5 deletions.
diff --git a/go/CHANGELOG.md b/go/CHANGELOG.md
@@ -3,6 +3,7 @@
 # Unreleased
 
 - deps: bump golang to v1.22 ([#363](https://github.com/cosmos/ics23/pull/363)).
+- fix: guarantee that `spec.InnerSpec.MaxPrefixLength` < `spec.InnerSpec.MinPrefixLength` + `spec.InnerSpec.ChildSize` ([#369](https://github.com/cosmos/ics23/pull/369))
 
 # v0.11.0
 

diff --git a/go/ops.go b/go/ops.go
@@ -166,6 +166,10 @@ func (op *InnerOp) CheckAgainstSpec(spec *ProofSpec, b int) error {
 		return errors.New("spec.InnerSpec.ChildSize must be >= 1")
 	}
 
+	if spec.InnerSpec.MaxPrefixLength >= spec.InnerSpec.MinPrefixLength+spec.InnerSpec.ChildSize {
+		return errors.New("spec.InnerSpec.MaxPrefixLength must be < spec.InnerSpec.MinPrefixLength + spec.InnerSpec.ChildSize")
+	}
+
 	// ensures soundness, with suffix having to be of correct length
 	if len(op.Suffix)%int(spec.InnerSpec.ChildSize) != 0 {
 		return fmt.Errorf("InnerOp suffix malformed")

diff --git a/go/ops_test.go b/go/ops_test.go
@@ -150,6 +150,13 @@ func TestInnerOpCheckAgainstSpec(t *testing.T) {
 			},
 			errors.New("spec.InnerSpec.ChildSize must be >= 1"),
 		},
+		{
+			"failure: MaxPrefixLength >= MinPrefixLength + ChildSize",
+			func() {
+				spec.InnerSpec.MaxPrefixLength = spec.InnerSpec.MinPrefixLength + spec.InnerSpec.ChildSize
+			},
+			errors.New("spec.InnerSpec.MaxPrefixLength must be < spec.InnerSpec.MinPrefixLength + spec.InnerSpec.ChildSize"),
+		},
 		{
 			"failure: inner op suffix malformed",
 			func() {
@@ -201,6 +208,133 @@ func TestInnerOpCheckAgainstSpec(t *testing.T) {
 	}
 }
 
+func TestForgeNonExistenceProofWithIncorrectMaxPrefixLength(t *testing.T) {
+	spec := &ProofSpec{ // TendermintSpec
+		LeafSpec: &LeafOp{
+			Prefix:       []byte{0},
+			PrehashKey:   HashOp_NO_HASH,
+			Hash:         HashOp_SHA256,
+			PrehashValue: HashOp_SHA256,
+			Length:       LengthOp_VAR_PROTO,
+		},
+		InnerSpec: &InnerSpec{
+			ChildOrder:      []int32{0, 1},
+			MinPrefixLength: 1,
+			MaxPrefixLength: 1,
+			ChildSize:       32, // (no length byte)
+			Hash:            HashOp_SHA256,
+		},
+	}
+
+	spec.InnerSpec.MaxPrefixLength = 33
+	leafOp := spec.LeafSpec
+	aLeaf, _ := leafOp.Apply([]byte("a"), []byte("a"))
+	bLeaf, _ := leafOp.Apply([]byte("b"), []byte("b"))
+	b2Leaf, _ := leafOp.Apply([]byte("b2"), []byte("b2"))
+
+	cLeaf, _ := leafOp.Apply([]byte("c"), []byte("c"))
+	aExist := ExistenceProof{
+		Key:   []byte("a"),
+		Value: []byte("a"),
+		Leaf:  leafOp,
+		Path: []*InnerOp{
+			{
+				Hash:   spec.InnerSpec.Hash,
+				Prefix: []byte{1},
+				Suffix: append(bLeaf, b2Leaf...),
+			},
+			{
+				Hash:   spec.InnerSpec.Hash,
+				Prefix: []byte{1},
+				Suffix: cLeaf,
+			},
+		},
+	}
+	bExist := ExistenceProof{
+		Key:   []byte("b"),
+		Value: []byte("b"),
+		Leaf:  leafOp,
+		Path: []*InnerOp{
+			{
+				Hash:   spec.InnerSpec.Hash,
+				Prefix: append([]byte{1}, aLeaf...),
+				Suffix: b2Leaf,
+			},
+			{
+				Hash:   spec.InnerSpec.Hash,
+				Prefix: []byte{1},
+				Suffix: cLeaf,
+			},
+		},
+	}
+	b2Exist := ExistenceProof{
+		Key:   []byte("b2"),
+		Value: []byte("b2"),
+		Leaf:  leafOp,
+		Path: []*InnerOp{
+			{
+				Hash:   spec.InnerSpec.Hash,
+				Prefix: append(append([]byte{1}, aLeaf...), bLeaf...),
+				Suffix: []byte{},
+			},
+			{
+				Hash:   spec.InnerSpec.Hash,
+				Prefix: []byte{1},
+				Suffix: cLeaf,
+			},
+		},
+	}
+	yHash, _ := aExist.Path[0].Apply(aLeaf)
+	cExist := ExistenceProof{
+		Key:   []byte("c"),
+		Value: []byte("c"),
+		Leaf:  leafOp,
+		Path: []*InnerOp{
+			{
+				Hash:   spec.InnerSpec.Hash,
+				Prefix: append([]byte{1}, yHash...),
+				Suffix: []byte{},
+			},
+		},
+	}
+	aNotExist := NonExistenceProof{
+		Key:   []byte("a"),
+		Left:  nil,
+		Right: &bExist,
+	}
+	root, err := aExist.Calculate()
+	if err != nil {
+		t.Fatal("failed to calculate existence proof of leaf a")
+	}
+
+	expError := fmt.Errorf("inner, %w", errors.New("spec.InnerSpec.MaxPrefixLength must be < spec.InnerSpec.MinPrefixLength + spec.InnerSpec.ChildSize"))
+	err = aExist.Verify(spec, root, []byte("a"), []byte("a"))
+	if err.Error() != expError.Error() {
+		t.Fatal("attempting to prove existence of leaf a returned incorrect error")
+	}
+
+	err = bExist.Verify(spec, root, []byte("b"), []byte("b"))
+	if err.Error() != expError.Error() {
+		t.Fatal("attempting to prove existence of leaf b returned incorrect error")
+	}
+
+	err = b2Exist.Verify(spec, root, []byte("b2"), []byte("b2"))
+	if err.Error() != expError.Error() {
+		t.Fatal("attempting to prove existence of third leaf returned incorrect error")
+	}
+
+	err = cExist.Verify(spec, root, []byte("c"), []byte("c"))
+	if err.Error() != expError.Error() {
+		t.Fatal("attempting to prove existence of leaf c returned incorrect error")
+	}
+
+	err = aNotExist.Verify(spec, root, []byte("a"))
+	expError = fmt.Errorf("right proof, %w", expError)
+	if err.Error() != expError.Error() {
+		t.Fatal("attempting to prove non-existence of leaf a returned incorrect error")
+	}
+}
+
 // generatePrefix generates a valid iavl prefix for an inner op.
 func generateInnerOpPrefix() []byte {
 	var (

diff --git a/go/proofs.pb.go b/go/proofs.pb.go
diff --git a/proto/cosmos/ics23/v1/proofs.proto b/proto/cosmos/ics23/v1/proofs.proto
@@ -190,7 +190,8 @@ message InnerSpec {
   repeated int32 child_order       = 1;
   int32          child_size        = 2;
   int32          min_prefix_length = 3;
-  int32          max_prefix_length = 4;
+  // the max prefix length must be less than the minimum prefix length + child size
+  int32 max_prefix_length = 4;
   // empty child is the prehash image that is used when one child is nil (eg. 20 bytes of 0)
   bytes empty_child = 5;
   // hash is the algorithm that must be used for each InnerOp

diff --git a/rust/CHANGELOG.md b/rust/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Changelog
 
+# Unreleased
+
+- fix: guarantee that `spec.InnerSpec.MaxPrefixLength` < `spec.InnerSpec.MinPrefixLength` + `spec.InnerSpec.ChildSize` ([#369](https://github.com/cosmos/ics23/pull/369))
+
 # v0.12.0
 
 - chore(rust): Update `prost` to v0.13 ([#335](https://github.com/cosmos/ics23/pull/335), [#336](https://github.com/cosmos/ics23/pull/336))

diff --git a/rust/src/cosmos.ics23.v1.rs b/rust/src/cosmos.ics23.v1.rs
@@ -180,6 +180,7 @@ pub struct InnerSpec {
     pub child_size: i32,
     #[prost(int32, tag = "3")]
     pub min_prefix_length: i32,
+    /// the max prefix length must be less than the minimum prefix length + child size
     #[prost(int32, tag = "4")]
     pub max_prefix_length: i32,
     /// empty child is the prehash image that is used when one child is nil (eg. 20 bytes of 0)

diff --git a/rust/src/proto_descriptor.bin b/rust/src/proto_descriptor.bin
diff --git a/rust/src/verify.rs b/rust/src/verify.rs
@@ -203,7 +203,11 @@ fn ensure_inner(inner: &ics23::InnerOp, spec: &ics23::ProofSpec) -> Result<()> {
             );
             ensure!(
                 inner_spec.child_size > 0,
-                "spec.InnerSpec.ChildSize must be >= 1"
+                "spec.inner_spec.child_size must be >= 1"
+            );
+            ensure!(
+                inner_spec.max_prefix_length < inner_spec.min_prefix_length + inner_spec.child_size,
+                "spec.inner_spec.max_prefix_length must be < spec.inner_spec.min_prefix_length + spec.inner_spec.child_size"
             );
             ensure!(
                 inner.suffix.len() % (inner_spec.child_size as usize) == 0,
@@ -484,6 +488,13 @@ mod tests {
         depth_limited_spec.min_depth = 2;
         depth_limited_spec.max_depth = 4;
 
+        let mut max_prefix_length_too_large_spec = api::iavl_spec();
+        let inner_spec = max_prefix_length_too_large_spec
+            .inner_spec
+            .as_mut()
+            .unwrap();
+        inner_spec.max_prefix_length = 100;
+
         let cases: HashMap<&'static str, ExistenceCase> = [
             (
                 "empty proof fails",
@@ -612,19 +623,32 @@ mod tests {
                     proof: ExistenceProof {
                         key: b"foo".to_vec(),
                         value: b"bar".to_vec(),
-                        leaf: Some(leaf),
+                        leaf: Some(leaf.clone()),
                         path: vec![
                             valid_inner.clone(),
                             valid_inner.clone(),
                             valid_inner.clone(),
                             valid_inner.clone(),
-                            valid_inner,
+                            valid_inner.clone(),
                         ],
                     },
                     spec: depth_limited_spec,
                     valid: false,
                 },
             ),
+            (
+                "rejects inner spec with max prefix length >= min prefix length + child size",
+                ExistenceCase {
+                    proof: ExistenceProof {
+                        key: b"foo".to_vec(),
+                        value: b"bar".to_vec(),
+                        leaf: Some(leaf),
+                        path: vec![valid_inner],
+                    },
+                    spec: max_prefix_length_too_large_spec,
+                    valid: false,
+                },
+            ),
         ]
         .into_iter()
         .collect();