Remove the LocalAssets pallet-asset instance #2634
Conversation
RomarQ
added
the
B7-runtimenoteworthy
RomarQ
added
D5-nicetohaveaudit⚠️
|
@RomarQ seem's to be a good start, but we also need the migration that remove the pallet storage in the same PR. |
|
To properly revert for all previously "used" addresses of the assets precompile set, you should implement a new precompile set, which the "discriminant" function filtering on the list of addresses (probably hardcoded if they aren't many), and that will always revert. Something like the following: #[precompile_utils::precompile]
#[precompile::precompile_set]
impl<Filter> BlacklistPrecompileSet<Filter>
where
Filter: InstanceFilter<H160>
{
#[precompile::discriminant]
fn discriminant(address: H160, _gas: u64) -> DiscriminantResult<()> {
if Filter::filter(&address) {
return DiscriminantResult::Some((), 0);
}
DiscriminantResult::None(0)
}
#[precompile::fallback]
#[precompile::payable]
fn fallback(_handle: &mut impl PrecompileHandle) -> EvmResult {
Err(revert("disabled precompile"))
}
}Avoid filtering all addresses with the prefix, as it may ban users. |
|
I will give it a try, thanks for the suggestion @nanocryk 👍 |
|
Hi @nanocryk, Is the blacklist really necessary? After digging a bit into it, my understanding is that "destroyed local assets" are already not considered part of the PrecompileSet by this check in the discriminant verification: if pallet_assets::Pallet::<Runtime, Instance>::maybe_total_supply(asset_id.clone())
.is_some()
{
DiscriminantResult::Some(asset_id, extra_cost)
} else {
DiscriminantResult::None(extra_cost) // <------ Excludes the address as being part of the PrecompileSet
}If my understanding is correct, shouldn't be enough to just remove all storage keys prefixed by All local xc-20 addresses that were not destroyed yet would be considered as simple codeless accounts I suspect. |
If it's true it's a bug, destroyed local asset should still revert, otherwise it might open an attack vector to existing contracts that call old xc-20s (by deploying a new contract to the same address)
I think we should still revert for all the addresses that had an xc-20 in the past. @albertov19 do we have a list of all the old xc-20s for moonriver and moonbeam? |
In what sense? The only non-destroyed Mintable XC-20 right now is on Moonbeam with AssetID The other three were destroyed successfully -> https://moonbeam.subscan.io/event?page=1&time_dimension=date&module=localassets&event_id=destroyed |
because it seem's that we have a bug, so we should list all destroyed local assets to replace them by explicit RevertPrecompile, to not break the precompile registry |
Ok cool so the destroyed assets ID are: Asset 1:
Asset 2:
Asset 3:
Asset not yet destroyed (soon to be, allegedly):
|
|
The code that removes the pallet storage is documented here: https://github.com/paritytech/polkadot-sdk/blob/7df1ae3b8111d534cce108b2b405b6a33fcdedc3/substrate/frame/support/src/migrations.rs#L299 |
We should not use that directly, this |
Ok, will add a new extrinsic to |
|
@RomarQ with the merge of the Pr that mvoe all tes tests files the changes become very unreadable :/ Can you exceptionally rebase your branch against master and force push? |
Those moved test files come from this PR (I ended up merging it to this branch since I needed the changes, it is not on master yet): #2636 I can move the tests to another PR if necessary. |
Co-authored-by: Éloïs <c@elo.tf>
Co-authored-by: Éloïs <c@elo.tf>
librelois
added
D9-needsaudit👮
|
@ahmadkaouk can you review this PR? |
noandrea
removed
the
D9-needsaudit👮
What does it do?
Removes the local assets pallet.
LocalAssetsand revert codes of local assets stored inpallet_evm::AccountCodes)Erc20AssetsPrecompileSet<R, IsForeign, ForeignAssetInstance>with another PrecompileSet for reverting calls on all local xc-20 addresses.What important points reviewers should know?
The mintable xc-20s assets were deprecated in #2508
Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
What value does it bring to the blockchain users?