fix: Enhance contract validation and error handling for voting and proposal processes

Septimus4 · Septimus4 · commit 2107eaf989e9 · 2025-09-29T21:40:37.000Z
diff --git a/contracts/tansu/src/contract_dao.rs b/contracts/tansu/src/contract_dao.rs
@@ -116,6 +116,11 @@ impl DaoTrait for Tansu {
         votes: Vec<u128>,
         seeds: Vec<u128>,
     ) -> Vec<BytesN<96>> {
+        // Validate that votes and seeds have the same length
+        if votes.len() != seeds.len() {
+            panic_with_error!(&env, &errors::ContractErrors::TallySeedError);
+        }
+
         let vote_config = Self::get_anonymous_voting_config(env.clone(), project_key);
 
         let bls12_381 = env.crypto().bls12_381();
@@ -248,12 +253,18 @@ impl DaoTrait for Tansu {
         };
 
         let next_id = proposal_id + 1;
+        let page = proposal_id / MAX_PROPOSALS_PER_PAGE;
+        
+        // Prevent exceeding maximum page limit
+        if page >= MAX_PAGES {
+            panic_with_error!(&env, &errors::ContractErrors::NoProposalorPageFound);
+        }
+
         env.storage().persistent().set(
             &types::ProjectKey::DaoTotalProposals(project_key.clone()),
             &next_id,
         );
 
-        let page = proposal_id / MAX_PROPOSALS_PER_PAGE;
         let mut dao_page = Self::get_dao(env.clone(), project_key.clone(), page);
         dao_page.proposals.push_back(proposal.clone());
 
@@ -359,6 +370,12 @@ impl DaoTrait for Tansu {
             _ => panic_with_error!(&env, &errors::ContractErrors::NoProposalorPageFound),
         };
 
+        // Check that voting period has not ended
+        let curr_timestamp = env.ledger().timestamp();
+        if curr_timestamp >= proposal.vote_data.voting_ends_at {
+            panic_with_error!(&env, &errors::ContractErrors::ProposalVotingTime);
+        }
+
         // Check vote limits for DoS protection
         if proposal.vote_data.votes.len() >= MAX_VOTES_PER_PROPOSAL {
             panic_with_error!(&env, &errors::ContractErrors::VoteLimitExceeded);
@@ -381,14 +398,16 @@ impl DaoTrait for Tansu {
             panic_with_error!(&env, &errors::ContractErrors::WrongVoteType);
         }
 
-        if !is_public_vote && let types::Vote::AnonymousVote(vote_choice) = &vote {
-            if vote_choice.commitments.len() != 3 {
-                panic_with_error!(&env, &errors::ContractErrors::BadCommitment)
-            }
-            for commitment in &vote_choice.commitments {
-                G1Affine::from_bytes(commitment);
+        // For anonymous votes, validate commitment structure
+        if !is_public_vote
+            && let types::Vote::AnonymousVote(vote_choice) = &vote {
+                if vote_choice.commitments.len() != 3 {
+                    panic_with_error!(&env, &errors::ContractErrors::BadCommitment)
+                }
+                for commitment in &vote_choice.commitments {
+                    G1Affine::from_bytes(commitment);
+                }
             }
-        }
 
         // can only vote for yourself so address must match
         let vote_address = match &vote {
@@ -411,6 +430,10 @@ impl DaoTrait for Tansu {
             vote_address.clone(),
         );
 
+        if voter_max_weight == 0 {
+            panic_with_error!(&env, &errors::ContractErrors::UnknownMember);
+        }
+
         if vote_weight > &voter_max_weight {
             panic_with_error!(&env, &errors::ContractErrors::VoterWeight);
         }
@@ -525,22 +548,28 @@ impl DaoTrait for Tansu {
         // tally to results
         proposal.status = match proposal.vote_data.public_voting {
             true => {
-                if tallies.is_some() | seeds.is_some() {
+                if tallies.is_some() || seeds.is_some() {
                     panic_with_error!(&env, &errors::ContractErrors::TallySeedError);
                 }
                 public_execute(&proposal)
             }
             false => {
-                if !(tallies.is_some() & seeds.is_some()) {
+                let (tallies_, seeds_) = match (tallies, seeds) {
+                    (Some(t), Some(s)) => (t, s),
+                    _ => panic_with_error!(&env, &errors::ContractErrors::TallySeedError),
+                };
+                
+                // Validate tallies and seeds have expected length (3: approve, reject, abstain)
+                if tallies_.len() != 3 || seeds_.len() != 3 {
                     panic_with_error!(&env, &errors::ContractErrors::TallySeedError);
                 }
-                let tallies_ = tallies.unwrap();
+                
                 if !Self::proof(
                     env.clone(),
                     project_key.clone(),
                     proposal.clone(),
                     tallies_.clone(),
-                    seeds.unwrap(),
+                    seeds_,
                 ) {
                     panic_with_error!(&env, &errors::ContractErrors::InvalidProof)
                 }
@@ -604,7 +633,7 @@ impl DaoTrait for Tansu {
         tallies: Vec<u128>,
         seeds: Vec<u128>,
     ) -> bool {
-        // only allow to proof if proposal is not active
+        // Proof validation only applies to active proposals (before execution)
         if proposal.status != types::ProposalStatus::Active {
             panic_with_error!(&env, &errors::ContractErrors::ProposalActive);
         }
@@ -620,7 +649,9 @@ impl DaoTrait for Tansu {
             .storage()
             .instance()
             .get(&types::ProjectKey::AnonymousVoteConfig(project_key))
-            .unwrap();
+            .unwrap_or_else(|| {
+                panic_with_error!(&env, &errors::ContractErrors::NoAnonymousVotingConfig);
+            });
 
         let seed_generator_point = G1Affine::from_bytes(vote_config.seed_generator_point);
         let vote_generator_point = G1Affine::from_bytes(vote_config.vote_generator_point);
@@ -777,11 +808,16 @@ pub fn public_execute(proposal: &types::Proposal) -> types::ProposalStatus {
 /// # Returns
 /// * `types::ProposalStatus` - The final status (Approved if approve > reject, Rejected if reject > approve, Cancelled if equal)
 pub fn anonymous_execute(tallies: &Vec<u128>) -> types::ProposalStatus {
-    let mut iter = tallies.iter();
-
-    let voted_approve = iter.next().unwrap();
-    let voted_reject = iter.next().unwrap();
-    let voted_abstain = iter.next().unwrap();
+    // Use get() method to access elements safely
+    let voted_approve = tallies
+        .get(0)
+        .expect("anonymous_execute missing approve tally entry");
+    let voted_reject = tallies
+        .get(1)
+        .expect("anonymous_execute missing reject tally entry");
+    let voted_abstain = tallies
+        .get(2)
+        .expect("anonymous_execute missing abstain tally entry");
 
     tallies_to_result(voted_approve, voted_reject, voted_abstain)
 }
@@ -804,10 +840,14 @@ fn tallies_to_result(
     voted_reject: u128,
     voted_abstain: u128,
 ) -> types::ProposalStatus {
-    // accept or reject if we have a majority
-    if voted_approve > (voted_abstain + voted_reject) {
+    // Supermajority governance: requires more than half of all votes (including abstains)
+    // This ensures broad consensus before passing any proposal
+    // Approve needs: approve > (reject + abstain)
+    // Reject needs: reject > (approve + abstain)
+    // Otherwise: cancelled (tie or no clear supermajority)
+    if voted_approve > (voted_reject + voted_abstain) {
         types::ProposalStatus::Approved
-    } else if voted_reject > (voted_abstain + voted_approve) {
+    } else if voted_reject > (voted_approve + voted_abstain) {
         types::ProposalStatus::Rejected
     } else {
         types::ProposalStatus::Cancelled
diff --git a/contracts/tansu/src/contract_membership.rs b/contracts/tansu/src/contract_membership.rs
@@ -101,12 +101,13 @@ impl MembershipTrait for Tansu {
         // a project
         'member_projects_badges: {
             for i in 0..member_.projects.len() {
-                if member_.projects.get(i).unwrap().project == key {
-                    let mut project_badges = member_.projects.get(i).unwrap().clone();
-                    project_badges.badges = badges.clone();
-                    member_.projects.set(i, project_badges);
-                    break 'member_projects_badges;
-                }
+                if let Some(project_badge) = member_.projects.get(i)
+                    && project_badge.project == key {
+                        let mut project_badges = project_badge.clone();
+                        project_badges.badges = badges.clone();
+                        member_.projects.set(i, project_badges);
+                        break 'member_projects_badges;
+                    }
             }
             let project_badges = types::ProjectBadges {
                 project: key.clone(),
diff --git a/contracts/tansu/src/contract_tansu.rs b/contracts/tansu/src/contract_tansu.rs
@@ -80,7 +80,9 @@ impl TansuTrait for Tansu {
         env.storage()
             .instance()
             .get(&types::DataKey::AdminsConfig)
-            .unwrap()
+            .unwrap_or_else(|| {
+                panic_with_error!(&env, &crate::errors::ContractErrors::UnexpectedError);
+            })
     }
 
     /// Set the Soroban Domain contract.
@@ -164,6 +166,11 @@ impl TansuTrait for Tansu {
         let admins_config =
             new_admins_config.unwrap_or_else(|| Self::get_admins_config(env.clone()));
 
+        // Validate that threshold is reasonable
+        if admins_config.threshold == 0 || admins_config.threshold > admins_config.admins.len() {
+            panic_with_error!(&env, &crate::errors::ContractErrors::UpgradeError);
+        }
+
         let upgrade_proposal = types::UpgradeProposal {
             wasm_hash: new_wasm_hash.clone(),
             executable_at,
diff --git a/contracts/tansu/src/lib.rs b/contracts/tansu/src/lib.rs
@@ -195,8 +195,7 @@ fn validate_contract(env: &Env, contract: &types::Contract) {
     let contract_executable = contract.address.executable();
 
     if let Some(wasm_hash) = contract.clone().wasm_hash {
-        if let Some(wasm_hash) = contract.clone().wasm_hash
-        && contract_executable != Some(Executable::Wasm(wasm_hash)) {
+        if contract_executable != Some(Executable::Wasm(wasm_hash)) {
             panic_with_error!(&env, &errors::ContractErrors::ContractValidation)
         }
     }
diff --git a/contracts/tansu/src/tests/test_commit.rs b/contracts/tansu/src/tests/test_commit.rs
@@ -56,3 +56,72 @@ fn commit_unregistered_maintainer_error() {
         .unwrap();
     assert_eq!(err, ContractErrors::UnauthorizedSigner.into());
 }
+
+#[test]
+fn test_anonymous_vote_commitment_validation() {
+    let setup = create_test_data();
+    let id = init_contract(&setup);
+
+    // Setup anonymous voting first
+    setup.contract.anonymous_voting_setup(
+        &setup.grogu,
+        &id,
+        &String::from_str(&setup.env, "test_public_key"),
+    );
+
+    // Test mismatched votes and seeds length
+    let result = setup.contract.try_build_commitments_from_votes(
+        &id,
+        &vec![&setup.env, 1u128, 2u128], // 2 votes
+        &vec![&setup.env, 1u128],        // 1 seed - mismatch!
+    );
+
+    assert_eq!(
+        result,
+        Err(Ok(soroban_sdk::Error::from_contract_error(
+            ContractErrors::TallySeedError as u32
+        )))
+    );
+}
+
+/// Test malformed inputs for build_commitments_from_votes with mismatched lengths
+#[test]
+fn test_malformed_commitments_mismatched_lengths() {
+    let setup = create_test_data();
+    let id = init_contract(&setup);
+
+    // Setup anonymous voting
+    setup.contract.anonymous_voting_setup(
+        &setup.grogu,
+        &id,
+        &String::from_str(&setup.env, "test_public_key"),
+    );
+
+    // Test mismatched votes and seeds length - votes longer
+    let result = setup.contract.try_build_commitments_from_votes(
+        &id,
+        &vec![&setup.env, 1u128, 2u128, 3u128, 4u128], // 4 votes
+        &vec![&setup.env, 1u128, 2u128, 3u128],        // 3 seeds - mismatch!
+    );
+
+    assert_eq!(
+        result,
+        Err(Ok(soroban_sdk::Error::from_contract_error(
+            ContractErrors::TallySeedError as u32
+        )))
+    );
+
+    // Test mismatched votes and seeds length - seeds longer
+    let result2 = setup.contract.try_build_commitments_from_votes(
+        &id,
+        &vec![&setup.env, 1u128, 2u128], // 2 votes
+        &vec![&setup.env, 1u128, 2u128, 3u128], // 3 seeds - mismatch!
+    );
+
+    assert_eq!(
+        result2,
+        Err(Ok(soroban_sdk::Error::from_contract_error(
+            ContractErrors::TallySeedError as u32
+        )))
+    );
+}
diff --git a/contracts/tansu/src/tests/test_dao.rs b/contracts/tansu/src/tests/test_dao.rs
@@ -116,39 +116,39 @@ fn proposal_flow() {
         .contract
         .execute(&setup.mando, &id, &proposal_id, &None, &None);
 
-    // // Verify proposal executed event
-    // let mut all_events = setup.env.events().all();
+    // Verify proposal executed event
+    let mut all_events = setup.env.events().all();
     all_events.pop_front();
     all_events.pop_front();
     all_events.pop_front();
-    // assert_eq!(
-    //     all_events,
-    //     vec![
-    //         &setup.env,
-    //         (
-    //             setup.contract_id.clone(),
-    //             (Symbol::new(&setup.env, "proposal_executed"), id.clone()).into_val(&setup.env),
-    //             Map::<Symbol, Val>::from_array(
-    //                 &setup.env,
-    //                 [
-    //                     (
-    //                         Symbol::new(&setup.env, "proposal_id"),
-    //                         proposal_id.into_val(&setup.env)
-    //                     ),
-    //                     (
-    //                         Symbol::new(&setup.env, "status"),
-    //                         String::from_str(&setup.env, "Cancelled").into_val(&setup.env)
-    //                     ),
-    //                     (
-    //                         Symbol::new(&setup.env, "maintainer"),
-    //                         setup.mando.clone().into_val(&setup.env)
-    //                     ),
-    //                 ],
-    //             )
-    //             .into_val(&setup.env),
-    //         ),
-    //     ]
-    // );
+    assert_eq!(
+        all_events,
+        vec![
+            &setup.env,
+            (
+                setup.contract_id.clone(),
+                (Symbol::new(&setup.env, "proposal_executed"), id.clone()).into_val(&setup.env),
+                Map::<Symbol, Val>::from_array(
+                    &setup.env,
+                    [
+                        (
+                            Symbol::new(&setup.env, "proposal_id"),
+                            proposal_id.into_val(&setup.env)
+                        ),
+                        (
+                            Symbol::new(&setup.env, "status"),
+                            String::from_str(&setup.env, "Cancelled").into_val(&setup.env)
+                        ),
+                        (
+                            Symbol::new(&setup.env, "maintainer"),
+                            setup.mando.clone().into_val(&setup.env)
+                        ),
+                    ],
+                )
+                .into_val(&setup.env),
+            ),
+        ]
+    );
 
     assert_eq!(result, ProposalStatus::Cancelled);
 
@@ -748,3 +748,4 @@ fn voter_weight_validation() {
         assert_eq!(public_vote.vote_choice, VoteChoice::Approve);
     }
 }
+
diff --git a/contracts/tansu/src/tests/test_pause_upgrade.rs b/contracts/tansu/src/tests/test_pause_upgrade.rs
diff --git a/contracts/tansu/src/types.rs b/contracts/tansu/src/types.rs
diff --git a/website/docs/developers/governance.mdx b/website/docs/developers/governance.mdx

Original file line number	Diff line number	Diff line change
`@@ -195,8 +195,7 @@ fn validate_contract(env: &Env, contract: &types::Contract) {`
`195`	`195`	`let contract_executable = contract.address.executable();`
`196`	`196`
`197`	`197`	`if let Some(wasm_hash) = contract.clone().wasm_hash {`
`198`		`- if let Some(wasm_hash) = contract.clone().wasm_hash`
`199`		`- && contract_executable != Some(Executable::Wasm(wasm_hash)) {`
	`198`	`+ if contract_executable != Some(Executable::Wasm(wasm_hash)) {`
`200`	`199`	`panic_with_error!(&env, &errors::ContractErrors::ContractValidation)`
`201`	`200`	`}`
`202`	`201`	`}`