pezkuwichain/pezkuwi-subxt
1
0
Fork 0
mirror of https://github.com/pezkuwichain/pezkuwi-subxt.git synced 2026-06-14 16:51:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

Update SRML Council: prevent excess votes/computation (#1162)

Browse Source 
* fix: Add assertions to prevent excess votes and computation, and to end council seat election when no empty seats

* Add assertion to prevent a vote from voters that provide a list of votes that exceeds the desired seats length, since otherise an attacker may be able to submit a very long list of `votes` that far exceeds the amount of candidates and waste more computation than a reasonable voting bond would cover. Added additional associated test that may be run with `cargo test -p srml-council`

* Add assertion so expired council seats are not up for election when desired seat count changes during the voting period such that there are no longer any empty seats

* Update comment to refer to `reporter` instead of `who` (target of inactivity), since the origin is the `reporter`

* Update commment to refer more specifically to how many vote indexes, since `InactiveGracePeriod` is measured in vote indexes

* Update comment for `ApprovalsOf` since previously the comment was a duplicate of the comment for `LastActiveOf`'s

* Create variable to refer to `retaining_seats` to improve readability

* Reference Notes: https://hackmd.io/nr6kPD2sR4urmljtvHs0CQ

* WIP - length votes should be less than length candidates. pushing changes for reference so can fix

* fix: Amount of candidate approval votes cannot exceed candidates amount. Candidates amount must be over zero.

* Fix so that amount of candidate approval votes cannot exceed amount of candidates (instead of desired seats)

* Add assertion to `set_approvals` such that amount of candidates to receive approval votes must be greater than zero. Add associated test

* fix: Remove assertion preventing votes when empty seats is 0

* review-fix: Replace with is_zero and add corresponding test

* Update seats.rs

* Update seats.rs
This commit is contained in:
Luke Schoen
2019-01-08 01:53:37 +11:00
committed by Gav Wood
parent eb0ff291d6
commit cb52401350
1 changed files with 75 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files
substrate/srml/council/src/seats.rs
+75 -8
View File
@@ -94,9 +94,16 @@ decl_module! {
fn set_approvals(origin, votes: Vec<bool>, index: Compact<VoteIndex>) {
let who = ensure_signed(origin)?;
let index: VoteIndex = index.into();
let candidates = Self::candidates();
ensure!(!Self::presentation_active(), "no approval changes during presentation period");
ensure!(index == Self::vote_index(), "incorrect vote index");
ensure!(!candidates.len().is_zero(), "amount of candidates to receive approval votes should be non-zero");
// Prevent a vote from voters that provide a list of votes that exceeds the candidates length
// since otherise an attacker may be able to submit a very long list of `votes` that far exceeds
// the amount of candidates and waste more computation than a reasonable voting bond would cover.
ensure!(candidates.len() >= votes.len(), "amount of candidate approval votes cannot exceed amount of candidates");
if !<LastActiveOf<T>>::exists(&who) {
// not yet a voter - deduct bond.
// NOTE: this must be the last potential bailer, since it changes state.
@@ -132,7 +139,7 @@ decl_module! {
ensure!(Self::voter_last_active(&reporter).is_some(), "reporter must be a voter");
let last_active = Self::voter_last_active(&who).ok_or("target for inactivity cleanup must be active")?;
ensure!(assumed_vote_index == Self::vote_index(), "vote index not current");
ensure!(last_active < assumed_vote_index - Self::inactivity_grace_period(), "cannot reap during grace perid");
ensure!(assumed_vote_index > last_active + Self::inactivity_grace_period(), "cannot reap during grace period");
let voters = Self::voters();
let reporter_index: u32 = reporter_index.into();
let reporter_index = reporter_index as usize;
@@ -157,7 +164,7 @@ decl_module! {
voters
);
if valid {
// This only fails if `who` doesn't exist, which it clearly must do since its the origin.
// This only fails if `reporter` doesn't exist, which it clearly must do since its the origin.
// Still, it's no more harmful to propagate any error at this point.
<balances::Module<T>>::repatriate_reserved(&who, &reporter, Self::voting_bond())?;
Self::deposit_event(RawEvent::VoterReaped(who, reporter));
@@ -225,6 +232,7 @@ decl_module! {
) -> Result {
let who = ensure_signed(origin)?;
let total = total.into();
ensure!(!total.is_zero(), "stake deposited to present winner and be added to leaderboard should be non-zero");
let index: VoteIndex = index.into();
let candidate = <balances::Module<T>>::lookup(candidate)?;
@@ -324,7 +332,7 @@ decl_storage! {
pub CarryCount get(carry_count) config(): u32 = 2;
/// How long to give each top candidate to present themselves after the vote ends.
pub PresentationDuration get(presentation_duration) config(): T::BlockNumber = T::BlockNumber::sa(1000);
/// How many votes need to go by after a voter's last vote before they can be reaped if their
/// How many vote indexes need to go by after a target voter's last vote before they can be reaped if their
/// approvals are moot.
pub InactiveGracePeriod get(inactivity_grace_period) config(inactive_grace_period): VoteIndex = 1;
/// How often (in blocks) to check for new votes.
@@ -336,13 +344,16 @@ decl_storage! {
// permanent state (always relevant, changes only at the finalisation of voting)
/// The current council. When there's a vote going on, this should still be used for executive
/// matters.
/// matters. The block number (second element in the tuple) is the block that their position is
/// active until (calculated by the sum of the block number when the council member was elected
/// and their term duration).
pub ActiveCouncil get(active_council) config(): Vec<(T::AccountId, T::BlockNumber)>;
/// The total number of votes that have happened or are in progress.
pub VoteCount get(vote_index): VoteIndex;
// persistent state (always relevant, changes constantly)
/// The last cleared vote index that this voter was last active at.
/// A list of votes for each voter, respecting the last cleared vote index that this voter was
/// last active at.
pub ApprovalsOf get(approvals_of): map T::AccountId => Vec<bool>;
/// The vote index and list slot that the candidate `who` was registered or `None` if they are not
/// currently registered.
@@ -459,8 +470,9 @@ impl<T: Trait> Module<T> {
let desired_seats = Self::desired_seats() as usize;
let number = <system::Module<T>>::block_number();
let expiring = active_council.iter().take_while(|i| i.1 == number).map(|i| i.0.clone()).collect::<Vec<_>>();
if active_council.len() - expiring.len() < desired_seats {
let empty_seats = desired_seats - (active_council.len() - expiring.len());
let retaining_seats = active_council.len() - expiring.len();
if retaining_seats < desired_seats {
let empty_seats = desired_seats - retaining_seats;
<NextFinalise<T>>::put((number + Self::presentation_duration(), empty_seats as u32, expiring));
let voters = Self::voters();
@@ -558,6 +570,7 @@ mod tests {
assert_eq!(Council::voting_bond(), 3);
assert_eq!(Council::present_slash_per_voter(), 1);
assert_eq!(Council::presentation_duration(), 2);
assert_eq!(Council::inactivity_grace_period(), 1);
assert_eq!(Council::voting_period(), 4);
assert_eq!(Council::term_duration(), 5);
assert_eq!(Council::desired_seats(), 2);
@@ -723,6 +736,29 @@ mod tests {
});
}
#[test]
fn setting_any_approval_vote_count_without_any_candidate_count_should_not_work() {
with_externalities(&mut new_test_ext(false), || {
System::set_block_number(1);
assert_eq!(Council::candidates().len(), 0);
assert_noop!(Council::set_approvals(Origin::signed(4), vec![], 0.into()), "amount of candidates to receive approval votes should be non-zero");
});
}
#[test]
fn setting_an_approval_vote_count_more_than_candidate_count_should_not_work() {
with_externalities(&mut new_test_ext(false), || {
System::set_block_number(1);
assert_ok!(Council::submit_candidacy(Origin::signed(5), 0.into()));
assert_eq!(Council::candidates().len(), 1);
assert_noop!(Council::set_approvals(Origin::signed(4), vec![true, true], 0.into()), "amount of candidate approval votes cannot exceed amount of candidates");
});
}
#[test]
fn resubmitting_voting_should_work() {
with_externalities(&mut new_test_ext(false), || {
@@ -735,6 +771,7 @@ mod tests {
assert_ok!(Council::submit_candidacy(Origin::signed(2), 1.into()));
assert_ok!(Council::submit_candidacy(Origin::signed(3), 2.into()));
assert_eq!(Council::candidates().len(), 3);
assert_ok!(Council::set_approvals(Origin::signed(4), vec![true, false, true], 0.into()));
assert_eq!(Council::approvals_of(4), vec![true, false, true]);
@@ -749,6 +786,7 @@ mod tests {
assert_ok!(Council::submit_candidacy(Origin::signed(5), 0.into()));
assert_ok!(Council::submit_candidacy(Origin::signed(2), 1.into()));
assert_ok!(Council::submit_candidacy(Origin::signed(3), 2.into()));
assert_eq!(Council::candidates().len(), 3);
assert_ok!(Council::set_approvals(Origin::signed(1), vec![true], 0.into()));
assert_ok!(Council::set_approvals(Origin::signed(2), vec![false, true, true], 0.into()));
@@ -853,6 +891,19 @@ mod tests {
});
}
#[test]
fn presentations_with_zero_staked_deposit_should_not_work() {
with_externalities(&mut new_test_ext(false), || {
System::set_block_number(4);
assert_ok!(Council::submit_candidacy(Origin::signed(2), 0.into()));
assert_ok!(Council::set_approvals(Origin::signed(2), vec![true], 0.into()));
assert_ok!(Council::end_block(System::block_number()));
System::set_block_number(6);
assert_noop!(Council::present_winner(Origin::signed(4), 2.into(), 0.into(), 0.into()), "stake deposited to present winner and be added to leaderboard should be non-zero");
});
}
#[test]
fn double_presentations_should_be_punished() {
with_externalities(&mut new_test_ext(false), || {
@@ -1057,9 +1108,15 @@ mod tests {
assert_ok!(Council::present_winner(Origin::signed(4), 3.into(), 30.into(), 1.into()));
assert_ok!(Council::end_block(System::block_number()));
assert_eq!(Council::vote_index(), 2);
assert_eq!(Council::inactivity_grace_period(), 1);
assert_eq!(Council::voting_period(), 4);
assert_eq!(Council::voter_last_active(4), Some(0));
assert_ok!(Council::reap_inactive_voter(Origin::signed(4),
(Council::voters().iter().position(|&i| i == 4).unwrap() as u32).into(),
2.into(), (Council::voters().iter().position(|&i| i == 2).unwrap() as u32).into(),
2.into(),
(Council::voters().iter().position(|&i| i == 2).unwrap() as u32).into(),
2.into()
));
@@ -1119,6 +1176,14 @@ mod tests {
assert_ok!(Council::present_winner(Origin::signed(4), 3.into(), 30.into(), 0.into()));
assert_ok!(Council::present_winner(Origin::signed(4), 4.into(), 40.into(), 0.into()));
assert_ok!(Council::present_winner(Origin::signed(4), 5.into(), 50.into(), 0.into()));
assert_eq!(Council::leaderboard(), Some(vec![
(30, 3),
(40, 4),
(50, 5),
(60, 1)
]));
assert_noop!(Council::present_winner(Origin::signed(4), 2.into(), 20.into(), 0.into()), "candidate not worthy of leaderboard");
});
}
@@ -1240,6 +1305,8 @@ mod tests {
System::set_block_number(6);
assert!(Council::presentation_active());
assert_ok!(Council::present_winner(Origin::signed(4), 1.into(), 60.into(), 0.into()));
// leaderboard length is the empty seats plus the carry count (i.e. 5 + 2), where those
// to be carried are the lowest and stored in lowest indexes
assert_eq!(Council::leaderboard(), Some(vec![
(0, 0),
(0, 0),