Description

Verification of arguments to the uniswapCollectFees function are handled by _validateCollectFeesArgs, which is implemented as follows.

function _validateCollectFeesArgs(
    UniswapCollectFeesArgs memory args
) internal view {
    _validateMerkleRootIsAllowed(args.merkleRoot);
    _validateNullifierIsNotUsed(args.feeNoteFooters[0]);
    _validateNullifierIsNotUsed(args.feeNoteFooters[1]);
    _validateRelayerIsRegistered(args.relayer);
}

Here the two note footers should be checked to not have been used before using _validateNoteFooterIsNotUsed. However, the incorrect function _validateNullifierIsNotUsed is used instead, making the check ineffective.

Impact

Note footers can be reused, making all but one of the resulting notes with the same nullifier unspendable.

As position notes are not spent when collecting fees, the missing check allows replaying calls to uniswapCollectFees, with the same proof. This means that an attacker could grief a position's fees after the first time fees were collected; they can periodically replay the call, thereby transferring the position's fees to notes with duplicate nullifiers that thus cannot all be spent.

On each replay, the relayer will still get their fee. Hence, the relayer would be incentivized to replay the call each time the Uniswap fees are high enough to cover the relayer fee, thereby stealing most of the Uniswap fees after the very first fee collection for themselves.

Relayers are registered by the owner of the relayer hub. Logic deciding who will get registered is not in scope for this audit. Singularity informed us that the plan is to decentralize relayers, with anyone staking certain tokens having the possibility to become relayers. With that context, we judge that relayers cannot be considered trusted, though the staking aspect may introduce certain hurdles for potential attackers. Because of this, we rate the likelihood as Medium.

Other impact depends on the front-end used.

Recommendations

Call the _validateNoteFooterIsNotUsed function instead:

function _validateCollectFeesArgs(
    UniswapCollectFeesArgs memory args
) internal view {
    _validateMerkleRootIsAllowed(args.merkleRoot);
-     _validateNullifierIsNotUsed(args.feeNoteFooters[0]);
-     _validateNullifierIsNotUsed(args.feeNoteFooters[1]);
+     _validateNoteFooterIsNotUsed(args.feeNoteFooters[0]);
+     _validateNoteFooterIsNotUsed(args.feeNoteFooters[1]);
    _validateRelayerIsRegistered(args.relayer);
}

Remediation

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