Skip to content

Commit a1e2679

Browse files
Septimus4Septimus4
authored
Security review (#253)
1 parent 49f7cd4 commit a1e2679

File tree

8 files changed

+237
-37
lines changed

8 files changed

+237
-37
lines changed

‎contracts/tansu/src/contract_dao.rs‎

Lines changed: 62 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,11 @@ impl DaoTrait for Tansu {
116116
votes: Vec<u128>,
117117
seeds: Vec<u128>,
118118
) -> Vec<BytesN<96>> {
119+
// Validate that votes and seeds have the same length
120+
if votes.len() != seeds.len() {
121+
panic_with_error!(&env, &errors::ContractErrors::TallySeedError);
122+
}
123+
119124
let vote_config = Self::get_anonymous_voting_config(env.clone(), project_key);
120125

121126
let bls12_381 = env.crypto().bls12_381();
@@ -248,12 +253,18 @@ impl DaoTrait for Tansu {
248253
};
249254

250255
let next_id = proposal_id + 1;
256+
let page = proposal_id / MAX_PROPOSALS_PER_PAGE;
257+
258+
// Prevent exceeding maximum page limit
259+
if page >= MAX_PAGES {
260+
panic_with_error!(&env, &errors::ContractErrors::NoProposalorPageFound);
261+
}
262+
251263
env.storage().persistent().set(
252264
&types::ProjectKey::DaoTotalProposals(project_key.clone()),
253265
&next_id,
254266
);
255267

256-
let page = proposal_id / MAX_PROPOSALS_PER_PAGE;
257268
let mut dao_page = Self::get_dao(env.clone(), project_key.clone(), page);
258269
dao_page.proposals.push_back(proposal.clone());
259270

@@ -359,6 +370,12 @@ impl DaoTrait for Tansu {
359370
_ => panic_with_error!(&env, &errors::ContractErrors::NoProposalorPageFound),
360371
};
361372

373+
// Check that voting period has not ended
374+
let curr_timestamp = env.ledger().timestamp();
375+
if curr_timestamp >= proposal.vote_data.voting_ends_at {
376+
panic_with_error!(&env, &errors::ContractErrors::ProposalVotingTime);
377+
}
378+
362379
// Check vote limits for DoS protection
363380
if proposal.vote_data.votes.len() >= MAX_VOTES_PER_PROPOSAL {
364381
panic_with_error!(&env, &errors::ContractErrors::VoteLimitExceeded);
@@ -381,14 +398,16 @@ impl DaoTrait for Tansu {
381398
panic_with_error!(&env, &errors::ContractErrors::WrongVoteType);
382399
}
383400

384-
if !is_public_vote && let types::Vote::AnonymousVote(vote_choice) = &vote {
385-
if vote_choice.commitments.len() != 3 {
386-
panic_with_error!(&env, &errors::ContractErrors::BadCommitment)
387-
}
388-
for commitment in &vote_choice.commitments {
389-
G1Affine::from_bytes(commitment);
401+
// For anonymous votes, validate commitment structure
402+
if !is_public_vote
403+
&& let types::Vote::AnonymousVote(vote_choice) = &vote {
404+
if vote_choice.commitments.len() != 3 {
405+
panic_with_error!(&env, &errors::ContractErrors::BadCommitment)
406+
}
407+
for commitment in &vote_choice.commitments {
408+
G1Affine::from_bytes(commitment);
409+
}
390410
}
391-
}
392411

393412
// can only vote for yourself so address must match
394413
let vote_address = match &vote {
@@ -411,6 +430,10 @@ impl DaoTrait for Tansu {
411430
vote_address.clone(),
412431
);
413432

433+
if voter_max_weight == 0 {
434+
panic_with_error!(&env, &errors::ContractErrors::UnknownMember);
435+
}
436+
414437
if vote_weight > &voter_max_weight {
415438
panic_with_error!(&env, &errors::ContractErrors::VoterWeight);
416439
}
@@ -525,22 +548,28 @@ impl DaoTrait for Tansu {
525548
// tally to results
526549
proposal.status = match proposal.vote_data.public_voting {
527550
true => {
528-
if tallies.is_some() | seeds.is_some() {
551+
if tallies.is_some() || seeds.is_some() {
529552
panic_with_error!(&env, &errors::ContractErrors::TallySeedError);
530553
}
531554
public_execute(&proposal)
532555
}
533556
false => {
534-
if !(tallies.is_some() & seeds.is_some()) {
557+
let (tallies_, seeds_) = match (tallies, seeds) {
558+
(Some(t), Some(s)) => (t, s),
559+
_ => panic_with_error!(&env, &errors::ContractErrors::TallySeedError),
560+
};
561+
562+
// Validate tallies and seeds have expected length (3: approve, reject, abstain)
563+
if tallies_.len() != 3 || seeds_.len() != 3 {
535564
panic_with_error!(&env, &errors::ContractErrors::TallySeedError);
536565
}
537-
let tallies_ = tallies.unwrap();
566+
538567
if !Self::proof(
539568
env.clone(),
540569
project_key.clone(),
541570
proposal.clone(),
542571
tallies_.clone(),
543-
seeds.unwrap(),
572+
seeds_,
544573
) {
545574
panic_with_error!(&env, &errors::ContractErrors::InvalidProof)
546575
}
@@ -604,7 +633,7 @@ impl DaoTrait for Tansu {
604633
tallies: Vec<u128>,
605634
seeds: Vec<u128>,
606635
) -> bool {
607-
// only allow to proof if proposal is not active
636+
// Proof validation only applies to active proposals (before execution)
608637
if proposal.status != types::ProposalStatus::Active {
609638
panic_with_error!(&env, &errors::ContractErrors::ProposalActive);
610639
}
@@ -620,7 +649,9 @@ impl DaoTrait for Tansu {
620649
.storage()
621650
.instance()
622651
.get(&types::ProjectKey::AnonymousVoteConfig(project_key))
623-
.unwrap();
652+
.unwrap_or_else(|| {
653+
panic_with_error!(&env, &errors::ContractErrors::NoAnonymousVotingConfig);
654+
});
624655

625656
let seed_generator_point = G1Affine::from_bytes(vote_config.seed_generator_point);
626657
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 {
777808
/// # Returns
778809
/// * `types::ProposalStatus` - The final status (Approved if approve > reject, Rejected if reject > approve, Cancelled if equal)
779810
pub fn anonymous_execute(tallies: &Vec<u128>) -> types::ProposalStatus {
780-
let mut iter = tallies.iter();
781-
782-
let voted_approve = iter.next().unwrap();
783-
let voted_reject = iter.next().unwrap();
784-
let voted_abstain = iter.next().unwrap();
811+
// Use get() method to access elements safely
812+
let voted_approve = tallies
813+
.get(0)
814+
.expect("anonymous_execute missing approve tally entry");
815+
let voted_reject = tallies
816+
.get(1)
817+
.expect("anonymous_execute missing reject tally entry");
818+
let voted_abstain = tallies
819+
.get(2)
820+
.expect("anonymous_execute missing abstain tally entry");
785821

786822
tallies_to_result(voted_approve, voted_reject, voted_abstain)
787823
}
@@ -804,10 +840,14 @@ fn tallies_to_result(
804840
voted_reject: u128,
805841
voted_abstain: u128,
806842
) -> types::ProposalStatus {
807-
// accept or reject if we have a majority
808-
if voted_approve > (voted_abstain + voted_reject) {
843+
// Supermajority governance: requires more than half of all votes (including abstains)
844+
// This ensures broad consensus before passing any proposal
845+
// Approve needs: approve > (reject + abstain)
846+
// Reject needs: reject > (approve + abstain)
847+
// Otherwise: cancelled (tie or no clear supermajority)
848+
if voted_approve > (voted_reject + voted_abstain) {
809849
types::ProposalStatus::Approved
810-
} else if voted_reject > (voted_abstain + voted_approve) {
850+
} else if voted_reject > (voted_approve + voted_abstain) {
811851
types::ProposalStatus::Rejected
812852
} else {
813853
types::ProposalStatus::Cancelled

‎contracts/tansu/src/contract_membership.rs‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,13 @@ impl MembershipTrait for Tansu {
101101
// a project
102102
'member_projects_badges: {
103103
for i in 0..member_.projects.len() {
104-
if member_.projects.get(i).unwrap().project == key {
105-
let mut project_badges = member_.projects.get(i).unwrap().clone();
106-
project_badges.badges = badges.clone();
107-
member_.projects.set(i, project_badges);
108-
break 'member_projects_badges;
109-
}
104+
if let Some(project_badge) = member_.projects.get(i)
105+
&& project_badge.project == key {
106+
let mut project_badges = project_badge.clone();
107+
project_badges.badges = badges.clone();
108+
member_.projects.set(i, project_badges);
109+
break 'member_projects_badges;
110+
}
110111
}
111112
let project_badges = types::ProjectBadges {
112113
project: key.clone(),

‎contracts/tansu/src/contract_tansu.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,9 @@ impl TansuTrait for Tansu {
8080
env.storage()
8181
.instance()
8282
.get(&types::DataKey::AdminsConfig)
83-
.unwrap()
83+
.unwrap_or_else(|| {
84+
panic_with_error!(&env, &crate::errors::ContractErrors::UnexpectedError);
85+
})
8486
}
8587

8688
/// Set the Soroban Domain contract.
@@ -164,6 +166,11 @@ impl TansuTrait for Tansu {
164166
let admins_config =
165167
new_admins_config.unwrap_or_else(|| Self::get_admins_config(env.clone()));
166168

169+
// Validate that threshold is reasonable
170+
if admins_config.threshold == 0 || admins_config.threshold > admins_config.admins.len() {
171+
panic_with_error!(&env, &crate::errors::ContractErrors::UpgradeError);
172+
}
173+
167174
let upgrade_proposal = types::UpgradeProposal {
168175
wasm_hash: new_wasm_hash.clone(),
169176
executable_at,

‎contracts/tansu/src/lib.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -194,9 +194,9 @@ fn retrieve_contract(env: &Env, key: types::ContractKey) -> types::Contract {
194194
fn validate_contract(env: &Env, contract: &types::Contract) {
195195
let contract_executable = contract.address.executable();
196196

197-
if let Some(wasm_hash) = contract.clone().wasm_hash
198-
&& contract_executable != Some(Executable::Wasm(wasm_hash))
199-
{
200-
panic_with_error!(&env, &errors::ContractErrors::ContractValidation)
197+
if let Some(wasm_hash) = contract.clone().wasm_hash {
198+
if contract_executable != Some(Executable::Wasm(wasm_hash)) {
199+
panic_with_error!(&env, &errors::ContractErrors::ContractValidation)
200+
}
201201
}
202202
}

‎contracts/tansu/src/tests/test_commit.rs‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,3 +56,72 @@ fn commit_unregistered_maintainer_error() {
5656
.unwrap();
5757
assert_eq!(err, ContractErrors::UnauthorizedSigner.into());
5858
}
59+
60+
#[test]
61+
fn test_anonymous_vote_commitment_validation() {
62+
let setup = create_test_data();
63+
let id = init_contract(&setup);
64+
65+
// Setup anonymous voting first
66+
setup.contract.anonymous_voting_setup(
67+
&setup.grogu,
68+
&id,
69+
&String::from_str(&setup.env, "test_public_key"),
70+
);
71+
72+
// Test mismatched votes and seeds length
73+
let result = setup.contract.try_build_commitments_from_votes(
74+
&id,
75+
&vec![&setup.env, 1u128, 2u128], // 2 votes
76+
&vec![&setup.env, 1u128], // 1 seed - mismatch!
77+
);
78+
79+
assert_eq!(
80+
result,
81+
Err(Ok(soroban_sdk::Error::from_contract_error(
82+
ContractErrors::TallySeedError as u32
83+
)))
84+
);
85+
}
86+
87+
/// Test malformed inputs for build_commitments_from_votes with mismatched lengths
88+
#[test]
89+
fn test_malformed_commitments_mismatched_lengths() {
90+
let setup = create_test_data();
91+
let id = init_contract(&setup);
92+
93+
// Setup anonymous voting
94+
setup.contract.anonymous_voting_setup(
95+
&setup.grogu,
96+
&id,
97+
&String::from_str(&setup.env, "test_public_key"),
98+
);
99+
100+
// Test mismatched votes and seeds length - votes longer
101+
let result = setup.contract.try_build_commitments_from_votes(
102+
&id,
103+
&vec![&setup.env, 1u128, 2u128, 3u128, 4u128], // 4 votes
104+
&vec![&setup.env, 1u128, 2u128, 3u128], // 3 seeds - mismatch!
105+
);
106+
107+
assert_eq!(
108+
result,
109+
Err(Ok(soroban_sdk::Error::from_contract_error(
110+
ContractErrors::TallySeedError as u32
111+
)))
112+
);
113+
114+
// Test mismatched votes and seeds length - seeds longer
115+
let result2 = setup.contract.try_build_commitments_from_votes(
116+
&id,
117+
&vec![&setup.env, 1u128, 2u128], // 2 votes
118+
&vec![&setup.env, 1u128, 2u128, 3u128], // 3 seeds - mismatch!
119+
);
120+
121+
assert_eq!(
122+
result2,
123+
Err(Ok(soroban_sdk::Error::from_contract_error(
124+
ContractErrors::TallySeedError as u32
125+
)))
126+
);
127+
}

‎contracts/tansu/src/tests/test_dao.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -748,3 +748,4 @@ fn voter_weight_validation() {
748748
assert_eq!(public_vote.vote_choice, VoteChoice::Approve);
749749
}
750750
}
751+

‎contracts/tansu/src/tests/test_pause_upgrade.rs‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use super::test_utils::create_test_data;
22
use crate::errors::ContractErrors;
33
use crate::{domain_contract, types};
44
use soroban_sdk::testutils::{Address as _, Events, Ledger};
5-
use soroban_sdk::{Address, BytesN, Executable, IntoVal, Map, String, Symbol, Val, bytesn, vec};
5+
use soroban_sdk::{Address, Bytes, BytesN, Executable, IntoVal, Map, String, Symbol, Val, bytesn, vec};
66

77
#[test]
88
fn test_pause_unpause() {
@@ -464,3 +464,51 @@ fn test_domain_contract_update() {
464464
// .unwrap();
465465
// assert_eq!(retrieved_domain, new_domain);
466466
}
467+
468+
#[test]
469+
fn test_upgrade_invalid_threshold() {
470+
let setup = create_test_data();
471+
472+
// Zero threshold is invalid
473+
let invalid_config = types::AdminsConfig {
474+
threshold: 0,
475+
admins: vec![&setup.env, setup.contract_admin.clone()],
476+
};
477+
478+
// Compute a dummy wasm hash
479+
let wasm_bytes = Bytes::from_slice(&setup.env, b"new_wasm");
480+
let new_wasm_hash: BytesN<32> = setup.env.crypto().keccak256(&wasm_bytes).into();
481+
482+
// Proposing upgrade with invalid config should fail
483+
let err = setup
484+
.contract
485+
.try_propose_upgrade(&setup.contract_admin, &new_wasm_hash, &Some(invalid_config))
486+
.unwrap_err()
487+
.unwrap();
488+
489+
assert_eq!(err, ContractErrors::UpgradeError.into());
490+
}
491+
492+
#[test]
493+
fn test_upgrade_threshold_exceeds_admins() {
494+
let setup = create_test_data();
495+
496+
// Threshold greater than admin count is invalid
497+
let invalid_config = types::AdminsConfig {
498+
threshold: 3, // More than the single admin present
499+
admins: vec![&setup.env, setup.contract_admin.clone()],
500+
};
501+
502+
// Compute a dummy wasm hash
503+
let wasm_bytes = Bytes::from_slice(&setup.env, b"new_wasm");
504+
let new_wasm_hash: BytesN<32> = setup.env.crypto().keccak256(&wasm_bytes).into();
505+
506+
// Proposing upgrade with invalid threshold should fail
507+
let err = setup
508+
.contract
509+
.try_propose_upgrade(&setup.contract_admin, &new_wasm_hash, &Some(invalid_config))
510+
.unwrap_err()
511+
.unwrap();
512+
513+
assert_eq!(err, ContractErrors::UpgradeError.into());
514+
}

0 commit comments

Comments
 (0)