Fix vetoer bypassing only_members_execute flag

contracts/proposal/dao-proposal-multiple/src/contract.rs

@@ -453,6 +453,9 @@
         .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(),
@@ -461,16 +464,7 @@
             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.
@@ -481,27 +475,31 @@
     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 {});

contracts/proposal/dao-proposal-multiple/src/testing/tests.rs

@@ -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(())
+}

contracts/proposal/dao-proposal-single/src/contract.rs

@@ -346,6 +346,9 @@
         .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(),
@@ -354,16 +357,7 @@
             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.
@@ -374,27 +368,32 @@
     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 {});

contracts/proposal/dao-proposal-single/src/testing/tests.rs

@@ -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 {