Caviar Private Pools - tsvetanovv's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L230-L231 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L323-L324

Vulnerability details

Impact

In PrivatePool.sol we have functions buy() and sell(). The buy() function is responsible for allowing users to buy a basket of NFTs represented by a list of token IDs with different weights. The sell() function sells NFTs into the pool and transfers base tokens to the caller.

In these functions, we have unsafe cast from uint128 to uint256.

Proof of Concept

In buy() we have:

231: virtualNftReserves -= uint128(weightSum);

weightSum is calculated as:

219: uint256 weightSum = sumWeightsAndValidateProof(
            tokenIds,
            tokenWeights,
            proof
        );

Which returns uint256 weightSum. We have the potential risk of overflow if the sum of the NFT weights is larger than what can be represented by a uint128 integer. If the value of weightSum is greater than 2^128-1, the subtraction operation could result in an integer underflow.

We have a similar situation in sell() function.

323: virtualBaseTokenReserves -= uint128(
            netOutputAmount + protocolFeeAmount + feeAmount
        );
324:        virtualNftReserves += uint128(weightSum); 

virtualNftReserves is a uint128 variable that tracks the virtual reserves of the NFTs being sold: https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L112

112: uint128 public virtualNftReserves;

weightSum is a uint256 variable that represents the total weight of the NFTs being sold.

To update the virtualNftReserves variable, the weightSum variable is cast to uint128 using the unsafe cast operator. This may result in data loss if the weightSum variable is larger than the maximum value that can be represented by uint128.

The same applies to virtualBaseTokenReserves.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using OpenZeppelin's SafeCast library to prevent unexpected overflow/underflow when casting or change uint128 to uint256 where necessary.

// update the virtual reserves
virtualBaseTokenReserves += SafeCast.toUint128(netInputAmount - feeAmount - protocolFeeAmount);
virtualNftReserves -= SafeCast.toUint128(weightSum);

// -- snip --//

// update the virtual reserves
virtualBaseTokenReserves -= SafeCast.toUint128(netOutputAmount + protocolFeeAmount + feeAmount);
virtualNftReserves += SafeCast.toUint128(weightSum);