fix: Multisig - auto-cleaning expanded

[czareko]

No description provided.

[czareko]

This is not related to my changes, but we had a Clippy warning here, so I'm fixing this as well.

[n13]

PR Review: PR #364 (fix: Multisig - auto-cleaning expanded)

Summary

The PR expands the auto-cleaning mechanism for the Multisig pallet. Instead of only cleaning up expired proposals during propose, it now attempts to clean up expired proposals on any multisig activity (propose, approve, cancel). This ensures better state hygiene and return of deposits without manual intervention.

Critical Feedback

1. Weight Refund Vulnerability (Potential DoS)

In approve and cancel, the weight logic charges for the worst-case scenario (MaxTotalProposalsInStorage) upfront, which is good. However, the refund logic is potentially flawed:

// pallets/multisig/src/lib.rs

// Returns count of cleaned proposals for accurate weight refund
let cleaned_count = Self::auto_cleanup_expired_proposals(&multisig_address, &approver);

// ...

// Calculate actual weight based on real call size and actual cleanup count
let actual_weight = <T as Config>::WeightInfo::approve(actual_call_size, cleaned_count);

The auto_cleanup_expired_proposals function iterates over ALL proposals (using iter_prefix) to find the expired ones.

• The Issue: If a multisig has many active, non-expired proposals (e.g., 50 pending proposals), iter_prefix will still read and decode all of them to check their expiry.
• The Flaw: The weight is refunded based on cleaned_count. If 0 proposals are expired (cleaned_count == 0), the final weight charged corresponds to processing 0 items. However, the node actually performed the work of reading/iterating 50 items.
• Risk: An attacker could fill a multisig with many active proposals and repeatedly call approve. The execution would be expensive (O(N) reads/decodes), but the transaction fee would be cheap (refunded to near zero).

Recommendation:
The weight refund should account for the total number of proposals iterated, not just the ones deleted. You might need to adjust the weight formula to separate "iteration cost" from "deletion cost", or simply not refund the read/iteration portion of the weight (only refunding the write/deletion portion).

2. Performance of iter_prefix

The auto_cleanup_expired_proposals uses Proposals::<T>::iter_prefix(multisig_address).

.filter_map(|(id, proposal)| { ... })

This loads the full Proposal struct for every pending proposal. If the Proposal contains the full Call (which can be large), this operation effectively loads all pending transaction data for that multisig from storage into memory on every interaction.

• If T::MaxTotalProposalsInStorage is small (e.g., < 20), this is acceptable.
• If the limit is high, this is a significant performance bottleneck.

Recommendation: Verify that MaxTotalProposalsInStorage is configured to a low number in the runtime, or consider a secondary index for expiry to avoid loading full proposal data.

Other Observations

• Logic: The auto_cleanup_expired_proposals logic correctly identifies and removes expired proposals, returning deposits to the original proposers. This is a solid improvement.
• Tests: The new test auto_cleanup_on_approve_and_cancel covers the new behavior well.
• Documentation: The README updates clearly explain the new auto-cleanup behavior and the threshold=1 execution.
• Code Style: The changes respect the "concise and minimal" rule. The fix in mining-rewards (unwrap_or_else(BalanceOf::<T>::zero)) is a nice Rust idiomatic improvement.
• Benchmarking: The benchmarks correctly set up the "worst case" for deletion (e expired proposals), but as noted above, they might bundle iteration and deletion costs in a way that makes partial refunds inaccurate for the "high iteration, low deletion" case.

Conclusion

The feature is valuable, but the weight refund logic needs to be fixed to prevent undercharging for the iteration of non-expired proposals.

[n13]

I still think we should clean up at most 5 proposals, and always charge for 5 (which is a small charge) to make this less fragile.

I am not super comfortable with iterating over 200 items potentially.

[n13]

Update:

I checked the Gemini issue, and it is correct, we are refunding fees when we don't find expired proposals yet we are doing the work of checking all active proposals, which we need to charge for.

[n13]

I think an explicit cleanup call would be a better architecture

However, there's nothing wrong with this as is, so GTG

[ethan-crypto]

LGTM