Description

Funds held by users in the darkpool can be in the form of ETH, ERC-20 tokens, or ERC-712 tokens. For bookkeeping of these funds, notes are used that consist of three fields:

1. The asset field, which is used for the address of the token contract, or a special value in the case of ETH.

2. The amount field, which stores the amount of ETH or ERC-20 token in those cases, or the NFT token ID in the case of ERC-712 tokens.

3. The footer field, which stores a hash of a random value rho and the public key. The usage of this field is not relevant for this finding.

These notes are stored on chain in a single Merkle tree. As the ETH/ERC-20 and ERC-712 notes are not domain separated, this opens the possibility of using actions intended for one note type for a note of the other type.

Concretely, this leads to the possibility of the following kind of attack in which an attacker, A, can steal a liquidity position from another user, B.

1. B provides liquidity to some Uniswap pool via the UniswapLiquidityAssetManager. They get minted a position NFT with a certain token ID, say token ID b, and a position note in the darkpool reflecting the NFT. Note that while the identity of B is protected, the token ID as well as the pool interacted with is visible on chain.

2. The attacker A also provides liquidity to a Uniswap pool while using the minimum amount of funds possible (thus, this step costs the attacker nearly nothing), receiving a position NFT with token ID a and a corresponding position note. In practice, on minting of new position NFTs, the NonfungiblePositionManager increments the IDs. So it will hold that a > b.

3. A now calls split, using their position note as input. The split action assumes a fungible type of note and interprets the second field as an amount. The attacker can thus use split to split the note into two, one where the amount field holds the value b and one in which it holds the value b-a.

4. The attacker now calls uniswapRemoveLiquidity with their position note with token ID b. They receive notes in return reflecting the liquidity that was originally provided by B. Should B try to remove the liquidity, they will get nothing. The attacker has thus successfully stolen B's liquidity position.

Impact

Anyone can steal Uniswap liquidity positions.

Recommendations

Ensure that non-fungible notes cannot be used as if they were fungible and vice-versa. This can be done by domain separating them. Currently, fungible note commitments are given by hash(asset, amount, footer) while non-fungible note commitments are given by hash(asset, id, footer). These could be domain separated by using instead hash(DOMAIN_SEPARATOR_FUNGIBLE, asset, amount, footer) and hash(DOMAIN_SEPARATOR_NON_FUNGIBLE, asset, id, footer), where DOMAIN_SEPARATOR_FUNGIBLE and DOMAIN_SEPARATOR_NON_FUNGIBLE are different constants. Proofs should then check that the new domain-separator field of notes is of the expected type. For example, the split circuit should fail for input notes that do not have DOMAIN_SEPARATOR_FUNGIBLE as the first field.

Remediation

This issue has been acknowledged by Singularity. The issue was fixed with commits , , and . The first commit adds domain separation functionality and the relevant checks to all relevant circuits except depoit and swap, the second commit adds the relevant check to the deposit circuit, and the third commit disables the swap entrypoint in the DarkpoolAssetManager.