Repository: DA0-DA0/dao-contracts
Pull request #287 (2022-05-10 18:24:05)

Prevent multiple executions of proposals and closing of passed proposals.
This upstreams two important security fixes for our single choice proposals module. These changes have already been applied for all the DAOs currently using these contracts.


Patch:
diff --git a/contracts/cw-proposal-single/src/contract.rs b/contracts/cw-proposal-single/src/contract.rs
index 797d2239..5817c762 100644
--- a/contracts/cw-proposal-single/src/contract.rs
+++ b/contracts/cw-proposal-single/src/contract.rs
@@ -235,6 +235,17 @@ pub fn execute_execute(
     if !prop.is_passed(&env.block) {
         return Err(ContractError::NotPassed {});
     }
+
+    // Can not execute a proposal more than once.
+    if prop.status == Status::Executed {
+        return Err(ContractError::AlreadyExecuted {});
+    }
+
+    // Can not execute a closed proposal.
+    if prop.status == Status::Closed {
+        return Err(ContractError::Closed {});
+    }
+
     prop.status = Status::Executed;
     PROPOSALS.save(deps.storage, proposal_id, &prop)?;
 
@@ -345,8 +356,8 @@ pub fn execute_close(
     match prop.status {
         Status::Rejected => (),
         Status::Open => {
-            if !prop.expiration.is_expired(&env.block) {
-                return Err(ContractError::NotExpired {});
+            if !prop.expiration.is_expired(&env.block) || prop.is_passed(&env.block) {
+                return Err(ContractError::WrongCloseStatus {});
             }
         }
         _ => return Err(ContractError::WrongCloseStatus {}),
diff --git a/contracts/cw-proposal-single/src/error.rs b/contracts/cw-proposal-single/src/error.rs
index e70de7e2..88e07c6f 100644
--- a/contracts/cw-proposal-single/src/error.rs
+++ b/contracts/cw-proposal-single/src/error.rs
@@ -42,8 +42,11 @@ pub enum ContractError {
     #[error("Proposal is not passed.")]
     NotPassed {},
 
-    #[error("Proposal is not expired.")]
-    NotExpired {},
+    #[error("Proposal has already been executed.")]
+    AlreadyExecuted {},
+
+    #[error("Proposal is closed.")]
+    Closed {},
 
     #[error("Only rejected or expired proposals may be closed.")]
     WrongCloseStatus {},


Test patch:
diff --git a/contracts/cw-proposal-single/src/tests.rs b/contracts/cw-proposal-single/src/tests.rs
index 7075f4a0..59228bd6 100644
--- a/contracts/cw-proposal-single/src/tests.rs
+++ b/contracts/cw-proposal-single/src/tests.rs
@@ -1442,6 +1442,16 @@ fn test_execute_expired_proposal() {
         .unwrap();
     assert_eq!(proposal.proposal.status, Status::Passed);
 
+    // Try to close the proposal. This should fail as the proposal is
+    // passed.
+    app.execute_contract(
+        Addr::unchecked("ekez"),
+        proposal_single.clone(),
+        &ExecuteMsg::Close { proposal_id: 1 },
+        &[],
+    )
+    .unwrap_err();
+
     // Check that we can execute the proposal despite the fact that it
     // is technically expired.
     app.execute_contract(
@@ -1451,6 +1461,16 @@ fn test_execute_expired_proposal() {
         &[],
     )
     .unwrap();
+
+    // Can't execute more than once.
+    app.execute_contract(
+        Addr::unchecked("ekez"),
+        proposal_single.clone(),
+        &ExecuteMsg::Execute { proposal_id: 1 },
+        &[],
+    )
+    .unwrap_err();
+
     let proposal: ProposalResponse = app
         .wrap()
         .query_wasm_smart(proposal_single, &QueryMsg::Proposal { proposal_id: 1 })