6.1 Burning users' funds with incorrect nullifiers
The transfer circuit, when receiving zero in the valueIn parameter, disables checks for nullifiers and the Merkle tree inclusion of leaves. However, this allows for creating a proof of transaction with nullifiers of another user. This transaction can be executed on-chain with the correct proof, and malicious nullifiers could be used to nullify another user's leaves, leading to the burning of the user's funds. The knowledge of nullifiers would not be difficult, as users post their nullifiers during selective disclosure.
Recommendation
Consider removing the isDummy mechanism. Alternatively, do not allow passing zero values on deposit/transfer and execute checks of nullifiers and Merkle tree inclusion for any value, including zero.
Resolution
The isDummy mechanism is removed.
6.2 Token which was not deposited can be transferred
When the valueIn provided to the circuit is 0, the circuit does not perform checks on the Merkle tree inclusion of notes or nullifiers' correctness. If both valueIn and valueOut are provided as zero, the sums would be equal. The malicious user can provide a token that was not deposited by them since the Merkle tree inclusion is not checked. This may lead to a few vulnerabilities during a transfer.
Scenario 1: The provided token may be an NFT token identifier: hash(tokenType: ERC721, tokenAddress, tokenSubID) that exists in a wallet and was deposited by another user, e.g., hash(ERC721, 0x123, ID: 1). In the case of valueIn equals to valueOut equals to 0 provided in the circuit, the malicious user can provide such token and the circuit would execute correctly.
The proved input note would look like: note equals to hash(holder_npk, token: hash(ERC721, 0x123, ID: 1), value: 0) - such note would not exist in the tree (all ERC721 tokens must have value = 1 during deposit) but since the Merkle tree inclusion is not checked the proof would be generated correctly. The malicious user could provide this token (deposited by another user) in commitmentsOut: hash(malicious_receiver_address, token: hash(ERC721, 0x123, ID: 1), value: 0).
During the on-chain verification, the invalid input note would be added to a tree, invalid nullifiers would be marked, and the transfer would be executed correctly, effectively allowing to steal the token from another user. Note that the note is not checked to have value == 1 during the public transfer.
Scenario 2: Another possible scenario is when a malicious user correctly deposited their NFT to the wallet and created a note: hash(ERC721, 0x123, ID: 1). Then they use the exploit described above where the valueIn == valueOut == 0 and provided commitmentsIn and nullifiers are incorrect and do not point to the correct note. The user transfers this NFT from the wallet, but the corresponding nullifier is unused. Then, the malicious user can use selective transparency to prove that they still hold the NFT in the wallet since the correct nullifier was not used.
The provided token may be an Ethereum address that does not hold any code. If such a transfer is public and called in a batch, then the ERC20 safe transfer would revert and the batch. This could lead to DOS of batches of transactions executed by a third party.
Recommendation
Consider removing the isDummy mechanism. Alternatively consider checking for Merkle root inclusion and nullifier correctness on every value, including zero. Additionally, consider asserting that the note.value equals 1 during the transfer of ERC721.
Resolution
The isDummy mechanism is removed.
6.3 The feature user-controlled selective disclosure fails when there is no nullifier
The SelectiveTransparency circuit verifies that a user owns more than a specified threshold amount ( valueCheck) of a given token. In this design, the use of a nullifier is optional. The public inputs to this feature include:
merkleRoot, boundParamsHash, nullifiers[nInputs], valueCheck, token
As the valueCheck input must always be greater than zero, the proof cannot be generated if there is no nullifier for input notes. The scenarios and their implications are outlined below:
Scenario 1: nInputs > 0 and valueCheck > 0. In this case, the prover will fail due to the inability to generate the signature. The message hash to be signed is constructed as follows:
messageHash = hash(merkleRoot, boundParamsHash, nullifiers)
Here, nullifiers is a list of length nInputs. If the list does not contain nullifiers, the message generation for signing fails. Even if the message is signed with this list without any nullifier, the *Nullifier Check* step must be skipped because there is no nullifier to verify. For this step to succeed, the condition isDummy[i].out = 0 must hold for all indices in the range [0:nInputs). The process for handling nullifiers is defined as:
for (var i = 0; i < nInputs; i++) {
nullifiersHash[i] = NullifierCheck();
nullifiersHash[i].nullifyingKey <== nullifyingKey;
nullifiersHash[i].leafIndex <== leavesIndices[i];
nullifiersHash[i].nullifier <== nullifiers[i];
// @audit To disable the nullifier check for the whole nullifiers array, isDummy[i].out should always be zero
nullifiersHash[i].enabled <== 1-isDummy[i].out;
}
The isDummy component is set with valueIn[nInputs] private input to verify against zero values. Consequently, valueIn must be zero for all indices in the range [0:nInputs]. This results in a sumIn = 0, causing the verification sumIn === valueCheck to fail.
Scenario 2: nInputs = 0 and valueCheck > 0 This scenario also fails. With nInputs = 0, all inputs associated with it such as, nullifiers[nInputs] and private list inputs (e.g., valueIn[nInputs]) are empty. The message hash for signature generation simplifies to:
messageHash = hash(merkleRoot, boundParamsHash)
As nInputs = 0, nullifier and Merkle proof checks are skipped. The sumIn value remains zero, and the verification sumIn === valueCheck fails.
Scenario 3: nInputs = 0 and valueCheck = 0 This scenario passes. With nInputs = 0, all inputs associated with it, such as nullifiers[nInputs] and private list inputs (e.g., valueIn[nInputs]), are empty. The message hash for signature generation simplifies to:
messageHash = hash(merkleRoot, boundParamsHash)
As nInputs = 0, nullifier and Merkle proof checks are skipped. The sumIn value remains zero, and the verification sumIn === valueCheck passes. This allows a user to provide no nullifiers and generate a valid proof.
Recommendation
Ensure valueCheck is greater than zero.
6.4 The nullifiers are not checked during the selective-transparency
The selective-transparency circuit allows users to prove holding some amount of funds. However, the nullifiers are not checked on-chain if they have not been used before. In effect, it is proved that the user held the disclosed amount at some point in history, but it is not ensured that the user still holds those funds. Note that anyone can still check the nullifiers' status on-chain but expects to perform additional checks from the information receiver. As a result, users can lie that they hold some value even if it was already spent if the information recipient won't compare it with the on-chain state.
Recommendation
Consider adding on-chain logic to ensure that the nullifiers are not already used.
Resolution
Added the check on-chain to check if the nullifier is used or not. Commit: c9f5f2b
6.5 The strict equality check in selective transparency may lead to data leak
The selective-transparency circuit does a strict equality check at the end, checking if the sumIn equals the public input valueCheck. This may lead to data leakage since if the user provided only one input note, it must be exactly equal to the valueCheck, revealing the exact note held in the tree.
Recommendation
Consider changing the strict equality check to a greater or equal one to ensure that data in the note is not revealed.
Resolution
Fixed the equality check from equal to greater or equal.
6.6 Centralization Risk
The verification keys are set by the owner of the Verifier.sol contract, and it is possible to change the owner due to OwnableUpgradeable. Therefore, the new or original owner could maliciously change the values of each verifying key, thus preventing the circuits from verifying.
Recommendation
Create a function that allows only adding to the list of verifying keys.
Resolution
We need the possibility for removing old keys if a vulnerability is found in a circuit so we can override a verification key for a certain number of inputs and outputs.
6.7 Not all public inputs are verified in the signature check
The SelectiveTransparency computes the hash over public inputs to generate the signed message. These signals are listed below:
template SelectiveTransparency(nInputs, nOutputs, MerkleTreeDepth) {
/* ========== PUBLIC SIGNALS ========== */
signal input merkleRoot;
signal input boundParamsHash;
signal input nullifiers[nInputs];
signal input valueCheck;
signal input token;
...
}
However, the computed hash does not contain valueCheck and token:
// 1. Compute hash over public signals to get the signed message
var size = nInputs + 2;
component messageHash = Poseidon(size);
messageHash.inputs[0] <== merkleRoot;
messageHash.inputs[1] <== boundParamsHash;
for (var i = 0; i < nInputs; i++) {
messageHash.inputs[i+2] <== nullifiers[i];
}
// @audit valueCheck and token are not hashed
// 2. Verify EDDSA signature over hash of public inputs
component eddsaVerifier = EdDSAPoseidonVerifier();
eddsaVerifier.enabled <== 1;
eddsaVerifier.Ax <== publicKey[0];
eddsaVerifier.Ay <== publicKey[1];
eddsaVerifier.R8x <== signature[0];
eddsaVerifier.R8y <== signature[1];
eddsaVerifier.S <== signature[2];
eddsaVerifier.M <== messageHash.out;
If the message signed contains both valueCheck and token, the signature verification step will fail.
Recommendation
Add valueCheck and token to messageHash.
Resolution
All public inputs are added to the signature now.
6.8 nOutputs is not needed in SelectiveTransparency
The nOutputs is not used in the selective transparency logic but is passed as an argument for the circuit.
template SelectiveTransparency(nInputs, nOutputs, MerkleTreeDepth) {
Recommendation
Remove the nOutputs
Resolution
Removed.
