From 5682f9a273309c7160722a7937a75152a2793b23 Mon Sep 17 00:00:00 2001 From: Alistair Singh Date: Thu, 24 Oct 2024 14:27:01 +0200 Subject: [PATCH] Snowbridge: PNA Audit Better Documentation and minor Refactorings (#6216) # Description Snowbridge PNA has been audited. A number of issues where raised due to not understanding the fee model for Polkadot Native Assets(PNA) implementation. This PR addresses this by adding more comments and better naming of private functions. ## Integration None, documentation and private method name changes. --- .../primitives/router/src/inbound/mod.rs | 2 ++ .../primitives/router/src/outbound/mod.rs | 26 ++++++++++++++----- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/bridges/snowbridge/primitives/router/src/inbound/mod.rs b/bridges/snowbridge/primitives/router/src/inbound/mod.rs index fbfc52d01c83..a9324ac42470 100644 --- a/bridges/snowbridge/primitives/router/src/inbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/inbound/mod.rs @@ -416,6 +416,8 @@ where // Final destination is a 32-byte account on AssetHub Destination::AccountId32 { id } => Ok(Location::new(0, [AccountId32 { network: None, id }])), + // Forwarding to a destination parachain is not allowed for PNA and is validated on the + // Ethereum side. https://github.com/Snowfork/snowbridge/blob/e87ddb2215b513455c844463a25323bb9c01ff36/contracts/src/Assets.sol#L216-L224 _ => Err(ConvertMessageError::InvalidDestination), }?; diff --git a/bridges/snowbridge/primitives/router/src/outbound/mod.rs b/bridges/snowbridge/primitives/router/src/outbound/mod.rs index efc1ef56f304..3b5dbdb77c89 100644 --- a/bridges/snowbridge/primitives/router/src/outbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/outbound/mod.rs @@ -207,9 +207,9 @@ where fn convert(&mut self) -> Result<(Command, [u8; 32]), XcmConverterError> { let result = match self.peek() { - Ok(ReserveAssetDeposited { .. }) => self.send_native_tokens_message(), + Ok(ReserveAssetDeposited { .. }) => self.make_mint_foreign_token_command(), // Get withdraw/deposit and make native tokens create message. - Ok(WithdrawAsset { .. }) => self.send_tokens_message(), + Ok(WithdrawAsset { .. }) => self.make_unlock_native_token_command(), Err(e) => Err(e), _ => return Err(XcmConverterError::UnexpectedInstruction), }?; @@ -222,7 +222,9 @@ where Ok(result) } - fn send_tokens_message(&mut self) -> Result<(Command, [u8; 32]), XcmConverterError> { + fn make_unlock_native_token_command( + &mut self, + ) -> Result<(Command, [u8; 32]), XcmConverterError> { use XcmConverterError::*; // Get the reserve assets from WithdrawAsset. @@ -271,7 +273,12 @@ where ensure!(reserve_assets.len() == 1, TooManyAssets); let reserve_asset = reserve_assets.get(0).ok_or(AssetResolutionFailed)?; - // If there was a fee specified verify it. + // Fees are collected on AH, up front and directly from the user, to cover the + // complete cost of the transfer. Any additional fees provided in the XCM program are + // refunded to the beneficiary. We only validate the fee here if its provided to make sure + // the XCM program is well formed. Another way to think about this from an XCM perspective + // would be that the user offered to pay X amount in fees, but we charge 0 of that X amount + // (no fee) and refund X to the user. if let Some(fee_asset) = fee_asset { // The fee asset must be the same as the reserve asset. if fee_asset.id != reserve_asset.id || fee_asset.fun > reserve_asset.fun { @@ -328,7 +335,9 @@ where /// # BuyExecution /// # DepositAsset /// # SetTopic - fn send_native_tokens_message(&mut self) -> Result<(Command, [u8; 32]), XcmConverterError> { + fn make_mint_foreign_token_command( + &mut self, + ) -> Result<(Command, [u8; 32]), XcmConverterError> { use XcmConverterError::*; // Get the reserve assets. @@ -377,7 +386,12 @@ where ensure!(reserve_assets.len() == 1, TooManyAssets); let reserve_asset = reserve_assets.get(0).ok_or(AssetResolutionFailed)?; - // If there was a fee specified verify it. + // Fees are collected on AH, up front and directly from the user, to cover the + // complete cost of the transfer. Any additional fees provided in the XCM program are + // refunded to the beneficiary. We only validate the fee here if its provided to make sure + // the XCM program is well formed. Another way to think about this from an XCM perspective + // would be that the user offered to pay X amount in fees, but we charge 0 of that X amount + // (no fee) and refund X to the user. if let Some(fee_asset) = fee_asset { // The fee asset must be the same as the reserve asset. if fee_asset.id != reserve_asset.id || fee_asset.fun > reserve_asset.fun {