Fix withdraw() methods in nullifier contracts for not validating the root

[jimthematrix]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.37%. Comparing base (8d6263c) to head (bb64054).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   70.44%   71.37%   +0.92%     
==========================================
  Files          12       12              
  Lines         538      538              
==========================================
+ Hits          379      384       +5     
+ Misses        112      110       -2     
+ Partials       47       44       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Chengxuan]

@jimthematrix 👍 good catch, added a comment about testing.

Also, a minor point about the three functions in withdraw seem can be grouped into a _withdraw() function into zeto_nullifier like _mint.

[Chengxuan]

Should we also add a test for this scenario?

[jimthematrix]

good point. added failure tests for all of the nullifiers tokens attempting to withdraw using spent UTXOs

[EnriqueL8]

This looks good to me - agree on the point above from Cheng but understand they are similar flows and tests have been included

[jimthematrix]

just added another commit to also validate the input from the withdraw() calls of the non-nullifier tokens, which turn out to have the same security hole.

@Chengxuan @EnriqueL8

[Chengxuan]

Thanks @jimthematrix

It seems the security hole had to be patched to multiple contracts, so considering refactoring the check into the shared library will make it more reusable. But it's a minor comment for you to consider.

[jimthematrix]

Regarding the idea to push the calls in the withdraw() method to a library, the issue is that the 3 function calls already came from 2 parent classes:

        validateTransactionProposal(inputs, [output, 0], proof);   <<-- this is from Zeto_Base/Zeto_Nullifier
        _withdraw(amount, inputs, output, proof);                  <<-- this is from Zeto_Fungible
        processInputsAndOutputs(inputs, [output, 0]);              <<-- this is from Zeto_Base/Zeto_Nullifier