Caviar contest - unforgiven's results

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L63-L99 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L417-L428

Vulnerability details

Impact

Function add() adds liquidity to the pair, it transfers base token and fractional token from user and mint lp token for user, but during the calculation of the lp token amount contract take minimum amount of lp token based on percentage of user supplied base and fractional token to contract balance, but contract won't return the extra amount of base token or fractional token to the user. users don't know exactly how much of each token is needed they may send extra tokens to make sure transaction won't get reverted because of low amount of tokens but this would cause to lose those extra tokens. and when a user makes mistake send very large amount of base or fractional tokens those all would be lost.

Proof of Concept

This is add() code:

    function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
        public
        payable
        returns (uint256 lpTokenAmount)
    {
        // *** Checks *** //

        // check the token amount inputs are not zero
        require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");

        // check that correct eth input was sent - if the baseToken equals address(0) then native ETH is used
        require(baseToken == address(0) ? msg.value == baseTokenAmount : msg.value == 0, "Invalid ether input");

        // calculate the lp token shares to mint
        lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);

        // check that the amount of lp tokens outputted is greater than the min amount
        require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");

        // *** Effects *** //

        // transfer fractional tokens in
        _transferFrom(msg.sender, address(this), fractionalTokenAmount);

        // *** Interactions *** //

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

        // 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);
        }

        emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
    }

As you can see code calls addQuote() and mint amount of lp tokens returned by that function for user and there is no logic to return extra base token or fractional token to the user. This is addQuote() code:

    function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        if (lpTokenSupply > 0) {
            // calculate amount of lp tokens as a fraction of existing reserves
            uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
            uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
            return Math.min(baseTokenShare, fractionalTokenShare);
        } else {
            // if there is no liquidity then init
            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        }
    }

As you can see to calculate the amount of lp token contract calculates amount of lp tokens as a fraction of existing reserves and then took the minimum values. so when this scenario happens:

1. contract has 100 * 1e18 base token balance and 50 * 1e18 fractional token balance and lpTokenSupply is 10 * 1e18.
2. user1 wants to add liquidity and calls add(11 * 1e18 base token, 5 * 1e18 fractional token, 1 * 1e18 minimum lp token).
3. contract would calculate lp token amount as baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves() = (11 * 1e18 * 10 *1e18) / (100 * 1e18) = 11 * 1e17 and fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves() = (5 * 1e18 * 10 * 1e18) / (50 * 1e18) = 1e18 and lpTokenAmount = min(11 * 1e17 , 1e18) = 1e18. so contract would mint 1e18 LP token for user1 and transfer 11 * 1e18 base token and 5 * 1e18 fractional token from user to contract address.
4. now contract has 111 * 1e18 base token balance and 55 * 1e18 fractional token balance and lpTokenSupply is 11 * 1e18.
5. now user has 1e18 lp token which if user wants to remove the liquidity user would receive baseTokenOutputAmount = (baseTokenReserves() * lpTokenAmount) / lpTokenSupply = (111 * 1e18 * 1e18) / (11 * 118) = 10*1e18 base token and fractionalTokenOutputAmount = (fractionalTokenReserves() * lpTokenAmount) / lpTokenSupply = (55 * 1e18 * 1e18) / (11 * 118) = 5 * 1e18 fractional token amount.
6. so user1 provided liquidity and then withdraw it immediately and lost 1e18 amount of base token in doing so.

the problem is that in the step #3 contract calculates lp token amount based on user supplied base token and fractional token and take the minimum between them but in the token that is not generating minimum lp tokens, user supplied more tokens (in the above example base token) so contract should return those extra tokens. in the above example the 1e18 lp token worth 10 * 1e18 base token and user supplied 11 * 1e18 amount. so contract should transfer that extra amount after calculating the lp amount.

This issue would happen in each call to add() function as users don't know exactly how much of each token they need to supply and to make sure transaction won't fail they would supply more tokens and it would make them to lose funds. in each call one of the base token amount or fractional token amount would generate lower lp token and user other tokens would have some extra amount. And in cases where user makes a mistake and transfer a lot of tokens from one of the base token or fractional token those tokens would be lost.

Tools Used

VIM

Recommended Mitigation Steps

after calculating the amount of minting lp tokens, calculate the amount of base token and fractional token required based on lp token amount and transfer those exact required amount of tokens to contract address and return the extra amount if necessary. something like this:

    function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
        public
        payable
        returns (uint256 lpTokenAmount)
    {
        // *** Checks *** //

        // check the token amount inputs are not zero
        require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");

        // check that correct eth input was sent - if the baseToken equals address(0) then native ETH is used
        require(baseToken == address(0) ? msg.value == baseTokenAmount : msg.value == 0, "Invalid ether input");

        // calculate the lp token shares to mint
        lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);

        // check that the amount of lp tokens outputted is greater than the min amount
        require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");

       // calculate actual amount of baseToken and fractionalToken needed
       (actualBaseTokenAmount, actualFractionalTokenAmount) = removeQuote(lpTokenAmount);  // calculate it by upper bound

        // *** Effects *** //

        // transfer fractional tokens in
        _transferFrom(msg.sender, address(this), actualFractionalTokenAmount); // transfer actual needed amount

        // *** Interactions *** //

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

        // 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), actualBaseTokenAmount);  // transfer actual needed amount
        }else { // transfer back extra ETH
           (msg.sender).msg.sender.safeTransferETH(baseTokenAmount -actualBaseTokenAmount);
        }

       

        emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
    }

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L411-L428 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L63-L99

Vulnerability details

Impact

Function addQuote() calculates the amount of LP tokens received for adding a given amount of base tokens and fractional tokens. attacker can manipulate token per liquidity value and make it so high that whenever users wants to provide liquidity they lose a lot of funds because of the rounding. by doing this attacker can steal other users funds and make Pair contract to be useless.

Proof of Concept

This is addQuote() code:

    function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        if (lpTokenSupply > 0) {
            // calculate amount of lp tokens as a fraction of existing reserves
            uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
            uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
            return Math.min(baseTokenShare, fractionalTokenShare);
        } else {
            // if there is no liquidity then init
            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        }
    }

As you can see the lp token amount is calculated as a share of existing deposits. If there are no existing deposits, then initializes to sqrt(baseTokenAmount * fractionalTokenAmount). and in the function add() this amount is used to mint lp token for user.

    function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
        public
        payable
        returns (uint256 lpTokenAmount)
    {
.....
.....
        // calculate the lp token shares to mint
        lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);
.....
.....
.....
        // mint lp tokens to sender
        lpToken.mint(msg.sender, lpTokenAmount);
....
....
    }

So to exploit this attacker can perform this steps:

1. create a Pair for NFT1 and BaseToken1 and merkle root hash as 0x0.
2. wrap 10 NFT token for 10 * 1e18 fractional token.
3. first deposit 1 fractional token and 1 baseToken as liquidity with function add() and contract would mint 1 LP token for attacker.
4. directly transfer rest of 10 * 1e18 fractional token to contract address and 1000 * 1e18 - 1 amount of baseToken to contract address.
5. now lpToken.totalSupply() is 1, baseTokenReserves() is 1000 * 1e18, fractionalTokenReserves() is 10 * 10e18.
6. user1 wants to deposit 1600 * 1e18 baseToken and 16 * 10e18 fractional token as liquidity and he would call add() for this.
7. contract in addQuote() would calculate lp token amount as: baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves() = 1500 * 10e18 * 1 / 1000 * 1e18 = 1, fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves() = 16 * 1e18 * 1 / 10 * 10e18 = 1 and lpTokenAmount = Math.min(baseTokenShare, fractionalTokenShare) = min(1,1) =1 and contract would mint 1 lp token for user and transfer 1600 * 1e18 baseToken and 16 * 10e18 fractional token from user.
8. now lpToken.totalSupply() is 2, baseTokenReserves() is 2600 * 1e18, fractionalTokenReserves() is 26 * 10e18.
9. user1 would remove his liquidity by calling remove(1) and contract would transfer 50% of the liquidity to user (because user1 has 1 LP token of the total 2 LP tokens) (calculations are done in the removeQuote()) and user1 would receive 1300 * 1e18 baseToken and 13 * 10e18 fractional token.
10. if attacker remove his liquidity he would receive 1300 * 1e18 baseToken and 13 * 10e18 fractional token. and as you can see user1 add token and removed them and lost 300 * 1e18 baseToken and 3 * 10e18 fractional token immediately and attacker took them.

even if user1 wants to specify minLpTokenAmount when calling add() function it would cause whole transaction to fail. the problem is that attacker manipulated token per share value and make it so high that users would lose their liquidity deposits through rounding error in calculations. the numbers in the POC was just an example and scenario can be different and the attack would still be effective. the early direct transfer of funds can be done by attacker with smart contract (which create Pair and add liquidity and transfer funds in one transaction) for popular NFT collection and merkle root 0x0 and popular baseTokens (like ETH) so attacker can perform the attack for common Pairs contract. also attacker doesn't need to be the first liquidity provider and if the first liquidity provider provide small amount of tokens attacker can directly transfer the tokens and perform the attack.

Tools Used

VIM

Recommended Mitigation Steps

one simple and fast solution would be add some precision for LP token amount so that token per liquidity can't be manipulate so easily.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L417-L428 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L398-L400 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L406-L409

Vulnerability details

Impact

contract Pair has an AMM inside it which is inspired by Uniswap V2 but some of the details which are implemented in Uniswap V2 are not implemented here. there are 4 actions to interact with this AMM: add liquidity, remove liquidity, buy fractional, sell fractional. traders and arbitrators by trading make the price the NFT to be fair and mechanism of the AMM is based on the fact that in each situation one of this actions are profitable and users always perform that action and AMM would be updated and functional all the time. but there are some states (balance of fractional and base token) in the Pair contract which makes every action unprofitable, these state can be created by first liquidity provider by mistake or intentionally. if first liquidity provider provide wrong ratio of base token and fractional token and small amount of them then contract would be in a state that selling and buying fractional token would cause loss (high slippage and wrong price) and adding liquidity would cause loss (wrong price) and in this state no user would interact with Pair AMM pool and one would take some loss if wants to interact with contract.

Proof of Concept

Uniswap V2 would permanently lock some minimum liquidity when first supply happens (https://github.com/Uniswap/v2-core/blob/ee547b17853e71ed4e0101ccfd52e70d5acded58/contracts/UniswapV2Pair.sol#L121) and this would prevent the bad states that AMM could stuck in them but in the Pair contract this behavior isn't implemented and there are scenarios that when the liquidity amounts are low in the Pair contract, then because of the slippage and wrong price all actions of the AMM would cause loss for users and users doesn't want to interact with AMM unless they want to take some losses. for example this scenario:

1. user1 creates a Pair1 which is for NFT1 and baseToken1 and merkle tree root hash 0x0. the fair price of the 1 NFT1 is 1000 baseToken1.
2. attacker provide 700 baseToken1 and 1 fractional token as first liquidity provider.
3. now all the actions of the AMM would cause loss or revert.
4. add(): if a user wants to provide liquidity he need to provide the amounts that the ratio is 1 to 700 because if he supply tokens with other ratios contract would calculate lp token amount based on current balance of the base and fractional token and user would lose the extra amount, for example if user provides 1 fractional token and 1000 base token then he would receive 1 lp token which is now worth 1 fractional token and 850 base token and user loss his funds as he supplied them as liquidity. and if user provides 1 to 700 ratio he would take impermanent loss when the price of AMM changes.
5. buy(): user can't buy fractional token because there is 1 fractional token and call would revert if user wants to buy that 1 token.
6. sell(): if user wants to sell 1 fractional token he would receive 350 base token while the fair price is 1000 base token so user would lose some funds.

so in this state all the actions are causing losses for users and no one would wants to interact with Pair. This was just a sample scenario and there are more general scenarios when the liquidity in the AMM pool become low enough that every action would cause loss because of high slippage and wrong price. for example in the above example if there were 1500 base token and 2 fractional token in the contract then buy and sell would cause loss because of the high slippage and liquidity providing would cause loss because of the wrong ratio(price) and impermanent loss.

Tools Used

VIM

Recommended Mitigation Steps

like Uniswap V2 add some minimum reserve when first liquidity happens to make sure that this bad states can't be reached.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L147-L176

Vulnerability details

Impact

Function buy() buys fractional tokens from the pair, there is no check in those functions that what user pays or receives isn't 0 so it's possible for attacker to buy some fractional tokens and pay 0 base token. so liquidity providers would lose funds because of this.

Proof of Concept

This is buy() code:

    function buy(uint256 outputAmount, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {
        // *** Checks *** //

        // check that correct eth input was sent - if the baseToken equals address(0) then native ETH is used
        require(baseToken == address(0) ? msg.value == maxInputAmount : msg.value == 0, "Invalid ether input");

        // calculate required input amount using xyk invariant
        inputAmount = buyQuote(outputAmount);

        // check that the required amount of base tokens is less than the max amount
        require(inputAmount <= maxInputAmount, "Slippage: amount in");

        // *** Effects *** //

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

        // *** Interactions *** //

        if (baseToken == address(0)) {
            // refund surplus eth
            uint256 refundAmount = maxInputAmount - inputAmount;
            if (refundAmount > 0) msg.sender.safeTransferETH(refundAmount);
        } else {
            // transfer base tokens in
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), inputAmount);
        }

        emit Buy(inputAmount, outputAmount);
    }

As you can see the amount user pays calculated in buyQuote() and the return value of that function has been used to transfer funds from user. there is no check that what user pays is not 0 and if the return value was 0 then user would pay 0 base token and receive some fractional tokens. This is buyQuote() code:

    function buyQuote(uint256 outputAmount) public view returns (uint256) {
        return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
    }

so when outputAmount * 1000 * baseTokenReserves() < (fractionalTokenReserves() - outputAmount) * 997 then user would pay 0 amount but receives fractional tokens. for example if:

1. fractionalTokenReserves() would equal to 1000 * (1e18 +1)
2. baseTokenReserves() equal to 997 * 1e15
3. then the expression which makes inputAmount 0 would become outputAmount * 1000 * 997 * 1e15 < (1000 * (1e18 +1) - outputAmount) * 997
4. which equals to outputAmount * 997 * 1e18 < (1000 * (1e18 +1) - outputAmount) * 997
5. which equals to outputAmount * 1e18 < 1000 * (1e18 +1) - outputAmount
6. which equals outputAmount (1+1e18) < 1000 * (1e18 +1)
7. which equals to outputAmount < 1000.
8. so in this situation attacker can withdraw buy any amount less than 1000 of fractional tokens and he would pay 0 amount of base token.

the attack is more practical if those 1000 fractional token worth more than fee, but if the numbers changes the amount could be higher and attack could become practical, the severity is Medium and not High because the attack is not always practical. the real cause if this issue is rounding error when price (base token to fractional token) is too high or too low but contract should prevent this cases too because it's a permission-less protocol which wants to support all pairs of NFTs and base tokens.

Tools Used

VIM

Recommended Mitigation Steps

add more precision to fractional token and also revert if user is going to pay 0 amount of base token.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L217-L243 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L248-L262

Vulnerability details

Impact

users can wrap() their NFT tokens (which id is whitelisted) and receive 1e18 fractional token or they can pay 1e18 fractional token and unwrap NFT token. there is two issue here:

1. anyone can swap their NFT token id with another NFT token id without paying any fee(both ids should be whitelisted). it's swap without fee.
2. attacker can swap his NFT token(with whitelisted id) for all the NFT balance of contract and steal those NFT tokens airdrop all in one transaction.

Proof of Concept

This is wrap() and unwrap() code:

    function wrap(uint256[] calldata tokenIds, bytes32[][] calldata proofs)
        public
        returns (uint256 fractionalTokenAmount)
    {
        // *** Checks *** //

        // check that wrapping is not closed
        require(closeTimestamp == 0, "Wrap: closed");

        // check the tokens exist in the merkle root
        _validateTokenIds(tokenIds, proofs);

        // *** Effects *** //

        // mint fractional tokens to sender
        fractionalTokenAmount = tokenIds.length * ONE;
        _mint(msg.sender, fractionalTokenAmount);

        // *** Interactions *** //

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

        emit Wrap(tokenIds);
    }

    function unwrap(uint256[] calldata tokenIds) public returns (uint256 fractionalTokenAmount) {
        // *** Effects *** //

        // burn fractional tokens from sender
        fractionalTokenAmount = tokenIds.length * ONE;
        _burn(msg.sender, fractionalTokenAmount);

        // *** Interactions *** //

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

        emit Unwrap(tokenIds);
    }

As you can see it's possible to wrap one NFT token (which id is whitelisted and is in merkle tree) and unwrap another NFT token without paying fee. so Pair contract create NFT swap without fee for users but there is no fee generated for those who wrapped and put their fractional tokens as liquidity providers. The other issue with this is that some NFT tokens air drop new NFT tokens for NFT holders by making NFT holders to call getAirdrop() function. attacker can use this swap functionality to get air drop token for all the NFT balance of the Pair contract. to steps to perform this attack:

1. if Pair contract is for NFT1 and baseToken1 and also merkle tree root hash is 0x0.
2. users deposited 100 NFT1 tokens to the Pair contract.
3. NFT1 decide to airdrop some new tokens for token holders and token holders need to call nft.getAirDrop(id) while they own the NFT id.
4. attacker would create a contract and buy one of the NFT1 tokens (attackerID1) and wrap it to receive 1e18 fractional tokens and perform this steps in the contract: 4.1 loop through all the NFT tokens in the Pair contract balance and: 4.2 unwrap NFT token id=i from Pair contract by paying 1e18 fractional token. 4.3 call nft.getAirDrop(i) and receive the new airdrop token. (the name of the function can be other thing not exactly getAirDrop()) 4.4 wrap NFT token id=i and receive 1e18 fractional token.
5. in the end attacker would unwrap attackerID1 token from Pair contract. so attacker was abled to receive all the air drops of the NFT tokens that were in the contract address, there could be 100 or 1000 NFT tokens in the contract address and attacker can steal their air drops in one transaction(by writing a contract). those air drops belongs to all the fractional owners and contract shouldn't allow one user to take all the air drops for himself. as airdrops are common in NFT collections so this bug is critical and would happen.

also some of the NFT tokens allows users to stake some tokens for their NFT tokens and receive rewards(for example BAYC/MAYC). if a user stakes tokens for his NFT tokens then wrap those NFT tokens then it would be possible for attacker to unwrap those tokens and steal user staked amounts. in this scenario user made a risky move and wrapped NFT tokens while they have stake but as a lot of users wants to stake for their NFTs this would make them unable to use caviar protocol.

also any other action that attacker can perform by becoming the owner of the NFT token is possible by this attack and if that action can harm the NFT token holders then attacker can harm by doing this attack and performing that action.

Tools Used

VIM

Recommended Mitigation Steps

the real solution to prevent this attack (stealing air drops) can be hard. some of the things can be done is: create functionality so admin can call getAirDrop() functions during the airdrops before attacker. call getAirDrop() (which admin specified) function before unwrapping tokens. make some fee for NFT token unwrapping. create some lock time(some days) for each wrapped NFT that in that lock time only the one who supplied that token can unwrap it. create some delay for unwrapping tokens and if user wants to unwrap token he would receive it after this delay.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L245-L263

Vulnerability details

Impact

Function unwrap() unwraps fractional tokens into NFTs that user specified in the arguments. but code doesn't check that specified token ids are valid wrapped ids in the past by the contract. the Pair contract should supposed to wrap and unwrap token ids that are in merkle tree, but Pair contract can have other token ids in its balance, tokens ids that are transferred directly to contract and air dropped token ids. these tokens are not wrapped tokens and contract shouldn't allow users to unwrap this tokens ids. this can cause two issues:

1. if users call unwrap() with wrong ids by mistake then contract would burn their fractional tokens and users would receive some other unworthy tokens.
2. attacker can call unwrap() with more valuable air dropped token ids and steal them.

Proof of Concept

This is unwrap() code:

    function unwrap(uint256[] calldata tokenIds) public returns (uint256 fractionalTokenAmount) {
        // *** Effects *** //

        // burn fractional tokens from sender
        fractionalTokenAmount = tokenIds.length * ONE;
        _burn(msg.sender, fractionalTokenAmount);

        // *** Interactions *** //

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

        emit Unwrap(tokenIds);
    }

As you can see there is no check that token ids are valid ids that are wrapped by contract in the past. to issue happens with this steps:

Scenario 1: Users lose funds because of wrong token ids

1. a Pair contract with 10 NFT ids=[1,2,...,9,10] has been created in the Collection1 and these ids are very valuable NFTS and have same values but other ids have lower price.
2. attacker sends token id 0 of the Collection1 to the Pair contract address and token id 0 has less value.
3. user1 tries to buy token 0 by sending baseToken and buying fractional token and unwrapping token 0. (can call buyNFT directly too).
4. user1 would pay token ids [1,2,....9,10] price but he would receive less valuable token 0. As there could be a lot users who wrap() and unwrap() in the Pair contract so users could make mistake and if they do they would lose their funds. the extra ids can be worthless airdropped tokens too.

scenario 2: attacker steal transferred valuable air dropped tokens

1. a Pair contract with 10 NFT ids=[1,2,...,9,10] has been created in the Collection1.
2. users deposit the 10 NFT into the Pair contract.
3. Collection1 airdrop one of valuable token ids>1000 to any user who has 10 NFT token.
4. Pair contract would receive one very valuable token id>1000 (for example id=1200).
5. attacker would pay baseToken worth of one of NFT token ids [1,2,3,...,9,10] and receive fractional token and then unwrap token id 1200.
6. attacker paid the price of cheap tokens ids and received a valuable token. those valuable airdropped tokens belongs to all the fractional token holders and contract should allow anyone to unwrap and withdraw them.

Tools Used

VIM

Recommended Mitigation Steps

don't allow unwrapping of other tokens ids that are not wrapped. when token ids are wrapped or unwrapped just keep track of them by variable isWrapped[] and only allow wrapped token to be unwrapped. owner can distribute transferred tokens or airdropped token between users manually when Pair is closed as there is no logic to do that in the code.