code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Caviar.sol#L28 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L46 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/LpToken.sol#L13

Vulnerability details

Impact

Creating a new AMM pool needs a unique combination of NFT, BaseToken and MerkleRoot.

On creating a new AMM pool, a LP token is created whose name is string concatenation of pair symbol and LP token as defined in Line 13 of LpToken.sol. Same convention is followed for symbol.

Note that key LP token identifiers (name & symbol) are common for AMM pools sharing same NFT/BaseToken but different merkleroots. And a malicious user can create (effectively) unlimited subsets of existing token Id's to create different merkleroots. Also note that the only way to destroy a pair mapping is by calling destroy function in Caviar.sol which can only be called by the pair contract itself.

Initiating a call by the pair contract would mean that close function needs to be called by Caviar owner which comes with a timelock. This makes response to an attacker slow & can potentially flood the protocol with duplicate pools

Since this can significantly degrade the user experience (although not causing any material loss of tokens), I've marked it as MEDIUM risk

Proof of Concept

Bob creates a Azuki/USDC AMM pool by creating a merkle root that uses token ID's

 "tokenIds": [
    "2678",
    "9196",
    "7832",
    "416",
    "5351",
    "262",
]

Bob creates a second Azuki/USDC AMM pool by creating a merkle root that uses token ID's

 "tokenIds": [
    "2678",
    "9196",
    "7832",
    "416",
    "5351",
    "262",
]

Both have identical looking LP tokens but with different addresses.

Tools Used

Recommended Mitigation Steps

Can be multiple ways of mitigating the risk of duplicate pools

Whitelist users who can create new pools
Check if LP token with same name is already issued - if yes, then prevent users from creating duplicate pools

berndartmueller commented 1 year ago

Known risk by the sponsor. Closing as out of scope.

c4-judge commented 1 year ago

berndartmueller marked the issue as unsatisfactory: Out of scope

code-423n4 / 2022-12-caviar-findings