Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addresses the boundary conditions for the Masp VP predicate #1104

Merged
merged 2 commits into from
Apr 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions wasm/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions wasm/wasm_source/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ wee_alloc = "0.4.5"
getrandom = { version = "0.2", features = ["custom"] }
masp_proofs = { git = "https://github.com/anoma/masp", rev = "bee40fc465f6afbd10558d12fe96eb1742eee45c", optional = true }
masp_primitives = { git = "https://github.com/anoma/masp", rev = "bee40fc465f6afbd10558d12fe96eb1742eee45c", optional = true }
ripemd160 = "0.9.1"

[dev-dependencies]
namada = {path = "../../shared"}
Expand Down
148 changes: 139 additions & 9 deletions wasm/wasm_source/src/vp_masp.rs
Original file line number Diff line number Diff line change
@@ -1,25 +1,67 @@
use std::cmp::Ordering;

use masp_primitives::asset_type::AssetType;
use masp_primitives::transaction::components::Amount;
use masp_primitives::legacy::TransparentAddress::{PublicKey, Script};
use masp_primitives::transaction::components::{Amount, TxOut};
/// Multi-asset shielded pool VP.
use namada_vp_prelude::address::masp;
use namada_vp_prelude::storage::Epoch;
use namada_vp_prelude::*;
use ripemd160::{Digest, Ripemd160};

/// Generates the current asset type given the current epoch and an
/// unique token address
fn asset_type_from_epoched_address(epoch: Epoch, token: &Address) -> AssetType {
// Timestamp the chosen token with the current epoch
let token_bytes = (token, epoch.0)
.try_to_vec()
.expect("token should serialize");
// Generate the unique asset identifier from the unique token address
AssetType::new(token_bytes.as_ref()).expect("unable to create asset type")
}

/// Checks if the asset type matches the expected asset type, Adds a
/// debug log if the values do not match.
fn valid_asset_type(
asset_type: &AssetType,
asset_type_to_test: &AssetType,
) -> bool {
let res =
asset_type.get_identifier() == asset_type_to_test.get_identifier();
if !res {
debug_log!(
"The asset type must be derived from the token address and \
current epoch"
);
}
res
}

/// Checks if the reported transparent amount and the unshielded
/// values agree, if not adds to the debug log
fn valid_transfer_amount(
reporeted_transparent_value: u64,
unshielded_transfer_value: u64,
) -> bool {
let res = reporeted_transparent_value == unshielded_transfer_value;
if !res {
debug_log!(
"The unshielded amount {} disagrees with the calculated masp \
transparented value {}",
unshielded_transfer_value,
reporeted_transparent_value
)
}
res
}

/// Convert Namada amount and token type to MASP equivalents
fn convert_amount(
epoch: Epoch,
token: &Address,
val: token::Amount,
) -> (AssetType, Amount) {
// Timestamp the chosen token with the current epoch
let token_bytes = (token, epoch.0)
.try_to_vec()
.expect("token should serialize");
// Generate the unique asset identifier from the unique token address
let asset_type = AssetType::new(token_bytes.as_ref())
.expect("unable to create asset type");
let asset_type = asset_type_from_epoched_address(epoch, token);
// Combine the value and unit into one amount
let amount = Amount::from_nonnegative(asset_type, u64::from(val))
.expect("invalid value or asset type for amount");
Expand Down Expand Up @@ -55,7 +97,32 @@ fn validate_tx(
transparent_tx_pool += shielded_tx.value_balance.clone();

// Handle shielding/transparent input
// The following boundary conditions must be satisfied
// 1. One transparent input
// 2. Zero transparent output
// 3. the transparent transaction value pool's amount must equal the
// containing wrapper transaction's fee amount
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where do we check that condition (3) is met? It seems to me that there should be some mention of the wrapper transaction's fee somewhere in this code?

if transfer.source != masp() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where do we check for the for the following condition?: If the source address is the MASP validity predicate, then no transparent inputs are permitted in the shielded transaction.

It seems to me that if the source and target are MASP, then both the conditional on line 105 and line 146 are skipped. This would allow for these sorts of transactions to have unlimited transparent inputs and outputs.

// Satisfies 1.
if shielded_tx.vin.len() != 1 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that we no longer intend to check the amount and asset type of the transparent input, this condition is no longer necessary. The value balance checks will ensure that the user has indeed provided sufficient inputs.

debug_log!(
"Transparent input to a transaction from the masp must be \
1 but is {}",
shielded_tx.vin.len()
);
return reject();
}

// Satisfies 2.
if !shielded_tx.vout.is_empty() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check enforces that transactions with a non-MASP source must have no transparent outputs. While this check can be done, the transaction would still be valid (i.e. leaving the shielded pool and transparent pool in a consistent state) without it. Perhaps we can remove this check to simplify the logic?

debug_log!(
"Transparent output to a transaction from the masp must \
be 0 but is {}",
shielded_tx.vin.len()
);
return reject();
}

// Note that the asset type is timestamped so shields
// where the shielded value has an incorrect timestamp
// are automatically rejected
Expand All @@ -70,9 +137,50 @@ fn validate_tx(
}

// Handle unshielding/transparent output
// The following boundary conditions must be satisfied
// 1. Zero transparent inupt
// 2. One transparent output
// 3. Asset type must be properly derived
// 4. Value from the output must be the same as the containing transfer
// 5. Public key must be the hash of the target
if transfer.target != masp() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where do we check for the following condition?: If the target address is the MASP validity predicate, then no transparent outputs are permitted in the shielded transaction.

It seems to me that if the source and target are MASP, then both the conditional on line 105 and line 146 are skipped. This would allow for these sorts of transactions to have unlimited transparent inputs and outputs.

// Satisfies 1.
if !shielded_tx.vin.is_empty() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check enforces that transactions with non-MASP targets mut not have any transparent inputs. While this check can be done, the transaction would still be valid (i.e. leaving the shielded pool and transparent pool in a consistent state) without it. Perhaps we can remove this check to simplify the logic?

debug_log!(
"Transparent input to a transaction to the masp must be 0 \
but is {}",
shielded_tx.vin.len()
);
return reject();
}

// Satisfies 2.
if shielded_tx.vout.len() != 1 {
debug_log!(
"Transparent output to a transaction to the masp must be \
1 but is {}",
shielded_tx.vin.len()
);
return reject();
}

let out: &TxOut = &shielded_tx.vout[0];

let expected_asset_type: AssetType =
asset_type_from_epoched_address(
ctx.get_block_epoch().unwrap(),
&transfer.token,
);

// Satisfies 3. and 4.
if !(valid_asset_type(&expected_asset_type, &out.asset_type)
&& valid_transfer_amount(out.value, u64::from(transfer.amount)))
{
return reject();
}

// Timestamp is derived to allow unshields for older tokens
let atype =
let atype: &AssetType =
Copy link
Collaborator

Choose a reason for hiding this comment

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

This use of .components().next().unwrap() seems to be unsafe. What guarantees that shielded_tx.value_balance has at least one component? And when there are multiple components, what does the first extracted component mean semantically?

shielded_tx.value_balance.components().next().unwrap().0;

let transp_amt =
Expand All @@ -81,6 +189,28 @@ fn validate_tx(

// Non-masp destinations subtract from transparent tx pool
transparent_tx_pool -= transp_amt;

// Satisfies 5.
match out.script_pubkey.address() {
None | Some(Script(_)) => {}
Some(PublicKey(pub_bytes)) => {
let target_enc = transfer
.target
.try_to_vec()
.expect("target address encoding");

let hash =
Ripemd160::digest(sha256(&target_enc).as_slice());

if <[u8; 20]>::from(hash) != pub_bytes {
debug_log!(
"the public key of the output account does not \
match the transfer target"
);
return reject();
}
}
}
}

match transparent_tx_pool.partial_cmp(&Amount::zero()) {
Expand Down