wrong token address may lead to DoS AMM Functionalities

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Caviar.sol#L37

Vulnerability details

Impact

solmate SafeTransferLib does not check the existence of code at token address. creating a pair using baseToken address that not exists, may lead to DoS the functionalities of the pair related to AMM.

Proof of Concept

How to reproduce:

1. Pair is created using non valid baseToken address.
2. when nftAdd is called it will set _baseTokenReserves to zero.

All the next interaction with the contract using the following functions will revert:

• nftAdd
• nftRemove
• nftBuy
• nftSell

PoC

pragma solidity ^0.8.17;

import "forge-std/Test.sol";

import "../../script/CreatePair.s.sol";

import "../shared/mocks/MockERC721.sol";
import "../../src/Caviar.sol";
import "../../src/Pair.sol";

contract DoS_Pair is Test {
    CreatePairScript public createPairScript;

    MockERC721 public bayc;

    Caviar public c;
    Pair public p;

    uint256[] public tokenIds1;
    uint256[] public tokenIds2;
    bytes32[][] public proofs;

    constructor() {
        createPairScript = new CreatePairScript();

        c = new Caviar();

        bayc = new MockERC721("bayc", "BAYC");

        p = c.create(address(bayc), address(0xbeef), bytes32(0));
    }

    function setUp() public {
        bayc.mint(address(this), 1);
        bayc.mint(address(this), 2);
        tokenIds1.push(1);
        tokenIds2.push(2);

        bayc.setApprovalForAll(address(p), true);
    }

    function testDosNftAdd() public {
        uint256 baseTokenAmount = 1000000000000000000;
        uint256 minLpTokenAmount = 1000000000000000000;

        uint256 lpTokenAmount = p.nftAdd(baseTokenAmount, tokenIds1, minLpTokenAmount, proofs);
        assert(lpTokenAmount > 0);

        // this one should revert
        vm.expectRevert();
        p.nftAdd(baseTokenAmount, tokenIds2, minLpTokenAmount, proofs);
    }

    function testDosNftRemove() public {
        uint256 baseTokenAmount = 1000000000000000000;
        uint256 minLpTokenAmount = 1000000000000000000;

        uint256 lpTokenAmount = p.nftAdd(baseTokenAmount, tokenIds1, minLpTokenAmount, proofs);
        assert(lpTokenAmount > 0);

        vm.expectRevert();
        p.nftRemove(lpTokenAmount, 1, tokenIds1);
    }

    function testDosNftBuy() public {
        uint256 baseTokenAmount = 1000000000000000000;
        uint256 minLpTokenAmount = 1000000000000000000;

        uint256 lpTokenAmount = p.nftAdd(baseTokenAmount, tokenIds1, minLpTokenAmount, proofs);
        assert(lpTokenAmount > 0);

        vm.expectRevert();
        p.nftBuy(tokenIds1, 10000000000000000000);
    }

    function testDosNftSell() public {
        uint256 baseTokenAmount = 1000000000000000000;
        uint256 minLpTokenAmount = 1000000000000000000;

        uint256 lpTokenAmount = p.nftAdd(baseTokenAmount, tokenIds1, minLpTokenAmount, proofs);
        assert(lpTokenAmount > 0);

        vm.expectRevert();
        p.nftSell(tokenIds1, 1, proofs);
    }

}

Tools Used

manual review, foundry

Recommended Mitigation Steps

Before creating the pair check if baseToken address is a contract.

[Shungy]

Seems invalid.

In all AMM functions base token reserve is checked, hence it would revert if token did not exist.

So yeah, creating a pair with wrong or malicious token is not an issue. It is also acknowledged in README.

[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]

berndartmueller changed the severity to QA (Quality Assurance)

[c4-judge]

berndartmueller marked the issue as grade-c