Description

The CurveMultiExchangeAssetManager contract's curveMultiExchange function verifies a proof for the curveMultiExchange circuit, while the CurveSingleExchangeAssetManager contract's curveSingleExchange function verifies a proof for the curveExchange circuit.

However, those two circuits are identical up to variable names, and the verifier contracts are precisely identical. This means that proofs for one of the two circuits will also verify by the other circuit's verifier contract. Proofs can thus be used for an unintended action, such as using a curveSingleExchange proof for curveMultiExchange instead.

The different curveAddLiquidity entry points (in the CurveAddLiquidityAssetManager, CurveFSNAddLiquidityAssetManager, and CurveMPAddLiquidityAssetManager contracts) all use the same curveAddLiquidity circuit. So here, too, can a proof that was intended to be used with one of those entry points be used for another entry point — analogous for the curveRemoveLiquidity entry points.

Impact

It is not ensured that proofs are only usable in the entry point that the user intended them for. No practical impact was identified at this time.

Recommendations

The suggested fix for Finding ref↗ would make the curveMultiExchange and curveExchange circuits distinct.

As there is no practical impact of using the same circuit for the three curveAddLiquidity (and three curveRemoveLiquidity) entry points, no change is necessary. If making sure that the proofs cannot be used for an unintended variant is desired, then this can of course be done by creating three variants of the circuit that use a different domain separator as in Finding ref↗. An alternative that would not require duplicating the circuit (which comes with the need to deploy more contracts and possible other practical disadvantages) would be to include a variant identifier in the data that is signed inside the circuit and expose that variant identifier as a public circuit variable, checking it in the contract, for example like this:

let m = std::hash::mimc::mimc_bn254(
    [
+     DOMAIN_SEPARATOR_CURVE_ADD_LIQUIDITY,
+     add_liquidity_variant, // this is a new public input to the circuit. 
    note1,
    note2,
    note3,
    note4,
    nullifier1,
    nullifier2,
    nullifier3,
    nullifier4,
    pool,
    out_note_footer
]
);

let m_bytes = fuzk::to_bytes(m);

let v = std::schnorr::verify_signature(pub_key[0], pub_key[1], signature, m_bytes);

In CurveAddLiquidityAssetManager, CurveFSNAddLiquidityAssetManager, and CurveMPAddLiquidityAssetManager, it might then be checked that add_liquidity_variant is equal to a constant that differs between these three variants.

Remediation

This issue has been acknowledged by Singularity, and a fix was implemented in commit 0094b4e1↗.