feat: prevent zero address for author/voter (#492)

* revert if autauthor/voter is zero address * chore: test propose with zero starknet and ethereum address * chore: test propose with zero custom address * chore: UserAddress zeroable tests * chore: zero address untegration tests vote and update proposal * chore: updated scarb version to 0.6.2 in CI * chore: attempt fix CI * chore: formatting * chore: fix CI 2 * chore: formatting * chore: fixed space test * chore: add code coverage for zero/non_zero --------- Co-authored-by: Orlando <[email protected]> Co-authored-by: Scott Piriou <[email protected]>
snapshot-labs · Aug 23, 2023 · 6f02445 · 6f02445
1 parent d0c2ea8
commit 6f02445
Show file tree

Hide file tree

Showing 4 changed files with 232 additions and 1 deletion.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -54,7 +54,7 @@ jobs:
       - name: Step 2 - Install Scarb 
         uses: software-mansion/setup-scarb@v1
         with: 
-          scarb-version: 0.6.0-alpha.1
+          scarb-version: 0.6.2
 
       - name: Step 3 - Check formatting
         working-directory: ./starknet

diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo
@@ -257,6 +257,7 @@ mod Space {
             metadata_URI: Array<felt252>,
         ) {
             assert_only_authenticator(@self);
+            assert(author.is_non_zero(), 'Zero Address');
             let proposal_id = self._next_proposal_id.read();
 
             // Proposal Validation
@@ -312,6 +313,7 @@ mod Space {
             metadata_URI: Array<felt252>
         ) {
             assert_only_authenticator(@self);
+            assert(voter.is_non_zero(), 'Zero Address');
             let proposal = self._proposals.read(proposal_id);
             assert_proposal_exists(@proposal);
 
@@ -377,6 +379,7 @@ mod Space {
             metadata_URI: Array<felt252>,
         ) {
             assert_only_authenticator(@self);
+            assert(author.is_non_zero(), 'Zero Address');
             let mut proposal = self._proposals.read(proposal_id);
             assert_proposal_exists(@proposal);
             assert(proposal.author == author, 'Only Author');

diff --git a/starknet/src/tests/test_space.cairo b/starknet/src/tests/test_space.cairo
@@ -405,4 +405,168 @@ mod tests {
         ArrayTrait::<felt252>::new().serialize(ref vote_calldata);
         authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata);
     }
+
+    #[test]
+    #[available_gas(10000000000)]
+    #[should_panic(expected: ('Zero Address', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED'))]
+    fn test_propose_zero_address() {
+        let config = setup();
+        let (factory, space) = deploy(@config);
+
+        let authenticator = IVanillaAuthenticatorDispatcher {
+            contract_address: *config.authenticators.at(0), 
+        };
+
+        let quorum = u256_from_felt252(1);
+        let mut constructor_calldata = ArrayTrait::<felt252>::new();
+        quorum.serialize(ref constructor_calldata);
+
+        let (vanilla_execution_strategy_address, _) = deploy_syscall(
+            VanillaExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(),
+            0,
+            constructor_calldata.span(),
+            false
+        )
+            .unwrap();
+        let vanilla_execution_strategy = StrategyImpl::from_address(
+            vanilla_execution_strategy_address
+        );
+        // author is the zero address
+        let author = UserAddress::Starknet(contract_address_const::<0x0>());
+        let mut propose_calldata = array::ArrayTrait::<felt252>::new();
+        author.serialize(ref propose_calldata);
+        vanilla_execution_strategy.serialize(ref propose_calldata);
+        ArrayTrait::<felt252>::new().serialize(ref propose_calldata);
+        ArrayTrait::<felt252>::new().serialize(ref propose_calldata);
+
+        // Create Proposal
+        authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata);
+    }
+
+    #[test]
+    #[available_gas(10000000000)]
+    #[should_panic(expected: ('Zero Address', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED'))]
+    fn test_update_zero_address() {
+        let config = setup();
+        let (factory, space) = deploy(@config);
+
+        let authenticator = IVanillaAuthenticatorDispatcher {
+            contract_address: *config.authenticators.at(0), 
+        };
+
+        let quorum = u256_from_felt252(1);
+        let mut constructor_calldata = ArrayTrait::<felt252>::new();
+        quorum.serialize(ref constructor_calldata);
+
+        let (vanilla_execution_strategy_address, _) = deploy_syscall(
+            VanillaExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(),
+            0,
+            constructor_calldata.span(),
+            false
+        )
+            .unwrap();
+        let vanilla_execution_strategy = StrategyImpl::from_address(
+            vanilla_execution_strategy_address
+        );
+        // author is the zero address
+        let author = UserAddress::Starknet(contract_address_const::<0x0>());
+        let mut propose_calldata = array::ArrayTrait::<felt252>::new();
+        author.serialize(ref propose_calldata);
+        vanilla_execution_strategy.serialize(ref propose_calldata);
+        ArrayTrait::<felt252>::new().serialize(ref propose_calldata);
+        ArrayTrait::<felt252>::new().serialize(ref propose_calldata);
+
+        // Create Proposal
+        authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata);
+
+        // Update Proposal
+        let mut update_calldata = array::ArrayTrait::<felt252>::new();
+        author.serialize(ref update_calldata);
+        let proposal_id = u256_from_felt252(1);
+        proposal_id.serialize(ref update_calldata);
+        // Keeping the same execution strategy contract but changing the payload
+        let mut new_payload = ArrayTrait::<felt252>::new();
+        new_payload.append(1);
+        let execution_strategy = Strategy {
+            address: vanilla_execution_strategy.address, params: new_payload
+        };
+        execution_strategy.serialize(ref update_calldata);
+        ArrayTrait::<felt252>::new().serialize(ref update_calldata);
+
+        authenticator
+            .authenticate(space.contract_address, UPDATE_PROPOSAL_SELECTOR, update_calldata);
+    }
+
+    #[test]
+    #[available_gas(10000000000)]
+    #[should_panic(expected: ('Zero Address', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED'))]
+    fn test_vote_zero_address() {
+        let config = setup();
+        let (factory, space) = deploy(@config);
+
+        let authenticator = IVanillaAuthenticatorDispatcher {
+            contract_address: *config.authenticators.at(0), 
+        };
+
+        let quorum = u256_from_felt252(1);
+        let mut constructor_calldata = ArrayTrait::<felt252>::new();
+        quorum.serialize(ref constructor_calldata);
+
+        let (vanilla_execution_strategy_address, _) = deploy_syscall(
+            VanillaExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(),
+            0,
+            constructor_calldata.span(),
+            false
+        )
+            .unwrap();
+        let vanilla_execution_strategy = StrategyImpl::from_address(
+            vanilla_execution_strategy_address
+        );
+        let author = UserAddress::Starknet(contract_address_const::<0x5678>());
+        let mut propose_calldata = array::ArrayTrait::<felt252>::new();
+        author.serialize(ref propose_calldata);
+        vanilla_execution_strategy.serialize(ref propose_calldata);
+        ArrayTrait::<felt252>::new().serialize(ref propose_calldata);
+        ArrayTrait::<felt252>::new().serialize(ref propose_calldata);
+
+        // Create Proposal
+        authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata);
+
+        // Update Proposal
+        let mut update_calldata = array::ArrayTrait::<felt252>::new();
+        author.serialize(ref update_calldata);
+        let proposal_id = u256_from_felt252(1);
+        proposal_id.serialize(ref update_calldata);
+        // Keeping the same execution strategy contract but changing the payload
+        let mut new_payload = ArrayTrait::<felt252>::new();
+        new_payload.append(1);
+        let execution_strategy = Strategy {
+            address: vanilla_execution_strategy.address, params: new_payload
+        };
+        execution_strategy.serialize(ref update_calldata);
+        ArrayTrait::<felt252>::new().serialize(ref update_calldata);
+
+        authenticator
+            .authenticate(space.contract_address, UPDATE_PROPOSAL_SELECTOR, update_calldata);
+
+        // Increasing block block_number by 1 to pass voting delay
+        testing::set_block_number(1_u64);
+
+        let mut vote_calldata = array::ArrayTrait::<felt252>::new();
+        // Voter is the zero address
+        let voter = UserAddress::Starknet(contract_address_const::<0x0>());
+        voter.serialize(ref vote_calldata);
+        let proposal_id = u256_from_felt252(1);
+        proposal_id.serialize(ref vote_calldata);
+        let choice = Choice::For(());
+        choice.serialize(ref vote_calldata);
+        let mut user_voting_strategies = ArrayTrait::<IndexedStrategy>::new();
+        user_voting_strategies
+            .append(IndexedStrategy { index: 0_u8, params: ArrayTrait::<felt252>::new() });
+        user_voting_strategies.serialize(ref vote_calldata);
+        ArrayTrait::<felt252>::new().serialize(ref vote_calldata);
+
+        // Vote on Proposal
+        authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata);
+    }
 }
diff --git a/starknet/src/types/user_address.cairo b/starknet/src/types/user_address.cairo
@@ -1,5 +1,6 @@
 use starknet::{ContractAddress, EthAddress};
 use traits::{PartialEq, TryInto, Into};
+use zeroable::Zeroable;
 use serde::Serde;
 use array::ArrayTrait;
 use sx::utils::legacy_hash::LegacyHashUserAddress;
@@ -57,3 +58,66 @@ impl UserAddressImpl of UserAddressTrait {
         }
     }
 }
+
+impl UserAddressZeroable of Zeroable<UserAddress> {
+    fn zero() -> UserAddress {
+        panic_with_felt252('Undefined')
+    }
+    fn is_zero(self: UserAddress) -> bool {
+        match self {
+            UserAddress::Starknet(address) => address.is_zero(),
+            UserAddress::Ethereum(address) => address.is_zero(),
+            UserAddress::Custom(address) => address.is_zero(),
+        }
+    }
+    fn is_non_zero(self: UserAddress) -> bool {
+        !self.is_zero()
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use zeroable::Zeroable;
+    use super::{UserAddress, UserAddressZeroable};
+    use starknet::{EthAddress, contract_address_const};
+
+    #[test]
+    fn test_is_zero() {
+        assert(UserAddress::Starknet(contract_address_const::<0>()).is_zero(), 'is not zero');
+        assert(UserAddress::Ethereum(EthAddress { address: 0 }).is_zero(), 'is not zero');
+        assert(UserAddress::Custom(0_u256).is_zero(), 'is not zero');
+    }
+
+    #[test]
+    fn test_is_zero_false_positive() {
+        assert(
+            UserAddress::Starknet(contract_address_const::<1>()).is_zero() == false,
+            'false positive not zero'
+        );
+        assert(
+            UserAddress::Ethereum(EthAddress { address: 1 }).is_zero() == false,
+            'false positive not zero'
+        );
+        assert(UserAddress::Custom(1_u256).is_zero() == false, 'false positive not zero');
+    }
+
+    #[test]
+    fn test_is_non_zero() {
+        assert(UserAddress::Starknet(contract_address_const::<1>()).is_non_zero(), 'is zero');
+        assert(UserAddress::Ethereum(EthAddress { address: 1 }).is_non_zero(), 'is zero');
+        assert(UserAddress::Custom(1_u256).is_non_zero(), 'is zero');
+    }
+
+    #[test]
+    fn test_is_non_zero_false_positive() {
+        assert(
+            UserAddress::Starknet(contract_address_const::<0>()).is_non_zero() == false,
+            'false positive is zero'
+        );
+        assert(
+            UserAddress::Ethereum(EthAddress { address: 0 }).is_non_zero() == false,
+            'false positive is zero'
+        );
+        assert(UserAddress::Custom(0_u256).is_non_zero() == false, 'false positve not zero');
+    }
+}