Fix audit decompressor security miscellaneous comments (#1402)

* Update arkworks-rs dependencies

* Extend get_collection_id comment

* Distinguish collection id generation error

* Use immutable transformations

* fmt

* Update copyrights

* Use retrieval instead of generation for collection id error

* Fix misprint

* fix clippy

Cargo.toml

@@ -276,8 +276,8 @@ url = "2.5.0"
 
 # Other (wasm)
 arbitrary = { version = "1.3.2", default-features = false }
-ark-bn254 = { version = "0.4.0", default-features = false, features = ["curve"] }
-ark-ff = { version = "0.4.0", default-features = false }
+ark-bn254 = { version = "0.5.0", default-features = false, features = ["curve"] }
+ark-ff = { version = "0.5.0", default-features = false }
 arrayvec = { version = "0.7.4", default-features = false }
 cfg-if = { version = "1.0.0" }
 fixed = { version = "=1.15.0", default-features = false, features = ["num-traits"] }

zrml/combinatorial-tokens/src/lib.rs

@@ -1,4 +1,4 @@
-// Copyright 2024 Forecasting Technologies LTD.
+// Copyright 2025 Forecasting Technologies LTD.
 //
 // This file is part of Zeitgeist.
 //
@@ -39,7 +39,9 @@ pub use pallet::*;
 #[frame_support::pallet]
 mod pallet {
     use crate::{
-        traits::CombinatorialIdManager, types::TransmutationType, weights::WeightInfoZeitgeist,
+        traits::CombinatorialIdManager,
+        types::{CollectionIdError, TransmutationType},
+        weights::WeightInfoZeitgeist,
     };
     use alloc::{vec, vec::Vec};
     use core::{fmt::Debug, marker::PhantomData};
@@ -186,16 +188,16 @@ mod pallet {
 
     #[pallet::error]
     pub enum Error<T> {
+        /// An error for the collection ID retrieval occured.
+        CollectionIdRetrievalFailed(CollectionIdError),
+
         /// Specified index set is trival, empty, or doesn't match the market's number of outcomes.
         InvalidIndexSet,
 
         /// Specified partition is empty, contains overlaps, is too long or doesn't match the
         /// market's number of outcomes.
         InvalidPartition,
 
-        /// Specified collection ID is invalid.
-        InvalidCollectionId,
-
         /// Specified market is not resolved.
         PayoutVectorNotFound,
 
@@ -643,7 +645,9 @@ mod pallet {
                 index_set,
                 fuel,
             )
-            .ok_or(Error::<T>::InvalidCollectionId.into())
+            .map_err(|collection_id_error| {
+                Error::<T>::CollectionIdRetrievalFailed(collection_id_error).into()
+            })
         }
 
         pub(crate) fn position_from_collection_id(

zrml/combinatorial-tokens/src/traits/combinatorial_id_manager.rs

@@ -1,4 +1,4 @@
-// Copyright 2024 Forecasting Technologies LTD.
+// Copyright 2025 Forecasting Technologies LTD.
 //
 // This file is part of Zeitgeist.
 //
@@ -22,6 +22,7 @@
 // <https://github.com/gnosis/conditional-tokens-contracts>,
 // and has been relicensed under GPL-3.0-or-later in this repository.
 
+use crate::types::CollectionIdError;
 use alloc::vec::Vec;
 
 pub trait CombinatorialIdManager {
@@ -40,7 +41,7 @@ pub trait CombinatorialIdManager {
         market_id: Self::MarketId,
         index_set: Vec<bool>,
         fuel: Self::Fuel,
-    ) -> Option<Self::CombinatorialId>;
+    ) -> Result<Self::CombinatorialId, CollectionIdError>;
 
     fn get_position_id(
         collateral: Self::Asset,

zrml/combinatorial-tokens/src/types/collection_id_error.rs

@@ -0,0 +1,35 @@
+// Copyright 2025 Forecasting Technologies LTD.
+//
+// This file is part of Zeitgeist.
+//
+// Zeitgeist is free software: you can redistribute it and/or modify it
+// under the terms of the GNU General Public License as published by the
+// Free Software Foundation, either version 3 of the License, or (at
+// your option) any later version.
+//
+// Zeitgeist is distributed in the hope that it will be useful, but
+// WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+// General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Zeitgeist. If not, see <https://www.gnu.org/licenses/>.
+//
+// This file incorporates work licensed under the GNU Lesser General
+// Public License 3.0 but published without copyright notice by Gnosis
+// (<https://gnosis.io>, [email protected]) in the
+// conditional-tokens-contracts repository
+// <https://github.com/gnosis/conditional-tokens-contracts>,
+// and has been relicensed under GPL-3.0-or-later in this repository.
+
+use frame_support::PalletError;
+use parity_scale_codec::{Decode, Encode};
+use scale_info::TypeInfo;
+use sp_runtime::RuntimeDebug;
+
+#[derive(Decode, Encode, Eq, PartialEq, PalletError, RuntimeDebug, TypeInfo)]
+pub enum CollectionIdError {
+    InvalidParentCollectionId,
+    EllipticCurvePointNotFoundWithFuel,
+    EllipticCurvePointXToBytesConversionFailed,
+}

zrml/combinatorial-tokens/src/types/cryptographic_id_manager/decompressor/mod.rs

@@ -1,4 +1,4 @@
-// Copyright 2024 Forecasting Technologies LTD.
+// Copyright 2025 Forecasting Technologies LTD.
 //
 // This file is part of Zeitgeist.
 //
@@ -26,19 +26,23 @@
 
 mod tests;
 
-use crate::types::cryptographic_id_manager::Fuel;
+use crate::types::{cryptographic_id_manager::Fuel, CollectionIdError};
 use ark_bn254::{g1::G1Affine, Fq};
 use ark_ff::{BigInteger, PrimeField};
 use core::ops::Neg;
 use sp_runtime::traits::{One, Zero};
 use zeitgeist_primitives::{traits::CombinatorialTokensFuel, types::CombinatorialId};
 
-/// Will return `None` if and only if `parent_collection_id` is not a valid collection ID.
+/// Returns a valid collection ID from an `hash` and an optional `parent_collection_id`.
+///
+/// Will return `None` if `parent_collection_id` is not a valid collection ID or
+/// the decompression of the hash doesn't return a valid point of `alt_bn128`
+/// (maybe insufficient `fuel` parameter) or because of a failing bytes conversion.
 pub(crate) fn get_collection_id(
     hash: CombinatorialId,
     parent_collection_id: Option<CombinatorialId>,
     fuel: Fuel,
-) -> Option<CombinatorialId> {
+) -> Result<CombinatorialId, CollectionIdError> {
     let mut u = decompress_hash(hash, fuel)?;
 
     if let Some(pci) = parent_collection_id {
@@ -49,13 +53,19 @@ pub(crate) fn get_collection_id(
 
     // Convert back to bytes _before_ flipping, as flipping will sometimes result in numbers larger
     // than the base field modulus.
-    let mut bytes: CombinatorialId = u.x.into_bigint().to_bytes_be().try_into().ok()?;
+    let bytes_y_even: CombinatorialId =
+        u.x.into_bigint()
+            .to_bytes_be()
+            .try_into()
+            .map_err(|_| CollectionIdError::EllipticCurvePointXToBytesConversionFailed)?;
 
-    if u.y.into_bigint().is_odd() {
-        flip_second_highest_bit(&mut bytes);
-    }
+    let bytes = if u.y.into_bigint().is_odd() {
+        flip_second_highest_bit(&bytes_y_even)
+    } else {
+        bytes_y_even
+    };
 
-    Some(bytes)
+    Ok(bytes)
 }
 
 /// Decompresses a collection ID `hash` to a point of `alt_bn128`. The amount of work done can be
@@ -66,7 +76,7 @@ pub(crate) fn get_collection_id(
 /// evidence that input hash requires more than `log_2(P) = 507.19338271000436` iterations. With a
 /// `fuel.total` value of `32`, statistical evidence suggests a 1 in 500_000_000 chance that the
 /// number of iterations will not be enough.
-fn decompress_hash(hash: CombinatorialId, fuel: Fuel) -> Option<G1Affine> {
+fn decompress_hash(hash: CombinatorialId, fuel: Fuel) -> Result<G1Affine, CollectionIdError> {
     // Calculate `odd` first, then get congruent point `x` in `Fq`. As `hash` might represent a
     // larger big endian number than `field_modulus()`, the MSB of `x` might be different from the
     // MSB of `x_u256`.
@@ -103,44 +113,49 @@ fn decompress_hash(hash: CombinatorialId, fuel: Fuel) -> Option<G1Affine> {
             }
         }
     }
-    core::hint::black_box(dummy_x); // Ensure that the dummies are considered "read" by rustc.
+    // Ensure that the dummies are considered "read" by rustc.
+    core::hint::black_box(dummy_x);
     core::hint::black_box(dummy_y);
-    let mut y = y_opt?; // This **should** be infallible if `DECOMPRESS_HASH_MAX_ITERS` is large.
+    // This **should** be infallible if `fuel.total()` is large.
+    let mut y = y_opt.ok_or(CollectionIdError::EllipticCurvePointNotFoundWithFuel)?;
 
     // We have two options for the y-coordinate of the corresponding point: `y` and `P - y`. If
     // `odd` is set but `y` isn't odd, we switch to the other option.
     if (odd && y.into_bigint().is_even()) || (!odd && y.into_bigint().is_odd()) {
         y = y.neg();
     }
 
-    Some(G1Affine::new(x, y))
+    Ok(G1Affine::new(x, y))
 }
 
-fn decompress_collection_id(mut collection_id: CombinatorialId) -> Option<G1Affine> {
+fn decompress_collection_id(collection_id: CombinatorialId) -> Result<G1Affine, CollectionIdError> {
     let odd = is_second_msb_set(&collection_id);
-    chop_off_two_highest_bits(&mut collection_id);
-    let x = Fq::from_be_bytes_mod_order(&collection_id);
+    let chopped_collection_id = chop_off_two_highest_bits(&collection_id);
+    let x = Fq::from_be_bytes_mod_order(&chopped_collection_id);
 
     // Ensure that the big-endian integer represented by `collection_id` was less than the field
     // modulus. Otherwise, we consider `collection_id` an invalid ID.
-    if x.into_bigint().to_bytes_be() != collection_id {
-        return None;
+    if x.into_bigint().to_bytes_be() != chopped_collection_id {
+        return Err(CollectionIdError::InvalidParentCollectionId);
     }
 
-    let mut y = matching_y_coordinate(x)?; // Fails if `collection_id` is not a collection ID.
+    // Fails if `collection_id` is not a collection ID.
+    let mut y = matching_y_coordinate(x).ok_or(CollectionIdError::InvalidParentCollectionId)?;
 
     // We have two options for the y-coordinate of the corresponding point: `y` and `P - y`. If
     // `odd` is set but `y` isn't odd, we switch to the other option.
     if (odd && y.into_bigint().is_even()) || (!odd && y.into_bigint().is_odd()) {
         y = y.neg();
     }
 
-    Some(G1Affine::new(x, y))
+    Ok(G1Affine::new(x, y))
 }
 
-/// Flips the second highests bit of big-endian `bytes`.
-fn flip_second_highest_bit(bytes: &mut CombinatorialId) {
+/// Flips the second highest bit of big-endian `bytes`.
+fn flip_second_highest_bit(bytes: &CombinatorialId) -> CombinatorialId {
+    let mut bytes = *bytes;
     bytes[0] ^= 0b01000000;
+    bytes
 }
 
 /// Checks if the most significant bit of the big-endian `bytes` is set.
@@ -154,8 +169,10 @@ fn is_second_msb_set(bytes: &CombinatorialId) -> bool {
 }
 
 /// Zeroes out the two most significant bits off the big-endian `bytes`.
-fn chop_off_two_highest_bits(bytes: &mut CombinatorialId) {
+fn chop_off_two_highest_bits(bytes: &CombinatorialId) -> CombinatorialId {
+    let mut bytes = *bytes;
     bytes[0] &= 0b00111111;
+    bytes
 }
 
 /// Returns a value `y` of `Fq` so that `(x, y)` is a point on `alt_bn128` or `None` if there is no

zrml/combinatorial-tokens/src/types/cryptographic_id_manager/decompressor/tests/decompress_collection_id.rs

@@ -1,4 +1,4 @@
-// Copyright 2024 Forecasting Technologies LTD.
+// Copyright 2025 Forecasting Technologies LTD.
 //
 // This file is part of Zeitgeist.
 //
@@ -574,5 +574,5 @@ fn decompress_collection_id_works(collection_id: CombinatorialId, expected: (&st
 )]
 fn decompress_collection_id_fails_on_invalid_collection_id(collection_id: CombinatorialId) {
     let actual = decompress_collection_id(collection_id);
-    assert_eq!(actual, None);
+    assert_eq!(actual, Err(CollectionIdError::InvalidParentCollectionId));
 }

zrml/combinatorial-tokens/src/types/cryptographic_id_manager/decompressor/tests/get_collection_id.rs

@@ -1,4 +1,4 @@
-// Copyright 2024 Forecasting Technologies LTD.
+// Copyright 2025 Forecasting Technologies LTD.
 //
 // This file is part of Zeitgeist.
 //
@@ -86,6 +86,7 @@ fn get_collection_id_works(
     #[values(false, true)] consume_all: bool,
     #[case] expected: Option<CombinatorialId>,
 ) {
-    let actual = get_collection_id(hash, parent_collection_id, Fuel { total: 16, consume_all });
+    let actual =
+        get_collection_id(hash, parent_collection_id, Fuel { total: 16, consume_all }).ok();
     assert_eq!(actual, expected);
 }

zrml/combinatorial-tokens/src/types/cryptographic_id_manager/mod.rs

@@ -1,4 +1,4 @@
-// Copyright 2024 Forecasting Technologies LTD.
+// Copyright 2025 Forecasting Technologies LTD.
 //
 // This file is part of Zeitgeist.
 //
@@ -25,6 +25,7 @@
 mod decompressor;
 mod hash_tuple;
 
+use super::CollectionIdError;
 use crate::traits::CombinatorialIdManager;
 use alloc::vec::Vec;
 use core::marker::PhantomData;
@@ -83,7 +84,7 @@ where
         market_id: Self::MarketId,
         index_set: Vec<bool>,
         fuel: Self::Fuel,
-    ) -> Option<Self::CombinatorialId> {
+    ) -> Result<Self::CombinatorialId, CollectionIdError> {
         let input = (market_id, index_set);
         let hash = Hasher::hash_tuple(input);
 

zrml/combinatorial-tokens/src/types/mod.rs

@@ -1,4 +1,4 @@
-// Copyright 2024 Forecasting Technologies LTD.
+// Copyright 2025 Forecasting Technologies LTD.
 //
 // This file is part of Zeitgeist.
 //
@@ -15,10 +15,12 @@
 // You should have received a copy of the GNU General Public License
 // along with Zeitgeist. If not, see <https://www.gnu.org/licenses/>.
 
+mod collection_id_error;
 pub(crate) mod cryptographic_id_manager;
 pub(crate) mod hash;
 mod transmutation_type;
 
+pub use collection_id_error::CollectionIdError;
 pub use cryptographic_id_manager::{CryptographicIdManager, Fuel};
 pub(crate) use hash::Hash256;
 pub use transmutation_type::TransmutationType;