Skip to content

Commit

Permalink
Fixed vetoer bypassing only_members_execute flag. (#780)
Browse files Browse the repository at this point in the history
  • Loading branch information
@NoahSaso
NoahSaso committed Dec 13, 2023
1 parent 0ac5aba commit a3a72db
Show file tree
Hide file tree
Showing 4 changed files with 272 additions and 48 deletions.
46 changes: 22 additions & 24 deletions contracts/proposal/dao-proposal-multiple/src/contract.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,9 @@ pub fn execute_execute(
.ok_or(ContractError::NoSuchProposal { id: proposal_id })?;

let config = CONFIG.load(deps.storage)?;

// determine if this sender can execute
let mut sender_can_execute = true;
if config.only_members_execute {
let power = get_voting_power(
deps.as_ref(),
Expand All @@ -461,16 +464,7 @@ pub fn execute_execute(
Some(prop.start_height),
)?;

// if there is no veto config, then caller is not the vetoer
// if there is, we validate the caller addr
let vetoer_call = prop
.veto
.as_ref()
.map_or(false, |veto_config| veto_config.vetoer == info.sender);

if power.is_zero() && !vetoer_call {
return Err(ContractError::Unauthorized {});
}
sender_can_execute = !power.is_zero();
}

// Check here that the proposal is passed or timelocked.
Expand All @@ -481,27 +475,31 @@ pub fn execute_execute(
prop.update_status(&env.block)?;
let old_status = prop.status;
match &prop.status {
Status::Passed => (),
Status::VetoTimelock { expiration } => {
Status::Passed => {
// if passed, verify sender can execute
if !sender_can_execute {
return Err(ContractError::Unauthorized {});
}
}
Status::VetoTimelock { .. } => {
let veto_config = prop
.veto
.as_ref()
.ok_or(VetoError::NoVetoConfiguration {})?;

// Check if the sender is the vetoer
match veto_config.vetoer == info.sender {
// if sender is the vetoer we validate the early exec flag
true => veto_config.check_early_execute_enabled()?,
// otherwise timelock must be expired in order to execute
false => {
// it should never be expired here since the status updates
// to passed after the timelock expires, but let's check
// anyway. i.e. this error should always be returned.
if !expiration.is_expired(&env.block) {
return Err(ContractError::VetoError(VetoError::Timelocked {}));
}
// check that the sender is the vetoer
if veto_config.vetoer != info.sender {
// if the sender can normally execute, but is not the vetoer,
// return timelocked error. otherwise return unauthorized.
if sender_can_execute {
return Err(ContractError::VetoError(VetoError::Timelocked {}));
} else {
return Err(ContractError::Unauthorized {});
}
}

// if veto timelocked, only allow execution if early_execute enabled
veto_config.check_early_execute_enabled()?;
}
_ => {
return Err(ContractError::NotPassed {});
Expand Down
128 changes: 128 additions & 0 deletions contracts/proposal/dao-proposal-multiple/src/testing/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5481,3 +5481,131 @@ fn test_veto_timelock_expires_happy() -> anyhow::Result<()> {

Ok(())
}

#[test]
fn test_veto_only_members_execute_proposal() -> anyhow::Result<()> {
let mut app = App::default();
let timelock_duration = Duration::Height(3);
let veto_config = VetoConfig {
timelock_duration,
vetoer: "vetoer".to_string(),
early_execute: true,
veto_before_passed: false,
};

let core_addr = instantiate_with_staked_balances_governance(
&mut app,
InstantiateMsg {
min_voting_period: None,
max_voting_period: Duration::Height(6),
only_members_execute: true,
allow_revoting: false,
voting_strategy: VotingStrategy::SingleChoice {
quorum: PercentageThreshold::Majority {},
},
close_proposal_on_execution_failure: false,
pre_propose_info: PreProposeInfo::AnyoneMayPropose {},
veto: Some(veto_config),
},
Some(vec![
Cw20Coin {
address: "a-1".to_string(),
amount: Uint128::new(110_000_000),
},
Cw20Coin {
address: "a-2".to_string(),
amount: Uint128::new(100_000_000),
},
]),
);
let govmod = query_multiple_proposal_module(&app, &core_addr);

let proposal_module = query_multiple_proposal_module(&app, &core_addr);

let next_proposal_id: u64 = app
.wrap()
.query_wasm_smart(&proposal_module, &QueryMsg::NextProposalId {})
.unwrap();
assert_eq!(next_proposal_id, 1);

let options = vec![
MultipleChoiceOption {
description: "multiple choice option 1".to_string(),
msgs: vec![],
title: "title".to_string(),
},
MultipleChoiceOption {
description: "multiple choice option 2".to_string(),
msgs: vec![],
title: "title".to_string(),
},
];
let mc_options = MultipleChoiceOptions { options };

// Create a basic proposal with 2 options
app.execute_contract(
Addr::unchecked("a-1"),
proposal_module.clone(),
&ExecuteMsg::Propose {
title: "A simple text proposal".to_string(),
description: "A simple text proposal".to_string(),
choices: mc_options,
proposer: None,
},
&[],
)
.unwrap();

app.execute_contract(
Addr::unchecked("a-1"),
proposal_module.clone(),
&ExecuteMsg::Vote {
proposal_id: 1,
vote: MultipleChoiceVote { option_id: 0 },
rationale: None,
},
&[],
)
.unwrap();

let proposal: ProposalResponse = query_proposal(&app, &govmod, 1);

let expiration = proposal.proposal.expiration.add(timelock_duration)?;
assert_eq!(
proposal.proposal.status,
Status::VetoTimelock { expiration },
);

app.update_block(|b| b.height += 10);
// assert timelock is expired
assert!(expiration.is_expired(&app.block_info()));

let proposal: ProposalResponse = query_proposal(&app, &govmod, 1);
assert_eq!(proposal.proposal.status, Status::Passed);

// Proposal cannot be executed by vetoer once timelock expired
let err: ContractError = app
.execute_contract(
Addr::unchecked("vetoer"),
proposal_module.clone(),
&ExecuteMsg::Execute { proposal_id: 1 },
&[],
)
.unwrap_err()
.downcast()
.unwrap();
assert_eq!(err, ContractError::Unauthorized {});

// Proposal can be executed by member once timelock expired
app.execute_contract(
Addr::unchecked("a-2"),
proposal_module.clone(),
&ExecuteMsg::Execute { proposal_id: 1 },
&[],
)
.unwrap();
let proposal: ProposalResponse = query_proposal(&app, &govmod, 1);
assert_eq!(proposal.proposal.status, Status::Executed {},);

Ok(())
}
47 changes: 23 additions & 24 deletions contracts/proposal/dao-proposal-single/src/contract.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,9 @@ pub fn execute_execute(
.ok_or(ContractError::NoSuchProposal { id: proposal_id })?;

let config = CONFIG.load(deps.storage)?;

// determine if this sender can execute
let mut sender_can_execute = true;
if config.only_members_execute {
let power = get_voting_power(
deps.as_ref(),
Expand All @@ -354,16 +357,7 @@ pub fn execute_execute(
Some(prop.start_height),
)?;

// if there is no veto config, then caller is not the vetoer
// if there is, we validate the caller addr
let vetoer_call = prop
.veto
.as_ref()
.map_or(false, |veto_config| veto_config.vetoer == info.sender);

if power.is_zero() && !vetoer_call {
return Err(ContractError::Unauthorized {});
}
sender_can_execute = !power.is_zero();
}

// Check here that the proposal is passed or timelocked.
Expand All @@ -374,27 +368,32 @@ pub fn execute_execute(
prop.update_status(&env.block)?;
let old_status = prop.status;
match &prop.status {
Status::Passed => (),
Status::VetoTimelock { expiration } => {
Status::Passed => {
// if passed, verify sender can execute
if !sender_can_execute {
return Err(ContractError::Unauthorized {});
}
}
Status::VetoTimelock { .. } => {
// should never error if in veto timelock state
let veto_config = prop
.veto
.as_ref()
.ok_or(VetoError::NoVetoConfiguration {})?;

// Check if the sender is the vetoer
match veto_config.vetoer == info.sender {
// if sender is the vetoer we validate the early exec flag
true => veto_config.check_early_execute_enabled()?,
// otherwise timelock must be expired in order to execute
false => {
// it should never be expired here since the status updates
// to passed after the timelock expires, but let's check
// anyway. i.e. this error should always be returned.
if !expiration.is_expired(&env.block) {
return Err(ContractError::VetoError(VetoError::Timelocked {}));
}
// check that the sender is the vetoer
if veto_config.vetoer != info.sender {
// if the sender can normally execute, but is not the vetoer,
// return timelocked error. otherwise return unauthorized.
if sender_can_execute {
return Err(ContractError::VetoError(VetoError::Timelocked {}));
} else {
return Err(ContractError::Unauthorized {});
}
}

// if veto timelocked, only allow execution if early_execute enabled
veto_config.check_early_execute_enabled()?;
}
_ => {
return Err(ContractError::NotPassed {});
Expand Down
99 changes: 99 additions & 0 deletions contracts/proposal/dao-proposal-single/src/testing/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1643,6 +1643,105 @@ fn test_proposal_message_timelock_veto_before_passed() {
// assert_eq!(proposal.proposal.status, Status::Executed);
}

#[test]
fn test_veto_only_members_execute_proposal() -> anyhow::Result<()> {
let mut app = App::default();
let mut instantiate = get_default_token_dao_proposal_module_instantiate(&mut app);
instantiate.close_proposal_on_execution_failure = false;
let veto_config = VetoConfig {
timelock_duration: Duration::Time(100),
vetoer: "oversight".to_string(),
early_execute: true,
veto_before_passed: false,
};
instantiate.veto = Some(veto_config.clone());
let core_addr = instantiate_with_staked_balances_governance(
&mut app,
instantiate,
Some(vec![Cw20Coin {
address: CREATOR_ADDR.to_string(),
amount: Uint128::new(85),
}]),
);
let proposal_module = query_single_proposal_module(&app, &core_addr);
let gov_token = query_dao_token(&app, &core_addr);

mint_cw20s(&mut app, &gov_token, &core_addr, CREATOR_ADDR, 10_000_000);
let proposal_id = make_proposal(
&mut app,
&proposal_module,
CREATOR_ADDR,
vec![
WasmMsg::Execute {
contract_addr: gov_token.to_string(),
msg: to_json_binary(&cw20::Cw20ExecuteMsg::Mint {
recipient: CREATOR_ADDR.to_string(),
amount: Uint128::new(10_000_000),
})
.unwrap(),
funds: vec![],
}
.into(),
BankMsg::Send {
to_address: CREATOR_ADDR.to_string(),
amount: coins(10, "ujuno"),
}
.into(),
],
);
let cw20_balance = query_balance_cw20(&app, &gov_token, CREATOR_ADDR);
let native_balance = query_balance_native(&app, CREATOR_ADDR, "ujuno");
assert_eq!(cw20_balance, Uint128::zero());
assert_eq!(native_balance, Uint128::zero());

vote_on_proposal(
&mut app,
&proposal_module,
CREATOR_ADDR,
proposal_id,
Vote::Yes,
);
let proposal = query_proposal(&app, &proposal_module, proposal_id);

// Proposal is timelocked to the moment of prop expiring + timelock delay
let expiration = proposal
.proposal
.expiration
.add(veto_config.timelock_duration)?;
assert_eq!(
proposal.proposal.status,
Status::VetoTimelock { expiration }
);

app.update_block(|b| b.time = b.time.plus_seconds(604800 + 101));
// assert timelock is expired
assert!(expiration.is_expired(&app.block_info()));
mint_natives(&mut app, core_addr.as_str(), coins(10, "ujuno"));

let proposal = query_proposal(&app, &proposal_module, proposal_id);
assert_eq!(proposal.proposal.status, Status::Passed);

// Proposal cannot be executed by vetoer once timelock expired
let err: ContractError = app
.execute_contract(
Addr::unchecked("oversight"),
proposal_module.clone(),
&ExecuteMsg::Execute { proposal_id },
&[],
)
.unwrap_err()
.downcast()
.unwrap();
assert_eq!(err, ContractError::Unauthorized {});

// Proposal can be executed by member once timelock expired
execute_proposal(&mut app, &proposal_module, CREATOR_ADDR, proposal_id);
let proposal = query_proposal(&app, &proposal_module, proposal_id);
assert_eq!(proposal.proposal.status, Status::Executed);

Ok(())
}

#[test]
fn test_proposal_close_after_expiry() {
let CommonTest {
Expand Down

0 comments on commit a3a72db

Please sign in to comment.