Pallet Treasury Review

[n13]

Here's my review of the treasury pallet:

---

Overall: The pallet is well-structured and simple. Root-only access control is correct, portion validation is sound, and the TreasuryProvider trait integration with mining-rewards works properly. That said, there are a few items worth flagging.

---

Security Concerns

1. Zero-account fallback silently swallows funds

	impl<T: Config> Pallet<T> {
		/// Get the treasury account. Returns zero account if not configured.
		pub fn account_id() -> T::AccountId {
			TreasuryAccount::<T>::get().unwrap_or_else(|| {
				T::AccountId::decode(&mut sp_runtime::traits::TrailingZeroInput::zeroes())
					.unwrap_or_else(|_| {
						// Fallback: zero account
						T::AccountId::decode(&mut &[0u8; 32][..])
							.unwrap_or_else(|_| panic!("Cannot create fallback AccountId"))
					})
			})
		}

If TreasuryAccount is None (storage cleared, migration error, etc.), this returns a zero-address that nobody controls. Mining-rewards will happily mint_into that address, permanently locking funds. Consider returning Option<T::AccountId> and letting the caller decide, or panicking early in genesis/integrity checks to guarantee it's always set.

2. No validation on set_treasury_account

		pub fn set_treasury_account(origin: OriginFor<T>, account: T::AccountId) -> DispatchResult {
			ensure_root(origin)?;
			TreasuryAccount::<T>::put(&account);
			Self::deposit_event(Event::TreasuryAccountUpdated { new_account: account });
			Ok(())
		}

Root can set the treasury to the zero address (or any address). A minimal guard against the zero-account would prevent accidental misconfiguration.

3. Redundant decode fallback is dead code

The inner T::AccountId::decode(&mut &[0u8; 32][..]) branch is unreachable -- TrailingZeroInput::zeroes() produces an infinite stream of zero bytes, so decoding AccountId32 from it will always succeed. Dead fallback branches create a false sense of safety; simplify to a single decode.

---

Correctness

4. TreasuryPortion defaults to 0 via ValueQuery

	pub type TreasuryPortion<T: Config> = StorageValue<_, u8, ValueQuery>;

If genesis doesn't run (e.g., in a test or a misconfigured chain spec), the portion silently defaults to 0, meaning the treasury receives nothing and the miner gets 100% of rewards. This is a safe-ish default, but a ValueQuery with a default of 0 combined with the fact that this directly controls fund flow is worth being explicit about. You could add an integrity_test hook that asserts TreasuryAccount is Some.

5. 1% granularity ceiling

portion is u8 (0-100), and mining-rewards converts it via Permill::from_percent. This locks you to 1% increments. If you ever need 33.3% or similar precision, you'd need a storage migration. Consider using Permill or Perbill directly if finer control may be needed later.

---

Test Coverage Gaps

The tests cover the happy path but miss several edge cases:

• No test for set_treasury_portion requiring root (there is one for set_treasury_account requiring root, but not for portion)
• No test for portion boundary values (0 and 100) -- these map to "treasury gets nothing" and "miner gets nothing" respectively
• No test for the zero-account fallback when TreasuryAccount is None
• No test for TreasuryProvider trait impl -- the actual interface consumed by mining-rewards
• No event assertions in any test

---

Summary

Category	Item	Severity
Security	Zero-account fallback silently absorbs funds	Medium
Security	No validation on set_treasury_account target	Low
Correctness	Dead code in decode fallback	Low
Correctness	Portion defaults to 0 silently	Low
Design	1% granularity ceiling on portion	Low
Testing	Missing edge-case and negative tests	Medium

The pallet is functional and safe for root-only governance. The main actionable item is hardening the zero-account fallback path -- either make account_id() return Option, add an integrity_test, or both. Want me to address any of these?

[n13]

I think % granularity should be permill probably... the zero account should throw errors