lock approver in pending proposal on propose instead of using most re…

…cent approver

contracts/pre-propose/dao-pre-propose-approval-multiple/src/contract.rs

@@ -125,13 +125,16 @@ pub fn execute_propose(
                 Ok(SubMsg::new(execute_msg))
             })?;
 
+    let approver = APPROVER.load(deps.storage)?;
+
     // Save the proposal and its information as pending.
     PENDING_PROPOSALS.save(
         deps.storage,
         approval_id,
         &Proposal {
             status: ApprovalProposalStatus::Pending {},
             approval_id,
+            approver: approver.clone(),
             proposer: info.sender,
             msg: propose_msg_internal,
             deposit: config.deposit_info,
@@ -142,24 +145,24 @@ pub fn execute_propose(
         .add_messages(deposit_messages)
         .add_submessages(hooks_msgs)
         .add_attribute("method", "pre-propose")
-        .add_attribute("id", approval_id.to_string()))
+        .add_attribute("id", approval_id.to_string())
+        .add_attribute("approver", approver.to_string()))
 }
 
 pub fn execute_approve(
     deps: DepsMut,
     info: MessageInfo,
     id: u64,
 ) -> Result<Response, PreProposeError> {
-    // Check sender is the approver
-    let approver = APPROVER.load(deps.storage)?;
-    if approver != info.sender {
-        return Err(PreProposeError::Unauthorized {});
-    }
-
     // Load proposal and send propose message to the proposal module
     let proposal = PENDING_PROPOSALS.may_load(deps.storage, id)?;
     match proposal {
         Some(proposal) => {
+            // Check sender is the approver
+            if proposal.approver != info.sender {
+                return Err(PreProposeError::Unauthorized {});
+            }
+
             let proposal_module = PrePropose::default().proposal_module.load(deps.storage)?;
 
             // Snapshot the deposit for the proposal that we're about
@@ -188,6 +191,7 @@ pub fn execute_approve(
                         created_proposal_id: proposal_id,
                     },
                     approval_id: proposal.approval_id,
+                    approver: proposal.approver,
                     proposer: proposal.proposer,
                     msg: proposal.msg,
                     deposit: proposal.deposit,
@@ -211,14 +215,9 @@ pub fn execute_reject(
     info: MessageInfo,
     id: u64,
 ) -> Result<Response, PreProposeError> {
-    // Check sender is the approver
-    let approver = APPROVER.load(deps.storage)?;
-    if approver != info.sender {
-        return Err(PreProposeError::Unauthorized {});
-    }
-
     let Proposal {
         approval_id,
+        approver,
         proposer,
         msg,
         deposit,
@@ -227,12 +226,18 @@ pub fn execute_reject(
         .may_load(deps.storage, id)?
         .ok_or(PreProposeError::ProposalNotFound {})?;
 
+    // Check sender is the approver
+    if approver != info.sender {
+        return Err(PreProposeError::Unauthorized {});
+    }
+
     COMPLETED_PROPOSALS.save(
         deps.storage,
         id,
         &Proposal {
             status: ApprovalProposalStatus::Rejected {},
             approval_id,
+            approver,
             proposer: proposer.clone(),
             msg: msg.clone(),
             deposit: deposit.clone(),

contracts/pre-propose/dao-pre-propose-approval-multiple/src/tests.rs

@@ -1257,10 +1257,10 @@ fn test_approval_and_rejection_permissions() {
         &coins(10, "ujuno"),
     );
 
-    // Only approver can propose
+    // Only approver can approve
     let err: PreProposeError = app
         .execute_contract(
-            Addr::unchecked("nonmember"),
+            Addr::unchecked("nonapprover"),
             pre_propose.clone(),
             &ExecuteMsg::Extension {
                 msg: ExecuteExt::Approve { id: pre_propose_id },
@@ -1272,11 +1272,11 @@ fn test_approval_and_rejection_permissions() {
         .unwrap();
     assert_eq!(err, PreProposeError::Unauthorized {});
 
-    // Only approver can propose
+    // Only approver can reject
     let err: PreProposeError = app
         .execute_contract(
-            Addr::unchecked("nonmember"),
-            pre_propose,
+            Addr::unchecked("nonapprover"),
+            pre_propose.clone(),
             &ExecuteMsg::Extension {
                 msg: ExecuteExt::Reject { id: pre_propose_id },
             },
@@ -1286,6 +1286,94 @@ fn test_approval_and_rejection_permissions() {
         .downcast()
         .unwrap();
     assert_eq!(err, PreProposeError::Unauthorized {});
+
+    // Updating approver after proposal created does not change old proposal's
+    // approver
+    app.execute_contract(
+        Addr::unchecked("approver"),
+        pre_propose.clone(),
+        &ExecuteMsg::Extension {
+            msg: ExecuteExt::UpdateApprover {
+                address: "newapprover".to_string(),
+            },
+        },
+        &[],
+    )
+    .unwrap();
+
+    let err: PreProposeError = app
+        .execute_contract(
+            Addr::unchecked("newapprover"),
+            pre_propose.clone(),
+            &ExecuteMsg::Extension {
+                msg: ExecuteExt::Approve { id: pre_propose_id },
+            },
+            &[],
+        )
+        .unwrap_err()
+        .downcast()
+        .unwrap();
+    assert_eq!(err, PreProposeError::Unauthorized {});
+
+    // Old approver can still approve.
+    app.execute_contract(
+        Addr::unchecked("approver"),
+        pre_propose.clone(),
+        &ExecuteMsg::Extension {
+            msg: ExecuteExt::Approve { id: pre_propose_id },
+        },
+        &[],
+    )
+    .unwrap();
+
+    // Non-member proposes.
+    mint_natives(&mut app, "nonmember", coins(10, "ujuno"));
+    let pre_propose_id = make_pre_proposal(
+        &mut app,
+        pre_propose.clone(),
+        "nonmember",
+        &coins(10, "ujuno"),
+    );
+
+    // Old approver cannot approve nor reject.
+    let err: PreProposeError = app
+        .execute_contract(
+            Addr::unchecked("approver"),
+            pre_propose.clone(),
+            &ExecuteMsg::Extension {
+                msg: ExecuteExt::Approve { id: pre_propose_id },
+            },
+            &[],
+        )
+        .unwrap_err()
+        .downcast()
+        .unwrap();
+    assert_eq!(err, PreProposeError::Unauthorized {});
+
+    let err: PreProposeError = app
+        .execute_contract(
+            Addr::unchecked("approver"),
+            pre_propose.clone(),
+            &ExecuteMsg::Extension {
+                msg: ExecuteExt::Reject { id: pre_propose_id },
+            },
+            &[],
+        )
+        .unwrap_err()
+        .downcast()
+        .unwrap();
+    assert_eq!(err, PreProposeError::Unauthorized {});
+
+    // New approver can now approve.
+    app.execute_contract(
+        Addr::unchecked("newapprover"),
+        pre_propose.clone(),
+        &ExecuteMsg::Extension {
+            msg: ExecuteExt::Approve { id: pre_propose_id },
+        },
+        &[],
+    )
+    .unwrap();
 }
 
 #[test]

contracts/pre-propose/dao-pre-propose-approval-single/Cargo.toml

@@ -1,6 +1,9 @@
 [package]
 name = "dao-pre-propose-approval-single"
-authors = ["ekez <[email protected]>", "Jake Hartnell <[email protected]>"]
+authors = [
+  "ekez <[email protected]>",
+  "Jake Hartnell <[email protected]>",
+]
 description = "A DAO DAO pre-propose module handling a proposal approval flow for for dao-proposal-single."
 edition = { workspace = true }
 license = { workspace = true }
@@ -21,14 +24,14 @@ cosmwasm-std = { workspace = true }
 cosmwasm-schema = { workspace = true }
 cw-storage-plus = { workspace = true }
 cw2 = { workspace = true }
+cw-denom = { workspace = true }
 cw-paginate-storage = { workspace = true }
 dao-pre-propose-base = { workspace = true }
 dao-voting = { workspace = true }
 thiserror = { workspace = true }
 dao-interface = { workspace = true }
 
 [dev-dependencies]
-cw-denom = { workspace = true }
 cw-multi-test = { workspace = true }
 cw-utils = { workspace = true }
 cw4 = { workspace = true }
@@ -47,6 +50,6 @@ dao-proposal-single = { workspace = true }
 dao-dao-core-v241 = { workspace = true }
 dao-interface-v241 = { workspace = true }
 dao-pre-propose-approval-single-v241 = { workspace = true }
-dao-proposal-single-v241  = { workspace = true }
+dao-proposal-single-v241 = { workspace = true }
 dao-voting-cw4-v241 = { workspace = true }
 dao-voting-v241 = { workspace = true }