Pair.sol:close() does not recover any of the base token / liquidity tokens before destroying it

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L341-L352

Vulnerability details

Impact

If pair hold any of the native or any other liquidity tokens, that never be recovered if the pair is destroyed.

Proof of Concept

owner has special rights to destroy the pair if it is compromised. But before destroy, it does not recover any of of the tokens from the pair pool.

function close() public {
    // check that the sender is the caviar owner
    require(caviar.owner() == msg.sender, "Close: not owner");

    // set the close timestamp with a grace period
    closeTimestamp = block.timestamp + CLOSE_GRACE_PERIOD;

    // remove the pair from the Caviar contract
    caviar.destroy(nft, baseToken, merkleRoot);

    emit Close(closeTimestamp);
}.

Tools Used

Manual review

Recommended Mitigation Steps

Recover any of the native/other liguidity tokens before destroying

[Shungy]

Seems invalid.

close does not destroy the contract. Hence base tokens can still be recovered through removing the liquidity. NFT balance in the contract is independent of the reserve tokens. Removing NFTs do not effect the AMM functionality.

Although there is an argument that by removing the NFTs fractional tokens are made valueless, hence after close people will rush to dump fractional tokens to get base token, hurting existing liquidity providers. But grace period of one week before close prevents that. So I don't think there is much of an issue here.

[aktech297]

Whats is purpose of the caviar.destroy then ?

[c4-judge]

berndartmueller marked the issue as primary issue

[JeffCX]

I do not find this issue valid. Even after the pair is closed. The nftRemove and unwrap are not blocked.

[aktech297]

one case is,

after close, if same nft is used to create another pair, then all the fractional related amount would be new and it may be less or more that what was earlier. imo, it should not be like that

[c4-sponsor]

outdoteth marked the issue as sponsor disputed

[outdoteth]

If pair hold any of the native or any other liquidity tokens, that never be recovered if the pair is destroyed.

This assumption is wrong. After destroy all of unwrap() and buy(), sell(), add() and withdraw() are still all functioning.

[c4-judge]

berndartmueller marked the issue as unsatisfactory: Invalid