# Singularity audit Three Sigma · Private DeFi access · 26th February 2024 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](https://0xsimao.com/findings/singularity-staking-lock-erc20-resulting) - [MerkleRoot is not validated in all StakingAssetManager functions](https://0xsimao.com/findings/singularity-merkle-root-validated-staking) - [Checks-effects-interations pattern is not always followed, which can be used to drain all tokens](https://0xsimao.com/findings/singularity-effects-interations-followed-drain) - [Curve multi exchange does not validate assetIn and assetOut against route](https://0xsimao.com/findings/singularity-curve-validate-against-route) - [DarkPoolAssetManager can be drained by looping over join(), joinSplit() or swap() with only some initial amount](https://0xsimao.com/findings/singularity-dark-looping-join-split) - [Stuck ETH in Curve exchanges due to sending msg.value to the exchange instead of amountIn](https://0xsimao.com/findings/singularity-stuck-eth-curve-exchanges) - [Reusing the same rho and pubKey in different deposits leads to lost tokens](https://0xsimao.com/findings/singularity-reusing-rho-pub-deposits) - [Note footers should not be 0 in curveRemoveLiquidity() if the corresponding assetOuts are non null](https://0xsimao.com/findings/singularity-footers-curve-corresponding-outs) - [UniswapLiquidityAssetManager::_validateCollectFeesArgs() validates nullifier instead of note footer](https://0xsimao.com/findings/singularity-uniswap-fees-validates-nullifier) - [CurveAssetManagerHelper::_validateAssets() should check that the number of assets provided is smaller than the maximum of the pool](https://0xsimao.com/findings/singularity-curve-helper-validate-provided) - [All curve params should be signed by the schnorr private key in the proof, or users may be griefed](https://0xsimao.com/findings/singularity-curve-schnorr-proof-griefed) - [Curve multi exchange can be used to withdraw assets without paying fees](https://0xsimao.com/findings/singularity-curve-withdraw-paying-fees) - [Anyone can deposit to DarkpoolAssetManager as the owner can be freely chosen without any implication](https://0xsimao.com/findings/singularity-deposit-darkpool-freely-implication) - [Attackers can include other users nullifiers to make their funds stuck when adding liquidity to curve](https://0xsimao.com/findings/singularity-include-nullifiers-stuck-curve) - [_addLiquidity() slippage is incorrectly set](https://0xsimao.com/findings/singularity-add-liquidity-slippage-incorrectly) - [Anyone can frontrun a relayer interaction with the same arguments but a much higher/lower relayer fee](https://0xsimao.com/findings/singularity-frontrun-relayer-interaction-fee) - [DarkpoolAssetManager::Split() into 2 equal amounts leads to lost funds](https://0xsimao.com/findings/singularity-darkpool-split-equal-amounts) - [No support for fee on transfer tokens](https://0xsimao.com/findings/singularity-support-fee-transfer-tokens) ## Medium severity findings - [Curve pools should be whitelisted as some of them may not be 100% compatible](https://0xsimao.com/findings/singularity-curve-whitelisted-100-compatible) - [Uniswap collect fees should skip collecting fees of one of the tokens if the amount is 0](https://0xsimao.com/findings/singularity-uniswap-fees-skip-collecting) - [Service fees should depend on asset](https://0xsimao.com/findings/singularity-service-fees-depend-asset) - [CurveAddLiquidityAssetManager::curveAddLiquidity() does not deal correctly with isLegacy = 0b10 and ETH](https://0xsimao.com/findings/singularity-curve-legacy-0b10-eth) - [Uniswap asset managers are missing slippage checks](https://0xsimao.com/findings/singularity-uniswap-managers-slippage-checks) - [Some ETH transfers don't revert if they fail](https://0xsimao.com/findings/singularity-eth-transfers-don-revert) ## Low severity findings - [Decimals in ZKToken are not set to the underlying decimals, which will likely harm tvl calculations in aggregators](https://0xsimao.com/findings/singularity-decimals-underlying-calculations-aggregators) - [StakingOperator::setCollateralToken() will cause issues if it changes tokens relations that have already been set](https://0xsimao.com/findings/singularity-staking-collateral-changes-relations) - [Missing event in VerifierHub::setVerifier()](https://0xsimao.com/findings/singularity-event-verifier-hub-set) - [Setting note commitments, nullifiers and note footers used should revert if they are already set to prevent exploits](https://0xsimao.com/findings/singularity-commitments-nullifiers-footers-revert) - [ERC20AssetPool and ERC721AssetPool should have the nonReentrant modifier as ERC721 and some tokens have callbacks](https://0xsimao.com/findings/singularity-erc20-erc721-reentrant-callbacks) - [Generating numbers smaller than P by doing % P might be vulnerable](https://0xsimao.com/findings/singularity-generating-numbers-smaller-doing) - [MerkleTreeOperator::getMerklePath() will revert due to OOG after enough elements](https://0xsimao.com/findings/singularity-merkle-path-revert-elements) ## Informational findings - [stakingAssetManager in ZKToken may be immutable as it is never changed](https://0xsimao.com/findings/singularity-staking-asset-immutable-changed) - [StakingOperator::_setUnlockWindow() checks if the times are negative, which is impossible](https://0xsimao.com/findings/singularity-staking-window-times-negative) - [StakingOperator does not set isUnlockWindowActive to true in the constructor if the flag passed is true](https://0xsimao.com/findings/singularity-staking-window-active-flag) - [Checks effects interactions pattern is not always followed](https://0xsimao.com/findings/singularity-effects-interactions-pattern-followed) - [Variables are initialized to 0 by default](https://0xsimao.com/findings/singularity-variables-initialized-default) - [UniswapLiquidityAssetManager::uniswapLiquidityProvision() could return tokenId](https://0xsimao.com/findings/singularity-uniswap-liquidity-provision-return) - [Spelling errors throughout the codebase](https://0xsimao.com/findings/singularity-spelling-errors-throughout-codebase) - [Unused noteCommitment parameter in UniswapRemoveLiquidityInputs struct](https://0xsimao.com/findings/singularity-parameter-uniswap-inputs-struct) - [Missing proof identifier, which could lead to using the same proof in another method](https://0xsimao.com/findings/singularity-proof-identifier-same-method) - [UniswapLiquidityAssetManager registers the note footer twice](https://0xsimao.com/findings/singularity-uniswap-registers-footer-twice) - [BaseAssetManager missing 0 address checks in the constructor](https://0xsimao.com/findings/singularity-asset-address-checks-constructor) ## The report [Download the report (PDF)](https://0xsimao.com/reports/2024-02-26-singularity.pdf)