namada icon indicating copy to clipboard operation
namada copied to clipboard

Masp/Base boundary conditions

Open CarloModicaPortfolio opened this issue 1 year ago • 1 comments

Looking at the MASP VP, and it looks like some of the coundary conditions are not checked. More specifically:

According to specs: If the source address is not the MASP validity predicate, then the transparent transaction value pool's amount must equal zero. But in vp_masp.rs:

        if transfer.source != masp() {
            // Handle transparent input
            // Note that the asset type is timestamped so shields
            // where the shielded value has an incorrect timestamp
            // are automatically rejected
            for denom in token::MaspDenom::iter() {
                let (_transp_asset, transp_amt) = convert_amount(
                    ctx.get_block_epoch().unwrap(),
                    &transfer.token,
                    transfer.amount.into(),
                    denom,
                );

                // Non-masp sources add to transparent tx pool
                transparent_tx_pool += transp_amt;
           

looks like that transparent_tx_pool is never checked to be zero.

Similarly when the source is the MASP vp, both according to specs and code comment the transparent transaction value pool's amount must equal the containing wrapper transaction's fee amount. This checks seems to be missing in the code. Nor seems to be checked if the transparent transaction value pool's asset type is be derived from the containing wrapper transaction's fee token. The code below:

            // Handle shielded input
            // The following boundary conditions must be satisfied
            // 1. Zero transparent input
            // 2. the transparent transaction value pool's amount must equal< the
            // containing wrapper transaction's fee amount
            // Satisfies 1.
            if let Some(transp_bundle) = shielded_tx.transparent_bundle() {
                if !transp_bundle.vin.is_empty() {
                    debug_log!(
                        "Transparent input to a transaction from the masp \
                         must be 0 but is {}",
                        transp_bundle.vin.len()
                    );
                    return reject();

Lastly, there is a missmatch between specs and code when the target adress is not the MASP vp, as the specs say there must be exactly one transparent output, while in the code there can be up to 4.

CarloModicaPortfolio avatar Oct 02 '23 16:10 CarloModicaPortfolio

With regards to the fees, the logic in the code should be correct as we decided to not allow fee payment directly in a masp transfer, the check is here:

https://github.com/anoma/namada/blob/a07a3b2bf7a8e77732f23309ebd8dc8fa28e0aa3/shared/src/ledger/native_vp/masp.rs#L280-L284

I probably forgot to remove the misleading comment and update the specs

grarco avatar Nov 27 '23 10:11 grarco

Superseded by more recent work.

cwgoes avatar Feb 02 '24 16:02 cwgoes