Calling `sell()` in `Pair.sol` could be at risk without contract existence check

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L182-L207

Vulnerability details

Impact

In Pair.sol, sell() could be used to fool users into selling fractional tokens to receive base tokens whose contract has already been self-destructed. As a result, the swappers would end up getting zero base tokens out because the base token contract had been destroyed by its malicious owner.

Proof of Concept

As denoted by File: SafeTransferLib.sol#L9:

/// @dev Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.

As such, the following token transfer, when externally called to Solmate's SafeTransferLib.sol, is performing low-level calls without confirming contract’s existence that could return success even though no function call was executed:

File: Pair.sol#L182-L207

            ERC20(baseToken).safeTransfer(msg.sender, outputAmount);

Here is an example of the hypothetical situation:

1. Bob, a malicious actor, deploys a standard ERC20 token contract with name and symbol similar to a mainstream token, e.g. "USDT", "DAI", "LINK", etc on top of a subtle function embedded that could lead to selfdestruct().
2. He then calls create() in Caviar.sol creating a pool using his malicious token contract as baseToken.
3. Next, he starts promoting a campaign inviting people to buy his base tokens to add liquidity and/or swap into the pool.
4. When the pool gets relevantly deep, he calls selfdestruct() on his token contract.
5. Consequently, all future swaps via sell() will lead to users losing their sent in tokens since no tokens out will be realized.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider implementing contract existence check on ERC20(baseToken).safeTransfer. This will ensure sell() to revert when a token contract no longer exists, preventing traders from losing their funds.

Note: When a contract calls SELFDESTRUCT , it does not get deleted. Instead, its code gets zeroed out and its nonce increased by 2**40 . As such, the protocol's zero address check will not stem this from happening.

[c4-judge]

berndartmueller marked the issue as duplicate of #245

[berndartmueller]

Downgrading to QA (Low). Please see https://github.com/code-423n4/2022-12-caviar-findings/issues/245#issuecomment-1382854995 for my reasoning.

[c4-judge]

berndartmueller marked the issue as not a duplicate

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-12-caviar-findings/issues/206