Crowdloan optional per-contributor maximum contribution and exclusive call/target address.#2697
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// - `crowdloan_id`: The id of the crowdloan to update the maximum contribution of. | ||
| /// - `new_max_contribution`: The new optional maximum contribution. | ||
| #[pallet::call_index(9)] | ||
| #[pallet::weight(T::WeightInfo::set_maximum_contribution())] |
There was a problem hiding this comment.
[MEDIUM] Regenerate weights for the new max-contribution storage paths
set_maximum_contribution is wired into runtime dispatch, but the PR does not update pallets/crowdloan/src/weights.rs for this new extrinsic or for the extra MaxContributions reads/writes added to contribute, update_min_contribution, and dissolve. In a runtime pallet, under-accounted database reads/writes are a block-resource accounting issue: users can execute more state access than the declared weight permits. Regenerate and commit the crowdloan weights so the new extrinsic exists in WeightInfo and all modified extrinsics account for MaxContributions.
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE BASELINE scrutiny: l0r1s has repo write permission, an established 2018 account, substantial prior opentensor PR history, and same-author commits; branch crowdloan-fixes -> devnet-ready. No Findings
Prior-comment reconciliation
ConclusionThe PR remains vulnerable because call-route finalization can leave stale 📜 Previous run (superseded)
# 🔍 AI Review — Auditor (domain review) has not yet run on this PR. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| cap, | ||
| end, | ||
| Some(call), | ||
| None, |
There was a problem hiding this comment.
[MEDIUM] Finalize weight only benchmarks the transfer route
This benchmark now creates the finalized crowdloan with call = None and target_address = Some(...), so generated WeightInfo::finalize() only accounts for the transfer route. Runtime finalize still supports (Some(call), None) and then writes/kills CurrentCrowdloanId, looks up the preimage, and dispatches the stored call under the same static #[pallet::weight(T::WeightInfo::finalize())]. That lets call-route finalization consume unaccounted runtime work in the block. Regenerate or restructure the weight so finalization covers the worst supported route, including the dispatched call weight and extra storage I/O, or split the routes into separately weighted calls.
|
🔄 AI review updated — Skeptic: VULNERABLE |
l0r1s
added
the
skip-cargo-audit
| CurrentCrowdloanId::<T>::put(crowdloan_id); | ||
|
|
||
| // Retrieve the call from the preimage storage | ||
| let stored_call = match T::Preimages::peek(call) { | ||
| Ok((call, _)) => call, | ||
| Err(_) => { | ||
| // If the call is not found, we drop it from the preimage storage | ||
| // because it's not needed anymore | ||
| T::Preimages::drop(call); | ||
| return Err(Error::<T>::CallUnavailable)?; | ||
| } | ||
| }; | ||
|
|
||
| // Dispatch the call with creator origin | ||
| stored_call | ||
| .dispatch(frame_system::RawOrigin::Signed(who).into()) | ||
| .map(|_| ()) | ||
| .map_err(|e| e.error)?; | ||
|
|
||
| // Clear the current crowdloan id | ||
| CurrentCrowdloanId::<T>::kill(); |
There was a problem hiding this comment.
[HIGH] Clear CurrentCrowdloanId on all finalize errors
finalize writes CurrentCrowdloanId before both preimage lookup and dispatch, but the CallUnavailable branch and any dispatched-call error return before CurrentCrowdloanId::<T>::kill() runs. Runtime storage changes are not automatically rolled back on DispatchError, so a creator can leave a stale crowdloan id set after a failing finalization. That id gates register_leased_network, which would let the creator later execute leasing outside the immutable stored finalization call and choose fresh parameters. Retrieve the preimage before exposing the id, then always clear the id before propagating the dispatch result.
| CurrentCrowdloanId::<T>::put(crowdloan_id); | |
| // Retrieve the call from the preimage storage | |
| let stored_call = match T::Preimages::peek(call) { | |
| Ok((call, _)) => call, | |
| Err(_) => { | |
| // If the call is not found, we drop it from the preimage storage | |
| // because it's not needed anymore | |
| T::Preimages::drop(call); | |
| return Err(Error::<T>::CallUnavailable)?; | |
| } | |
| }; | |
| // Dispatch the call with creator origin | |
| stored_call | |
| .dispatch(frame_system::RawOrigin::Signed(who).into()) | |
| .map(|_| ()) | |
| .map_err(|e| e.error)?; | |
| // Clear the current crowdloan id | |
| CurrentCrowdloanId::<T>::kill(); | |
| // Retrieve the call from the preimage storage before exposing the | |
| // crowdloan id to the dispatched call. | |
| let stored_call = match T::Preimages::peek(call) { | |
| Ok((call, _)) => call, | |
| Err(_) => { | |
| // If the call is not found, we drop it from the preimage storage | |
| // because it's not needed anymore | |
| T::Preimages::drop(call); | |
| return Err(Error::<T>::CallUnavailable)?; | |
| } | |
| }; | |
| // Set the current crowdloan id only while the call is being dispatched. | |
| CurrentCrowdloanId::<T>::put(crowdloan_id); | |
| let dispatch_result = stored_call | |
| .dispatch(frame_system::RawOrigin::Signed(who).into()) | |
| .map(|_| ()) | |
| .map_err(|e| e.error); | |
| CurrentCrowdloanId::<T>::kill(); | |
| dispatch_result?; |
There was a problem hiding this comment.
Runtime storage changes are not automatically rolled back on DispatchError. It is.
| @@ -238,11 +242,17 @@ mod benchmarks { | |||
| cap, | |||
| end, | |||
| Some(call), | |||
| Some(target_address.clone()), | |||
| None, | |||
There was a problem hiding this comment.
[MEDIUM] Finalize weight still covers only one supported route
The benchmark now creates only a call-route crowdloan (Some(call), None), while finalize still supports the target-address route that performs a balance transfer. The committed finalize weight is a single constant, so it must cover the worst supported path: preimage lookup plus dispatched-call accounting, and the target transfer path's account reads/writes. As written, one supported route remains unmeasured, and the stored call's own dispatch weight is not added at finalization time.
There was a problem hiding this comment.
We are benchmarking worst case here, not all cases.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| (Some(call), None) => { | ||
| // Set the current crowdloan id so the dispatched call | ||
| // can access it temporarily | ||
| CurrentCrowdloanId::<T>::put(crowdloan_id); |
There was a problem hiding this comment.
[HIGH] Clear CurrentCrowdloanId on all finalize errors
CurrentCrowdloanId is set before preimage lookup and inner dispatch, but it is only cleared after a successful dispatch. If T::Preimages::peek(call) fails or stored_call.dispatch(...) returns an error, finalize exits through ?/return Err with CurrentCrowdloanId still set. Any later runtime logic that reads this temporary storage can observe stale finalization context for a crowdloan that did not finalize. Clear the storage on every exit path after it is set, e.g. wrap the call-route work so CurrentCrowdloanId::<T>::kill() runs before returning both success and error.
| @@ -238,11 +243,17 @@ mod benchmarks { | |||
| cap, | |||
| end, | |||
| Some(call), | |||
There was a problem hiding this comment.
[MEDIUM] Finalize weight still covers only one supported route
This benchmark now exercises the stored-call route, but finalize still exposes one constant T::WeightInfo::finalize() while supporting two materially different paths: a balances transfer route and an arbitrary stored-call dispatch route. The stored call's own dispatch weight is also not charged at finalization time, even though that is when the work executes. A creator can therefore finalize a heavy stored call under the fixed finalize weight. Split the weights by route or add the stored call's dispatch weight to the finalization weight before dispatching it.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| // Set the current crowdloan id so the dispatched call | ||
| // can access it temporarily | ||
| CurrentCrowdloanId::<T>::put(crowdloan_id); | ||
|
|
||
| // Retrieve the call from the preimage storage | ||
| let stored_call = match T::Preimages::peek(call) { | ||
| Ok((call, _)) => call, | ||
| Err(_) => { | ||
| // If the call is not found, we drop it from the preimage storage | ||
| // because it's not needed anymore | ||
| T::Preimages::drop(call); | ||
| return Err(Error::<T>::CallUnavailable)?; | ||
| } | ||
| }; | ||
|
|
||
| // Dispatch the call with creator origin | ||
| stored_call | ||
| .dispatch(frame_system::RawOrigin::Signed(who).into()) | ||
| .map(|_| ()) | ||
| .map_err(|e| e.error)?; | ||
|
|
||
| // Clear the current crowdloan id | ||
| CurrentCrowdloanId::<T>::kill(); |
There was a problem hiding this comment.
[HIGH] Clear CurrentCrowdloanId on all finalize errors
CurrentCrowdloanId is written before fallible operations and is only cleared on the success path. If the preimage lookup/drop path returns CallUnavailable, or if the stored call dispatch returns an error, this storage item remains set after finalize exits. Any later logic that trusts CurrentCrowdloanId as proof that it is executing inside a crowdloan finalization can then observe a stale crowdloan id outside the intended dispatch window.
Move the put until after the preimage lookup and always clear it before propagating the inner dispatch result.
| // Set the current crowdloan id so the dispatched call | |
| // can access it temporarily | |
| CurrentCrowdloanId::<T>::put(crowdloan_id); | |
| // Retrieve the call from the preimage storage | |
| let stored_call = match T::Preimages::peek(call) { | |
| Ok((call, _)) => call, | |
| Err(_) => { | |
| // If the call is not found, we drop it from the preimage storage | |
| // because it's not needed anymore | |
| T::Preimages::drop(call); | |
| return Err(Error::<T>::CallUnavailable)?; | |
| } | |
| }; | |
| // Dispatch the call with creator origin | |
| stored_call | |
| .dispatch(frame_system::RawOrigin::Signed(who).into()) | |
| .map(|_| ()) | |
| .map_err(|e| e.error)?; | |
| // Clear the current crowdloan id | |
| CurrentCrowdloanId::<T>::kill(); | |
| // Retrieve the call from the preimage storage | |
| let stored_call = match T::Preimages::peek(call) { | |
| Ok((call, _)) => call, | |
| Err(_) => { | |
| // If the call is not found, we drop it from the preimage storage | |
| // because it's not needed anymore | |
| T::Preimages::drop(call); | |
| return Err(Error::<T>::CallUnavailable)?; | |
| } | |
| }; | |
| // Set the current crowdloan id so the dispatched call | |
| // can access it temporarily | |
| CurrentCrowdloanId::<T>::put(crowdloan_id); | |
| // Dispatch the call with creator origin | |
| let dispatch_result = stored_call | |
| .dispatch(frame_system::RawOrigin::Signed(who).into()) | |
| .map(|_| ()) | |
| .map_err(|e| e.error); | |
| // Clear the current crowdloan id before propagating dispatch errors | |
| CurrentCrowdloanId::<T>::kill(); | |
| dispatch_result?; |
| @@ -258,10 +269,10 @@ mod benchmarks { | |||
| #[extrinsic_call] | |||
| _(RawOrigin::Signed(creator.clone()), crowdloan_id); | |||
There was a problem hiding this comment.
[MEDIUM] Finalize weight still covers only one supported route
finalize now supports two materially different routes: a stored-call dispatch route and a balance-transfer route. This benchmark exercises only the stored-call route, and the extrinsic still charges a single T::WeightInfo::finalize() value. The generated weight therefore does not prove the target-address transfer path is covered, and it also does not charge the actual stored call's dispatch weight at finalization time. A user can select whichever route is more expensive than the measured one while paying the same constant weight.
Split the benchmark/weight by route or make finalize charge the maximum of the supported paths, including the dispatched call's own weight when using the call route.
|
🔄 AI review updated — Skeptic: VULNERABLE |
l0r1s
requested review from
JohnReedV,
evgeny-s,
gztensor,
open-junius and
sam0x17
4 participants
Description
This PR tightens crowdloan finalization configuration and adds an optional per-contributor maximum contribution.
Crowdloans now require exactly one finalization route: either a
callor atarget_address. Creating a crowdloan with both, or with neither, is rejected.finalizealso enforces the same invariant before any transfer or dispatch, so invalid legacy or manually-written storage state cannot execute both paths.This also adds
set_maximum_contribution, a creator-only extrinsic for non-finalized crowdloans. The setting is stored separately fromCrowdloanInfo, so the existing storage struct layout is preserved. Contributions are capped by both the remaining crowdloan cap and the contributor's remaining maximum contribution. The requested contribution must still satisfymin_contribution, but the accepted amount can be lower when filling the remaining cap or remaining per-contributor allowance.Additional changes:
min_contribution <= max_contributionfrom both update directions.benchmark_all.shvariable naming around discovered pallet output paths.spec_versionto410.Breaking Change
pallet_crowdloan::createnow rejects configurations where bothcallandtarget_addressare provided, or where neither is provided. Clients must provide exactly one finalization route.finalizealso rejects stored crowdloans that do not satisfy this invariant before performing side effects. Any existing crowdloan storage with both fields set, or neither field set, must be corrected before it can be finalized.