Caviar contest - obront's results

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L417-L428

Vulnerability details

Impact

When a user adds liquidity to the pool, separate calculations are done based on the number of baseTokens and fractionalTokens sent. Each of these calculates the number of LP tokens that it expects should be generated based on the number of inputs. Then, the minimum of these two values are taken and this number of LP tokens is minted to the user.

However, the full value of both baseTokens and fractionalTokens are taken from the user. This can create a dramatic mismatch between the amount spent by the user and the amount of LP tokens earned.

It is true that this amount can be counteracted by setting a strict minLpTokenAmount in the function call. However, for most users, they will not be so careful with slippage and can lose unnecessary tokens through this inaccurate math.

Proof of Concept

• A user submits a much larger value of baseTokens than fractionalTokens
• The math is performed which concludes that their baseTokens submitted should mint X LP tokens, but their fractionalTokens submitted should mint Y LP tokens, where Y < X
• In the end, Y LP tokens are minted
• However, the full value of baseTokens and fractionalTokens are taken from the user
• As a result, the user overpays for their LP Tokens

Tools Used

Manual Review

Recommended Mitigation Steps

After calculating the LP tokens to be minted, return the minimum number of baseTokens and fractionalTokens that were required to mint this number of LP tokens, and only take this number of tokens from the user.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L387-L392

Vulnerability details

Impact

The price() function is expected to return the price of one fractional tokens, represented in base tokens, to 18 decimals of precision. This is laid out clearly in the comments:

/// @notice The current price of one fractional token in base tokens with 18 decimals of precision. /// @dev Calculated by dividing the base token reserves by the fractional token reserves. /// @return price The price of one fractional token in base tokens * 1e18.

However, the formula incorrectly calculates the price to be represented in whatever number of decimals the base token is in. Since there are many common base tokens (such as USDC) that will have fewer than 18 decimals, this will create a large mismatch between expected prices and the prices that result from the function.

Proof of Concept

Prices are calculated with the following formula, where ONE = 1e18:

return (_baseTokenReserves() * ONE) / fractionalTokenReserves();

We know that fractionalTokenReserves will always be represented in 18 decimals. This means that the ONE and the fractionalTokenReserves will cancel each other out, and we are left with the baseTokenReserves number of decimals for the final price.

As an example:

• We have $1000 USDC in reserves, which at 6 decimals is 1e9
• We have 1000 fractional tokens in reserve, which at 18 decimals is 1e21
• The price calculation is 1e9 * 1e18 / 1e21 = 1e6
• While the value should be 1 token, the 1e6 will be interpreted as just 1/1e12 tokens if we expect the price to be in 1e18

Tools Used

Manual Review

Recommended Mitigation Steps

The formula should use the decimals value of the baseToken to ensure that the decimals of the resulting price ends up with 18 decimals as expected:

return (_baseTokenReserves() * 10 ** (36 - ERC20(baseToken).decimals()) / fractionalTokenReserves();

This will multiple baseTokenReserves by 1e18, and then additionally by the gap between 1e18 and its own decimals count, which will result in the correct decimals value for the outputted price.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Caviar.sol#L28-L43

Vulnerability details

Impact

When a new Pair is created, it's checked that an identical pair doesn't already exist:

require(pairs[nft][baseToken][merkleRoot] == address(0), "Pair already exists");

Afterwards, we create the pair symbol, create the pair, add it to the mapping and emit an event.

When we create the pair symbol, we call the following functions to gather NFT information:

string memory baseTokenSymbol = baseToken == address(0) ? "ETH" : baseToken.tokenSymbol();
string memory nftSymbol = nft.tokenSymbol();
string memory nftName = nft.tokenName();

With a custom malicious ERC721, these external calls create the opportunity for reentrancy.

Although there is no way to cause any harm to the protocol logic using this, it will cause multiple identical Create events to be emitted from the contract, which should not be possible.

Proof of Concept

We implement an ERC721 where one of the functions calls back to Caviar.sol to reenter and create a new pair.

...
function tokenSymbol() external returns (string memory) {
    if (count == TIMES_TO_CALL) return;
    Caviar(msg.sender).create(address(this), BASE_TOKEN, MERKLE_ROOT);
    count++;
    return "NFT";
}

This will result in the create() function being called with the same parameters TIMES_TO_CALL times. Each time the pair address will overwrite the previous time, but the event will be emitted each time, causing confusion and possibly disturbing the front end.

Tools Used

Manual Review

Recommended Mitigation Steps

Move the requirement check down immediately above where the pair is saved in the mapping, so the flow is:

• get the token data
• create the pair
• check to ensure the mapping doesn't already contain the pair
• add the pair to the mapping
• emit the event

Although this doesn't conform to the typical rule of "checks - effects - interactions", you aren't able to use that rule because you need the pair address in order to save the information in storage. This accomplishes the same thing in this specific situation, ensuring that the check isn't bypassed when it shouldn't be.