Caviar doesn’t check whether NFT or BaseToken exists

[code423n4]

Lines of code

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

Vulnerability details

The create() function in Caviar.sol is not checking whether the NFT or baseToken actually exists. If the supplied address is not a contract or doesn’t implement the required name() and symbol() function, the name/symbol is derived from the first symbols of the hex address. While this doesn’t have any impact upon creation of the pair, a non-existing NFT or baseToken contract might become risky when interacting with the Pair.sol contract.

Pair.sol uses solmate’s SafeTransferLib for transferring ERC20’s, which doesn’t check whether the ERC20 contract actually exists (https://github.com/Rari-Capital/solmate/blob/main/src/utils/SafeTransferLib.sol#L9). As a result, when the token's address has no code, the transaction will just succeed with no error.

One possible, albeit unlikely exploit scenario would be the following:

1. User creates a new pair with a non-existing base token.
2. User wraps one or multiple NFTs to acquire fractional tokens.
3. User adds liquidity using their acquired fractional tokens. While adding liquidity requires both base tokens and fractional tokens, the user can specify any amount of base tokens as the transfer of base tokens from the user to the contract will succeed if the base token contract doesn’t exist yet.
4. This way, the user is able to acquire LP tokens by only providing fractional tokens
5. Next, once the base token contract exists, and other users deposited actual base tokens in the Pair contract, the user can sell their LP tokens to receive both base tokens and fractional tokens.

More likely would be leveraging the fact it's becoming popular for protocols to deploy their token across multiple networks. In this case, a common practice is to deploy the token contract from the same deployer address and with the same nonce so that the token address can be the same for all the networks.

For example: 1INCH is using the same token address for both Ethereum and BSC; Gelato's GEL token is using the same token address for Ethereum, Fantom and Polygon.

A sophisticated attacker can exploit it by taking advantage of this and creating vaults on multiple other networks with base tokens which are not yet deployed on this network. The attacker could follow the same pattern as above and alter the state of the contract such that he/she is able to steal base tokens from future users once they start using the vault.

Impact

As it might be possible to steal funds from the Pair contract and future users of the system, the impact is considered high.

Tool Used

Manual Review

Recommended Mitigation Steps

We recommend to check whether the NFT and base token contract actually exist when creating the new pair in create() of Caviar.sol:

function create(address nft, address baseToken, bytes32 merkleRoot) public returns (Pair pair) {
    require (nft.code.length > 0 && baseToken.code.length > 0, "invalid NFT or baseToken contract");
    ...
}

[c4-judge]

berndartmueller marked the issue as primary issue

[c4-sponsor]

outdoteth marked the issue as sponsor confirmed

[outdoteth]

Fixed in: https://github.com/outdoteth/caviar/pull/1

[berndartmueller]

I agree with the warden that checking the existence of the base and NFT token contract is lacking (in terms of a QA improvement). However, deploying pairs with non-existent token contracts leads to a non-functioning Pair contract. Please see the great explanation from @Shungy for further details -> https://github.com/code-423n4/2022-12-caviar-findings/issues/455#issuecomment-1360966128

Additionally, one of the stated security considerations from the sponsors mentions the risk of malicious pairs. Please see https://github.com/code-423n4/2022-12-caviar#malicious-base-token-or-nft-contracts

Due to the fact that deployed pairs with non-existent token contracts will cause the add, remove, buy, sell functions to revert anyway and the known risk stated by the sponsor, I'm downgrading the finding to QA (low).

[c4-judge]

berndartmueller changed the severity to QA (Quality Assurance)

[c4-judge]

berndartmueller marked the issue as grade-c