Potential error in nullifier logic

[merkle-groot]

// TODO(security): ensure noteHash does not already exist in the noteHashTree. If it exists, the tx will never be rolled up and the money will be lost.

pi.push(note.noteHash);

I came across this comment while reading the code. I'm guessing the thinking here was that even if a shield function is called multiple times with the same proof, the user would only be able to spend the note once, since the nullifier is unique and can't be added multiple times to the indexed Merkle tree.

However, looking at the code of insert_subtree_to_snapshot_tree inside indexed_tree.nr in the Aztec libs, the Noir code doesn't throw an error if a duplicate nullifier is provided. Instead, it simply updates the existing leaf:

let is_update = value_key == low_leaf_key;
// ...
// Calculate the new value of the low_leaf
let updated_low_leaf = if is_update {
    low_leaf_preimage.update_value(value)
} else {
    low_leaf_preimage.update_pointers(
        value_key,
        start_insertion_index as u32 + value_index,
    )
};

Please correct me if I'm wrong, but I believe this could potentially allow a replay attack to drain the contract.
Maybe we need a non-membership proof before adding it to the indexed tree?

[olehmisar]

The codebase is highly experimental and undertested. This is rather a POC. IIRC, I did some experiments and the Noir code did throw an error if the same nullifier was inserted multiple times. Maybe aztec team changed the behaviour between versions

[merkle-groot]

That makes sense, I'll try to replicate the duplicate nullifier behaviour in tests and get back here.