A better approach to the emergency exit logic

[code423n4]

Lines of code

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

Vulnerability details

Impact

The edge case scenario described in SPECIFICATION.md should not be the reason behind the emergency exit logic. In a non-custodial crypto world, the 0.000001 fractional tokens could also be irretrievable due to holders turning inactive and/or losing their private keys other than sending the tokens to the zero address. Moreover, due to the nature of AMM trades, it is unlikely that 1 fractional token, 200 USDC and 1 NFT are going to unanimously exist in the pool even if none of the fractional tokens have been deemed inaccessible. For instance, Alice might possess 0.2 fractional tokens in her wallet while Bob had 0.3 fractional tokens, and only 0.5 fractional tokens remained in the pool.

Additionally, when such situation were to happen, the 7 day grace period served no purpose considering no one would be able to withdraw any NFT from the contract simply because there was not enough fractional tokens in the pool to permit that.

Proof of Concept

Here is a hypothetical situation:

1. Alice deposited 1 NFT and 200 USDC into a pool
2. Bob then bought 0.000001 fractional tokens from the pool and sent it to the zero address.
3. The grievance was acknowledged by the admin who proceeded to closing the pair and kicking off the 1 week grace period.

Now, what is the point of having a grace period here when there is no other staker involved other than Alice?

In a more practical scenario similar to the above situation where there were other liquidity providers other than Alice with less than 1 fractional tokens left in the pool, again, the 7 day grace period serves no purpose here.

Additionally, the exit plan could only solve the issue of withdrawing the NFTs. What about the base tokens in the pool? If all fractional tokens were to return to the pool in exchange for base tokens, baseTokenReserves() and fractionalTokenReserves() would both reach close to a point where there was minimal impermanent loss to the liquidity providers. It would no doubt continue to serve stakers who have provided lp via add() and facilitate buy() and sell(), but this was going to impair the accounting part because lpToken.totalSupply() included all lp holders who have also staked via nftAdd().

For the same reason just described, the proceeds from the auction are distributed pro rata to fractional token holders who can burn their tokens in exchange is another part enhancing the grievance. Fractional token holders got their tokens via buy() with base tokens, not their NFTs. Why should they be compensated with a part of the proceeds?

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider refactoring withdraw() by adding the following require statement:

+ require(balanceOf[address(this)] < ONE); 

Increase the threshold to ONE * NUMBER where NUMBER could either be a constant or a settable variable where deemed fit.

And when the above condition is reached, withdraw the base token balance or at least the portion belonging to the lp holders who have staked via nftAdd() too.

This will incentivize the nasty fractional holders to sell their fractional tokens to the pool to usually get back more base tokens than they originally paid for given the significantly low fractionalTokenReserves().

This should revive the hope of deepening the pool catering to the myriad needs of the traders again. That said, there is no economical incentive to maliciously sending more than a fractional tokens to the zero address. As long as active tradings have been established, the perceivable 1 fractional token short to unwrap an NFT will never surface.

And, amidst adopting the above recommendation, the admin trustfulness issue could thereby be resolved by associating a multisig wallet to the owner.

[JeffCX]

lol. Duplicate of my https://github.com/code-423n4/2022-12-caviar-findings/issues/16 that is already marked as invalid.

[outdoteth]

The 1 week grace period allows for a nice buffer so that holders of fractional tokens can decide if they want to get the auction proceeds or not. If they don't, they can withdraw unwrap their fractional tokens and get whole NFTs instead.

[c4-sponsor]

outdoteth marked the issue as sponsor disputed

[c4-judge]

berndartmueller marked the issue as unsatisfactory: Invalid