Description

The CurveFSNAddLiquidityAssetManager contract's curveAddLiquidity entry point can be used to add liquidity to Curve pools. As part of its arguments, arrays of four assets and amounts as well as nullifiers are passed:

struct AddLiquidityArgs {
    bytes32 merkleRoot;
    bytes32[4] nullifiers;
    address[4] assets;
    uint256[4] amounts;
    address pool;
    address lpToken;
    bytes32 noteFooter;
    address payable relayer;
    uint256[4] gasRefund;
}

Not all of the four indexes are necessarily used, with the remaining amounting to dummy notes, which correspond to indexes i for which assets[i] and amounts[i] are zero. This finding concerns such dummy notes, so we will go through the code to discuss how the nullifiers, assets, and amounts arguments are used. Here is an extract from the implementation of the curveAddLiquidity function:

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

    // ...
    
    if (
        _args.amounts[0] +
            _args.amounts[1] +
            _args.amounts[2] +
            _args.amounts[3] ==
        0
    ) {
        revert AmountNotCorrect();
    }
    
    // ...

    _verifyProof(
        _proof,
        _buildLPInputs(
            LPRawInputs(
                _args.merkleRoot,
                _args.nullifiers,
                _args.assets,
                _args.amounts,
                _args.pool,
                _args.noteFooter
            )
        ),
        "curveAddLiquidity"
    );

    if (
        !_validateAssets(_getMetaFactoryCoins(_args.pool), _args.assets) 
    ) {
        revert AssetNotInPool();
    }

    uint256[] memory actualAmounts;
    uint256[4] memory serviceFees;

    (actualAmounts, serviceFees, ) = IFeeManager(_feeManager).calculateFeeForFSN(
        _args.amounts,
        _args.gasRefund
    );

    uint256 mintAmount = _addLiquidity(_args, actualAmounts);

    // ...
}

As can be seen here, all nullifiers are first checked to be unused so far. The check on the sum of amounts implies not all input notes can be dummy notes. The proof is then verified. In the proof circuit, a signature is checked that includes four input notes and nullifiers. Furthermore, for each of the four input notes, it is checked that they and the corresponding nullifier are correct, as long as the corresponding amount is nonzero. If the corresponding amount is zero, then verification is skipped:

if amount1 != 0 {
    fuzk::assert_note_with_membership(
        merkle_root,
        merkle_index1,
        merkle_path1,
        note1,
        asset1,
        amount1,
        rho1,
        pub_key
    );
    fuzk::assert_nullifier(nullifier1, rho1, pub_key);
}

This means that if for some i the argument amounts[i] is zero, then nullifiers[i] and assets[i] can be chosen arbitrarily by the caller in the proof.

The next check involving these three values in curveAddLiquidity is the call to _validateAssets, which is implemented as follows:

function _validateAssets(
    address[8] memory coins,
    address[4] memory assets
) internal pure returns (bool) {
    for (uint256 i = 0; i < 4; i++) {
        if (coins[i] == address(0)) {
            continue;
        }
        if (assets[i] != address(0)) {
            if (assets[i] != coins[i]) {
                if (
                    assets[i] == _ETH_ADDRESS && coins[i] == _WETH_ADDRESS
                ) {
                    continue;
                }
                return false;
            }
        }
    }
    return true;
}

As can be seen here, if assets[i] is zero, then this check passes, no matter what the value of coins[i] is. So far, we can thus conclude that the caller can choose nullifier[i] to have an arbitrary value that has not been marked used yet, as long as assets[i] and amounts[i] are both zero.

The curveAddLiquidity function will then ultimately call _addLiquidity, which is implemented as follows:

function _addLiquidity(
        AddLiquidityArgs memory _args,
        uint256[] memory actualAmounts
    ) private returns (uint256) {
        uint256 coinNum = _getMetaFactoryCoinNum(_args.pool);
        uint256 ethAmount = 0;
        uint256 mintAmount = 0;
        uint256 i = 0;
        if (coinNum <= 4) {
        
            // ...
            
            for (i = 0; i < 4; i++) {
                _postWithdraw(_args.nullifiers[i]);
            }
        } else {
            revert PoolNotSupported();
        }
        return mintAmount;
    }

Note that _postWithdraw is called for all nullifiers, including the dummy nullifiers corresponding to the cases in which assets[i] and amounts[i] are both zero and which the caller can choose arbitrarily (as long as they have not been used yet).

An attacker can thus call CurveFSNAddLiquidityAssetManager contract's curveAddLiquidity function with one to three legitimate inputs as well as at least one dummy note, for which the attacker can choose an arbitrary unused nullifier. That nullifier will then be marked used and thus cannot be used anymore afterwards.

The analogous issue exists for the CurveAddLiquidityAssetManager and CurveMPAddLiquidityAssetManager contracts.

Impact

An attacker that sees a pending transaction in the mempool can front-run it with a call to one of the curveAddLiquidity functions with a dummy note's nullifier set to the nullifier from the front-run transaction, thereby making the victim's note unspendable.

Recommendations

Consider ensuring that dummy notes have a zero nullifier. For example, if amount1 == 0, assert that nullifier1 == 0 as well.

Remediation

This issue has been acknowledged by Singularity, and a fix was implemented in commit fe268599↗.