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: 17 high severity, 5 medium, 9 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.
Findings
High
- 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
Medium
- Service fees should depend on asset
- CurveAddLiquidityAssetManager::curveAddLiquidity() does not deal correctly with isLegacy = 0b10 and ETH
- Uniswap asset managers are missing slippage checks
- No support for fee on transfer tokens
- Some ETH transfers don't revert if they fail
Low
- 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
- Curve pools should be whitelisted as some of them may not be 100% compatible
- 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
- Uniswap collect fees should skip collecting fees of one of the tokens if the amount is 0
- MerkleTreeOperator::getMerklePath() will revert due to OOG after enough elements
Informational
- 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