Description

Entry points that result in transfer of funds to a user or relayer do not follow the checks-effects-interactions pattern, transferring those funds to the relayer and/or user before marking the relevant nullifier as used. This results in possible double-spends.

There is some commonality of this finding with Finding ref↗. This finding discusses the case in which the attacker is the recipient of a transfer and can reenter on reception, and it results in double spending of the same asset that was used for reentry. Entry points are thus withdrawals for a normal user and any actions resulting in relayer fees for relayers. In contrast, Finding ref↗ discusses the case in which the attacker controls the contract of a fully fake token and can reenter on any call to that token (including transfers where the attacker is not the recipient). As the fake token is worthless, double-spending it does not gain anything, so as the entry point for the attack, the attacker needs to use actions in which two different assets are paid as input in order to double-spend the second, legitimate one while reentering on the first.

Case of DarkpoolAssetManager's withdrawETH

In the case of withdrawETH, the function is implemented as follows:

function withdrawETH(
    bytes calldata _proof,
    bytes32 _merkleRoot,
    bytes32 _nullifier,
    address payable _recipient,
    address payable _relayer,
    uint256 _relayerGasFee,
    uint256 _amount
) public {
    require(
        _complianceManager.isPermitted(address(this), _recipient),
        "BaseAssetManager: invalid credential"
    );
    if (!_relayerHub.isRelayerRegistered(_relayer)) {
        revert RelayerNotRegistered();
    }
    if (!_merkleTreeOperator.nullifierIsNotUsed(_nullifier)) {
        revert NullifierUsed();
    }
    if (!_merkleTreeOperator.merkleRootIsAllowed(_merkleRoot)) {
        revert MerkleRootNotAllowed();
    }
    WithdrawRawInputs memory inputs = WithdrawRawInputs(
        _recipient,
        _merkleRoot,
        ETH_ADDRESS,
        _amount,
        _nullifier
    );

    _verifyProof(_proof, _buildWithdrawInputs(inputs), "withdraw");
    _releaseETHWithFee(_recipient, _relayer, _relayerGasFee, _amount);
    _postWithdraw(_nullifier);

    emit Withdraw(_nullifier, _amount, ETH_ADDRESS, _recipient);
}

An attacker can call this to withdraw a valid unspent note containing ETH. The funds will be transferred in the call _releaseETHWithFee(_recipient, _relayer, _relayerGasFee, _amount);, but only afterwards will the nullifier be marked as used with the call to _postWithdraw(_nullifier);, and thus the note becomes unusable. The function _releaseETHWithFee in the contract BaseAssetManager uses the ETH asset pool to transfer ETH to the relayer, fee manager, and the recipient specified as an argument to withdrawETH. Ultimately, the funds will be transferred using this function of the ETHAssetPool contract:

function release(
    address payable to,
    uint256 amount
) external onlyAssetManger nonReentrant transferNotLocked {
    require(amount > 0, "ETHAssetPool: amount must be greater than 0");

    require(
        address(this).balance >= amount,
        "ETHAssetPool: Insufficient balance"
    );

    (bool success, ) = to.call{value: amount}("");
    require(success, "ETHAssetPool: Failed to send Ether");
}

Control of execution is passed to the recipient's contract on to.call{value: amount}(""). Note that while the above function uses the nonReentrant modifier, this only prevents reentry into the release function of the ETHAssetPool contract. Reentry into functions of the DarkpoolAssetManager contract are still possible.

An attacker could reenter using, for example, the transfer function:

function transfer(
    bytes32 _merkleRoot,
    bytes32 _nullifierIn,
    bytes32 _noteOut,
    bytes calldata _proof
) public {
    if (!_merkleTreeOperator.noteIsNotCreated(_noteOut)) {
        revert NoteAlreadyCreated();
    }
    if (!_merkleTreeOperator.nullifierIsNotUsed(_nullifierIn)) {
        revert NullifierUsed();
    }
    if (!_merkleTreeOperator.merkleRootIsAllowed(_merkleRoot)) {
        revert MerkleRootNotAllowed();
    }

    TransferRawInputs memory inputs = TransferRawInputs(
        _merkleRoot,
        _nullifierIn,
        _noteOut
    );

    _verifyProof(_proof, _buildTransferInputs(inputs), "transfer");
    _postWithdraw(_nullifierIn);
    _postDeposit(_noteOut);

    emit Transfer(_nullifierIn, _noteOut);
}

All checks here would pass when using the same note as for the withdrawal. At the end, _postWithdraw(_nullifierIn) marks the spent note's nullifier as used, and _postDeposit(_noteOut) adds a new note holding the same amount of ETH that the spent note did.

When returning back from the call in the withdrawal, withdrawETH will call _postWithdraw(_nullifier) as well. However, as discussed in Finding ref↗, this is a idempotent operation, so marking the nullifier as used a second time does not fail.

At the end, the attacker would be left still having their original amount of ETH in a note in the darkpool but also having withdrawn that amount. The new note in the darkpool could still be withdrawn, thereby doubling the attacker's initial amount.

Case of DarkpoolAssetManager's withdrawERC20

In the case of withdrawERC20, the asset contract's transfer function is called to transfer the funds to the user. For normal ERC-20 tokens, these should not lead to reentrancy. However, reentrancy could occur for tokens that support receive hooks, such as ERC-777 tokens. As there are no restriction on which addresses can be used for ERC-20 token assets in the darkpool, such tokens could potentially be used.

While the attacker could also deploy their own (fake) token contract with receive hooks, this does not lead to an issue with respect to withdrawERC20, as double-spending a darkpool note denominated in their fake token does not advantage the attacker, as they could just mint their token directly anyway. Using a different entry point leveraging fake tokens is possible in a different manner, however; see Finding ref↗.

In the case of withdrawERC20 with receive hooks, the attacker can proceed as in withdrawETH to drain the darkpool.

Case of DarkpoolAssetManager's depositERC20

In the case of depositERC20, the asset contract's transferFrom function is called to transfer the tokens from the user. ERC-777 also allows for send hooks, so for these tokens, the possibility of reentrancy exists for depositERC20 as well. However, as no notes are spent in depositERC20, this can not lead to a double-spend vulnerability. The user could use this reentrancy to deposit the same amount twice and receive the same note for it (making one copy unspendable), which is otherwise not possible, but as this means the user loses half of their deposit, there is no advantage in doing so.

Case of relayer reentering

Whenever fees in are paid to the relayer, the relayer can reenter for ETH or ERC-20 with transfer hooks. They can thus carry out a similar attack to the one discussed in the withdrawETH case: the relayer acts as both relayer and user (using their own notes) to carry out an action causing fees to be paid to the relayer and to set the fee amount to the maximum possible. When receiving the fee, they reenter and transfer the input note to a fresh note so that they will retain the funds in the darkpool as well as having them paid out as relayer fees.

This type of issue occurs in all Darkpool and Uniswap entry points in which relayers get fees:

• DarkpoolAssetManager's withdrawETH and withdrawERC20

• UniswapSwapAssetManager's uniswapSimpleSwap

• UniswapLiquidityAssetManager's uniswapLiquidityProvision, uniswapCollectFees, and uniswapRemoveLiquidity

The issue does not appear to arise with the Curve integrations, as those mark nullfiers are used before transferring fees, as in this example of the CurveSingleExchangeAssetManager contract's curveSingleExchange functions:

function curveSingleExchange(
    bytes calldata _proof,
    ExchangeArgs calldata _args
) external payable {
    _validateRelayerIsRegistered(_args.relayer);
    _validateNullifierIsNotUsed(_args.nullifier);
    _validateMerkleRootIsAllowed(_args.merkleRoot);
    _validateNoteFooterIsNotUsed(_args.noteFooter);
    
    // ...

    if (_args.assetIn == ETH_ADDRESS) {
        _assetPoolETH.release(payable(address(this)), _args.amountIn);
        amountOut = IExchange(exchangeContract).exchange{value: msg.value}(
            _args.pool,
            _args.assetIn,
            _args.assetOut,
            _args.amountIn,
            amountOut
        );
    } else {
        // ...
    }

    _postWithdraw(_args.nullifier);

    // ...
    
    if (_args.assetOut == ETH_ADDRESS) {
        (bool success, ) = payable(address(_assetPoolETH)).call{
            value: noteAmount
        }("");
        (success, ) = payable(address(_feeManager)).call{value: serviceFee}(
            ""
        );
        (success, ) = payable(address(_args.relayer)).call{
            value: _args.gasRefund
        }("");
    } else {
        // ...
    }
    
    // ...
}

However, this function also does not follow the checks-effects-interactions pattern; to follow best practices, the nullifier should be marked used before releasing funds from the asset pool and calling exchange.

Impact

All ETH stored in the darkpool can be drained by the attacker. For ERC-20 assets, this is possible for those assets where the attacker can gain control of execution on transfer calls to the asset contract, such as in the case of ERC-777 receive hooks.

In the case of some entry points, the attack has to be carried out by a relayer.

Recommendations

Use the checks-effects-interactions pattern and mark the nullifier as used before interacting with external contracts, for example in the withdrawETH case:

function withdrawETH(
    bytes calldata _proof,
    bytes32 _merkleRoot,
    bytes32 _nullifier,
    address payable _recipient,
    address payable _relayer,
    uint256 _relayerGasFee,
    uint256 _amount
) public {
    require(
        _complianceManager.isPermitted(address(this), _recipient),
        "BaseAssetManager: invalid credential"
    );
    if (!_relayerHub.isRelayerRegistered(_relayer)) {
        revert RelayerNotRegistered();
    }
    if (!_merkleTreeOperator.nullifierIsNotUsed(_nullifier)) {
        revert NullifierUsed();
    }
    if (!_merkleTreeOperator.merkleRootIsAllowed(_merkleRoot)) {
        revert MerkleRootNotAllowed();
    }
    WithdrawRawInputs memory inputs = WithdrawRawInputs(
        _recipient,
        _merkleRoot,
        ETH_ADDRESS,
        _amount,
        _nullifier
    );

    _verifyProof(_proof, _buildWithdrawInputs(inputs), "withdraw");
-     _releaseETHWithFee(_recipient, _relayer, _relayerGasFee, _amount);
    _postWithdraw(_nullifier);
+     _releaseETHWithFee(_recipient, _relayer, _relayerGasFee, _amount);

    emit Withdraw(_nullifier, _amount, ETH_ADDRESS, _recipient);
}

For in-depth defense, we also recommend changing the setNullifierUsed function of the MerkleTreeOperator contract to revert whenever the nullifier was already marked used. This would prevent using the same nullifier twice in all cases. With this change, but without the change above, the attacker could still reenter, and the reentered call would succeed and mark the nullifier as used, but then in the original, outer call marking the nullifier as used would fail, causing a revert.

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