Caviar Private Pools - aviggiano's results

Lines of code

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

Vulnerability details

In PrivatePool.buy, the function PrivatePool._getRoyalty is called twice, which calls IERC2981(lookupAddress).royaltyInfo(tokenId, salePrice) under the hood. EIP-2981 states that

Implementers of this standard MUST calculate a percentage of the _salePrice when calculating the royalty amount. Subsequent invocations of royaltyInfo() MAY return a different royaltyAmount.

It is possible that the royalty lookup address implements a mechanism where different invocations return different royalty amounts. As a result, the first invocation of PrivatePool._getRoyalty may return a different number from the second invocation for the same NFT.

For example, the first invocation can return a value higher than the second value, which would make the PrivatePool.buy send more funds to the royalty recipient than necessary. As a result, users funds may be lost in the process.

Impact

Users loss of funds depending on the specific implementation of the royalty lookup address.

Proof of Concept

1. User calls PrivatePool.buy with a specific NFT address
2. The NFT has royalties, but the royalty calculation is dependent on external factors, and may yield different results for each invocation
3. If the first invocation returns a higher value than the second one, the excess funds will be stuck on the PrivatePool contract

Tools Used

Manual review

Recommended Mitigation Steps

Use an array to cache the royalty fee and recipient on the first invocation and use the cached value to pay royalties.

Lines of code

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

Vulnerability details

PrivatePool.flashLoan is designed to execute a flash loan for the given NFT. The function calculates the fee associated with the flash loan using the flashFee function, that uses changeFee under the hood. However, depending on the difference between changeFee and feeRate, there is a possibility that an attacker can drain token funds from the pool by executing a flash loan, selling the NFT, and then buying it back at a lower cost.

Impact

This vulnerability can lead to a loss of funds for the pool owner/liquidity provider, as an attacker can exploit the fee calculation issue to drain token funds from the pool.

Proof of Concept

1. The attacker takes a flash loan of an NFT.
2. The attacker sells the NFT for a lower price than the calculated fee.
3. The attacker buys back the NFT at the lower price.
4. The attacker returns the NFT to the pool and pays the fee.
5. The attacker repeats this process to drain token funds from the pool.

Tools Used

Manual review

Recommended Mitigation Steps

Change fees should be derived from buy/sell fees. These should not be two distinct parameters.

Uniswap, for example, derives the following formula for a "single-token swap":

Single-Token In the case where the token withdrawn is the same as the token returned (i.e. DAI was requested in the flash swap, used, then returned, or vice versa with WETH), the following condition must be satisfied: DAIReservePre - DAIWithdrawn + (DAIReturned * .997) >= DAIReservePre It may be more intuitive to rewrite this formula in terms of a "fee" levied on the withdrawn amount (despite the fact that Uniswap always levies fees on input amounts, in this case the returned amount, here we can simplify to an effective fee on the withdrawn amount). If we rearrange, the formula looks like: (DAIReturned * .997) - DAIWithdrawn >= 0 DAIReturned >= DAIWithdrawn / .997 So, the effective fee on the withdrawn amount is .003 / .997 ≈ 0.3009027%.

This can be generalized as

changeFee = feeRate / (1 - feeRate)

The recommendation is, in summary, to use a single feeRate parameter, and to derive the changeFee using the formula above.

Lines of code

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

Vulnerability details

PrivatePool.changeFeeQuote calculates the fee required to change a given amount of NFTs based on the current changeFee. The function accounts for the base token decimals by calculating an exponent depending on the base token's decimals. However, the current implementation does not support ERC-20 tokens with less than 4 decimals, as the exponent calculation may result in a negative number, leading to a revert.

Impact

The protocol does not support ERC-20 base tokens with less than 4 decimals

Proof of Concept

For example, Gemini dollar (GUSD) has only 2 decimals. Using it on a PrivatePool would make all PrivatePool.change calls revert due to an underflow.

Tools Used

Manual review

Recommended Mitigation Steps

Change the implementation of changeFee to use a different number of decimals of accuracy. Alternatively, assume fees to be zero in case of ERC-20 tokens with fewer than 4 decimals

    function changeFeeQuote(uint256 inputAmount) public view returns (uint256 feeAmount, uint256 protocolFeeAmount) {
        // multiply the changeFee to get the fee per NFT (4 decimals of accuracy)
        uint256 decimals = ERC20(baseToken).decimals();
        if (decimals >= 4) {
            uint256 exponent = baseToken == address(0) ? 18 - 4 : decimals - 4;
            uint256 feePerNft = changeFee * 10 ** exponent;

            feeAmount = inputAmount * feePerNft / 1e18;
            protocolFeeAmount = feeAmount * Factory(factory).protocolFeeRate() / 10_000;
        }
    }

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L235-L236 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L244 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L788 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L334-L335

Vulnerability details

In PrivatePool.{buy,sell}, there is an assumption that the sale price is the same for each NFT even if weights differ. This impacts the royalty calculation, as the same salePrice is used for different tokenIds on the _getRoyalty calculation. Nevertheless, EIP-2981 states that:

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.

As the EIP allows, it is possible that the royalty lookup address defines different percentage values for each unique _tokenId. As a result, assuming the same price value would yield different royalty amounts instead of using the weighted price.

Impact

User loss of funds depending on the implementation of the royalty lookup price.

Proof of Concept

Imagine the following scenario, where a pool contains three NFTs, each one with a different weight, and a different royalty percentage defined by the lookup address:

If we assume the weighted price of all NFTs to be 6 ether, the average salePrice as defined by the current PrivatePool implementation would be 2 ether per NFT. This would yield the following total royalty amount:

Total royalty amount (incorrect): 0.60 ether.

However, by assuming the salePrice to be the weighted price of each NFT, it will yield the following total royalty amount:

Total royalty amount (weighted): 0.70 ether.

Tools Used

Manual review

Recommended Mitigation Steps

Calculate the salePrice to be the weighted price for each tokenId and use that value for the _getRoyalty calculation.