Merge pull request #28 from script3/audit

Fixes from first round of auditor feedback

Cargo.toml

@@ -26,7 +26,7 @@ lto = true
 version = "20.5.0"
 
 [workspace.dependencies.sep-41-token]
-version = "0.3.0"
+version = "1.0.0"
 
 [workspace.dependencies.soroban-fixed-point-math]
 version = "1.0.0"

contracts/governor/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "soroban-governor"
-version = "1.0.0"
+version = "1.1.0"
 authors = ["Script3 Ltd. <[email protected]>"]
 license = "MIT"
 edition = "2021"

contracts/governor/src/constants.rs

@@ -1,4 +1,19 @@
-pub(crate) const ONE_DAY_LEDGERS: u32 = 17280; // assumes 5s a ledger
-pub(crate) const MAX_PROPOSAL_LIFETIME: u32 = 31 * ONE_DAY_LEDGERS; // 31 days
-pub(crate) const MAX_VOTE_PERIOD: u32 = 7 * ONE_DAY_LEDGERS; // 7 days
-pub(crate) const BPS_SCALAR: i128 = 10_000;
+/// One day assuming 5s a ledger
+pub(crate) const ONE_DAY_LEDGERS: u32 = 17280;
+/// One hour assuming 5s a ledger
+pub(crate) const ONE_HOUR_LEDGERS: u32 = 720;
+/// 1 in basis points
+pub(crate) const BPS_SCALAR: u32 = 10_000;
+
+/// The maximum number of ledgers a proposal can exist for (31 days)
+pub(crate) const MAX_PROPOSAL_LIFETIME: u32 = 31 * ONE_DAY_LEDGERS;
+/// The maximum number of ledgers a proposal can be voted on for (7 days)
+pub(crate) const MAX_VOTE_PERIOD: u32 = 7 * ONE_DAY_LEDGERS;
+/// The minimum number of ledgers a proposal can be voted on for
+pub(crate) const MIN_VOTE_PERIOD: u32 = ONE_HOUR_LEDGERS;
+/// The maximum number of ledgers a proposal has between state changes before expiration
+pub(crate) const MAX_GRACE_PERIOD: u32 = 7 * ONE_DAY_LEDGERS;
+/// The minimum number of ledgers a proposal has between state changes before expiration
+pub(crate) const MIN_GRACE_PERIOD: u32 = ONE_DAY_LEDGERS;
+/// The minimum number of tokens required to create a proposal
+pub(crate) const MIN_VOTE_THRESHOLD: i128 = 1;

contracts/governor/src/contract.rs

@@ -20,12 +20,13 @@ pub struct GovernorContract;
 
 #[contractimpl]
 impl Governor for GovernorContract {
-    fn initialize(e: Env, votes: Address, settings: GovernorSettings) {
+    fn initialize(e: Env, votes: Address, council: Address, settings: GovernorSettings) {
         if storage::get_is_init(&e) {
             panic_with_error!(&e, GovernorError::AlreadyInitializedError);
         }
         require_valid_settings(&e, &settings);
         storage::set_settings(&e, &settings);
+        storage::set_council_address(&e, &council);
         storage::set_voter_token_address(&e, &votes);
         storage::set_is_init(&e);
         storage::extend_instance(&e);
@@ -35,6 +36,14 @@ impl Governor for GovernorContract {
         storage::get_settings(&e)
     }
 
+    fn council(e: Env) -> Address {
+        storage::get_council_address(&e)
+    }
+
+    fn vote_token(e: Env) -> Address {
+        storage::get_voter_token_address(&e)
+    }
+
     fn propose(
         e: Env,
         creator: Address,
@@ -49,15 +58,16 @@ impl Governor for GovernorContract {
             panic_with_error!(&e, GovernorError::ProposalAlreadyOpenError);
         }
 
-        let settings = storage::get_settings(&e);
         match action {
             ProposalAction::Upgrade(_) => {
-                if creator != settings.council {
+                let council = storage::get_council_address(&e);
+                if creator != council {
                     panic_with_error!(&e, GovernorError::UnauthorizedError);
                 }
             }
             _ => {}
         };
+        let settings = storage::get_settings(&e);
         let votes_client = VotesClient::new(&e, &storage::get_voter_token_address(&e));
         let creater_votes = votes_client.get_votes(&creator);
         if creater_votes < settings.proposal_threshold {
@@ -136,6 +146,7 @@ impl Governor for GovernorContract {
         if e.ledger().sequence() > proposal_data.vote_end + settings.grace_period {
             // proposal took too long to be closed. Mark expired and close.
             proposal_data.status = ProposalStatus::Expired;
+            GovernorEvents::proposal_expired(&e, proposal_id);
         } else {
             // proposal closed in time. Check if it passed or failed.
             let votes_client = VotesClient::new(&e, &storage::get_voter_token_address(&e));
@@ -204,11 +215,22 @@ impl Governor for GovernorContract {
 
         let mut proposal_data = storage::get_proposal_data(&e, proposal_id)
             .unwrap_or_else(|| panic_with_error!(&e, GovernorError::NonExistentProposalError));
+
         // require from to be the creator or the council
         if from != proposal_data.creator {
-            let settings = storage::get_settings(&e);
-            if from != settings.council {
+            let council = storage::get_council_address(&e);
+            if from != council {
                 panic_with_error!(&e, GovernorError::UnauthorizedError);
+            } else {
+                // block the security council from canceling council proposals
+                let proposal_config =
+                    storage::get_proposal_config(&e, proposal_id).unwrap_optimized();
+                match proposal_config.action {
+                    ProposalAction::Council(_) => {
+                        panic_with_error!(&e, GovernorError::UnauthorizedError);
+                    }
+                    _ => {}
+                }
             }
         }
 

contracts/governor/src/governor.rs

@@ -8,12 +8,19 @@ pub trait Governor {
     ///
     /// ### Arguments
     /// * `votes` - The address of the contract used to track votes
+    /// * `council` - The address of the security council for the DAO
     /// * `settings` - The settings for the governor
-    fn initialize(e: Env, votes: Address, settings: GovernorSettings);
+    fn initialize(e: Env, votes: Address, council: Address, settings: GovernorSettings);
 
     /// Get the current settings of the governor
     fn settings(e: Env) -> GovernorSettings;
 
+    /// Get the address of the security council for the DAO
+    fn council(e: Env) -> Address;
+
+    /// Get the address of the votes token contract
+    fn vote_token(e: Env) -> Address;
+
     /// Create a new proposal
     ///
     /// Returns the id of the new proposal

contracts/governor/src/proposal_config.rs

@@ -26,6 +26,7 @@ impl ProposalConfig {
             }
             ProposalAction::Settings(ref settings) => require_valid_settings(e, settings),
             ProposalAction::Upgrade(_) => (),
+            ProposalAction::Council(_) => (),
             ProposalAction::Snapshot => (),
         }
 
@@ -54,6 +55,9 @@ impl ProposalConfig {
             ProposalAction::Upgrade(ref wasm_hash) => {
                 e.deployer().update_current_contract_wasm(wasm_hash.clone());
             }
+            ProposalAction::Council(ref council) => {
+                storage::set_council_address(e, council);
+            }
             ProposalAction::Snapshot => {
                 panic_with_error!(e, GovernorError::InvalidProposalType)
             }
@@ -63,10 +67,8 @@ impl ProposalConfig {
     /// Check if the proposal is executable
     pub fn is_executable(&self) -> bool {
         match self.action {
-            ProposalAction::Calldata(_) => true,
-            ProposalAction::Settings(_) => true,
-            ProposalAction::Upgrade(_) => true,
             ProposalAction::Snapshot => false,
+            _ => true,
         }
     }
 }