Description

This issue is similar to Finding ref↗, but it is for note footers instead of for nullifiers. In actions where multiple note footers are involved for outputs, checking whether these note footers have already been used is done first, and only later are both marked used. This allows for the same note footer to be used multiple times in such actions. For example, the UniswapLiquidityAssetManager contract's uniswapRemoveLiquidity function looks as follows:

function uniswapRemoveLiquidity(
    UniswapRemoveLiquidityArgs memory args,
    bytes calldata proof
)
    public
    returns (
        TransferFundsToVaultWithFeesAndCreateNoteData memory dataToken1,
        TransferFundsToVaultWithFeesAndCreateNoteData memory dataToken2
    )
{
    _validateRemoveLiquidityArgs(args);
    
    // ...

    _registerNoteFooter(args.outNoteFooters[0]);
    _registerNoteFooter(args.outNoteFooters[1]);

    // ...
}

Here, _validateRemoveLiquidityArgs checks that args.outNoteFooters[0] and args.outNoteFooters[1] have not been marked as used yet. The _registerNoteFooter function is a wrapper around the following function in the MerkleTreeOperator contract:

function setNoteFooterUsed(bytes32 noteFooter) external onlyAssetManager {
    if (noteFooter != bytes32(0)) {
        noteFootersUsed[noteFooter] = true;
    }
}

This is idempotent, so calling it twice in a row for the same footer marks that footer as used but does not fail.

The situation is similar for other actions and in other contracts wherever more than one footer is checked.

An analogous issue arises when checking for the entire note already having appeared, for example in DarkpoolAssetManager's split function:

function split(
    bytes32 _merkleRoot,
    bytes32 _nullifierIn1,
    bytes32 _noteOut1,
    bytes32 _noteOut2,
    bytes calldata _proof
) public payable {
    if (!_merkleTreeOperator.noteIsNotCreated(_noteOut1)) {
        revert NoteAlreadyCreated();
    }
    if (!_merkleTreeOperator.noteIsNotCreated(_noteOut2)) {
        revert NoteAlreadyCreated();
    }

    // ...

    _postDeposit(_noteOut1);
    _postDeposit(_noteOut2);
    
    // ...
}

The _postDeposit function is implemented as follows:

function _postDeposit(bytes32 _noteCommitment) internal {
    _merkleTreeOperator.setNoteCommitmentCreated(_noteCommitment);
    _merkleTreeOperator.appendMerkleLeaf(bytes32(_noteCommitment));
}

Neither setNoteCommitmentCreated nor appendMerkleLeaf revert when called again with the same argument. Using the same note for _noteOut1 and _noteOut2 will thus cause that note to appear twice in the Merkle tree, with the note commitment marked as used.

Impact

Note footers can be reused, making all but one of the resulting notes with the same nullifier unspendable. Precise impact depends on the front-end used. Singularity informed us that the intention for note-footer--reuse checks is to prevent user error, in particular copy-pasting the same footer twice and thereby accidentally making funds unusable. This type of user error appears likely to appear particularly within the same transaction. As user error is needed, we still rate likelihood as Low.

Recommendations

Ensure that the output footers must be distinct. This could be done by verifying the footers are distinct in the circuit or alternatively by verifying this in the contract, for example by modifying the function setNoteFooterUsed of the MerkleTreeOperator contract to revert whenever the note footer was already marked used:

function setNoteFooterUsed(bytes32 noteFooter) external onlyAssetManager {
+     require(noteFooterIsNotUsed(noteFooter));
    if (noteFooter != bytes32(0)) {
        noteFootersUsed[noteFooter] = true;
    }
}

For those cases in which the footers are not public (such as in the DarkpoolAssetManager's split function) and the note commitment is checked instead, the analogous change should be done. When checking for reuse in the contract rather than the circuit, this could be done by changing setNoteCommitmentCreated of the MerkleTreeOperator contract to revert whenever the note commitment was already marked used:

function setNoteCommitmentCreated(
    bytes32 commitment
) external onlyAssetManager {
+     require(noteIsNotCreated(commitment));
    noteCommitmentsCreated[commitment] = true;
}

Remediation

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