Remove Double Checks for Commitments

Author: agureev

contracts/src/ProtocolAdapter.sol

@@ -195,13 +195,7 @@ contract ProtocolAdapter is IProtocolAdapter, ReentrancyGuardTransient, Commitme
                     }
 
                     {
-                        // Check that the commitment does not already exist
-                        tags.checkCommitmentNonExistence(cm);
-
-                        // Check that the commitment does not exist in the commitment accumulator
-                        _checkCommitmentNonExistence(cm);
-
-                        // Add the nullifier to the list of tags
+                        // Add the commitment to the list of tags
                         tags[resCounter++] = cm;
                         actionTreeTags[(2 * j) + 1] = cm;
                     }

contracts/src/libs/TagLookup.sol

@@ -37,13 +37,4 @@ library TagLookup {
             revert NullifierDuplicated(nullifier);
         }
     }
-
-    /// @notice Checks if a commitment is non-existent in an array of tags and reverts if it not.
-    /// @param tags The tags array to check.
-    /// @param commitment The commitment to check non-existence for.
-    function checkCommitmentNonExistence(bytes32[] memory tags, bytes32 commitment) internal pure {
-        if (isFoundInEvenOrOddPosition({tags: tags, tag: commitment, even: false})) {
-            revert CommitmentDuplicated(commitment);
-        }
-    }
 }

contracts/test/ProtocolAdapter.t.sol

@@ -83,21 +83,6 @@ contract ProtocolAdapterTest is Test {
         _pa.verify(txn);
     }
 
-    function test_verify_reverts_on_action_with_duplicated_commitments() public {
-        Action[] memory actions = new Action[](1);
-        actions[0] = _actionWithDuplicatedComplianceUnit();
-
-        bytes32 duplicatedCommitment = actions[0].complianceVerifierInputs[0].instance.created.commitment;
-        actions[0].complianceVerifierInputs[1].instance.consumed.nullifier = keccak256("Not a duplicate");
-
-        Transaction memory txn = Transaction({actions: actions, deltaProof: ""});
-
-        vm.expectRevert(
-            abi.encodeWithSelector(TagLookup.CommitmentDuplicated.selector, duplicatedCommitment), address(_pa)
-        );
-        _pa.verify(txn);
-    }
-
     function test_verify_empty_tx() public view {
         Transaction memory txn = Transaction({actions: new Action[](0), deltaProof: ""});
         _pa.verify(txn);

contracts/test/ProtocolAdapterMock.t.sol

@@ -170,6 +170,7 @@ contract ProtocolAdapterMockTest is Test {
         (Transaction memory tx1, uint256 updatedNonce) =
             _mockVerifier.transaction({nonce: 0, configs: configs, commitmentTreeDepth: _TEST_COMMITMENT_TREE_DEPTH});
         bytes32 preExistingNf = tx1.actions[0].complianceVerifierInputs[0].instance.consumed.nullifier;
+        bytes32 preExistingCm = tx1.actions[0].complianceVerifierInputs[0].instance.created.commitment;
         _mockPa.execute(tx1);
 
         (Transaction memory tx2,) = _mockVerifier.transaction({
@@ -184,48 +185,17 @@ contract ProtocolAdapterMockTest is Test {
         _mockPa.verify(tx2);
     }
 
-    function test_verify_reverts_on_pre_existing_commitment() public {
-        TxGen.ActionConfig[] memory configs = TxGen.generateActionConfigs({nActions: 1, nCUs: 1});
-
-        (Transaction memory tx1, uint256 updatedNonce) =
-            _mockVerifier.transaction({nonce: 0, configs: configs, commitmentTreeDepth: _TEST_COMMITMENT_TREE_DEPTH});
-        bytes32 preExistingCm = tx1.actions[0].complianceVerifierInputs[0].instance.created.commitment;
-        _mockPa.execute(tx1);
-
-        (Transaction memory tx2,) = _mockVerifier.transaction({
-            nonce: updatedNonce,
-            configs: configs,
-            commitmentTreeDepth: _TEST_COMMITMENT_TREE_DEPTH
-        });
-        tx2.actions[0].complianceVerifierInputs[0].instance.created.commitment = preExistingCm;
-        vm.expectRevert(
-            abi.encodeWithSelector(CommitmentAccumulator.PreExistingCommitment.selector, preExistingCm),
-            address(_mockPa)
-        );
-        _mockPa.verify(tx2);
-    }
-
     function test_verify_reverts_on_duplicated_nullifier() public {
         TxGen.ActionConfig[] memory configs = TxGen.generateActionConfigs({nActions: 1, nCUs: 2});
 
         (Transaction memory txn,) =
             _mockVerifier.transaction({nonce: 0, configs: configs, commitmentTreeDepth: _TEST_COMMITMENT_TREE_DEPTH});
         bytes32 duplicatedNf = txn.actions[0].complianceVerifierInputs[0].instance.consumed.nullifier;
-        txn.actions[0].complianceVerifierInputs[1].instance.consumed.nullifier = duplicatedNf;
-
-        vm.expectRevert(abi.encodeWithSelector(TagLookup.NullifierDuplicated.selector, duplicatedNf), address(_mockPa));
-        _mockPa.verify(txn);
-    }
-
-    function test_verify_reverts_on_duplicated_commitment() public {
-        TxGen.ActionConfig[] memory configs = TxGen.generateActionConfigs({nActions: 1, nCUs: 2});
-
-        (Transaction memory txn,) =
-            _mockVerifier.transaction({nonce: 0, configs: configs, commitmentTreeDepth: _TEST_COMMITMENT_TREE_DEPTH});
         bytes32 duplicatedCm = txn.actions[0].complianceVerifierInputs[0].instance.created.commitment;
+        txn.actions[0].complianceVerifierInputs[1].instance.consumed.nullifier = duplicatedNf;
         txn.actions[0].complianceVerifierInputs[1].instance.created.commitment = duplicatedCm;
 
-        vm.expectRevert(abi.encodeWithSelector(TagLookup.CommitmentDuplicated.selector, duplicatedCm), address(_mockPa));
+        vm.expectRevert(abi.encodeWithSelector(TagLookup.NullifierDuplicated.selector, duplicatedNf), address(_mockPa));
         _mockPa.verify(txn);
     }
 }

contracts/test/TagLookup.t.sol

@@ -24,19 +24,10 @@ contract TagLookupTest is Test {
         _tags.checkNullifierNonExistence(_tags[0]);
     }
 
-    function test_checkCommitmentNonExistence_reverts_if_commitment_exists() public {
-        vm.expectRevert(abi.encodeWithSelector(TagLookup.CommitmentDuplicated.selector, _tags[1]), address(this));
-        _tags.checkCommitmentNonExistence(_tags[1]);
-    }
-
     function test_checkNullifierNonExistence_passes_if_the_nullifier_does_not_exist() public view {
         _tags.checkNullifierNonExistence(_tags[1]);
     }
 
-    function test_checkCommitmentNonExistence_passes_if_the_commitment_does_not_exist() public view {
-        _tags.checkCommitmentNonExistence(_tags[0]);
-    }
-
     function test_isFoundInEvenOrOddPosition_returns_true_for_existent_tag_in_even_positions() public view {
         assertEq(_tags.isFoundInEvenOrOddPosition({tag: _tags[0], even: true}), true);
         assertEq(_tags.isFoundInEvenOrOddPosition({tag: _tags[2], even: true}), true);