Fix SignersInfo packing issue (#1301)

* Address SignersInfo packing issue

* Update changelog

* Add test for unpacking zero value

* Move packing fuzz tests to a separate module

* Move changelog entry

CHANGELOG.md

@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fixed message type hash in SNIP12 doc (#1274)
 - Permit and Message SNIP12 hashes (#1283)
 - Fix Multisig component issue arising when removing signers with unchanged quorum (#1300)
+- Fix minor severity issue of `SignersInfo` packing impl in Multisig component (#1301)
 
 ## 0.20.0 (2024-12-06)
 

packages/governance/Scarb.toml

@@ -35,6 +35,9 @@ snforge_std.workspace = true
 openzeppelin_testing = { path = "../testing" }
 openzeppelin_test_common = { path = "../test_common" }
 
+[features]
+fuzzing = []
+
 [lib]
 
 [[target.starknet-contract]]

packages/governance/src/multisig/multisig.cairo

@@ -20,7 +20,7 @@ pub mod MultisigComponent {
     use core::panic_with_felt252;
     use core::pedersen::PedersenTrait;
     use crate::multisig::interface::{IMultisig, TransactionID, TransactionState};
-    use crate::multisig::storage_utils::{SignersInfo, SignersInfoStorePacking};
+    use crate::multisig::storage_utils::{SignersInfo, SignersInfoStorePackingV2};
     use crate::multisig::storage_utils::{TxInfo, TxInfoStorePacking};
     use crate::utils::call_impls::{CallPartialEq, HashCallImpl, HashCallsImpl};
     use starknet::account::Call;

packages/governance/src/multisig/storage_utils.cairo

@@ -4,8 +4,6 @@
 use core::integer::u128_safe_divmod;
 use starknet::storage_access::StorePacking;
 
-const _2_POW_32: NonZero<u128> = 0xffffffff;
-
 /// Helper struct for `MultisigComponent` that optimizes how transaction-related information
 /// is stored, including the transaction's execution status and the block it was submitted in.
 #[derive(Drop)]
@@ -45,19 +43,39 @@ pub struct SignersInfo {
     pub signers_count: u32,
 }
 
+const _2_POW_32: NonZero<u128> = 0x100000000;
+const MAX_U32: NonZero<u128> = 0xffffffff;
+const V2_SIGNAL_BIT: u128 = 0x10000000000000000;
+
 /// Packs a `SignersInfo` entity into a `u128` value.
 ///
 /// The packing is done as follows:
 /// - `quorum` value occupies 32 bits in bit range [64..95].
 /// - `signers_count` value occupies the highest 32 bits in bit range [96..127].
-pub impl SignersInfoStorePacking of StorePacking<SignersInfo, u128> {
+/// - The 63rd bit indicates the packing version: 0 for V1 (legacy) and 1 for V2 (current).
+///
+/// WARNING: A bug in the original `StorePacking` trait for `SignersInfo` multiplied the quorum
+/// value by 0xffffffff instead of 0x100000000. This affected only the case where `signers_count`
+/// was 0xffffffff, resulting in an off-by-one quorum value and a `signers_count` of 0 after
+/// unpacking. The issue is resolved in V2 of `StorePacking`. For backward compatibility, V2 can
+/// still correctly unpack values from both V1 and V2 by checking the 63rd bit (0 for V1, 1 for V2).
+pub impl SignersInfoStorePackingV2 of StorePacking<SignersInfo, u128> {
     fn pack(value: SignersInfo) -> u128 {
         let SignersInfo { quorum, signers_count } = value;
-        quorum.into() * _2_POW_32.into() + signers_count.into()
+        let quorum_val = quorum.into() * _2_POW_32.into();
+        V2_SIGNAL_BIT + quorum_val + signers_count.into()
     }
 
     fn unpack(value: u128) -> SignersInfo {
-        let (quorum, signers_count) = u128_safe_divmod(value, _2_POW_32);
+        if value == 0 {
+            return SignersInfo { quorum: 0, signers_count: 0 };
+        }
+        let is_packed_with_v1 = value < V2_SIGNAL_BIT;
+        let (quorum, signers_count) = if is_packed_with_v1 {
+            u128_safe_divmod(value, MAX_U32)
+        } else {
+            u128_safe_divmod(value - V2_SIGNAL_BIT, _2_POW_32)
+        };
         SignersInfo {
             quorum: quorum.try_into().unwrap(), signers_count: signers_count.try_into().unwrap(),
         }

packages/governance/src/tests.cairo

@@ -1,4 +1,6 @@
 mod governor;
+#[cfg(feature: 'fuzzing')]
+mod test_fuzz_packing;
 mod test_multisig;
 mod test_timelock;
 mod test_utils;

packages/governance/src/tests/test_fuzz_packing.cairo

@@ -0,0 +1,48 @@
+use core::integer::u128_safe_divmod;
+use core::num::traits::Bounded;
+use crate::multisig::storage_utils::{SignersInfo, SignersInfoStorePackingV2};
+use starknet::storage_access::StorePacking;
+
+#[test]
+fn test_signers_info_pack_unpack_v2(quorum: u32, signers_count: u32) {
+    let info = SignersInfo { quorum, signers_count };
+    let packed_value = SignersInfoStorePackingV2::pack(info);
+    let unpacked_info = SignersInfoStorePackingV2::unpack(packed_value);
+
+    assert_eq!(unpacked_info.quorum, quorum);
+    assert_eq!(unpacked_info.signers_count, signers_count);
+}
+
+#[test]
+fn test_signers_info_pack_with_v1_unpack_with_v2(quorum: u32, signers_count: u32) {
+    if signers_count == Bounded::MAX {
+        // Cannot properly unpack if packed with V1 and `signers_count` is max u32 value
+        return;
+    };
+    let info = SignersInfo { quorum, signers_count };
+    let packed_value = LegacySignersInfoStorePackingV1::pack(info);
+    let unpacked_info = SignersInfoStorePackingV2::unpack(packed_value);
+
+    assert_eq!(unpacked_info.quorum, quorum);
+    assert_eq!(unpacked_info.signers_count, signers_count);
+}
+
+//
+// Helpers
+//
+
+const MAX_U32: NonZero<u128> = 0xffffffff;
+
+impl LegacySignersInfoStorePackingV1 of StorePacking<SignersInfo, u128> {
+    fn pack(value: SignersInfo) -> u128 {
+        let SignersInfo { quorum, signers_count } = value;
+        quorum.into() * MAX_U32.into() + signers_count.into()
+    }
+
+    fn unpack(value: u128) -> SignersInfo {
+        let (quorum, signers_count) = u128_safe_divmod(value, MAX_U32);
+        SignersInfo {
+            quorum: quorum.try_into().unwrap(), signers_count: signers_count.try_into().unwrap(),
+        }
+    }
+}

packages/governance/src/tests/test_multisig.cairo

@@ -1,8 +1,10 @@
-use core::num::traits::Zero;
+use core::integer::u128_safe_divmod;
+use core::num::traits::{Bounded, Zero};
 use crate::multisig::MultisigComponent::{CallSalt, QuorumUpdated, SignerAdded, SignerRemoved};
 use crate::multisig::MultisigComponent::{ConfirmationRevoked, TransactionExecuted};
 use crate::multisig::MultisigComponent::{Event, InternalImpl, MultisigImpl};
 use crate::multisig::MultisigComponent::{TransactionConfirmed, TransactionSubmitted};
+use crate::multisig::storage_utils::{SignersInfo, SignersInfoStorePackingV2};
 use crate::multisig::{MultisigComponent, TransactionID, TransactionState};
 use openzeppelin_test_common::mocks::multisig::IMultisigTargetMockDispatcherTrait;
 use openzeppelin_test_common::mocks::multisig::{IMultisigTargetMockDispatcher, MultisigWalletMock};
@@ -12,6 +14,7 @@ use openzeppelin_testing::events::EventSpyExt;
 use snforge_std::{EventSpy, spy_events, test_address};
 use snforge_std::{start_cheat_block_number_global, start_cheat_caller_address};
 use starknet::account::Call;
+use starknet::storage_access::StorePacking;
 use starknet::{ContractAddress, contract_address_const};
 
 //
@@ -1405,6 +1408,51 @@ fn test_cannot_change_quorum_when_not_multisig_itself() {
     state.change_quorum(0);
 }
 
+#[test]
+fn test_signers_info_error_happens_with_v1() {
+    let quorum = 123;
+    let signers_count = Bounded::MAX;
+    let info = SignersInfo { quorum, signers_count };
+    let packed_value = LegacySignersInfoStorePackingV1::pack(info);
+    let unpacked_info = LegacySignersInfoStorePackingV1::unpack(packed_value);
+
+    assert_eq!(unpacked_info.quorum, quorum + 1);
+    assert_eq!(unpacked_info.signers_count, 0);
+}
+
+#[test]
+fn test_signers_info_no_error_happen_with_v2() {
+    let quorum = 123;
+    let signers_count = Bounded::MAX;
+    let info = SignersInfo { quorum, signers_count };
+    let packed_value = SignersInfoStorePackingV2::pack(info);
+    let unpacked_info = SignersInfoStorePackingV2::unpack(packed_value);
+
+    assert_eq!(unpacked_info.quorum, quorum);
+    assert_eq!(unpacked_info.signers_count, signers_count);
+}
+
+#[test]
+fn test_signers_info_pack_unpack_v2_max_values() {
+    let quorum = Bounded::MAX;
+    let signers_count = Bounded::MAX;
+    let info = SignersInfo { quorum, signers_count };
+    let packed_value = SignersInfoStorePackingV2::pack(info);
+    let unpacked_info = SignersInfoStorePackingV2::unpack(packed_value);
+
+    assert_eq!(unpacked_info.quorum, quorum);
+    assert_eq!(unpacked_info.signers_count, signers_count);
+}
+
+#[test]
+fn test_signers_info_unpack_zero_value_v2() {
+    let packed_value = 0;
+    let unpacked_info = SignersInfoStorePackingV2::unpack(packed_value);
+
+    assert_eq!(unpacked_info.quorum, 0);
+    assert_eq!(unpacked_info.signers_count, 0);
+}
+
 //
 // Helpers
 //
@@ -1471,6 +1519,22 @@ fn assert_signers_list(expected_signers: Span<ContractAddress>) {
     };
 }
 
+const MAX_U32: NonZero<u128> = 0xffffffff;
+
+impl LegacySignersInfoStorePackingV1 of StorePacking<SignersInfo, u128> {
+    fn pack(value: SignersInfo) -> u128 {
+        let SignersInfo { quorum, signers_count } = value;
+        quorum.into() * MAX_U32.into() + signers_count.into()
+    }
+
+    fn unpack(value: u128) -> SignersInfo {
+        let (quorum, signers_count) = u128_safe_divmod(value, MAX_U32);
+        SignersInfo {
+            quorum: quorum.try_into().unwrap(), signers_count: signers_count.try_into().unwrap(),
+        }
+    }
+}
+
 //
 // Events
 //