Description

The DarkpoolAssetManager's joinSplit, join, and swap functions each invalidate two input notes by marking the respective nullifiers as used, preventing them from being spent a second time later. However, it is not ensured that the two nullifiers that are invalidated are distinct. Take the example of the join function:

function join(
    bytes32 _merkleRoot,
    bytes32 _nullifierIn1,
    bytes32 _nullifierIn2,
    bytes32 _noteOut1,
    bytes calldata _proof
) public payable {
    if (!_merkleTreeOperator.noteIsNotCreated(_noteOut1)) {
        revert NoteAlreadyCreated();
    }
    if (!_merkleTreeOperator.nullifierIsNotUsed(_nullifierIn1)) {
        revert NullifierUsed();
    }
    if (!_merkleTreeOperator.nullifierIsNotUsed(_nullifierIn2)) {
        revert NullifierUsed();
    }
    if (!_merkleTreeOperator.merkleRootIsAllowed(_merkleRoot)) {
        revert MerkleRootNotAllowed();
    }

    JoinRawInputs memory inputs = JoinRawInputs(
        _merkleRoot,
        _nullifierIn1,
        _nullifierIn2,
        _noteOut1
    );

    _verifyProof(_proof, _buildJoinInputs(inputs), "join");
    _postDeposit(_noteOut1);
    _postWithdraw(_nullifierIn1);
    _postWithdraw(_nullifierIn2);

    emit Join(_nullifierIn1, _nullifierIn2, _noteOut1);
}

As can be seen in the snippet above, it is checked at the start of the function that _nullifierIn1 and _nullifierIn2 are not yet marked as used. At the end of the function, both are marked as used. The call to _postWithdraw will succeed even if the nullifier was already marked used, as it is a wrapper around the following function, which is idempotent:

function setNullifierUsed(bytes32 nullifier) external onlyAssetManager {
    if (nullifier != bytes32(0)) {
        nullifiersUsed[nullifier] = true;
    }
}

The join function will thus accept the same value for _nullifierIn1 and _nullifierIn2, as long as that common nullifier has not been used yet. The situation is the same for the other two functions.

The respective join_split, join, and swap circuits do not ensure that the two input nullifiers are distinct either.

The Uniswap integration's uniswapLiquidityProvision also has two inputs. However, in this case, liquidity provision will ultimately revert when two identical inputs are used, as the assets will be the same (the NonfungiblePositionManager contract will indirectly call the PoolAddress libraries' computeAddress function, which requires the address of the first address to be smaller than the address of the second address; see here↗).

The Curve integration's curveAddLiquidity functions in the CurveAddLiquidityAssetManager, CurveMPAddLiquidityAssetManager and CurveFSNAddLiquidityAssetManager contracts similarly have four input notes, but they are not checked to be distinct. They are all checked to be unused at the start of the function and only marked as used at the end of the function:

function curveAddLiquidity(
    bytes calldata _proof,
    AddLiquidityArgs calldata _args
) external payable {
    // ...
    for (uint256 i = 0; i < 4; i++) {
        _validateNullifierIsNotUsed(_args.nullifiers[i]);
    }
    
    // ...
    
    for (uint256 i = 0; i < 4; i++) {
        _postWithdraw(_args.nullifiers[i]);
    }
    // ...
}

Thus, from the perspective of the nullifier check, reuse of the same note in multiple inputs is possible. Doing so would, as in the Uniswap case, imply that multiple inputs are for the same assets. Whether there is any impact thus depends on whether there exist Curve pools with duplicate coins for tokens that the attacker is interested in double spending. We did not verify conclusively whether or not that is the case.

Impact

It is possible to use any one of the joinSplit, join, and swap functions to double one's balance by using the same note for both inputs. For example, for the join function, using a note with amount $A$ for both inputs will allow one to obtain a joined note with amount $2\cdot A$. Repeating this allows reaching arbitrary amounts, which can be followed by withdrawing to drain the darkpool's entire balance.

Recommendations

Ensure that the input notes / nullifiers must be distinct. This could be done by verifying the nullifiers are distinct in the circuit or alternatively by verifying this in the contract, for example by modifying the function setNullifierUsed of the MerkleTreeOperator contract to revert whenever the nullifier was already marked used:

function setNullifierUsed(bytes32 nullifier) external onlyAssetManager {
+     require(nullifierIsNotUsed(nullifier), "Nullifier already used");
    if (nullifier != bytes32(0)) {
        nullifiersUsed[nullifier] = true;
    }
}

Remediation

This issue has been acknowledged by Singularity. The issue was fixed by the following commit . With the modifications, the setNoteCommitmentCreated, setNullifierUsed and setNoteFooterUsed functions revert in case the argument is already marked as used. Commit also adds an in-circuit check that the two input notes to the join circuit are distinct.