Singularity audit

A private security review of Singularity (private DeFi access), conducted with Three Sigma and completed on 26th February 2024 over 54 days. 0xSimao disclosed 42 findings: 18 high severity, 6 medium, 7 low and 11 informational.

What Singularity is

Singularity is KYC/KYB-gated confidential DeFi: institutions transact against existing on-chain liquidity through shielded pools, with UltraPLONK zero-knowledge circuits written in Noir hiding amounts and counterparties while keeping participants screenable.

Scope

The review covered the note machinery and its DeFi integrations: deposits creating commitments, the rho values that make each note unique, Merkle-root validation in staking flows, and the Curve exchange routes through which shielded balances move.

High severity findings

• In StakingAssetManager::lockERC20() the resulting note is created with asset instead of zkToken
• MerkleRoot is not validated in all StakingAssetManager functions
• Checks-effects-interations pattern is not always followed, which can be used to drain all tokens
• Curve multi exchange does not validate assetIn and assetOut against route
• DarkPoolAssetManager can be drained by looping over join(), joinSplit() or swap() with only some initial amount
• Stuck ETH in Curve exchanges due to sending msg.value to the exchange instead of amountIn
• Reusing the same rho and pubKey in different deposits leads to lost tokens
• Note footers should not be 0 in curveRemoveLiquidity() if the corresponding assetOuts are non null
• UniswapLiquidityAssetManager::_validateCollectFeesArgs() validates nullifier instead of note footer
• CurveAssetManagerHelper::_validateAssets() should check that the number of assets provided is smaller than the maximum of the pool
• All curve params should be signed by the schnorr private key in the proof, or users may be griefed
• Curve multi exchange can be used to withdraw assets without paying fees
• Anyone can deposit to DarkpoolAssetManager as the owner can be freely chosen without any implication
• Attackers can include other users nullifiers to make their funds stuck when adding liquidity to curve
• _addLiquidity() slippage is incorrectly set
• Anyone can frontrun a relayer interaction with the same arguments but a much higher/lower relayer fee
• DarkpoolAssetManager::Split() into 2 equal amounts leads to lost funds
• No support for fee on transfer tokens

Medium severity findings

• Curve pools should be whitelisted as some of them may not be 100% compatible
• Uniswap collect fees should skip collecting fees of one of the tokens if the amount is 0
• Service fees should depend on asset
• CurveAddLiquidityAssetManager::curveAddLiquidity() does not deal correctly with isLegacy = 0b10 and ETH
• Uniswap asset managers are missing slippage checks
• Some ETH transfers don't revert if they fail

Low severity findings

• Decimals in ZKToken are not set to the underlying decimals, which will likely harm tvl calculations in aggregators
• StakingOperator::setCollateralToken() will cause issues if it changes tokens relations that have already been set
• Missing event in VerifierHub::setVerifier()
• Setting note commitments, nullifiers and note footers used should revert if they are already set to prevent exploits
• ERC20AssetPool and ERC721AssetPool should have the nonReentrant modifier as ERC721 and some tokens have callbacks
• Generating numbers smaller than P by doing % P might be vulnerable
• MerkleTreeOperator::getMerklePath() will revert due to OOG after enough elements

Informational findings

• stakingAssetManager in ZKToken may be immutable as it is never changed
• StakingOperator::_setUnlockWindow() checks if the times are negative, which is impossible
• StakingOperator does not set isUnlockWindowActive to true in the constructor if the flag passed is true
• Checks effects interactions pattern is not always followed
• Variables are initialized to 0 by default
• UniswapLiquidityAssetManager::uniswapLiquidityProvision() could return tokenId
• Spelling errors throughout the codebase
• Unused noteCommitment parameter in UniswapRemoveLiquidityInputs struct
• Missing proof identifier, which could lead to using the same proof in another method
• UniswapLiquidityAssetManager registers the note footer twice
• BaseAssetManager missing 0 address checks in the constructor

The report

Download report (PDF)