Description

Several actions will cause calls to token contracts to move funds, for example from or to the asset pool. The implementations tend to not follow the checks-effects-interactions pattern, making external calls to the asset contracts before relevant nullifiers are marked as used. An attacker can thus double-spend tokens with the following generic steps:

1. The attacker creates a contract that supports relevant ERC-20 functions and deposits some of this token into the darkpool, obtaining a note denominated in the attacker's token.

2. The attacker calls an entry point in which they spend at least two notes. One of them is the note denominated in the attacker's token, and the rest are notes for legitimate assets. We assume that the attacker's contract will be called after the nullifiers for all notes were checked to be unused but before they are marked used.

3. On the call to the attacker's contract, the attacker reenters, spending the input notes for legitimate assets another time. As the nullifiers are not yet marked used, this will not revert.

4. Back in the original call, the original spend will not revert either, as the nullifiers are not checked a second time.

This issue occurs with the following entry points:

• UniswapLiquidityAssetManager's function uniswapLiquidityProvision

• CurveAddLiquidityAssetManager's function curveAddLiquidity

• CurveFSNAddLiquidityAssetManager's function curveAddLiquidity

• CurveMPAddLiquidityAssetManager's function curveAddLiquidity

We will explain the detailed steps of the attack with an example in the case of liquidity provision on Uniswap. The attack works very similarly in the case of the three liquidity provision entry points for Curve.

1. First, the attacker creates a contract with the relevant functionality for the later steps, in particular implementing the relevant parts of the ERC-20 interface. We will call this token FAKE.

2. The attacker then deploys a new Uniswap pool for FAKE and the target token they want to drain. Let us call that token REAL.

3. The attacker deposits 10 REAL and 10 FAKE into the darkpool.

4. The attacker calls uniswapLiquidityProvision to add liquidity to the REAL-FAKE pool, using the 10 FAKE and 10 REAL notes as input.

The following is an extract of that function's implementation:

function uniswapLiquidityProvision(
        UniswapLiquidityProvisionArgs memory args,
        bytes calldata proof
    ) public returns (FeesDetails memory feesDetails) {
        _validateLiquidityProvisionArgs(args);

        // ...

        (
            mintParams,
            feesDetails,
            originalIndices
        ) = _releaseFundsAndPrepareLiquidityProvisionArgs(args);

        // ...
    }

That the nullifiers of the two notes are not marked as used yet is only checked by _validateLiquidityProvisionArgs, which is called right at the start.

function _validateLiquidityProvisionArgs(
    UniswapLiquidityProvisionArgs memory args
) internal view {
    _validateMerkleRootIsAllowed(args.merkleRoot);
    _validateNullifierIsNotUsed(args.noteData1.nullifier);
    _validateNullifierIsNotUsed(args.noteData2.nullifier);
    // ...
}

The call to _releaseFundsAndPrepareLiquidityProvisionArgs will then release funds for each asset from the vault and then mark the relevant nullifier as used. As the attacker uses FAKE as the first of the two assets, when releasing FAKE funds from the vault (which calls the transfer function of the attacker's FAKE contract), none of the two nullifiers is marked used yet.

function _releaseFundsAndPrepareLiquidityProvisionArgs(
    UniswapLiquidityProvisionArgs memory args
)
    internal
    returns (
        INonfungiblePositionManager.MintParams memory mintParams,
        FeesDetails memory feesDetails,
        uint8[2] memory originalIndices
    )
{
    /**
     * * Release funds for token 1
     */

    FundReleaseDetails memory fundReleaseDetails1;

    fundReleaseDetails1.assetAddress = args.noteData1.assetAddress;
    fundReleaseDetails1.recipient = payable(address(this));
    fundReleaseDetails1.relayer = args.relayer;
    fundReleaseDetails1.relayerGasFee = args.relayerGasFees[0];
    fundReleaseDetails1.amount = args.noteData1.amount;

    (
        uint256 actualReleasedAmountToken1,
        FeesDetails memory feesDetails1
    ) = _releaseAndPackDetails(fundReleaseDetails1);

    _postWithdraw(bytes32(args.noteData1.nullifier));

    /**
     * * Release funds for token 2
     */

    // ...
}

5. In the call to the attacker's FAKE contract when releasing the 10 FAKE from the asset pool, the attacker calls the DarkpoolAssetManager's transfer function to transfer the 10 REAL note to a new note. The old 10 REAL note's nullifier will be marked used by transfer.

6. When returning to the original uniswapLiquidityProvision call, the 10 REAL-note nullifier is now marked used. However, execution is already past the check for this, so this does not cause an issue. The nullifier will get marked as used again by _postWithdraw. However, as discussed in Finding ref↗, this is a idempotent operation, so marking the nullifier as used a second time does not fail.

7. The attacker will thus be left with a liquidity position reflecting 10 REAL as well as the new 10 REAL note from the transfer. Removing the Uniswap liquidity position, they will be left with 20 REAL, having doubled their original investment. Repeating this process, the attacker can drain the REAL asset pool completely.

Impact

Attackers can completely drain ERC-20 and ETH asset pools.

Recommendations

Ensure all functions follow the checks-effects-interactions pattern and mark nullifiers as used before interacting with external contracts.

For in-depth defense, we also recommend changing the setNullifierUsed function of the MerkleTreeOperator contract to revert whenever the nullifier is already marked used. This would prevent using the same nullifier twice in all cases.

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 in commit where setNullifierUsed was changed to revert in case the argument is already marked as used.