Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix missing checks in execute and use match in loops #505

Merged
merged 12 commits into from
Aug 28, 2023
Merged

Fix missing checks in execute and use match in loops #505

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cffdced
fix missing checks in execute and add regression tests
pscott Aug 18, 2023
17e3528
fix: add checks when updating min/max voting durations
pscott Aug 18, 2023
b4c376a
refactor: use match in add_voting_strategies
pscott Aug 18, 2023
3410daf
refactor: use match in remove_voting_strategies
pscott Aug 18, 2023
1475e36
refactor: use match in add_authenticators
pscott Aug 18, 2023
9c92753
refactor: use match in remove_authenticators
pscott Aug 18, 2023
0520802
refactor: use match in assert_no_duplicate_indices
pscott Aug 18, 2023
a40fed6
refactor: use match in felt252array_into_u256array
pscott Aug 18, 2023
40ce7ef
optimize get_voting_power; use Span in get_voting_power signature
pscott Aug 19, 2023
43d5c62
Merge branch 'develop' of github.com:snapshot-labs/sx-starknet into f…
pscott Aug 19, 2023
104fb51
Merge branch 'develop' of github.com:snapshot-labs/sx-starknet into f…
pscott Aug 25, 2023
c9f9bae
fix: uncomment test
pscott Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions starknet/src/interfaces/i_voting_strategy.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ trait IVotingStrategy<TContractState> {
self: @TContractState,
timestamp: u32,
voter: UserAddress,
params: Array<felt252>,
user_params: Array<felt252>,
params: Span<felt252>,
user_params: Span<felt252>,
) -> u256;
}
171 changes: 100 additions & 71 deletions starknet/src/space/space.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,14 @@ mod Space {
let mut state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::initializer(ref state);
Ownable::transfer_ownership(ref state, owner);
_set_max_voting_duration(
ref self, max_voting_duration
); // Need to set max before min, or else `max == 0` and set_min will revert
_set_min_voting_duration(ref self, min_voting_duration);
_set_max_voting_duration(ref self, max_voting_duration);
_set_voting_delay(ref self, voting_delay);
_set_proposal_validation_strategy(ref self, proposal_validation_strategy);
_add_voting_strategies(ref self, voting_strategies);
_add_authenticators(ref self, authenticators);
_add_voting_strategies(ref self, voting_strategies.span());
_add_authenticators(ref self, authenticators.span());
self._next_proposal_id.write(1_u256);
}
fn propose(
Expand Down Expand Up @@ -330,7 +332,7 @@ mod Space {
@self,
voter,
proposal.start_timestamp,
user_voting_strategies,
user_voting_strategies.span(),
proposal.active_voting_strategies
);

Expand All @@ -350,6 +352,15 @@ mod Space {
let mut proposal = self._proposals.read(proposal_id);
assert_proposal_exists(@proposal);

let recovered_hash = poseidon::poseidon_hash_span(execution_payload.span());
// Check that payload matches
assert(recovered_hash == proposal.execution_payload_hash, 'Invalid payload hash');

// Check that finalization status is not pending
assert(
proposal.finalization_status == FinalizationStatus::Pending(()), 'Already finalized'
);

IExecutionStrategyDispatcher {
contract_address: proposal.execution_strategy
}
Expand Down Expand Up @@ -388,7 +399,7 @@ mod Space {

proposal
.execution_payload_hash =
poseidon::poseidon_hash_span(execution_strategy.clone().params.span());
poseidon::poseidon_hash_span(execution_strategy.params.span());

self._proposals.write(proposal_id, proposal);

Expand Down Expand Up @@ -483,15 +494,29 @@ mod Space {
let state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::assert_only_owner(@state);

// Needed because the compiiler will go crazy if we try to use `input` directly
let _min_voting_duration = input.min_voting_duration;
let _max_voting_duration = input.max_voting_duration;
// if not NO_UPDATE
if NoUpdateU32::should_update(@input.max_voting_duration) {
_set_max_voting_duration(ref self, input.max_voting_duration);
MaxVotingDurationUpdated(input.max_voting_duration);
}
if NoUpdateU32::should_update(@_max_voting_duration)
&& NoUpdateU32::should_update(@_min_voting_duration) {
// Check that min and max voting durations are valid
// We don't use the internal `_set_min_voting_duration` and `_set_max_voting_duration` functions because
// it would revert when `_min_voting_duration > max_voting_duration` (when the new `_min` is
// bigger than the current `max`).
assert(_min_voting_duration <= _max_voting_duration, 'Invalid duration');

self._min_voting_duration.write(input.min_voting_duration);
MinVotingDurationUpdated(input.min_voting_duration);

if NoUpdateU32::should_update(@input.min_voting_duration) {
self._max_voting_duration.write(input.max_voting_duration);
MaxVotingDurationUpdated(input.max_voting_duration);
} else if NoUpdateU32::should_update(@_min_voting_duration) {
_set_min_voting_duration(ref self, input.min_voting_duration);
MinVotingDurationUpdated(input.min_voting_duration);
} else if NoUpdateU32::should_update(@_max_voting_duration) {
_set_max_voting_duration(ref self, input.max_voting_duration);
MaxVotingDurationUpdated(input.max_voting_duration);
}

if NoUpdateU32::should_update(@input.voting_delay) {
Expand Down Expand Up @@ -521,27 +546,27 @@ mod Space {
}

if NoUpdateArray::should_update((@input).authenticators_to_add) {
_add_authenticators(ref self, input.authenticators_to_add.clone());
_add_authenticators(ref self, input.authenticators_to_add.span());
AuthenticatorsAdded(@input.authenticators_to_add);
}

// if not NO_UPDATE
if NoUpdateArray::should_update((@input).authenticators_to_remove) {
_remove_authenticators(ref self, input.authenticators_to_remove.clone());
_remove_authenticators(ref self, input.authenticators_to_remove.span());
AuthenticatorsRemoved(@input.authenticators_to_remove);
}

// if not NO_UPDATE
if NoUpdateArray::should_update((@input).voting_strategies_to_add) {
_add_voting_strategies(ref self, input.voting_strategies_to_add.clone());
_add_voting_strategies(ref self, input.voting_strategies_to_add.span());
VotingStrategiesAdded(
@input.voting_strategies_to_add, @input.voting_strategies_metadata_URIs_to_add
);
}

// if not NO_UPDATE
if NoUpdateArray::should_update((@input).voting_strategies_to_remove) {
_remove_voting_strategies(ref self, input.voting_strategies_to_remove.clone());
_remove_voting_strategies(ref self, input.voting_strategies_to_remove.span());
VotingStrategiesRemoved(@input.voting_strategies_to_remove);
}
}
Expand Down Expand Up @@ -578,35 +603,41 @@ mod Space {
self: @ContractState,
voter: UserAddress,
timestamp: u32,
user_strategies: Array<IndexedStrategy>,
mut user_strategies: Span<IndexedStrategy>,
allowed_strategies: u256
) -> u256 {
user_strategies.assert_no_duplicate_indices();
let mut total_voting_power = 0_u256;
let mut i = 0_usize;
loop {
if i >= user_strategies.len() {
break ();
}
let strategy_index = user_strategies.at(i).index;
assert(allowed_strategies.is_bit_set(*strategy_index), 'Invalid strategy index');
let strategy = self._voting_strategies.read(*strategy_index);
total_voting_power += IVotingStrategyDispatcher {
contract_address: strategy.address
}
.get_voting_power(
timestamp, voter, strategy.params, user_strategies.at(i).params.clone()
);
i += 1;
match user_strategies.pop_front() {
Option::Some(strategy_index) => {
assert(
allowed_strategies.is_bit_set(*strategy_index.index),
'Invalid strategy index'
);
let strategy = self._voting_strategies.read(*strategy_index.index);
total_voting_power += IVotingStrategyDispatcher {
contract_address: strategy.address
}
.get_voting_power(
timestamp, voter, strategy.params.span(), strategy_index.params.span()
);
},
Option::None => {
break;
},
};
};
total_voting_power
}

fn _set_max_voting_duration(ref self: ContractState, _max_voting_duration: u32) {
assert(_max_voting_duration >= self._min_voting_duration.read(), 'Invalid duration');
self._max_voting_duration.write(_max_voting_duration);
}

fn _set_min_voting_duration(ref self: ContractState, _min_voting_duration: u32) {
assert(_min_voting_duration <= self._max_voting_duration.read(), 'Invalid duration');
self._min_voting_duration.write(_min_voting_duration);
}

Expand All @@ -620,73 +651,71 @@ mod Space {
self._proposal_validation_strategy.write(_proposal_validation_strategy);
}

fn _add_voting_strategies(ref self: ContractState, _voting_strategies: Array<Strategy>) {
fn _add_voting_strategies(ref self: ContractState, mut _voting_strategies: Span<Strategy>) {
let mut cachedActiveVotingStrategies = self._active_voting_strategies.read();
let mut cachedNextVotingStrategyIndex = self._next_voting_strategy_index.read();
assert(
cachedNextVotingStrategyIndex.into() < 256_u32 - _voting_strategies.len(),
'Exceeds Voting Strategy Limit'
);
let mut _voting_strategies_span = _voting_strategies.span();
let mut i = 0_usize;
loop {
if i >= _voting_strategies.len() {
break ();
}

let strategy = _voting_strategies_span.pop_front().unwrap().clone();
assert(!strategy.address.is_zero(), 'Invalid voting strategy');
cachedActiveVotingStrategies.set_bit(cachedNextVotingStrategyIndex, true);
self._voting_strategies.write(cachedNextVotingStrategyIndex, strategy);
cachedNextVotingStrategyIndex += 1_u8;
i += 1;
match _voting_strategies.pop_front() {
Option::Some(strategy) => {
assert(!(*strategy.address).is_zero(), 'Invalid voting strategy');
cachedActiveVotingStrategies.set_bit(cachedNextVotingStrategyIndex, true);
self._voting_strategies.write(cachedNextVotingStrategyIndex, strategy.clone());
cachedNextVotingStrategyIndex += 1_u8;
},
Option::None => {
break;
},
};
};
self._active_voting_strategies.write(cachedActiveVotingStrategies);
self._next_voting_strategy_index.write(cachedNextVotingStrategyIndex);
}

fn _remove_voting_strategies(ref self: ContractState, _voting_strategies: Array<u8>) {
fn _remove_voting_strategies(ref self: ContractState, mut _voting_strategies: Span<u8>) {
let mut cachedActiveVotingStrategies = self._active_voting_strategies.read();
let mut _voting_strategies_span = _voting_strategies.span();
let mut i = 0_usize;
loop {
if i >= _voting_strategies.len() {
break ();
}

let index = _voting_strategies_span.pop_front().unwrap();
cachedActiveVotingStrategies.set_bit(*index, false);
i += 1;
match _voting_strategies.pop_front() {
Option::Some(index) => {
cachedActiveVotingStrategies.set_bit(*index, false);
},
Option::None => {
break;
},
};
};

if cachedActiveVotingStrategies == 0 {
panic_with_felt252('No active voting strategy left');
}
assert(cachedActiveVotingStrategies != 0, 'No active voting strategy left');

self._active_voting_strategies.write(cachedActiveVotingStrategies);
}

fn _add_authenticators(ref self: ContractState, _authenticators: Array<ContractAddress>) {
let mut _authenticators_span = _authenticators.span();
let mut i = 0_usize;
fn _add_authenticators(ref self: ContractState, mut _authenticators: Span<ContractAddress>) {
loop {
if i >= _authenticators.len() {
break ();
}
self._authenticators.write(*_authenticators_span.pop_front().unwrap(), true);
i += 1;
match _authenticators.pop_front() {
Option::Some(authenticator) => {
self._authenticators.write(*authenticator, true);
},
Option::None => {
break;
},
};
}
}

fn _remove_authenticators(ref self: ContractState, _authenticators: Array<ContractAddress>) {
let mut _authenticators_span = _authenticators.span();
let mut i = 0_usize;
fn _remove_authenticators(ref self: ContractState, mut _authenticators: Span<ContractAddress>) {
loop {
if i >= _authenticators.len() {
break ();
}
self._authenticators.write(*_authenticators_span.pop_front().unwrap(), false);
i += 1;
match _authenticators.pop_front() {
Option::Some(authenticator) => {
self._authenticators.write(*authenticator, false);
},
Option::None => {
break;
},
};
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions starknet/src/tests/test_merkle_whitelist.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ mod merkle_whitelist_voting_power {
leaf.serialize(ref user_params);
proof.serialize(ref user_params);

voting_strategy.get_voting_power(timestamp, voter, params, user_params);
voting_strategy.get_voting_power(timestamp, voter, params.span(), user_params.span());
}

#[test]
Expand Down Expand Up @@ -394,7 +394,7 @@ mod merkle_whitelist_voting_power {
fake_leaf.serialize(ref user_params);
proof.serialize(ref user_params);

voting_strategy.get_voting_power(timestamp, voter, params, user_params);
voting_strategy.get_voting_power(timestamp, voter, params.span(), user_params.span());
}

#[test]
Expand Down Expand Up @@ -431,6 +431,6 @@ mod merkle_whitelist_voting_power {
fake_leaf.serialize(ref user_params);
proof.serialize(ref user_params);

voting_strategy.get_voting_power(timestamp, voter, params, user_params);
voting_strategy.get_voting_power(timestamp, voter, params.span(), user_params.span());
}
}
Loading
Loading