N1xyz Nord Security Audit

Description

Core vulnerability

The Merkle root_from_proof function below, will successfully skip the for-loop's body in case leaves_len == 1 and an empty proof, i.e. proof.len() == 0.

pub fn root_from_proof<L>(
    leaf: L,
    leaf_idx: LeafIdx,
    leaves_len: LeafIdx,
    proof: &[[u8; 32]],
    digest_cb: impl Fn(L, &mut sha2::Sha256),
) -> [u8; 32] {
    let ceil_log2_n: u8 = leaves_len.checked_next_power_of_two().unwrap().ilog2() as u8;

    let mut root = hash_leaf(leaf, &digest_cb);
    let mut i: Height = 0;
    for h in 0..ceil_log2_n {    // @audit skipped if leaves_len == 1
        let begin = subtree_ofs(leaf_idx, h);
        if begin >= leaves_len {
            continue;
        }
        let is_leaf_left = leaf_idx & (1 << h) == 0;
        root = if is_leaf_left {
            hash_pair(root, proof[i as usize])
        } else {
            hash_pair(proof[i as usize], root)
        };
        i += 1;
    }
    assert_eq!(i as usize, proof.len());    // @audit passes if proof.len() == 0
    root    // @audit returns hash_leaf(leaf, &digest_cb) if loop is skipped
}

In this case, the leaf_idx parameter is ignored and the return value is just the digest of the leaf.

Attack vector

The operator who proposes a block can arbitrarily set the withdrawal_root allowing it to be just the digest of the leaf_to_prove. In this case, it suffices to invoke the withdraw instruction below with claim.leaves_count == 1 and empty claim.proof to pass the root check and proceed with the withdrawal.

pub fn withdraw(ctx: Context<Withdraw>, claim: WithdrawalClaim) -> Result<()> {
    let root = ctx.accounts.state_update.facts.withdrawal_root;

    let leaf_to_prove = TransferParams {
        user: claim.user,
        mint: ctx.accounts.to_account.mint,
        amount: claim.amount,
    };
    let computed_root = crate::merkle::root_from_proof(
        &leaf_to_prove,
        claim.leaf_index,
        claim.leaves_count,
        &claim.proof,
        TransferParams::digest,
    );
    if root != computed_root {    // @audit will pass if withdrawal_root is digest of leaf_to_prove 
        return err!(BridgeError::InvalidWithdrawalProof);
    }

    ...
}

In order to avoid multiple withdrawals reusing the same block, the Withdraw context implements the withdrawal_nullifier below.

#[derive(Accounts)]
#[instruction(claim: WithdrawalClaim)]
pub struct Withdraw<'info> {
    ...

    #[account(
        init,
        payer = payer,
        space = 8,
        seeds = [WITHDRAWAL_NULLIFIER_SEED, &claim.block_id.to_le_bytes(), &claim.leaf_index.to_le_bytes()],
        bump,
    )]
    pub withdrawal_nullifier: Account<'info, WithdrawalNullifier>,

    ...
}

However, in case of an empty proof as discussed above, the claim.leaf_index is ignored in the root computation and therefore can be set to arbitrary values to bypass the withdrawal_nullifier allowing multiple withdrawals.

Recommendation

It is recommended to incorporate the leaf_idx into the root computation in any case and assert that leaves_len > leaf idx to also evaluate the leaf_idx in case of an empty proof.

Description

Once a block is proposed by the operator, which might malicious e.g. containing a specifically crafted withdrawal root, anyone can immediately invoke the finalize_block instruction to finalize it.

Consequently, a malicious withdrawal can be performed immediately after proposing such a block.

Recommendation

It is recommended to introduce a challenge as well as a validity-proof mechanism that allows validators to challenge a block and prove its validity before it can be finalized.

Description

The initialize instruction and its account context impose no restrictions on the signer.
Consequently, the first one to invoke initialize after deployment is in charge of setting the protocol's operator role.

Recommendation

It is recommended to add the following code to the Initialize context to require co-signing of the initialize instruction with the program's private key (keypair) that should only be known to the deployer.

#[account(address = crate::ID)]
pub program: Signer<'info>

Description

The permissionless deposit_spl instruction imposes no restrictions on the amount.
Consequently, anyone can spam deposits with zero or dust amounts thereby unnecessarily increasing the last_deposit_index.

Recommendation

It is recommended to introduce a minimum deposit amount.

Description

The following shift operation in the subtree_ofs function can be performed with any given tree height h.

let mask0: LeafIdx = 1 << h;

However, the LeafIdx type is defined as u64. Consequently, any tree height h >= 64 leads to invalid results.

Recommendation

It is recommended to assert that h < 64.

Description

The following vulnerable crate dependencies were identified using cargo audit:

1. Timing variability in curve25519-dalek's Scalar29::sub/Scalar52::sub

Crate:     curve25519-dalek
Version:   3.2.0
Title:     Timing variability in `curve25519-dalek`'s `Scalar29::sub`/`Scalar52::sub`
Date:      2024-06-18
ID:        RUSTSEC-2024-0344
URL:       https://rustsec.org/advisories/RUSTSEC-2024-0344
Solution:  Upgrade to >=4.1.3

2. Double Public Key Signing Function Oracle Attack on ed25519-dalek

Crate:     ed25519-dalek
Version:   1.0.1
Title:     Double Public Key Signing Function Oracle Attack on `ed25519-dalek`
Date:      2022-06-11
ID:        RUSTSEC-2022-0093
URL:       https://rustsec.org/advisories/RUSTSEC-2022-0093
Solution:  Upgrade to >=2

3. ssl::select_next_proto use after free

Crate:     openssl
Version:   0.10.68
Title:     `ssl::select_next_proto` use after free
Date:      2025-02-02
ID:        RUSTSEC-2025-0004
URL:       https://rustsec.org/advisories/RUSTSEC-2025-0004
Solution:  Upgrade to >=0.10.70

4. Marvin Attack: potential key recovery through timing sidechannels

Crate:     rsa
Version:   0.9.7
Title:     Marvin Attack: potential key recovery through timing sidechannels
Date:      2023-11-22
ID:        RUSTSEC-2023-0071
URL:       https://rustsec.org/advisories/RUSTSEC-2023-0071
Severity:  5.9 (medium)
Solution:  No fixed upgrade is available!

Recommendation

It is recommended to upgrade the affected crate dependencies if applicable and viable in terms of compatibility.