Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

Audit-5: validate meta data of PNA #183

Open
wants to merge 2 commits into
base: audit-polkadot-stable2409
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions bridges/snowbridge/pallets/system/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ pub mod pallet {
InvalidTokenTransferFees,
InvalidPricingParameters,
InvalidUpgradeParameters,
InvalidMetadata,
}

/// The set of registered agents
Expand Down Expand Up @@ -624,6 +625,7 @@ pub mod pallet {
metadata: AssetMetadata,
) -> DispatchResultWithPostInfo {
ensure_root(origin)?;
ensure!(metadata.validate(), Error::<T>::InvalidMetadata);

let location: Location =
(*location).try_into().map_err(|_| Error::<T>::UnsupportedLocationVersion)?;
Expand Down
12 changes: 10 additions & 2 deletions bridges/snowbridge/pallets/system/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ use frame_support::{
derive_impl, parameter_types,
traits::{tokens::fungible::Mutate, ConstU128, ConstU8},
weights::IdentityFee,
PalletId,
BoundedVec, PalletId,
};
use sp_core::H256;
use xcm_executor::traits::ConvertLocation;

use snowbridge_core::{
gwei, meth, outbound::ConstantGasMeter, sibling_sovereign_account, AgentId, AllowSiblingsOnly,
ParaId, PricingParameters, Rewards,
AssetMetadata, ParaId, PricingParameters, Rewards,
};
use sp_runtime::{
traits::{AccountIdConversion, BlakeTwo256, IdentityLookup, Keccak256},
Expand Down Expand Up @@ -253,3 +253,11 @@ pub fn make_agent_id(location: Location) -> AgentId {
<Test as snowbridge_system::Config>::AgentIdOf::convert_location(&location)
.expect("convert location")
}

pub fn mock_asset_meta() -> AssetMetadata {
AssetMetadata {
name: BoundedVec::try_from("SDOT".as_bytes().to_vec()).unwrap(),
symbol: BoundedVec::try_from("SDOT".as_bytes().to_vec()).unwrap(),
decimals: 10,
}
}
4 changes: 2 additions & 2 deletions bridges/snowbridge/pallets/system/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,7 @@ fn register_all_tokens_succeeds() {
assert_ok!(EthereumSystem::register_token(
origin,
Box::new(versioned_location),
Default::default()
mock_asset_meta()
));

assert_eq!(NativeToForeignId::<Test>::get(tc.reanchored.clone()), Some(tc.foreign));
Expand Down Expand Up @@ -739,7 +739,7 @@ fn register_ethereum_native_token_fails() {
);
let versioned_location: Box<VersionedLocation> = Box::new(location.clone().into());
assert_noop!(
EthereumSystem::register_token(origin, versioned_location, Default::default()),
EthereumSystem::register_token(origin, versioned_location, mock_asset_meta()),
Error::<Test>::LocationConversionFailed
);
});
Expand Down
6 changes: 6 additions & 0 deletions bridges/snowbridge/primitives/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,12 @@ pub struct AssetMetadata {
pub decimals: u8,
}

impl AssetMetadata {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nonblocking: I would prefer to leave AssetMetadata as just plain old data and move this to the call site in the extrinsic. We do not re-use this validate method so its not needed. And I think we should add a test for the extrinsic to make sure the Error is raised in this case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pub fn validate(&self) -> bool {
self.decimals > 0 && self.name.len() > 0 && self.symbol.len() > 0
}
}

#[cfg(any(test, feature = "std", feature = "runtime-benchmarks"))]
impl Default for AssetMetadata {
fn default() -> Self {
Expand Down
Loading