Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L623-L654

Vulnerability details

Impact

PrivatePool functions buy() & change() refund excess eth to msg.sender. This is noticeably absent in PrivatePool.flashLoan() - if the user sends eth above the fee amount, the excess eth will not be refunded and remain inside PrivatePool. This is inconsistent with the way other functions operate & results in loss of funds for msg.sender.

Proof of Concept

PrivatePool.flashLoan() L635 reverts if "msg.value < fee", allowing the transaction to proceed if msg.value >= fee. If msg.value > fee, the excess eth will not be refunded resulting in loss of funds to msg.sender. Compare to buy() & change() where fees are deducted & excess eth is refunded.

Tools Used

Manual

Recommended Mitigation Steps

In PrivatePool.flashLoan() follow the same pattern as in the other functions; refund excess eth above the fee back to msg.sender. Or alternatively change the check in L635 to revert if msg.value != fee

Lines of code

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

Vulnerability details

Impact

EthRouter will revert when multiple changes are attempted. This is because it attempts to transfer the full ETH value originally passed by the user into each subsequent change() operation. After the fees are taken from the first swap, the router no longer contains enough ETH to send the full original amount to the next change().

Proof of Concept

function test_MultiChangeEth() public {
	uint256[] memory inputTokenIds = new uint256[](1);
	uint256[] memory inputTokenWeights = new uint256[](0);
	uint256[] memory outputTokenIds = new uint256[](1);
	uint256[] memory outputTokenWeights = new uint256[](0);


	uint256[] memory inputTwoTokenIds = new uint256[](1);
	uint256[] memory outputTwoTokenIds = new uint256[](1);

	for (uint256 i = 0; i < 1; i++) {
		inputTokenIds[i] = i + 5;
		outputTokenIds[i] = i;
	}
	for (uint256 i = 1; i < 1; i++) {
		inputTwoTokenIds[i] = i + 5;
		outputTwoTokenIds[i] = i;
	}

	EthRouter.Change[] memory changes = new EthRouter.Change[](2);
	changes[0] = EthRouter.Change({
		pool: payable(address(privatePool)),
		nft: address(milady),
		inputTokenIds: inputTokenIds,
		inputTokenWeights: inputTokenWeights,
		inputProof: PrivatePool.MerkleMultiProof(new bytes32[](0), new bool[](0)),
		stolenNftProofs: new IStolenNftOracle.Message[](0),
		outputTokenIds: outputTokenIds,
		outputTokenWeights: outputTokenWeights,
		outputProof: PrivatePool.MerkleMultiProof(new bytes32[](0), new bool[](0))
	});
	changes[1] = EthRouter.Change({
		pool: payable(address(privatePool)),
		nft: address(milady),
		inputTokenIds: inputTwoTokenIds,
		inputTokenWeights: inputTokenWeights,
		inputProof: PrivatePool.MerkleMultiProof(new bytes32[](0), new bool[](0)),
		stolenNftProofs: new IStolenNftOracle.Message[](0),
		outputTokenIds: outputTwoTokenIds,
		outputTokenWeights: outputTokenWeights,
		outputProof: PrivatePool.MerkleMultiProof(new bytes32[](0), new bool[](0))
	});

	(uint256 changeFee,) = privatePool.changeFeeQuote(inputTokenIds.length * 1e18);
	uint256 balanceBefore = address(this).balance;

	// act
	ethRouter.change{value: changeFee * 2 + 1000}(changes, 0);
}

Tools Used

Foundry

Recommended Mitigation Steps

Change PrivatePool(_change.pool).change{value: msg.value}( in EthRouter to use the EthRouter's current balance instead of the msg.value.

See the markdown file with the details of this report here.

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L142

Vulnerability details

Impact

A high protocolFeeRate is detrimental to users (especially bots) as it can be changed instantly without any on-chain warning.

The worst-case scenario is a max fee of ~650% of the base price (due to the fact that protocolFeeRate is a uint16), which can be applied instantly, and can greatly increase the price a user pays.

Proof of Concept

There are no upper limits and also no timelock in protocolFeeRate:

/// @notice Sets the protocol fee that is taken on each buy/sell/change. It's in basis points: 350 = 3.5%.
/// @param _protocolFeeRate The protocol fee.
function setProtocolFeeRate(uint16 _protocolFeeRate) public onlyOwner {
    protocolFeeRate = _protocolFeeRate;
}

Docs state that:

All protocol fees accrue to the factory contract and can be withdrawn by the protocol admin. Initially the protocol fee rate will be set to be 0% however it may be increased in the future, with advanced notice.

Often these systems are managed by bots, and an off-chain notice is not sufficient, as they aren't easily able to check for this change.

Due to the fact that this fee can instantly change on-chain, and potentially greatly increase the total price, it's essential to give users some time to adapt to the new fee with a timelock, before it takes place.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a timelock, an upper limit, and emitting a protocolFeeRateChanged event to safeguard the users from any protocol fee changes.

See the markdown file with the details of this report here.

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/Factory.sol#L161
https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePoolMetadata.sol#L17

Vulnerability details

Impact

• By invoking the Factory.tokenURI method for a maliciously provided NFT id, the returned data may deceive potential users, as the method will return data for a non-existent NFT id that appears to be a genuine PrivatePool. This can lead to a poor user experience or financial loss for users.
• Violation of the ERC721-Metadata part standard

Proof of Concept

• The Factory.tokenURI and PrivatePoolMetadata.tokenURI methods lack any requirements stating that the provided NFT id must be created. We can also see that in the standard implementation by OpenZeppelin, this check is present:
• Throws if _tokenId is not a valid NFT

Example

1. User creates a fake contract
   A simple example so that the tokenURI method does not revert:

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

contract NFT {
    function balanceOf(address) external pure returns (uint256) {
        1;
    }
}

contract NonNFT {
    address public immutable nft;

    address public constant baseToken = address(0);
    uint256 public constant virtualBaseTokenReserves = 1 ether;
    uint256 public constant virtualNftReserves = 1 ether;
    uint256 public constant feeRate = 500;

    constructor() {
        nft = address(new NFT());
    }
}

2. User deploy the contract
3. Now, by using tokenURI() for the deployed user's address, one can fetch information about a non-existent NFT.

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

• Throw an error if the NFT id is invalid.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L514-L532

Vulnerability details

Impact

User may be not able to sell NFT.

Proof of Concept

The PrivatePool protocol allows owner to Withdraws NFTs and tokens from the pool. However, if the token to withdraw is baseToken, it may break sell function.

function withdraw(address _nft, uint256[] calldata tokenIds, address token, uint256 tokenAmount) public onlyOwner {
        // ~~~ Interactions ~~~ //

        // transfer the nfts to the caller
        for (uint256 i = 0; i < tokenIds.length; i++) {
            ERC721(_nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);
        }

        if (token == address(0)) {
            // transfer the ETH to the caller
            msg.sender.safeTransferETH(tokenAmount);
        } else {
            // transfer the tokens to the caller
            ERC20(token).transfer(msg.sender, tokenAmount);
        }

        // emit the withdraw event
        emit Withdraw(_nft, tokenIds, token, tokenAmount);
    }

When user calls the sell() function to sell NFTs into the pool ,the protocol will transfer base tokens to the caller. If owner withdraws a large amount of base tokens, there may not be enough tokens to transfer to user.
1.Assume the virtualBaseTokenReserves is 100e18， the virtualNftReserves is 5e18. And there are 3 NFTs in the protocol，the weight of each nft is 1e18,1e18,3e18.
2.Bob buys the NFT with a weight of 3e18. Now the virtualNftReserves is 2e18.
Before purchase:
virtualBaseTokenReserves = 100e18
virtualNftReserves = 5e18
After purchase:
virtualBaseTokenReserves = 100e18+3e18100e18/(5e18-3e18) = 250e18
virtualNftReserves = 5e18-3e18 = 2e18
3.Owner calls the withdraw() function to withdraw 160e18 base tokens for other purposes.
4.After a period of time,Bob calls the sell() function to sell NFT into the pool and get back base tokens.
Before sell:
virtualBaseTokenReserves = 250e18
virtualNftReserves = 2e18
After sell:
netOutputAmount = 3e18250e18/(3e18+2e18)- feeAmount - protocolFeeAmount -royaltyFeeAmount = 150e18 - fee
virtualBaseTokenReserves = 250e18-3e18*250e18/(3e18+2e18) = 100e18
virtualNftReserves = 2e18+3e18 = 5e18
The protocol will transfer netOutputAmount base tokens to user,however, due to the third step where the owner withdrew most of the funds, the user was unable to sell.

    function sellQuote(uint256 inputAmount)
        public
        view
        returns (uint256 netOutputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
    {
        // calculate the output amount based on xy=k invariant
        uint256 outputAmount = inputAmount * virtualBaseTokenReserves / (virtualNftReserves + inputAmount);

        protocolFeeAmount = outputAmount * Factory(factory).protocolFeeRate() / 10_000;
        feeAmount = outputAmount * feeRate / 10_000;
        netOutputAmount = outputAmount - feeAmount - protocolFeeAmount;
    }

Tools Used

Vscode

Recommended Mitigation Steps

Withdrawing base token is not allowed

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L651
https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L115
https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L152
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L365
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L527

Vulnerability details

Impact

Since token transfers are not caught on failure,an attacker can possibly abuse thisfact and trick the protocol

Proof of Concept

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

 function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 tokenId, bytes calldata data)
        external
        payable
        returns (bool)
    {
        // check that the NFT is available for a flash loan
        if (!availableForFlashLoan(token, tokenId)) revert NotAvailableForFlashLoan();

        // calculate the fee
        uint256 fee = flashFee(token, tokenId);

        // if base token is ETH then check that caller sent enough for the fee
        if (baseToken == address(0) && msg.value < fee) revert InvalidEthAmount();

        // transfer the NFT to the borrower
        ERC721(token).safeTransferFrom(address(this), address(receiver), tokenId);

        // call the borrower
        bool success =
            receiver.onFlashLoan(msg.sender, token, tokenId, fee, data) == keccak256("ERC3156FlashBorrower.onFlashLoan");

        // check that flashloan was successful
        if (!success) revert FlashLoanFailed();

        // transfer the NFT from the borrower
        ERC721(token).safeTransferFrom(address(receiver), address(this), tokenId);

        // transfer the fee from the borrower
        if (baseToken != address(0)) ERC20(baseToken).transferFrom(msg.sender, address(this), fee);

        return success;
    }

Tools Used

vscode

Recommended Mitigation Steps

Use safeTransfer and safeTransferFrom() instead of transfer() and transferFrom()

Lines of code

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

Vulnerability details

Impact

Fund loss

Proof of Concept

Lines 254 to 293 have a vulnerability called "Dangerous Usage of msg.value inside a Loop". The function change accepts an array of Change struct as input and executes them one by one in a loop. Inside the loop, there is a call to PrivatePool.change function at line 273 which transfers ETH to the PrivatePool contract. The amount of ETH transferred is equal to msg.value which is set to the value of the value field in the transaction that calls the change function. Since the change function is called in a loop, the value of msg.value can be different for each iteration of the loop. This can lead to unexpected behavior and result in a loss of funds.

Tools Used

https://app.metatrust.io/

Recommended Mitigation Steps

To fix this vulnerability, we should move the transfer of ETH outside the loop and calculate the total amount of ETH to be transferred before calling the PrivatePool.change function. We can use a variable to keep track of the total amount of ETH to be transferred and update it inside the loop. Then, we can transfer the total amount of ETH to the PrivatePool contract after the loop.

Here is an example of the fixed code:

function change(Change[] calldata changes, uint256 deadline) public payable {
// check deadline has not passed (if any)
if (block.timestamp > deadline && deadline != 0) {
revert DeadlinePassed();
}

uint256 totalValue = 0;

// loop through and execute the changes
for (uint256 i = 0; i < changes.length; i++) {
    Change memory _change = changes[i];

    // transfer NFTs from caller
    for (uint256 j = 0; j < changes[i].inputTokenIds.length; j++) {
        ERC721(_change.nft).safeTransferFrom(msg.sender, address(this), _change.inputTokenIds[j]);
    }

    // approve pair to transfer NFTs from router
    ERC721(_change.nft).setApprovalForAll(_change.pool, true);

    // calculate the value to be transferred
    uint256 changeValue = calculateChangeValue(_change);

    // add the value to the total value
    totalValue += changeValue;

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

    // transfer NFTs to caller
    for (uint256 j = 0; j < changes[i].outputTokenIds.length; j++) {
        ERC721(_change.nft).safeTransferFrom(address(this), msg.sender, _change.outputTokenIds[j]);
    }
}

// transfer the total value to the PrivatePool contract
if (totalValue > 0) {
    (bool success, ) = address(PrivatePool).call{value: totalValue}("");
    require(success, "Transfer failed");
}

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

}

function calculateChangeValue(Change memory _change) internal view returns (uint256) {
// calculate the value to be transferred
uint256 inputValue = 0;
for (uint256 i = 0; i < _change.inputTokenIds.length; i++) {
inputValue += getTokenValue(_change.inputTokenIds[i], _change.inputTokenWeights[i]);
}

uint256 outputValue = 0;
for (uint256 i = 0; i < _change.outputTokenIds.length; i++) {
    outputValue += getTokenValue(_change.outputTokenIds[i], _change.outputTokenWeights[i]);
}

return inputValue > outputValue ? inputValue - outputValue : 0;

}

function getTokenValue(uint256 tokenId, uint256 tokenWeight) internal view returns (uint256) {
// calculate the value of the token
// ...
}

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L238#L241
https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L219#L226
https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L250#L254
https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L283#L286

Vulnerability details

Impact

Detailed description of the impact of this finding.

I am referring this as sandwich attached, becoz the attacker monitors for deposits of tokens into the private pool and then attempts to with draw them using the change function.

EthRouter.deposit() is a public payable function, can be called by anyone
EthRouter.change() is a public payable function, can be called by anyone

An attacker the monitor the deposits of tokens into router and attack to withdraw the tokens deposited, using the change function.

Since the deposit function transfers the tokens from msg.sender to EthRouter contract as below
for (uint256 i = 0; i < tokenIds.length; i++) {
ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);
}

and change function transfers the tokens from EthRouter to msg.sender. As long as the attacker can pass the token Ids that are deposited in the routers, he can attempt to withdraw.
If attacker passes outputTokenIds array with deposited token ids, the below code will tranfer those back to his account.

// transfer NFTs to caller
for (uint256 j = 0; j < changes[i].outputTokenIds.length; j++) {
ERC721(_change.nft).safeTransferFrom(address(this), msg.sender, _change.outputTokenIds[j]);
}

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Action by the user:

uint256[] public tokenIds;
uint256 depositAmount = 1e18;
uint256 price = privatePool.price();

// depositing 3

tokenIds.push[0];
tokenIds.push[1];
tokenIds.push[2];

ethRouter.deposit{value: depositAmount}(
payable(address(privatePool)), address(milady), tokenIds, price, price - 1, 0
);

===> this will transfer NFTs from caller to Eth Router.

Action by Attacker

uint256[] memory inputTokenIds = new uint256;
uint256[] memory inputTokenWeights = new uint256;
uint256[] memory outputTokenIds = new uint256;
uint256[] memory outputTokenWeights = new uint256;

for (uint256 i = 0; i < 3; i++) {
outputTokenIds[i] = i;
}

EthRouter.Change[] memory changes = new EthRouter.Change;
changes[0] = EthRouter.Change({
pool: payable(address(privatePool)),
nft: address(milady),
inputTokenIds: inputTokenIds,
inputTokenWeights: inputTokenWeights,
inputProof: PrivatePool.MerkleMultiProof(new bytes32, new bool),
stolenNftProofs: new IStolenNftOracle.Message,
outputTokenIds: outputTokenIds,
outputTokenWeights: outputTokenWeights,
outputProof: PrivatePool.MerkleMultiProof(new bytes32, new bool)
});

// transfer NFTs to caller
for (uint256 j = 0; j < changes[i].outputTokenIds.length; j++) {
ERC721(_change.nft).safeTransferFrom(address(this), msg.sender, _change.outputTokenIds[j]);
}

Tools Used

Manual review

Recommended Mitigation Steps

This risk is primarily associated with tokens in this and not segregated. If the transfer is based on the tokens deposited at each user level in change function, the risk can be minimised.

Lines of code

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

Vulnerability details

Impact

Even if a token transfer function returns false without actually completing the transfer, it may still be counted as a successful transfer. Additionally, some tokens may not adhere to the EIP20 standard, such as USDT which does not provide a success boolean and will instead revert.

POC

Finding 1

Finding 2

Tools Used

Manual review

Recommended Mitigation Steps

To address these issues, it is suggested utilizing OpenZeppelin's SafeERC20 versions which include safeTransfer and safeTransferFrom functions. These functions not only handle the return value check but also accommodate non-standard compliant tokens.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L240
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L331
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L442
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L497
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L519

Vulnerability details

Impact

PrivatePool.sol contract has no Re-Entrancy protection in some function

    function deposit(uint256[] calldata tokenIds, uint256 baseTokenAmount) public payable {
        // ~~~ Checks ~~~ //


        // ensure the caller sent a valid amount of ETH if base token is ETH or that the caller sent 0 ETH if base token
        // is not ETH
        if ((baseToken == address(0) && msg.value != baseTokenAmount) || (msg.value > 0 && baseToken != address(0))) {
            revert InvalidEthAmount();
        }


        // ~~~ Interactions ~~~ //


        // transfer the nfts from the caller
        for (uint256 i = 0; i < tokenIds.length; i++) {
            ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);
        }


        if (baseToken != address(0)) {
            // transfer the base tokens from the caller
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);
        }


        // emit the deposit event
        emit Deposit(tokenIds, baseTokenAmount);
    }

Proof of Concept

If the safeTransferFrom was called by a contract, then the contract will be checked for it's ability to receive ERC721 tokens. Without reentrancy guard, onERC721Received will allow an attacker controlled contract to do something they want, which is dangerous.

function onERC721Received(
    address,
    address,
    uint256,
    bytes memory
  ) public virtual override returns (bytes4) {
    // reentrancy and do something
    }
    return this.onERC721Received.selector;
  }

Tools Used

Manual review

Recommended Mitigation Steps

Use Openzeppelin or Solmate Re-Entrancy pattern.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L484-L507

Vulnerability details

Impact

If user accidentally calls the deposint() function to transfer NFT and tokens in to the pool, s/he will lose these assets.

Proof of Concept

The PrivatePool.deposit() function is used to deposits base tokens and NFTs into the pool.If the user accidentally calls this function to transfer NFT and token into the pool, s/he will lose these assets.

function deposit(uint256[] calldata tokenIds, uint256 baseTokenAmount) public payable {
        // ~~~ Checks ~~~ //

        // ensure the caller sent a valid amount of ETH if base token is ETH or that the caller sent 0 ETH if base token
        // is not ETH
        if ((baseToken == address(0) && msg.value != baseTokenAmount) || (msg.value > 0 && baseToken != address(0))) {
            revert InvalidEthAmount();
        }

        // ~~~ Interactions ~~~ //

        // transfer the nfts from the caller
        for (uint256 i = 0; i < tokenIds.length; i++) {
            ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);
        }

        if (baseToken != address(0)) {
            // transfer the base tokens from the caller
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);
        }

        // emit the deposit event
        emit Deposit(tokenIds, baseTokenAmount);
    }

Tools Used

vscode

Recommended Mitigation Steps

Only specific users can call the deposit() function

Lines of code

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

Vulnerability details

Impact

Arbitrary code execution, complete loss of user funds.

The create function in the Factory contract allows anyone to create a new private pool with specific values, using the minimal proxy pattern. An attacker can modify the _baseToken, _nft, and _salt parameters of this function to create a new contract instance and execute arbitrary code. This could result in the complete loss of user funds and compromise the security of the entire system, below is the link to the vulnerable line of code

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

Proof of Concept

here is the link to the creation of private pool where by an attacker could inject the value

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

below is the code where an attacker create a new private pool with malicious parameters by calling the create() function, with the values injected

_baseToken = address(0)
_nft = address(0)
_virtualBaseTokenReserves = 1000000000000000000
_virtualNftReserves = 1000000000000000000
_changeFee = 0
_feeRate = 10000
_merkleRoot = bytes32(0)
_useStolenNftOracle = false
_payRoyalties = false
_salt = bytes32(0)
tokenIds = []
baseTokenAmount = 0

This will create a new private pool with a very high _virtualBaseTokenReserves value and a very high _feeRate value. The attacker can then exploit the high _feeRate value to charge exorbitant fees on each buy/sell/change transaction in the private pool

Tools Used

VSCODE

Recommended Mitigation Steps

function create(
address _baseToken,
address _nft,
uint128 _virtualBaseTokenReserves,
uint128 _virtualNftReserves,
uint56 _changeFee,
uint16 _feeRate,
bytes32 _merkleRoot,
bool _useStolenNftOracle,
bool _payRoyalties,
bytes32 _salt,
uint256[] memory tokenIds,
uint256 baseTokenAmount
)
public payable returns (PrivatePool privatePool) {
// deploy a minimal proxy clone of the private pool implementation
privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));
// mint the nft to the caller
_safeMint(msg.sender, uint256(uint160(address(privatePool))));
// initialize the pool
privatePool.initialize(
_baseToken,
_nft,
_virtualBaseTokenReserves,
_virtualNftReserves,
_changeFee,
_feeRate,
_merkleRoot,
_useStolenNftOracle,
_payRoyalties
);
To mitigate the function must only allow deployment of the PrivatePool contract and use a whitelist of allowed contracts. The _salt parameter should be removed or a random salt generation function should be implemented.

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L623-L654

Vulnerability details

Impact

In flashLoan, if the baseToken address is not address(0) and the msg.value sent by the user is not 0, then the ETH will be locked in the contract.

function flashLoan(
    IERC3156FlashBorrower receiver,
    address token,
    uint256 tokenId,
    bytes calldata data
) external payable returns (bool) {
    // check that the NFT is available for a flash loan
    if (!availableForFlashLoan(token, tokenId))
        revert NotAvailableForFlashLoan();

    // calculate the fee
    uint256 fee = flashFee(token, tokenId);

    // if base token is ETH then check that caller sent enough for the fee
    if (baseToken == address(0) && msg.value < fee)
        revert InvalidEthAmount();

Proof of Concept

// test\\PrivatePool\\Flashloan.t.sol
function test_PaysFlashLoanFeeWithBaseToken() public {
    // arrange
    stdstore.target(address(privatePool)).sig("baseToken()").checked_write(
        address(shibaInu)
    );
    uint256 fee = privatePool.flashFee(address(0), 1);
    deal(address(shibaInu), address(flashBorrower), fee);
    deal(address(flashBorrower), fee);
    uint256 balanceBefore = shibaInu.balanceOf(address(privatePool));

    uint256 balanceBeforeETH = address(privatePool).balance;

    // act
    flashBorrower.initiateFlashLoan(address(milady), 1, "");

    assertEq(
        address(privatePool).balance,
        balanceBeforeETH + fee,
        "Should have paid fee"
    );

    // assert
    assertEq(
        shibaInu.balanceOf(address(privatePool)),
        balanceBefore + fee,
        "Should have paid fee"
    );
}

// test\\shared\\FlashBorrower.sol
function initiateFlashLoan(
    address token,
    uint256 tokenId,
    bytes calldata data
) public {
    if (lender.flashFeeToken() == address(0)) {
        uint256 flashFee = lender.flashFee(token, tokenId);
        lender.flashLoan{value: flashFee}(this, token, tokenId, data);
    } else {
        uint256 flashFee = lender.flashFee(token, tokenId);
        lender.flashLoan{value: flashFee}(this, token, tokenId, data);
    }
}

Tools Used

forge

Recommended Mitigation Steps

+ if (
+    (baseToken == address(0) && msg.value != baseTokenAmount) ||
+     (msg.value > 0 && baseToken != address(0))
+ ) {
+     revert InvalidEthAmount();
+ }

See the markdown file with the details of this report here.

Agreements

If you are a C4 Certified Contributor by commenting or interacting with this repo prior to public release of the contest report, you agree that you have read the Certified Warden docs and agree to be bound by:

• C4 Certified Contributor Terms and Conditions,
• C4 Code of Professional Conduct, and
• the disclosure guidelines below.

To signal your agreement to these terms, add a 👍 emoji to this issue.

Code4rena staff reserves the right to disqualify anyone from this role and similar future opportunities who is unable to participate within the above guidelines.

Disclosures

Sponsors may elect to add team members and contractors to assist in sponsor review and triage. All sponsor representatives added to the repo should comment on this issue to identify themselves.

To ensure contest integrity, the following potential conflicts of interest should also be disclosed with a comment in this issue:

1. any sponsor staff or sponsor contractors who are also participating as wardens
2. any wardens hired to assist with sponsor review (and thus presenting sponsor viewpoint on findings)
3. any wardens who have a relationship with a judge that would typically fall in the category of potential conflict of interest (family, employer, business partner, etc)
4. any other case where someone might reasonably infer a possible conflict of interest.

See the markdown file with the details of this report here.

See the markdown file with the details of this report here.

Lines of code

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

Vulnerability details

Impact

the sumWeightsAndValidateProof() does not check whether the lengths of the arrays submitted are equal.
This can lead to unexpected results.

If the tokenIds array is a shorter length than the tokenWeights array, the additional values in the
other array will be ignored. This could lead to unexpected results,
which would be better served by reverting.

Proof of Concept

    function sumWeightsAndValidateProof(
        uint256[] memory tokenIds,
        uint256[] memory tokenWeights,
        MerkleMultiProof memory proof
    ) public view returns (uint256) {
        // if the merkle root is not set then set the weight of each nft to be 1e18
        if (merkleRoot == bytes32(0)) {
            return tokenIds.length * 1e18;
        }

        uint256 sum;
        bytes32[] memory leafs = new bytes32[](tokenIds.length);
        for (uint256 i = 0; i < tokenIds.length; i++) {
            // create the leaf for the merkle proof
            leafs[i] = keccak256(bytes.concat(keccak256(abi.encode(tokenIds[i], tokenWeights[i]))));

            // sum each token weight
            sum += tokenWeights[i];
        }

        // validate that the weights are valid against the merkle proof
        if (!MerkleProofLib.verifyMultiProof(proof.proof, merkleRoot, leafs, proof.flags)) {
            revert InvalidMerkleProof();
        }

        return sum;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check to the sumWeightsAndValidateProof() function that confirms that tokenIds, and
tokenWeights are all equal length.

Lines of code

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

Vulnerability details

Impact

Some tokens do not revert on failure, but instead return false (e.g. USDT).
https://github.com/d-xo/weird-erc20/#no-revert-on-failure

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer and tokens that don't correctly implement the latest EIP20 spec will be unusable in the protocol as they revert the transaction because of the missing return value.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend using OpenZeppelin's SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

Lines of code

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

Vulnerability details

Impact

Malicious PrivatePool owner can withdraw all baseTokens and NFTs from the pool

Proof of Concept

The PrivatePool can be created by anyone through the Factory.create function. However the PrivatePool.withdraw enables the pool owner to withdraw all base tokens and NFTs. This leaves a huge vulnerability.

Tools Used

Manual Review

Recommended Mitigation Steps

Remove the PrivatePool.withdraw and disable withdraw capability from pool owner. Or change it to that only the Factory can withdraw tokens from the pool.

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L385-L452

Vulnerability details

Impact

Users can call change to switch their set of NFTs for another set of NFTs in a private pool. The sum of the caller's NFT weights must be less than or equal to the sum of the output pool NFTs weights.

However, if the new weight sum is less than the previous one, virtualNftReserves is not rebalanced. This results in a stale price, as virtualNftReserves is used to calculate the sell and buy quotes.

Proof of Concept

In change there isn't a rebalance for virtualNftReserve as there should be when NFTs are traded in/out of the pool, like in buy and sell:

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

//sell
virtualBaseTokenReserves -= uint128(netOutputAmount + protocolFeeAmount + feeAmount);
virtualNftReserves += uint128(weightSum);

Due to this, buyQuote and sellQuote will result in a stale price when a user will buy/sell if the previous operation was change and the new weights are higher than the previous ones:

//buy quote
uint256 inputAmount = FixedPointMathLib.mulDivUp(outputAmount, virtualBaseTokenReserves, (virtualNftReserves - outputAmount));

//sell quote
uint256 outputAmount = inputAmount * virtualBaseTokenReserves / (virtualNftReserves + inputAmount);

Tools Used

Manual review

Recommended Mitigation Steps

As for buy and sell, the reserves should be rebalanced to reflect the new weights:

{
    // calculate the sum of weights for the input nfts
    uint256 inputWeightSum = sumWeightsAndValidateProof(inputTokenIds, inputTokenWeights, inputProof);

    // calculate the sum of weights for the output nfts
    uint256 outputWeightSum = sumWeightsAndValidateProof(outputTokenIds, outputTokenWeights, outputProof);

    // check that the input weights are greater than or equal to the output weights
    if (inputWeightSum < outputWeightSum) revert InsufficientInputWeight();

+   //@audit rebalance virtualNftReserves
+   virtualNftReserves = virtualNftReserves + uint128(inputWeightSum) - uint128(outputWeightSum);

    // calculate the fee amount
    (feeAmount, protocolFeeAmount) = changeFeeQuote(inputWeightSum);
}

Lines of code

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

Vulnerability details

Impact

• An attacker can monitor the creation of PrivatePools and send fake creations with the same salt, which leads to the rejection of normal transactions.
• The attacker can create their own pools with the same address on other networks (in case the factors will be at the same address)
• In case there will be funds at the potential address, NFTs for certain reasons. An attacker can track it to take those funds, nft

Proof of Concept

To create an address, only salt is used, which is passed as a parameter. This leads to the fact that anyone can occupy potential addresses of pools of other users

An attacker can monitors the mempool to find transactions calling the function create of PrivatePool and front-runs them to create the corresponding PrivatePool to make the benign transaction fail.

In the event that, for certain reasons, the User sent funds to an address that has not yet been created. An attacker can trace the creation of this address and withdraw funds

Referenced code:

• Factory.sol:L92
• Factory.sol:L81

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

• Example: Calculate salt as msg.sender + salt

See the markdown file with the details of this report here.

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L274
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L236
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L338
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L335
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L778

Vulnerability details

Impact

The pool uses the wrong sale price to determine the NFT's royalty. Thus, NFT creators don't earn the correct amount of royalty when their token is sold through Caviar's private pools.

Since that can cause a loss of funds for the royalty recipient I rate the issue as HIGH.

Proof of Concept

The price of an NFT in a private pool depends on

• the pool's virtual reserves: virtualBaseTokenReserves & virtualNftReserves
• the weights of the NFTs that are bought/sold

When multiple tokens are bought from the pool, the buy() and sell() functions aggregate the total price and then divide it by the number of tokens to get the individual sale price. That is then used to get the royalty info for each token:

        // calculate the required net input amount and fee amount
        (netInputAmount, feeAmount, protocolFeeAmount) = buyQuote(weightSum);

        // ...

        // calculate the sale price (assume it's the same for each NFT even if weights differ)
        uint256 salePrice = (netInputAmount - feeAmount - protocolFeeAmount) / tokenIds.length;
        
        // ...
            for (uint256 i = 0; i < tokenIds.length; i++) {

                (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice);

                // transfer the royalty fee to the recipient if it's greater than 0
                if (royaltyFee > 0 && recipient != address(0)) {
                    if (baseToken != address(0)) {
                        ERC20(baseToken).safeTransfer(recipient, royaltyFee);
                    } else {
                        recipient.safeTransferETH(royaltyFee);
                    }
                }
            }

But, that doesn't represent the actual sale price of each individual token. That depends on each token's weight. Given that:

• virtualBaseTokenReserves = 2e18 (2 ETH)
• virtualNftReserves = 4e18 (weights: 2x 1e18 & 1x 2e18)
  If Alice buys two NFTs (1e18 & 2e18) she will pay:
  3e18 * 2e18 / 1e18 = 6e18. With the current implementation, 3e18 would be the sale price for both tokens. Although the one with weight 2e18 is worth twice as much.
  Instead, it should be totalSalePrice * tokenWeight / totalWeight: 6e18 * 2e18 / 3e18 = 4e18 and 6e18 * 1e18 / 3e18 = 2e18.

If the NFT contract distributes royalties to different addresses depending on the token ID, e.g. first 100 are super rare and go to address X while the rest of it goes to address Y, they won't receive the correct amounts. That possibility is even stated in EIP 2981:

The implementer MAY choose to change the percentage value based on other predictable variables that do not make assumptions about the unit of exchange. For example, the percentage value may drop linearly over time. An approach like this SHOULD NOT be based on variables that are unpredictable like block.timestamp, but instead on other more predictable state changes. One more reasonable approach MAY use the number of transfers of an NFT to decide which percentage value is used to calculate the royaltyAmount. The idea being that the percentage value could decrease after each transfer of the NFT. Another example could be using a different percentage value for each unique _tokenId.

Tools Used

none

Recommended Mitigation Steps

Determine the sale price according to their weight as described above.

See the markdown file with the details of this report here.

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L385-L438
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L701

Vulnerability details

Impact

User may need to wait for some time to buy NFT

Proof of Concept

The protocol allows user to change a set of NFTs that the caller owns for another set of NFTs in the pool. Inside the change() function, it requires inputWeightSum is greater than outputWeightSum. Then ,transfer the input NFT from the caller and transfer the output NFT to the caller. However, in the following situation, users are unable to buy NFT.

1.Assume the virtualBaseTokenReserves is 100e18， the virtualNftReserves is 5e18. And there are 5 NFTs in the protocol，The weight of each nft is 1e18,1e18,1e18,1e18,1e18.

2.User calls the change() function to change a set of NFT with 2e18 weight. Now the weight of each nft is 1e18,1e18,2e18,1e18,1e18

3.After a period of time，all NFTs with weight 1e18 are sold out，now the virtualNftReserves become 1e18. Bob calls the buy() function to buy an NFT with a weight of 2e18. The transaction will fail at line uint256 inputAmount = FixedPointMathLib.mulDivUp(outputAmount, virtualBaseTokenReserves, (virtualNftReserves - outputAmount)). He needs to wait for virtualBaseTokenReserves to be greater than 2e18 before purchasing.

function change(
        uint256[] memory inputTokenIds,
        uint256[] memory inputTokenWeights,
        MerkleMultiProof memory inputProof,
        IStolenNftOracle.Message[] memory stolenNftProofs,
        uint256[] memory outputTokenIds,
        uint256[] memory outputTokenWeights,
        MerkleMultiProof memory outputProof
    ) public payable returns (uint256 feeAmount, uint256 protocolFeeAmount) {
        // ~~~ Checks ~~~ //

        // check that the caller sent 0 ETH if base token is not ETH
        if (baseToken != address(0) && msg.value > 0) revert InvalidEthAmount();

        // check that NFTs are not stolen
        if (useStolenNftOracle) {
            IStolenNftOracle(stolenNftOracle).validateTokensAreNotStolen(nft, inputTokenIds, stolenNftProofs);
        }

        // fix stack too deep
        {
            // calculate the sum of weights for the input nfts
            uint256 inputWeightSum = sumWeightsAndValidateProof(inputTokenIds, inputTokenWeights, inputProof);

            // calculate the sum of weights for the output nfts
            uint256 outputWeightSum = sumWeightsAndValidateProof(outputTokenIds, outputTokenWeights, outputProof);

            // check that the input weights are greater than or equal to the output weights
            if (inputWeightSum < outputWeightSum) revert InsufficientInputWeight();

            // calculate the fee amount
            (feeAmount, protocolFeeAmount) = changeFeeQuote(inputWeightSum);
        }

        // ~~~ Interactions ~~~ //

        if (baseToken != address(0)) {
            // transfer the fee amount of base tokens from the caller
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), feeAmount);

            // if the protocol fee is non-zero then transfer the protocol fee to the factory
            if (protocolFeeAmount > 0) ERC20(baseToken).safeTransferFrom(msg.sender, factory, protocolFeeAmount);
        } else {
            // check that the caller sent enough ETH to cover the fee amount and protocol fee
            if (msg.value < feeAmount + protocolFeeAmount) revert InvalidEthAmount();

            // if the protocol fee is non-zero then transfer the protocol fee to the factory
            if (protocolFeeAmount > 0) factory.safeTransferETH(protocolFeeAmount);

            // refund any excess ETH to the caller
            if (msg.value > feeAmount + protocolFeeAmount) {
                msg.sender.safeTransferETH(msg.value - feeAmount - protocolFeeAmount);
            }
        }

        // transfer the input nfts from the caller
        for (uint256 i = 0; i < inputTokenIds.length; i++) {
            ERC721(nft).safeTransferFrom(msg.sender, address(this), inputTokenIds[i]);
        }

        // transfer the output nfts to the caller
        for (uint256 i = 0; i < outputTokenIds.length; i++) {
            ERC721(nft).safeTransferFrom(address(this), msg.sender, outputTokenIds[i]);
        }

        // emit the change event
        emit Change(inputTokenIds, inputTokenWeights, outputTokenIds, outputTokenWeights, feeAmount, protocolFeeAmount);
    }

function buyQuote(uint256 outputAmount)
        public
        view
        returns (uint256 netInputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
    {
        // calculate the input amount based on xy=k invariant and round up by 1 wei
        uint256 inputAmount =
            FixedPointMathLib.mulDivUp(outputAmount, virtualBaseTokenReserves, (virtualNftReserves - outputAmount));

        protocolFeeAmount = inputAmount * Factory(factory).protocolFeeRate() / 10_000;
        feeAmount = inputAmount * feeRate / 10_000;
        netInputAmount = inputAmount + feeAmount + protocolFeeAmount;
    }

  

Tools Used

vscode

Recommended Mitigation Steps

See the markdown file with the details of this report here.

See the markdown file with the details of this report here.

Lines of code

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

Vulnerability details

Impact

The ethRouter contract inherits ERC721TokenReceiver and can accept NFTs sent by users. However, if a user accidentally sends an NFT to the contract or airdrops an NFT to the contract, the administrator cannot rescue the NFT from the contract.

contract EthRouter is ERC721TokenReceiver {
    ......
    ......
}

Proof of Concept

Send nft directly to the contract

Tools Used

vscode

Recommended Mitigation Steps

Add a function similar to PrivatePool::withdraw to rescue NFTs sent to the contract.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L623-L654

Vulnerability details

Impact

After user borrows NFT through a flash loan, the remaining ETH is not refunded to user and user will lose ETH

Proof of Concept

The protocol allows users to borrow NFTs through a flash loan, and users only need to pay for the flash loan fees. However, when the base token is ETH, if msg.value is greater than the flash loan fee, the protocol does not return the remaining fee to the user.

function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 tokenId, bytes calldata data)
        external
        payable
        returns (bool)
    {
        // check that the NFT is available for a flash loan
        if (!availableForFlashLoan(token, tokenId)) revert NotAvailableForFlashLoan();

        // calculate the fee
        uint256 fee = flashFee(token, tokenId);

        // if base token is ETH then check that caller sent enough for the fee
        if (baseToken == address(0) && msg.value < fee) revert InvalidEthAmount();

        // transfer the NFT to the borrower
        ERC721(token).safeTransferFrom(address(this), address(receiver), tokenId);

        // call the borrower
        bool success =
            receiver.onFlashLoan(msg.sender, token, tokenId, fee, data) == keccak256("ERC3156FlashBorrower.onFlashLoan");

        // check that flashloan was successful
        if (!success) revert FlashLoanFailed();

        // transfer the NFT from the borrower
        ERC721(token).safeTransferFrom(address(receiver), address(this), tokenId);

        // transfer the fee from the borrower
        if (baseToken != address(0)) ERC20(baseToken).transferFrom(msg.sender, address(this), fee);

        return success;
    }

Tools Used

vscode

Recommended Mitigation Steps

Refund the remaining ETH to user

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L484-L532
https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L219-L248

Vulnerability details

Impact

The owner of a private pool has extensive privileges, most importantly can withdraw all ETH/tokens/NFTs from the pool.
While any user can call the deposit function to provide liquidity and transfer ETH/tokens/NFTs to the pool, there is no way for the depositor to withdraw the deposited assets. Instead, the pool owner is able to withdraw them. This will result in a loss of assets for any user who transfers assets to the pool via the deposit function in PrivatePool.sol or EthRouter.sol.

As the owner of the private pool is defined as the owner of the corresponding NFT (which could be transferred / sold) and as anyone can open a private pool, the private pool owner can not be seen as a trusted entity.

Side comment (not tested)

virtualBaseTokenReserves and virtualNftReserves are updated in the buy / sell functions, but not in the deposit function. There is no check in the deposit function to ensure the ratio of the deposited assets matches the reserve ratios. The pool owner can manually (re)set both values. This could have an impact on the price and potentially cause the pool to become illiquid.

Proof of Concept

Screenshot of Foundry Test
https://gateway.pinata.cloud/ipfs/QmS975d1iGk77KjZuhEccd9iivGWctX2SBGUBxGuiu6aX9

Tools Used

Foundry

Recommended Mitigation Steps

Review pool owner's privileges AND

(1) mark deposit function in PrivatePool.sol as onlyOwner, so only the private pool owner can provide liquidity. Remove deposit function in EthRouter.sol OR

(2) keep a record of which user has deposited ETH / tokens into the pool, provide a way to withdraw deposited assets and restrict owner's withdrawal rights over pool assets

If (2): logic of the deposit function in EthRouter.sol should not depend on the price() of the privatePool, as this value can be directly manipulated by the owner of the private pool
(--> owner can manually set virtualBaseTokenReserves and virtualNftReserves)

See the markdown file with the details of this report here.

Lines of code

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

Vulnerability details

Impact

Inside the PrivatePool.sol there is a function sell(), now the thing is if the user is selling NFT to the pool and eligible for royalties, he will not get it.

Proof of Concept

Let's see the issue step by step:

1. The netOutputAmount in sell() function is obtained from sellQuote() which Returns the netOutputAmount for selling NFT
   https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L713-L724
2. In sell() function the payment of royalties and the netOutputAmount works independently to each other, now after sending royalties, we don't have to decrease it from the netOutputAmount at line 355 in PrivatePool.sol (netOutputAmount -= royaltyFeeAmount;) because it will directly affect the user and will end up having no royalties.
   https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L333-L355

Recommended Mitigation Steps

Just by removing this line https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L355
the output amount will be corrected.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L115
https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L152
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L365
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L527
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L651

Vulnerability details

Impact

Use safeTransfer()/safeTransferFrom() instead of transfer()/transferFrom() to gracefully handle non-compliant ERC20 tokens. The code mostly does this apart from a few places.

Proof of Concept

Factory L115 L152

PrivatePool L365 L527 L651

Tools Used

Ctrl+F

Recommended Mitigation Steps

Use safeTransfer()/safeTransferFrom() instead of transfer()/transferFrom() when transferring ERC20 tokens

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L538-L545
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L701
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L719

Vulnerability details

A private pool owner can change the virtual reserves anytime after initialization by setting arbitrary amounts.

Those reserves are meant to be used to track the LP for tokens and NFTs, respectively:

function setVirtualReserves(uint128 newVirtualBaseTokenReserves, uint128 newVirtualNftReserves) public onlyOwner {
    // set the virtual base token reserves and virtual nft reserves
    virtualBaseTokenReserves = newVirtualBaseTokenReserves;
    virtualNftReserves = newVirtualNftReserves;

    // emit the set virtual reserves event
    emit SetVirtualReserves(newVirtualBaseTokenReserves, newVirtualNftReserves);
}

This is dangerous, as these values are used to define the price that will be used by users when buying and selling.

Impact

A malicious private pool owner can use setVirtualReserves to steal funds from users with the following methods:

1. Frontrun users before they buy an NFT by setting a high virtualBaseTokenReserves, to increase the total payout to the pool
2. Frontrun users before they sell their NFT to the pool by setting a high virtualNftReserves, to decrease their total payout from the pool

Proof of Concept

virtualBaseTokenReserves and virtualNftReserves are used to calculate the buy and sell quotes:

function buyQuote(uint256 outputAmount)
    public
    view
    returns (uint256 netInputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
{
    // calculate the input amount based on xy=k invariant and round up by 1 wei
    uint256 inputAmount =
        FixedPointMathLib.mulDivUp(outputAmount, virtualBaseTokenReserves, (virtualNftReserves - outputAmount));

    protocolFeeAmount = inputAmount * Factory(factory).protocolFeeRate() / 10_000;
    feeAmount = inputAmount * feeRate / 10_000;
    netInputAmount = inputAmount + feeAmount + protocolFeeAmount;
}

function sellQuote(uint256 inputAmount)
    public
    view
    returns (uint256 netOutputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
{
    // calculate the output amount based on xy=k invariant
    uint256 outputAmount = inputAmount * virtualBaseTokenReserves / (virtualNftReserves + inputAmount);

    protocolFeeAmount = outputAmount * Factory(factory).protocolFeeRate() / 10_000;
    feeAmount = outputAmount * feeRate / 10_000;
    netOutputAmount = outputAmount - feeAmount - protocolFeeAmount;
}

A owner can use one of the following scenarios to steal funds from the users:

Scenario 1:

1. Alice wants to buy an NFT and she calls the buy function
2. Pool owner front runs Alice and sets virtualBaseTokenReserves to a high amount relative to virtualNftReserves
3. Supposing that the baseToken is not address(0) and Alice set a high allowance, the owner can steal a big amount of funds as there is no slippage control

Scenario 2:

1. Alice wants to sell an NFT and she calls the sell function
2. Pool owner front runs Alice and sets virtualNftReserves to a high amount relative to virtualBaseTokenReserves
3. Supposing that the baseToken is not address(0) and Alice set a high allowance, the owner can let Alice sell her NFT at a fraction of the real price

Even if Alice uses a wrapper contract that will check the max input amount and revert if the slippage is too high, the owner can still act as MEV and manipulate the reserves to always get the maximum amount of funds possible.

Tools Used

Manual review

Recommended Mitigation Steps

Private pool owners shouldn't be able to manually change the reserves besides the first initialization.

See the markdown file with the details of this report here.

See the markdown file with the details of this report here.

See the markdown file with the details of this report here.

Lines of code

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

Vulnerability details

Impact

The create the () function in the Caviar Private Pool Factory contract is vulnerable to out-of-bounds errors and DoS attacks because it lacks a length check for the tokenIds array. If the tokenIds array length exceeds the expected limit the contract could encounter out-of-bounds errors and be vulnerable to DoS attacks, attackers can exploit this vulnerability to prevent legitimate users from creating private pools, resulting in a loss of funds and other damages.

Proof of Concept

code-423n4/2023-04-caviar#2 (comment)

Tools Used

VSCODE

Recommended Mitigation Steps

To mitigate this vulnerability, a require statement should be added to the create() function to check the length of the tokenIds array. The require statement should ensure length of the array does not exceed a certain limit. This limit should be set based on the expected number of NFTs that can be deposited to the pool.

code-423n4/2023-04-caviar#3 (comment)

Lines of code

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

Vulnerability details

Impact

Any NFT or ERC20 token approved for the pool can be stolen by the owner of the pool. The pool owner is just a user. They are not part of the Caviar team and thus not trusted.

Proof of Concept

Every PrivatePool has a execute() function allowing the pool owner to execute arbitrary calls through the pool:

    function execute(address target, bytes memory data) public payable onlyOwner returns (bytes memory) {
        // call the target with the value and data
        (bool success, bytes memory returnData) = target.call{value: msg.value}(data);

        // if the call succeeded return the return data
        if (success) return returnData;

        // if we got an error bubble up the error message
        if (returnData.length > 0) {
            // solhint-disable-next-line no-inline-assembly
            assembly {
                let returnData_size := mload(returnData)
                revert(add(32, returnData), returnData_size)
            }
        } else {
            revert();
        }
    }

To buy & sell NFTs users have to approve the pool to access their NFTs and tokens. Most of the time, dApp developers approve the maximum amount of ERC20 tokens or all NFT tokens for any given address to save gas. Thus, we can assume that multiple users will have the pool approved to spend funds/NFTs at any given time. Through the execute() function the owner is able to steal all of it.

They simply call the baseToken or nft contract's transferFrom() function to send the approved tokens to their own address.

Tools Used

none

Recommended Mitigation Steps

Prohibit the pool from calling the underlying NFT and base token contract:

    function execute(address target, bytes memory data) public payable onlyOwner returns (bytes memory) {
      require(target != nft && target != baseToken);
      // ...
    }

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L101
https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L154
https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L228
https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L256

Vulnerability details

Impact

the DeadlinePassed() check revert if block.timestamp is greater than deadline in some functions ,
since the deadline can be passed by the user it can always by bypassed.

Proof of Concept

    function sell(Sell[] calldata sells, uint256 minOutputAmount, uint256 deadline, bool payRoyalties) public {
        // check that the deadline has not passed (if any)
        if (block.timestamp > deadline && deadline != 0) {
            revert DeadlinePassed();
        }

we can see that the deadline is checked with block.timestamp since the deadline is passed by the user the user can set deadline > block.timestamp and bypass this if !

Tools Used

Manual Review

Recommended Mitigation Steps