Possible Reentrancy Vulnerability

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L89-L96 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L124-L130 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L232-L240

Vulnerability details

Impact

In Add() function, in case of non Eth Base Token, LP Tokens are minted for LP Provider before transferring the Base Token from Provider to Contract. In remove() function, Fractional Tokens are Transferred to LP Provider before Burning the LP Token. In wrap() function, Fractional Tokens are minted for User before transferring the NFT to Contract.

It is always Recommended to finish all internal work first, and only then calling the external function to avoid vulnerabilities due to reentrancy.

Proof of Concept

https://consensys.github.io/smart-contract-best-practices/attacks/reentrancy/

Recommended Mitigation Steps

In add() Function, Base Tokens Should be Transferred Before minting LP Tokens.

File: src/Pair.sol

90:    // transfer base tokens in if the base token is not ETH
        if (baseToken != address(0)) {
            // transfer base tokens in
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);
        }

        // mint lp tokens to sender
        lpToken.mint(msg.sender, lpTokenAmount);

Link to code

In remove() function, LP Tokens should be Burned before transferring Fractional Tokens.

File: src/Pair.sol

124:    // *** Interactions *** //

        // burn lp tokens from sender
        lpToken.burn(msg.sender, lpTokenAmount);

        // transfer fractional tokens to sender
        _transferFrom(address(this), msg.sender, fractionalTokenOutputAmount);

Link to code

In wrap() function, NFTs should be transferred to contract before minting Franctional Tokens.

File: src/Pair.sol

232:   // *** Interactions *** //

        // transfer nfts from sender
        for (uint256 i = 0; i < tokenIds.length; i++) {
            ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);
        }

        fractionalTokenAmount = tokenIds.length * ONE;
        _mint(msg.sender, fractionalTokenAmount);

Link to code

[iFrostizz]

The finding does not highlight any precise vulnerability, the control flow is fine because the safeTransferFrom doesn't set "to" to an address that could purposely take advantage of this reentrancy because "to" is equal to "address(this)".

[c4-judge]

berndartmueller marked the issue as unsatisfactory: Insufficient proof