Caviar Private Pools - 0xLanterns's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L733

Vulnerability details

Impact

In the changeFeeQuote function, the variable exponent is calculated based on the number of decimals in the base token, which is either 18 (for ETH) or retrieved using the decimals() function of an ERC20 token. This exponent is then used to calculate the feePerNft variable by multiplying changeFee by 10 to the power of the exponent.

The issue with using tokens like Gemini USD, a 2 decimal token, as the base token in this function is that subtracting 4 from 2 will result in a negative number (-2), which will cause underflow when calculating exponent. In Solidity version 0.8.0 and later, underflowing an unsigned integer will cause a revert, so attempting to execute this function with these types of tokens as the base token will result in a revert.

The impact of this issue is that the changeFeeQuote function will not work as expected when the base token is Gemini USD, which will prevent users from using this function to calculate fee amounts. Gemini USD is a well-known token and is likely to be used in a pool at some point. There is nothing preventing these pools from being created. However, an important function change() would not be usable in these pools.

Proof of Concept

The easiest way to test is to add this code snippet to ShibaInu.sol

file: ShibaInu.sol
contract GUSD is ERC20 {
    constructor() ERC20("Gemini USD", "gUSD", 2) {}
}

And add this snippet to Fixture.sol

file: fixture.sol    
GUSD public gUSD = new GUSD();

And then run this in change

file: change.t.sol   
// use mock gUSD contract address for base token
    // run: forge test --match-test test_SmallDecimals --ffi
    function test_SmallDecimals() public {
        privatePool = new PrivatePool(
            address(factory),
            address(royaltyRegistry),
            address(stolenNftOracle)
        );
        privatePool.initialize(
            address(gUSD),
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false
        );

        inputTokenIds.push(0);
        inputTokenIds.push(1);
        inputTokenIds.push(2);

        outputTokenIds.push(3);
        outputTokenIds.push(4);
        outputTokenIds.push(5);
        vm.expectRevert();
        privatePool.changeFeeQuote(outputTokenIds.length * 1e18);
    }

Tools Used

Manual Analysis, Foundry

Recommended Mitigation Steps

I would recommend implementing a check in the create() function in Factory.sol that checks if _basetoken has more than 4 decimals and reverts if it doesn't. This way, you do not need to change the implementation of your PrivatePool contract.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L236

Vulnerability details

Impact

Recipients of NFTs who accept royalties will not get their fair share of royalties. This is because royalties is calculated by dividing the sales price equally amongst all sold NFT's in that purchase. The issue with this is that it assumes all NFT's cost the same amount when it comes time to deal out royalties. If NFT's cost differnt amounts then they should be getting a amount of royalties based on that weight relative to the other NFT's. the impact of this is that Royalties will not be distributed evenly at the expense of the more expensive NFT. Meaning that recipients of the expensive NFT will always recieve less then they are owed. And the cheaper ones will get more than owed. In short the is a loss of funds or misdistribution of funds.

Proof of Concept

The easyiest way to test this will to be add this snippet into Milady.sol

Using this to have access to ERC2981's setRoyaltyInfo()

file: Milady.sol
    function setRoyaltyInfo(
        uint256 _royaltyFeeRate,
        address _royaltyRecipient
    ) public {
        royaltyFeeRate = _royaltyFeeRate;
        royaltyRecipient = _royaltyRecipient;
    }

    function supportsInterface(
        bytes4 interfaceId
    ) public view override(ERC2981, ERC721) returns (bool) {
        return super.supportsInterface(interfaceId);
    }

    function royaltyInfo(
        uint256 id,
        uint256 salePrice
    ) public view override returns (address, uint256) {
        return super.royaltyInfo(id, salePrice);
    }

    function setRoyaltyInfo(uint256 id, address reciever, uint96 fee) public {
        super._setTokenRoyalty(id, reciever, fee);
    }
}

Then add this snippet to Fixture.sol

file: Fixture.sol

    GodsUnchained public gu = new GodsUnchained();

Then add this snippet to token-weights.json

Changing the weights to represent the two NFT's nbeing bought in this case

[
  [
    1,
    1
  ],
  [
    2,
    10
  ]
]

Lastly to test this you need to add this test to Buy.t.sol

    // forge test --match-test test_unevenRoyalties --ffi
    function test_unevenRoyalties() public {
        // arrange
        privatePool = new PrivatePool(
            address(factory),
            address(royaltyRegistry),
            address(stolenNftOracle)
        );
        privatePool.initialize(
            baseToken,
            address(gu),
            virtualBaseTokenReserves,
            12e18,
            changeFee,
            feeRate,
            generateMerkleRoot(),
            true,
            true
        );
        //> owner of nft's
        address user1 = address(0xbeefbeef);
        address user2 = address(0xfeebfeeb);

        //> mint and push nft's one is 1x one is 10x
        gu.mint(address(privatePool), 1);
        tokenIds.push(1);
        tokenWeights.push(1e18);

        gu.mint(address(privatePool), 2);
        tokenIds.push(2);
        tokenWeights.push(10e18);

        //> set fees. 1% for one user, 10% for the other
        gu.setRoyaltyInfo(1, user1, 100);
        gu.setRoyaltyInfo(2, user2, 1000);

        //> set up
        proofs = generateMerkleProofs(tokenIds, tokenWeights);
        uint256 weightSum = privatePool.sumWeightsAndValidateProof(
            tokenIds,
            tokenWeights,
            proofs
        );
        (uint256 netInputAmount, , ) = privatePool.buyQuote(weightSum);

        //> buy
        privatePool.buy{value: netInputAmount * 2}( 
            tokenIds,
            tokenWeights,
            proofs
        );

        //> assert that users got equal reserves. with different weights and royalty fees users should NOT be getting the same amount of royalties
        //> the royalty fee of user2 is 10 times greater than user one. This shows that user2 is getting 10 times as much royaltys when user 2 should be getting 100 times as much as user one.
        //> user 1: 1% of 1 eth = 0.01 eth
        //> user 2: 10% of 10 eth = 1 eth
        //> 0.01 eth * 100 = 1eth.
        //> user 2 should be getting 100 times more royalties than user 1 but is only getting 10 times the amount.
        assertEq(user1.balance * 10, address(user2).balance);
    }

Tools Used

Manual Analysis, Foundry

Recommended Mitigation Steps

To address this issue, it is recommended that the weight of NFTs relative to other NFTs being purchased should be taken into consideration when calculating royalties.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L423

Vulnerability details

Impact

Some ERC20 tokens revert when 0 value is transferred. Tokens like LEND have this additional restriction and can cause the pool's functionality to break. When fees are set to 0, users cannot access the Change() function. This broken functionality can limit what users can do in the pool. Having a token like LEND in a private pool is reasonable as the token is relatively well-known, but its extra functionality can cause a key function in the Private pool to revert.

Proof of Concept

The easiest way to test this to is to add this code snippet to ShibaInu.sol

file: ShibaInu.sol


contract LEND is ERC20 {
    mapping(address => uint256) balances;

    constructor() ERC20("LEND", "lend", 18) {}

    function transferFrom(
        address src,
        address dst,
        uint wad
    ) public override returns (bool) {
        require(balances[dst] + wad > balances[dst]);
        return super.transferFrom(src, dst, wad);
    }

    function transfer(address dst, uint wad) public override returns (bool) {
        require(balances[dst] + wad > balances[dst]);
        return super.transfer(dst, wad);
    }
}

Then add this snippet to Fixture.sol

file: Fixture.sol

    LEND public lend = new LEND();

Lastly add and run this test in Change.t.sol

file: Change.t.sol

    // run: forge test --match-test test_changeWithZeroFee --ffi
    function test_changeWithZeroFee() public {
        privatePool = new PrivatePool(
            address(factory),
            address(royaltyRegistry),
            address(stolenNftOracle)
        );
        privatePool.initialize(
            address(lend),
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            0,
            feeRate,
            merkleRoot,
            true,
            false
        );

        milady.setApprovalForAll(address(privatePool), true);
        lend.approve(address(privatePool), type(uint256).max);

        inputTokenIds.push(0);
        inputTokenIds.push(1);
        inputTokenIds.push(2);

        milady.mint(address(privatePool), 10);
        milady.mint(address(privatePool), 11);
        milady.mint(address(privatePool), 12);
        outputTokenIds.push(10);
        outputTokenIds.push(11);
        outputTokenIds.push(12);
        (uint256 feeAmount, ) = privatePool.changeFeeQuote(
            outputTokenIds.length * 1e18
        );
        deal(address(lend), address(this), feeAmount);

        vm.expectRevert();
        privatePool.change(
            inputTokenIds,
            inputTokenWeights,
            inputProof,
            stolenNftProofs,
            outputTokenIds,
            outputTokenWeights,
            outputProof
        );
    }

Tools Used

Manual Analysis, Foundry

Recommended Mitigation Steps

Before transferring feeAmount from msg.sender to the pool in the change function, there should be a check to confirm that the fee amount is greater than 0. This would prevent any zero transfers from occurring and allow the change() function to work as intended with tokens that revert on zero transfers.