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

feat: prevent zero address for author/voter #492

Merged
merged 13 commits into from
Aug 23, 2023
Merged

feat: prevent zero address for author/voter #492

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5d43dc2
revert if autauthor/voter is zero address
Aug 16, 2023
f81216e
chore: test propose with zero starknet and ethereum address
Aug 16, 2023
ec78c93
chore: test propose with zero custom address
Aug 16, 2023
869f831
chore: UserAddress zeroable tests
Aug 18, 2023
01be3cc
chore: zero address untegration tests vote and update proposal
Aug 18, 2023
0011186
chore: updated scarb version to 0.6.2 in CI
Aug 18, 2023
0c193f0
chore: attempt fix CI
Aug 18, 2023
3b03169
Merge branch 'develop' into feat_prevent_zero_address
Orland0x Aug 18, 2023
2e817ea
chore: formatting
Aug 18, 2023
af25e96
chore: fix CI 2
Aug 18, 2023
95e010c
chore: formatting
Aug 18, 2023
38c0c10
chore: fixed space test
Aug 18, 2023
21e8f87
chore: add code coverage for zero/non_zero
pscott Aug 21, 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
3 changes: 3 additions & 0 deletions starknet/src/space/space.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ mod Space {
metadata_URI: Array<felt252>,
pscott marked this conversation as resolved.
Show resolved Hide resolved
) {
assert_only_authenticator(@self);
assert(author.is_non_zero(), 'Zero Address');
let proposal_id = self._next_proposal_id.read();

// Proposal Validation
Expand Down Expand Up @@ -246,6 +247,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);

Expand Down Expand Up @@ -311,6 +313,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');
Expand Down
113 changes: 112 additions & 1 deletion starknet/src/tests/test_space.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
mod tests {
use array::ArrayTrait;
use starknet::{
ContractAddress, syscalls::deploy_syscall, testing, contract_address_const, info
ContractAddress, EthAddress, syscalls::deploy_syscall, testing, contract_address_const, info
};
use traits::{Into, TryInto};
use result::ResultTrait;
Expand Down Expand Up @@ -333,4 +333,115 @@ 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_from_zero_starknet_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_propose_from_zero_eth_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::Ethereum(EthAddress { address: 0 });
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_propose_from_zero_custom_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::Custom(0_u256);
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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Erm I know you wrote a lot of the code but I would suggest to do these tests:

write a test module that only does a unit test on is_zero (could actually be inside of user_address.cairo tbh but doesn't have to).
In this unit test we try starknet, ethereum, and custom).

And then we actually test the three entrypoints (to get full code coverage and regression test), on cancel, propose, update_proposal, and vote. This time we don't care about whether they're starknet or ethereum, we just check to see if the code is here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep got the unit tests and all entry points tested. sorry was being a bit lazy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, perfect

}
17 changes: 17 additions & 0 deletions starknet/src/types/user_address.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -57,3 +58,19 @@ 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()
}
}