MESH-1813 - reject invalid distributions

pallets/corporate-actions/src/distribution/mod.rs

@@ -87,6 +87,7 @@ use polymesh_primitives::{
     SecondaryKey, Ticker,
 };
 use scale_info::TypeInfo;
+use sp_runtime::traits::Zero;
 #[cfg(feature = "std")]
 use sp_runtime::{Deserialize, Serialize};
 use sp_std::prelude::*;
@@ -202,6 +203,8 @@ decl_module! {
         /// - `InsufficientPortfolioBalance` if `portfolio` has less than `amount` of `currency`.
         /// - `InsufficientBalance` if the protocol fee couldn't be charged.
         /// - `CANotBenefit` if the CA is not of kind PredictableBenefit/UnpredictableBenefit
+        /// - `DistributionAmountIsZero` if the `amount` is zero.
+        /// - `DistributionPerShareIsZero` if the `per_share` is zero.
         ///
         /// # Permissions
         /// * Asset
@@ -217,15 +220,8 @@ decl_module! {
             payment_at: Moment,
             expires_at: Option<Moment>,
         ) -> DispatchResult {
-            let PermissionedCallOriginData {
-                primary_did: agent,
-                secondary_key,
-                ..
-            } = <ExternalAgents<T>>::ensure_agent_asset_perms(origin, ca_id.ticker)?;
-
-            Self::unsafe_distribute(
-                agent,
-                secondary_key,
+            Self::base_distribute(
+                origin,
                 ca_id,
                 portfolio,
                 currency,
@@ -261,8 +257,7 @@ decl_module! {
         /// - Other errors can occur if the compliance manager rejects the transfer.
         #[weight = <T as Config>::DistWeightInfo::claim(T::MaxTargetIds::get(), T::MaxDidWhts::get())]
         pub fn claim(origin, ca_id: CAId) {
-            let did = <Identity<T>>::ensure_perms(origin)?;
-            Self::transfer_benefit(did.for_event(), did, ca_id)?;
+            Self::base_claim(origin, ca_id)?;
         }
 
         /// Push benefit of an ongoing distribution to the given `holder`.
@@ -291,8 +286,7 @@ decl_module! {
         /// - Other errors can occur if the compliance manager rejects the transfer.
         #[weight = <T as Config>::DistWeightInfo::push_benefit(T::MaxTargetIds::get(), T::MaxDidWhts::get())]
         pub fn push_benefit(origin, ca_id: CAId, holder: IdentityId) {
-            let agent = <ExternalAgents<T>>::ensure_perms(origin, ca_id.ticker)?.for_event();
-            Self::transfer_benefit(agent, holder, ca_id)?;
+            Self::base_push_benefit(origin, ca_id, holder)?;
         }
 
         /// Assuming a distribution has expired,
@@ -308,27 +302,7 @@ decl_module! {
         /// - `NotExpired` if `now < expiry`.
         #[weight = <T as Config>::DistWeightInfo::reclaim()]
         pub fn reclaim(origin, ca_id: CAId) {
-            // Ensure distribution is created, they haven't reclaimed, and that expiry has passed.
-            // CA must be authorized and be the custodian.
-            let PermissionedCallOriginData {
-                primary_did: agent,
-                secondary_key,
-                ..
-            } = <ExternalAgents<T>>::ensure_agent_asset_perms(origin.clone(), ca_id.ticker)?;
-            let dist = Self::ensure_distribution_exists(ca_id)?;
-            ensure!(!dist.reclaimed, Error::<T>::AlreadyReclaimed);
-            ensure!(expired(dist.expires_at, <Checkpoint<T>>::now_unix()), Error::<T>::NotExpired);
-            <Portfolio<T>>::ensure_portfolio_custody_and_permission(dist.from, agent, secondary_key.as_ref())?;
-
-            // Unlock `remaining` of `currency` from DID's portfolio.
-            // This won't fail, as we've already locked the requisite amount prior.
-            Self::unlock(&dist, dist.remaining)?;
-
-            // Zero `remaining` + note that we've reclaimed.
-            Distributions::insert(ca_id, Distribution { reclaimed: true, remaining:0u32.into(), ..dist });
-
-            // Emit event.
-            Self::deposit_event(Event::Reclaimed(agent.for_event(), ca_id, dist.remaining));
+            Self::base_reclaim(origin, ca_id)?;
         }
 
         /// Removes a distribution that hasn't started yet,
@@ -344,9 +318,7 @@ decl_module! {
         /// - `DistributionStarted` if `payment_at <= now`.
         #[weight = <T as Config>::DistWeightInfo::remove_distribution()]
         pub fn remove_distribution(origin, ca_id: CAId) {
-            let agent = <ExternalAgents<T>>::ensure_perms(origin, ca_id.ticker)?.for_event();
-            let dist = Self::ensure_distribution_exists(ca_id)?;
-            Self::remove_distribution_base(agent, ca_id, &dist)?;
+            Self::base_remove_distribution(origin, ca_id)?;
         }
     }
 }
@@ -405,12 +377,107 @@ decl_error! {
         DistributionStarted,
         /// A distribution has insufficient remaining amount of currency to distribute.
         InsufficientRemainingAmount,
+        /// Distribution `amount` cannot be zero.
+        DistributionAmountIsZero,
+        /// Distribution `per_share` cannot be zero.
+        DistributionPerShareIsZero,
     }
 }
 
 impl<T: Config> Module<T> {
+    fn base_distribute(
+        origin: T::Origin,
+        ca_id: CAId,
+        portfolio: Option<PortfolioNumber>,
+        currency: Ticker,
+        per_share: Balance,
+        amount: Balance,
+        payment_at: Moment,
+        expires_at: Option<Moment>,
+    ) -> DispatchResult {
+        let PermissionedCallOriginData {
+            primary_did: agent,
+            secondary_key,
+            ..
+        } = <ExternalAgents<T>>::ensure_agent_asset_perms(origin, ca_id.ticker)?;
+
+        Self::unverified_distribute(
+            agent,
+            secondary_key,
+            ca_id,
+            portfolio,
+            currency,
+            per_share,
+            amount,
+            payment_at,
+            expires_at,
+        )
+    }
+
+    fn base_claim(origin: T::Origin, ca_id: CAId) -> DispatchResult {
+        let did = <Identity<T>>::ensure_perms(origin)?;
+        Self::transfer_benefit(did.for_event(), did, ca_id)?;
+        Ok(())
+    }
+
+    fn base_push_benefit(origin: T::Origin, ca_id: CAId, holder: IdentityId) -> DispatchResult {
+        let agent = <ExternalAgents<T>>::ensure_perms(origin, ca_id.ticker)?.for_event();
+        Self::transfer_benefit(agent, holder, ca_id)?;
+        Ok(())
+    }
+
+    fn base_reclaim(origin: T::Origin, ca_id: CAId) -> DispatchResult {
+        // Ensure distribution is created, they haven't reclaimed, and that expiry has passed.
+        // CA must be authorized and be the custodian.
+        let PermissionedCallOriginData {
+            primary_did: agent,
+            secondary_key,
+            ..
+        } = <ExternalAgents<T>>::ensure_agent_asset_perms(origin.clone(), ca_id.ticker)?;
+        let dist = Self::ensure_distribution_exists(ca_id)?;
+        ensure!(!dist.reclaimed, Error::<T>::AlreadyReclaimed);
+        ensure!(
+            expired(dist.expires_at, <Checkpoint<T>>::now_unix()),
+            Error::<T>::NotExpired
+        );
+        <Portfolio<T>>::ensure_portfolio_custody_and_permission(
+            dist.from,
+            agent,
+            secondary_key.as_ref(),
+        )?;
+
+        // Unlock `remaining` of `currency` from DID's portfolio.
+        // This won't fail, as we've already locked the requisite amount prior.
+        Self::unlock(&dist, dist.remaining)?;
+
+        // Zero `remaining` + note that we've reclaimed.
+        Distributions::insert(
+            ca_id,
+            Distribution {
+                reclaimed: true,
+                remaining: 0u32.into(),
+                ..dist
+            },
+        );
+
+        // Emit event.
+        Self::deposit_event(Event::Reclaimed(agent.for_event(), ca_id, dist.remaining));
+
+        Ok(())
+    }
+
+    fn base_remove_distribution(origin: T::Origin, ca_id: CAId) -> DispatchResult {
+        let agent = <ExternalAgents<T>>::ensure_perms(origin, ca_id.ticker)?.for_event();
+        let dist = Self::ensure_distribution_exists(ca_id)?;
+        Self::unverified_remove_distribution(agent, ca_id, &dist)?;
+
+        Ok(())
+    }
+
     /// Kill the distribution identified by `ca_id`.
-    crate fn remove_distribution_base(
+    ///
+    /// Unlike `base_remove_distribution`, this won't check permissions and that the dist exists.
+    crate fn unverified_remove_distribution(
         agent: EventDid,
         ca_id: CAId,
         dist: &Distribution,
@@ -531,7 +598,10 @@ impl<T: Config> Module<T> {
         Ok(dist)
     }
 
-    pub fn unsafe_distribute(
+    /// Create a capital distribution.
+    ///
+    /// Unlike `base_distribute`, this won't check permissions.
+    crate fn unverified_distribute(
         agent: IdentityId,
         secondary_key: Option<SecondaryKey<T::AccountId>>,
         ca_id: CAId,
@@ -542,6 +612,10 @@ impl<T: Config> Module<T> {
         payment_at: Moment,
         expires_at: Option<Moment>,
     ) -> DispatchResult {
+        // Ensure valid `amount` and `per_share`.
+        ensure!(!amount.is_zero(), Error::<T>::DistributionAmountIsZero);
+        ensure!(!per_share.is_zero(), Error::<T>::DistributionPerShareIsZero);
+
         // Ensure that any expiry date doesn't come before the payment date.
         ensure!(
             !expired(expires_at, payment_at),

pallets/corporate-actions/src/lib.rs

@@ -652,7 +652,7 @@ decl_module! {
                 }
                 CAKind::PredictableBenefit | CAKind::UnpredictableBenefit => {
                     if let Some(dist) = <Distribution<T>>::distributions(ca_id) {
-                        <Distribution<T>>::remove_distribution_base(agent, ca_id, &dist)?;
+                        <Distribution<T>>::unverified_remove_distribution(agent, ca_id, &dist)?;
                     }
                 }
             }
@@ -761,7 +761,7 @@ decl_module! {
                     withholding_tax
                 )?;
 
-                <distribution::Module<T>>::unsafe_distribute(
+                <distribution::Module<T>>::unverified_distribute(
                     agent,
                     secondary_key,
                     ca_id,

pallets/runtime/ci/src/runtime.rs

@@ -62,7 +62,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     spec_version: 5_000_000,
     impl_version: 0,
     apis: RUNTIME_API_VERSIONS,
-    transaction_version: 2,
+    transaction_version: 3,
 };
 
 parameter_types! {

pallets/runtime/develop/src/runtime.rs

@@ -60,7 +60,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     spec_version: 5_000_000,
     impl_version: 0,
     apis: RUNTIME_API_VERSIONS,
-    transaction_version: 2,
+    transaction_version: 3,
 };
 
 parameter_types! {

pallets/runtime/mainnet/src/runtime.rs

@@ -57,7 +57,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     spec_version: 5_000_000,
     impl_version: 0,
     apis: RUNTIME_API_VERSIONS,
-    transaction_version: 2,
+    transaction_version: 3,
 };
 
 parameter_types! {

pallets/runtime/testnet/src/runtime.rs

@@ -62,7 +62,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     spec_version: 5_000_000,
     impl_version: 0,
     apis: RUNTIME_API_VERSIONS,
-    transaction_version: 2,
+    transaction_version: 3,
 };
 
 parameter_types! {