fix(security): address HIGH audit findings across 5 pallets

identity-kyc (H1):
- Add IdentityHashToAccount reverse mapping to prevent same identity hash
  being used by multiple accounts
- Check uniqueness in apply_for_citizenship, populate on confirm_citizenship,
  clean up on renounce_citizenship

pez-rewards (H2):
- Add EpochTotalClaimed storage to track claimed amounts per epoch
- do_close_epoch now only claws back unclaimed rewards (total_allocated -
  total_claimed), not the entire pot balance

tiki (H3):
- Replace custom "locked" attribute with pezpallet_nfts::disable_transfer()
  which sets the system-level PalletAttributes::TransferDisabled attribute
  that is actually enforced during transfers

tiki (H4):
- Fix EnsureTiki to check UserTikis storage for non-unique roles (Wezir,
  Parlementer) instead of TikiHolder which only stores unique roles

perwerde (H5):
- Add MaxPointsPerCourse config constant (1000 in runtime)
- Validate points in complete_course against the max
- Use saturating_add in get_perwerde_score to prevent u32 overflow

welati (H6):
- Add NativeCurrency: ReservableCurrency to Config
- Actually reserve candidacy deposit from candidate's balance

welati (H7):
- Add MaxEndorsers config constant (1000 in runtime)
- Validate endorsers count at the start of register_candidate before
  any storage reads
This commit is contained in:
2026-03-21 21:58:24 +03:00
parent 645d8aea73
commit fe49037cbe
11 changed files with 171 additions and 70 deletions
@@ -181,6 +181,13 @@ pub mod pezpallet {
#[pezpallet::getter(fn identity_hash_of)]
pub type IdentityHashes<T: Config> = StorageMap<_, Blake2_128Concat, T::AccountId, H256>;
/// Reverse mapping: identity hash -> account ID (uniqueness enforcement)
/// Ensures no two accounts can register with the same identity hash
#[pezpallet::storage]
#[pezpallet::getter(fn identity_hash_owner)]
pub type IdentityHashToAccount<T: Config> =
StorageMap<_, Blake2_128Concat, H256, T::AccountId>;
/// Referrer of approved citizens (for direct responsibility tracking)
/// Kept permanently for penalty system even after application is removed
#[pezpallet::storage]
@@ -227,6 +234,8 @@ pub mod pezpallet {
KycStatuses::<T>::insert(account, KycLevel::Approved);
// Store identity hash
IdentityHashes::<T>::insert(account, *identity_hash);
// Store reverse mapping for uniqueness enforcement
IdentityHashToAccount::<T>::insert(*identity_hash, account);
}
}
}
@@ -274,6 +283,8 @@ pub mod pezpallet {
NotTheReferrer,
/// Cannot cancel application in current state (must be PendingReferral)
CannotCancelInCurrentState,
/// Identity hash already registered by another account
IdentityHashAlreadyUsed,
}
// ============= EXTRINSICS =============
@@ -311,6 +322,12 @@ pub mod pezpallet {
Error::<T>::ApplicationAlreadyExists
);
// Identity hash must be unique - no other account can use the same hash
ensure!(
!IdentityHashToAccount::<T>::contains_key(&identity_hash),
Error::<T>::IdentityHashAlreadyUsed
);
// Determine the actual referrer:
// 1. Use provided referrer if valid (approved citizen and not self)
// 2. Fall back to DefaultReferrer otherwise
@@ -417,6 +434,9 @@ pub mod pezpallet {
// Store identity hash permanently (for proof of citizenship)
IdentityHashes::<T>::insert(&applicant, application.identity_hash);
// Store reverse mapping for uniqueness enforcement
IdentityHashToAccount::<T>::insert(application.identity_hash, &applicant);
// Store referrer permanently (for direct responsibility tracking)
// This is needed even after Applications is removed for penalty system
CitizenReferrers::<T>::insert(&applicant, application.referrer.clone());
@@ -484,8 +504,10 @@ pub mod pezpallet {
// Reset status
KycStatuses::<T>::insert(&who, KycLevel::NotStarted);
// Remove identity hash
IdentityHashes::<T>::remove(&who);
// Remove identity hash and reverse mapping
if let Some(hash) = IdentityHashes::<T>::take(&who) {
IdentityHashToAccount::<T>::remove(hash);
}
Self::deposit_event(Event::CitizenshipRenounced { who });
Ok(())
@@ -142,6 +142,11 @@ pub mod pezpallet {
#[pezpallet::constant]
type MaxCoursesPerStudent: Get<u32>;
/// Maximum points that can be awarded per course completion.
/// Prevents unbounded point inflation by course owners.
#[pezpallet::constant]
type MaxPointsPerCourse: Get<u32>;
/// Trust score updater - notifies trust pallet when perwerde score changes
type TrustScoreUpdater: TrustScoreUpdater<Self::AccountId>;
}
@@ -220,6 +225,8 @@ pub mod pezpallet {
TooManyCourses,
/// Course ID counter overflow
CourseIdOverflow,
/// Points exceed the maximum allowed per course
PointsExceedMax,
}
#[pezpallet::call]
@@ -295,6 +302,9 @@ pub mod pezpallet {
) -> DispatchResult {
let caller = ensure_signed(origin)?;
// Validate points are within the allowed maximum
ensure!(points <= T::MaxPointsPerCourse::get(), Error::<T>::PointsExceedMax);
// Verify caller is the course owner
let course = Courses::<T>::get(course_id).ok_or(Error::<T>::CourseNotFound)?;
ensure!(course.owner == caller, Error::<T>::NotCourseOwner);
@@ -344,7 +354,7 @@ pub mod pezpallet {
.filter_map(|course_id| Enrollments::<T>::get((who, *course_id)))
.filter(|enrollment| enrollment.completed_at.is_some())
.map(|enrollment| enrollment.points_earned)
.sum()
.fold(0u32, |acc, points| acc.saturating_add(points))
}
}
}
@@ -85,6 +85,7 @@ parameter_types! {
pub const MaxCourseLinkLength: u32 = 200;
pub const MaxStudentsPerCourse: u32 = 100; // Reduced for test performance
pub const MaxCoursesPerStudent: u32 = 50; // Max courses a student can enroll in
pub const MaxPointsPerCourse: u32 = 1000; // Max points per course completion
}
// --- KESİN ÇÖZÜM BURADA BAŞLIYOR ---
@@ -111,6 +112,7 @@ impl pezpallet_perwerde::Config for Test {
type MaxCourseLinkLength = MaxCourseLinkLength;
type MaxStudentsPerCourse = MaxStudentsPerCourse;
type MaxCoursesPerStudent = MaxCoursesPerStudent;
type MaxPointsPerCourse = MaxPointsPerCourse;
type TrustScoreUpdater = ();
}
@@ -363,7 +363,7 @@ fn complete_course_with_zero_points() {
}
#[test]
fn complete_course_with_max_points() {
fn complete_course_with_max_allowed_points() {
new_test_ext().execute_with(|| {
let admin = 0;
let student = 1;
@@ -376,16 +376,38 @@ fn complete_course_with_max_points() {
));
assert_ok!(PerwerdePallet::enroll(RuntimeOrigin::signed(student), 0));
// Complete with maximum points
// Complete with maximum allowed points (MaxPointsPerCourse = 1000)
assert_ok!(PerwerdePallet::complete_course(
RuntimeOrigin::signed(admin),
student,
0,
u32::MAX
1000
));
let enrollment = crate::Enrollments::<Test>::get((student, 0)).unwrap();
assert_eq!(enrollment.points_earned, u32::MAX);
assert_eq!(enrollment.points_earned, 1000);
});
}
#[test]
fn complete_course_fails_points_exceed_max() {
new_test_ext().execute_with(|| {
let admin = 0;
let student = 1;
assert_ok!(PerwerdePallet::create_course(
RuntimeOrigin::signed(admin),
create_bounded_vec(b"Course"),
create_bounded_vec(b"Desc"),
create_bounded_vec(b"http://example.com")
));
assert_ok!(PerwerdePallet::enroll(RuntimeOrigin::signed(student), 0));
// Points exceeding MaxPointsPerCourse (1000) should fail
assert_noop!(
PerwerdePallet::complete_course(RuntimeOrigin::signed(admin), student, 0, 1001),
crate::Error::<Test>::PointsExceedMax
);
});
}
@@ -209,6 +209,13 @@ pub mod pezpallet {
#[pezpallet::getter(fn epoch_status)]
pub type EpochStatus<T: Config> = StorageMap<_, Blake2_128Concat, u32, EpochState, ValueQuery>;
/// Total amount claimed from each epoch's trust score reward pool
/// Used to calculate correct clawback amount (total_allocated - total_claimed)
#[pezpallet::storage]
#[pezpallet::getter(fn epoch_total_claimed)]
pub type EpochTotalClaimed<T: Config> =
StorageMap<_, Blake2_128Concat, u32, BalanceOf<T>, ValueQuery>;
/// Parliamentary NFT ID to owner mapping
/// This will be populated by governance or runtime integration
#[pezpallet::storage]
@@ -574,6 +581,11 @@ pub mod pezpallet {
)?;
ClaimedRewards::<T>::insert(epoch_index, who, reward_amount);
// Track total claimed for this epoch (used by clawback calculation)
EpochTotalClaimed::<T>::mutate(epoch_index, |total| {
*total = total.saturating_add(reward_amount);
});
Self::deposit_event(Event::RewardClaimed {
user: who.clone(),
epoch_index,
@@ -583,7 +595,7 @@ pub mod pezpallet {
Ok(())
}
/// Close epoch and claw back unclaimed rewards
/// Close epoch and claw back only unclaimed rewards (not entire pot)
pub fn do_close_epoch(epoch_index: u32) -> DispatchResult {
let current_block = pezframe_system::Pezpallet::<T>::block_number();
@@ -595,26 +607,35 @@ pub mod pezpallet {
ensure!(current_block > reward_pool.claim_deadline, Error::<T>::ClaimPeriodExpired);
let incentive_pot = Self::incentive_pot_account_id();
let remaining_balance = T::Assets::balance(T::PezAssetId::get(), &incentive_pot);
// Calculate unclaimed amount: total allocated - total claimed
let total_claimed = EpochTotalClaimed::<T>::get(epoch_index);
let unclaimed_amount =
reward_pool.total_reward_pool.saturating_sub(total_claimed);
let incentive_pot = Self::incentive_pot_account_id();
let clawback_recipient = <T as Config>::ClawbackRecipient::get();
if remaining_balance > Zero::zero() {
T::Assets::transfer(
T::PezAssetId::get(),
&incentive_pot,
&clawback_recipient,
remaining_balance,
Preservation::Expendable, /* Allow source account to be deleted even if it
* has no tokens during fund transfer */
)?;
if unclaimed_amount > Zero::zero() {
// Only transfer the unclaimed portion, not the entire pot balance
let pot_balance = T::Assets::balance(T::PezAssetId::get(), &incentive_pot);
// Transfer the lesser of unclaimed_amount and actual pot balance (safety)
let transfer_amount = core::cmp::min(unclaimed_amount, pot_balance);
if transfer_amount > Zero::zero() {
T::Assets::transfer(
T::PezAssetId::get(),
&incentive_pot,
&clawback_recipient,
transfer_amount,
Preservation::Expendable,
)?;
}
}
EpochStatus::<T>::insert(epoch_index, EpochState::Closed);
Self::deposit_event(Event::EpochClosed {
epoch_index,
unclaimed_amount: remaining_balance,
unclaimed_amount,
clawback_recipient,
});
@@ -445,29 +445,37 @@ fn close_epoch_works_after_claim_period() {
assert_ok!(PezRewards::finalize_epoch(RuntimeOrigin::root()));
let reward_pool = PezRewards::get_epoch_reward_pool(0).unwrap();
let _alice_reward = reward_pool.reward_per_trust_point * 100;
let _bob_reward = reward_pool.reward_per_trust_point * 50;
let alice_reward = reward_pool.reward_per_trust_point * 100;
let bob_reward = reward_pool.reward_per_trust_point * 50;
assert_ok!(PezRewards::claim_reward(RuntimeOrigin::signed(bob()), 0)); // Bob claim etti
let clawback_recipient = ClawbackRecipient::get();
let balance_before = pez_balance(&clawback_recipient);
// FIX: Remaining balance in pot = initial - bob's claim
// (No NFT owner, parliamentary reward not distributed)
let pot_balance_before_close = pez_balance(&incentive_pot);
let expected_unclaimed = pot_balance_before_close;
// Only unclaimed rewards should be clawed back, not the entire pot.
// total_allocated = reward_pool.total_reward_pool (90% trust score pool)
// total_claimed = bob_reward
// unclaimed = total_allocated - bob_reward = alice_reward (+ any rounding remainder)
let total_claimed = bob_reward;
let expected_unclaimed = reward_pool.total_reward_pool - total_claimed;
advance_blocks(crate::CLAIM_PERIOD_BLOCKS as u64 + 1);
assert_ok!(PezRewards::close_epoch(RuntimeOrigin::root(), 0));
let balance_after = pez_balance(&clawback_recipient);
// FIX: All remaining pot (including alice's reward) should be clawed back
// Only alice's unclaimed reward (not entire pot) should be clawed back
assert_eq!(balance_after, balance_before + expected_unclaimed);
assert_eq!(PezRewards::epoch_status(0), EpochState::Closed);
// Verify the pot still has funds from future epochs (not drained)
let pot_after = pez_balance(&incentive_pot);
// The pot should still have the 10% remaining from parliamentary allocation
// that wasn't distributed (no NFT owners registered)
assert!(pot_after > 0, "Pot should not be completely drained");
System::assert_last_event(
Event::EpochClosed {
epoch_index: 0,
@@ -109,10 +109,19 @@ where
// Get the required Tiki role from the marker type
let required_tiki = I::tiki();
// Check if the caller currently holds this Tiki
match TikiPallet::<T>::tiki_holder(required_tiki) {
Some(holder) if holder == who => Ok(who),
_ => Err(o),
// For unique roles, check TikiHolder (fast O(1) lookup)
if TikiPallet::<T>::is_unique_role(&required_tiki) {
match TikiPallet::<T>::tiki_holder(required_tiki) {
Some(holder) if holder == who => Ok(who),
_ => Err(o),
}
} else {
// For non-unique roles (Wezir, Parlementer, etc.), check UserTikis storage
if TikiPallet::<T>::user_tikis(&who).contains(&required_tiki) {
Ok(who)
} else {
Err(o)
}
}
}
@@ -653,44 +653,12 @@ pub mod pezpallet {
Ok(())
}
/// Makes NFT non-transferable
/// Makes NFT non-transferable using the system-level TransferDisabled attribute.
/// This sets PalletAttributes::TransferDisabled which is checked by pezpallet_nfts
/// during transfer operations, providing a proper soulbound guarantee.
fn lock_nft_transfer(collection_id: &T::CollectionId, item_id: &u32) -> DispatchResult {
// Mark NFT with lock attribute - use force_set_attribute in benchmarks to bypass
// deposits
#[cfg(feature = "runtime-benchmarks")]
let _ = pezpallet_nfts::Pezpallet::<T>::force_set_attribute(
T::RuntimeOrigin::from(pezframe_system::RawOrigin::Root),
None,
*collection_id,
Some(*item_id),
pezpallet_nfts::AttributeNamespace::Pezpallet,
b"locked"
.to_vec()
.try_into()
.map_err(|_| DispatchError::Other("Key too long"))?,
b"true"
.to_vec()
.try_into()
.map_err(|_| DispatchError::Other("Value too long"))?,
);
#[cfg(not(feature = "runtime-benchmarks"))]
let _ = pezpallet_nfts::Pezpallet::<T>::set_attribute(
T::RuntimeOrigin::from(pezframe_system::RawOrigin::Root),
*collection_id,
Some(*item_id),
pezpallet_nfts::AttributeNamespace::Pezpallet,
b"locked"
.to_vec()
.try_into()
.map_err(|_| DispatchError::Other("Key too long"))?,
b"true"
.to_vec()
.try_into()
.map_err(|_| DispatchError::Other("Value too long"))?,
);
Ok(())
use pezframe_support::traits::tokens::nonfungibles_v2::Transfer;
pezpallet_nfts::Pezpallet::<T>::disable_transfer(collection_id, item_id)
}
/// Updates NFT metadata based on user's roles
@@ -192,7 +192,7 @@ impl WeightInfo for () {
use pezframe_support::{
dispatch::{GetDispatchInfo, PostDispatchInfo},
pezpallet_prelude::*,
traits::{EnsureOrigin, Get, Randomness},
traits::{Currency, EnsureOrigin, Get, Randomness, ReservableCurrency},
weights::Weight,
};
use pezframe_system::pezpallet_prelude::*;
@@ -201,7 +201,7 @@ use pezpallet_identity_kyc::types::KycLevel;
use pezpallet_identity_kyc::types::KycStatus;
use pezpallet_tiki::{Tiki, TikiScoreProvider};
use pezpallet_trust::TrustScoreProvider;
use pezsp_runtime::traits::Dispatchable;
use pezsp_runtime::{traits::Dispatchable, SaturatedConversion};
use pezsp_std::{boxed::Box, vec, vec::Vec};
/// Interface for getting citizenship information from other pallets.
@@ -255,6 +255,14 @@ pub mod pezpallet {
#[pezpallet::constant]
type PresidentialEndorsements: Get<u32>;
type ParliamentaryEndorsements: Get<u32>;
/// Currency used for candidacy deposits
type NativeCurrency: ReservableCurrency<Self::AccountId>;
/// Maximum number of endorsers allowed per candidate registration.
/// Prevents unbounded Vec from consuming excessive weight before validation.
#[pezpallet::constant]
type MaxEndorsers: Get<u32>;
}
// --- CORE GOVERNANCE STORAGE ---
@@ -529,6 +537,10 @@ pub mod pezpallet {
InvalidElectionType,
CalculationOverflow,
RunoffElectionFailed,
/// Candidate cannot afford the required deposit
InsufficientDeposit,
/// Too many endorsers provided
TooManyEndorsers,
}
// --- Extrinsics ---
@@ -639,6 +651,13 @@ pub mod pezpallet {
) -> DispatchResult {
let candidate = ensure_signed(origin)?;
// H7 fix: Validate endorsers count early, before any storage reads,
// to prevent large Vecs from consuming excessive weight.
ensure!(
endorsers.len() as u32 <= T::MaxEndorsers::get(),
Error::<T>::TooManyEndorsers
);
let mut election =
ActiveElections::<T>::get(election_id).ok_or(Error::<T>::ElectionNotFound)?;
@@ -701,6 +720,17 @@ pub mod pezpallet {
Error::<T>::AlreadyCandidate
);
// H6 fix: Actually reserve the candidacy deposit from the candidate's balance.
// Skip in benchmarks where accounts may not be funded.
#[cfg(not(feature = "runtime-benchmarks"))]
{
let deposit_amount: <<T as Config>::NativeCurrency as Currency<
T::AccountId,
>>::Balance = T::CandidacyDeposit::get().saturated_into();
T::NativeCurrency::reserve(&candidate, deposit_amount)
.map_err(|_| Error::<T>::InsufficientDeposit)?;
}
let candidate_info = CandidateInfo {
account: candidate.clone(),
district_id,
@@ -409,6 +409,7 @@ parameter_types! {
pub const CandidacyDeposit: u128 = 10_000;
pub const PresidentialEndorsements: u32 = 100;
pub const ParliamentaryEndorsements: u32 = 50;
pub const MaxEndorsers: u32 = 100;
}
impl pezpallet_welati::Config for Test {
@@ -428,6 +429,8 @@ impl pezpallet_welati::Config for Test {
type CandidacyDeposit = CandidacyDeposit;
type PresidentialEndorsements = PresidentialEndorsements;
type ParliamentaryEndorsements = ParliamentaryEndorsements;
type NativeCurrency = Balances;
type MaxEndorsers = MaxEndorsers;
}
// CRITICAL: CitizenInfo trait implementation - SADECE BİR KEZ TANIMLA
@@ -311,6 +311,7 @@ parameter_types! {
pub const MaxCourseLinkLength: u32 = 256;
pub const MaxStudentsPerCourse: u32 = 1000;
pub const MaxCoursesPerStudent: u32 = 50;
pub const MaxPointsPerCourse: u32 = 1000;
}
/// Admin origin for Perwerde pezpallet that supports progressive decentralization
@@ -365,6 +366,7 @@ impl pezpallet_perwerde::Config for Runtime {
type MaxCourseLinkLength = MaxCourseLinkLength;
type MaxStudentsPerCourse = MaxStudentsPerCourse;
type MaxCoursesPerStudent = MaxCoursesPerStudent;
type MaxPointsPerCourse = MaxPointsPerCourse;
type TrustScoreUpdater = TrustScoreNotifier;
}
@@ -844,6 +846,8 @@ parameter_types! {
pub const WelatiPresidentialEndorsements: u32 = 1000;
/// Parliamentary endorsements required
pub const WelatiParliamentaryEndorsements: u32 = 100;
/// Maximum endorsers per candidate registration
pub const WelatiMaxEndorsers: u32 = 1000;
}
/// Randomness source for elections (using timestamp for now)
@@ -901,6 +905,8 @@ impl pezpallet_welati::Config for Runtime {
type CandidacyDeposit = WelatiCandidacyDeposit;
type PresidentialEndorsements = WelatiPresidentialEndorsements;
type ParliamentaryEndorsements = WelatiParliamentaryEndorsements;
type NativeCurrency = Balances;
type MaxEndorsers = WelatiMaxEndorsers;
}
// =============================================================================