feat: add governance fee withdraw cancel race test

contracts/src/governance.rs

@@ -1,10 +1,14 @@
 // SPDX-License-Identifier: MIT
 //! Dual-Approval Governance Mechanism for Critical Administrative Actions (Issue #272).
 
-use crate::admin::{_require_supported_schema, _set_mode};
-use crate::common::{_emit_action_rejected, _extend_persistent_ttl, DEFAULT_GOV_PROPOSAL_TTL_LEDGERS};
+use crate::admin::{_ensure_not_paused, _require_supported_schema, _set_mode};
+use crate::common::{
+    _emit_action_rejected, _extend_persistent_ttl, DEFAULT_GOV_PROPOSAL_TTL_LEDGERS,
+};
 use crate::errors::ContractError;
-use crate::types::{DataKeyCore, DataKeyScoped, GovAction, GovProposal, GovProposalStatus, RuntimeMode};
+use crate::types::{
+    DataKeyCore, DataKeyScoped, GovAction, GovProposal, GovProposalStatus, RuntimeMode,
+};
 use soroban_sdk::{symbol_short, Address, Env};
 
 /// Returns whether `user` is an authorized governance administrator or approver.
@@ -154,7 +158,12 @@ pub fn propose(
     #[allow(deprecated)]
     env.events().publish(
         (symbol_short!("gov"), symbol_short!("proposed")),
-        (proposal_id, proposer, _action_code(&action), expires_at_ledger),
+        (
+            proposal_id,
+            proposer,
+            _action_code(&action),
+            expires_at_ledger,
+        ),
     );
 
     Ok(proposal_id)
@@ -272,6 +281,27 @@ pub fn execute(env: Env, executor: Address, proposal_id: u64) -> Result<(), Cont
         GovProposalStatus::Approved => {}
     }
 
+    // Pause rules must be respected on execute. Emergency pause/unpause must
+    // remain callable while the protocol is fully paused, but every other
+    // protected action is gated by the same policy that applies to the direct
+    // admin entrypoints (AdminConfig is blocked by `FullyPaused`).
+    if !matches!(
+        proposal.action,
+        GovAction::PauseProtocol | GovAction::UnpauseProtocol
+    ) {
+        _ensure_not_paused(&env).inspect_err(|&e| {
+            _emit_action_rejected(&env, &executor, symbol_short!("execute"), e);
+        })?;
+    }
+
+    // CEI: mark the proposal Executed before performing any external
+    // interaction so a racing/reentrant caller can never double-execute (or
+    // cancel) a proposal whose effects have begun. On a failed action Soroban
+    // reverts this write atomically, so it cannot burn spent approvals.
+    proposal.status = GovProposalStatus::Executed;
+    env.storage().persistent().set(&p_key, &proposal);
+    _extend_persistent_ttl(&env, &p_key);
+
     // Execute the action payload
     match &proposal.action {
         GovAction::PauseProtocol => {
@@ -294,23 +324,25 @@ pub fn execute(env: Env, executor: Address, proposal_id: u64) -> Result<(), Cont
             _execute_withdraw_fee(&env, &recipient, *amount)?;
         }
         GovAction::SetTreasuryAddress(treasury) => {
-            env.storage().persistent().set(&DataKeyCore::ProtocolFeeTreasury, &treasury);
+            env.storage()
+                .persistent()
+                .set(&DataKeyCore::ProtocolFeeTreasury, &treasury);
             _extend_persistent_ttl(&env, &DataKeyCore::ProtocolFeeTreasury);
         }
         GovAction::SetAdmin(new_admin) => {
-            env.storage().persistent().set(&DataKeyCore::Admin, &new_admin);
+            env.storage()
+                .persistent()
+                .set(&DataKeyCore::Admin, &new_admin);
             _extend_persistent_ttl(&env, &DataKeyCore::Admin);
         }
         GovAction::SetOracle(new_oracle) => {
-            env.storage().persistent().set(&DataKeyCore::Oracle, &new_oracle);
+            env.storage()
+                .persistent()
+                .set(&DataKeyCore::Oracle, &new_oracle);
             _extend_persistent_ttl(&env, &DataKeyCore::Oracle);
         }
     }
 
-    proposal.status = GovProposalStatus::Executed;
-    env.storage().persistent().set(&p_key, &proposal);
-    _extend_persistent_ttl(&env, &p_key);
-
     #[allow(deprecated)]
     env.events().publish(
         (symbol_short!("gov"), symbol_short!("executed")),
@@ -401,7 +433,8 @@ pub fn get_gov_proposal(env: Env, proposal_id: u64) -> Option<GovProposal> {
     let mut proposal: GovProposal = env.storage().persistent().get(&p_key)?;
 
     let current_ledger = env.ledger().sequence();
-    if (proposal.status == GovProposalStatus::Pending || proposal.status == GovProposalStatus::Approved)
+    if (proposal.status == GovProposalStatus::Pending
+        || proposal.status == GovProposalStatus::Approved)
         && current_ledger > proposal.expires_at_ledger
     {
         proposal.status = GovProposalStatus::Expired;

contracts/src/tests/governance.rs

@@ -3,7 +3,7 @@
 
 use crate::contract::{VirtualTokenContract, VirtualTokenContractClient};
 use crate::errors::ContractError;
-use crate::types::{GovAction, GovProposalStatus};
+use crate::types::{DataKey, GovAction, GovProposalStatus};
 use soroban_sdk::{
     testutils::{Address as _, Events as _, Ledger as _},
     Address, Env,
@@ -230,3 +230,165 @@ fn test_audit_event_emission() {
 
     assert!(gov_events.len() >= 3);
 }
+
+fn seed_protocol_fee_treasury(
+    env: &Env,
+    client: &VirtualTokenContractClient<'static>,
+    amount: i128,
+) {
+    env.as_contract(&client.address, || {
+        env.storage()
+            .persistent()
+            .set(&DataKey::ProtocolFeeTreasury, &amount);
+    });
+}
+
+#[test]
+fn test_double_execute_impossible() {
+    let (_env, client, admin, approver, _oracle, _user) = setup_governance_env();
+
+    let pid = client.propose_gov_action(&admin, &GovAction::PauseProtocol, &None);
+    client.approve_gov_proposal(&approver, &pid);
+    client.execute_gov_proposal(&admin, &pid);
+
+    let prop = client.get_gov_proposal(&pid).expect("proposal must exist");
+    assert_eq!(prop.status, GovProposalStatus::Executed);
+
+    // A second execute of the same (already executed) proposal must be impossible.
+    let res = client.try_execute_gov_proposal(&approver, &pid);
+    assert_eq!(res, Err(Ok(ContractError::GovInvalidState)));
+}
+
+#[test]
+fn test_cancel_after_approve_blocks_execute_and_second_approve() {
+    let (_env, client, admin, approver, _oracle, _user) = setup_governance_env();
+
+    let pid = client.propose_gov_action(&admin, &GovAction::PauseProtocol, &None);
+    client.approve_gov_proposal(&approver, &pid);
+
+    // A proposal that is already approved can still be cancelled before execution.
+    client.cancel_gov_proposal(&admin, &pid);
+
+    let prop = client.get_gov_proposal(&pid).expect("proposal must exist");
+    assert_eq!(prop.status, GovProposalStatus::Cancelled);
+
+    // Once cancelled it can no longer be executed...
+    let exec_res = client.try_execute_gov_proposal(&admin, &pid);
+    assert_eq!(exec_res, Err(Ok(ContractError::GovInvalidState)));
+
+    // ...nor re-approved.
+    let app_res = client.try_approve_gov_proposal(&approver, &pid);
+    assert_eq!(app_res, Err(Ok(ContractError::GovInvalidState)));
+
+    // The pause side-effect must not have been applied.
+    assert!(!client.is_paused());
+}
+
+#[test]
+fn test_expired_proposal_rejected_on_execute() {
+    let (env, client, admin, approver, _oracle, _user) = setup_governance_env();
+
+    let pid = client.propose_gov_action(&admin, &GovAction::PauseProtocol, &Some(10));
+    client.approve_gov_proposal(&approver, &pid);
+
+    let initial_ledger = env.ledger().sequence();
+    env.ledger().with_mut(|li| {
+        li.sequence_number = initial_ledger + 15;
+    });
+
+    // Execution after the expiry ledger must be rejected.
+    let exec_res = client.try_execute_gov_proposal(&admin, &pid);
+    assert_eq!(exec_res, Err(Ok(ContractError::ProposalExpired)));
+
+    // The pause side-effect must not have been applied.
+    assert!(!client.is_paused());
+}
+
+#[test]
+fn test_pause_rules_respected_on_execute() {
+    let (_env, client, admin, approver, _oracle, recipient) = setup_governance_env();
+
+    // Bring the protocol into a fully-paused state via governance.
+    let pause_pid = client.propose_gov_action(&admin, &GovAction::PauseProtocol, &None);
+    client.approve_gov_proposal(&approver, &pause_pid);
+    client.execute_gov_proposal(&admin, &pause_pid);
+    assert!(client.is_paused());
+
+    // Fee withdrawal is an AdminConfig-class action and must be blocked while paused.
+    let withdraw_pid = client.propose_gov_action(
+        &admin,
+        &GovAction::WithdrawProtocolFee(recipient.clone(), 100),
+        &None,
+    );
+    client.approve_gov_proposal(&approver, &withdraw_pid);
+    let exec_res = client.try_execute_gov_proposal(&admin, &withdraw_pid);
+    assert_eq!(exec_res, Err(Ok(ContractError::ContractPaused)));
+
+    // Config mutation is likewise blocked while paused.
+    let fee_pid =
+        client.propose_gov_action(&admin, &GovAction::SetProtocolFeeBps(Some(100)), &None);
+    client.approve_gov_proposal(&approver, &fee_pid);
+    let exec_res = client.try_execute_gov_proposal(&admin, &fee_pid);
+    assert_eq!(exec_res, Err(Ok(ContractError::ContractPaused)));
+
+    // Unpause remains executable while fully paused (emergency recovery path).
+    let unpause_pid = client.propose_gov_action(&admin, &GovAction::UnpauseProtocol, &None);
+    client.approve_gov_proposal(&approver, &unpause_pid);
+    client.execute_gov_proposal(&admin, &unpause_pid);
+    assert!(!client.is_paused());
+}
+
+#[test]
+fn test_pause_proposal_remains_executable_while_paused() {
+    let (_env, client, admin, approver, _oracle, _user) = setup_governance_env();
+
+    let pause_pid = client.propose_gov_action(&admin, &GovAction::PauseProtocol, &None);
+    client.approve_gov_proposal(&approver, &pause_pid);
+    client.execute_gov_proposal(&admin, &pause_pid);
+    assert!(client.is_paused());
+
+    // Pausing an already-paused protocol is idempotent and still allowed.
+    let pause_again = client.propose_gov_action(&admin, &GovAction::PauseProtocol, &None);
+    client.approve_gov_proposal(&approver, &pause_again);
+    client.execute_gov_proposal(&admin, &pause_again);
+    assert!(client.is_paused());
+}
+
+#[test]
+fn test_governance_fee_withdraw_success_applies_cei() {
+    let (env, client, admin, approver, _oracle, recipient) = setup_governance_env();
+
+    seed_protocol_fee_treasury(&env, &client, 5_000_0000000i128);
+
+    let before_treasury = client.get_protocol_fee_treasury();
+    let before_balance = client.balance(&recipient);
+    assert!(before_treasury >= 1_000_0000000i128);
+
+    let pid = client.propose_gov_action(
+        &admin,
+        &GovAction::WithdrawProtocolFee(recipient.clone(), 1_000_0000000i128),
+        &None,
+    );
+    client.approve_gov_proposal(&approver, &pid);
+    client.execute_gov_proposal(&admin, &pid);
+
+    // Treasury debited and recipient credited exactly once.
+    assert_eq!(
+        client.get_protocol_fee_treasury(),
+        before_treasury - 1_000_0000000i128
+    );
+    assert_eq!(
+        client.balance(&recipient),
+        before_balance + 1_000_0000000i128
+    );
+
+    // The proposal is terminal — it cannot be executed a second time to drain again.
+    let prop = client.get_gov_proposal(&pid).expect("proposal must exist");
+    assert_eq!(prop.status, GovProposalStatus::Executed);
+    let res = client.try_execute_gov_proposal(&admin, &pid);
+    assert_eq!(res, Err(Ok(ContractError::GovInvalidState)));
+    assert_eq!(
+        client.get_protocol_fee_treasury(),
+        before_treasury - 1_000_0000000i128
+    );
+}

contracts/src/tests/mod.rs

@@ -19,6 +19,7 @@ mod edge_cases;
 mod drill;
 mod event_coverage;
 mod fee_model;
+mod governance;
 mod guard_tests;
 // mod initialization; // upstream bug
 mod invariant_harness;