Caviar contest - cozzetti's results

Lines of code

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

Vulnerability details

Impact

When the baseToken of a Pair contract has pre-transfer hooks on the sender, such as any ERC777 token, then it's possible to execute a reentrancy attack to repeateadly manipulate quotes and obtain better rates. I'll explain using the buy function as an example.

When the attacker calls the buy function to buy fractional tokens and pay base tokens in return, this would be the order of operations:

1. The buy quote is calculated with the current reserves amounts.
2. The fractional tokens are transferred to the attacker (lowering reserves of fractional tokens)
3. The transferFrom function of the ERC777 base token is called, which callbacks the attacker's tokensToSend hook, before updating balances. This means base token reserves remain unchanged.
4. The attacker calls buy again. This time, because the base token reserves haven't yet been increased (remain equal to step 1), the new calculated buy quote will be lower than expected, thus allowing the attacker to pay less base tokens.
5. The attacker can continue executing nested reentrant calls, each time getting a better rate compared to the rate that would get in a normal non-reentrant trade.
6. Once all reentrant calls are done, the execution will unwrap and perform the actual transfers of base tokens from the attacker to the contract, increasing the base token reserves of the pair.

A similar attack can be carried out in other functions that would interact with the ERC777 base token and therefore call hooks, such as sell.

Also, this attack vector is not uncommon in simple AMMs. It was described for Uniswap v1 here.

Tools Used

Manual review

Recommended Mitigation Steps

Implement reentrancy guards in all functions that can make unsafe external calls to attacker-controlled accounts.

Lines of code

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

Vulnerability details

Impact

The AMM implemented in the Pair contract is subject to a liquidity deflation attack upon deployment. In this attack:

1. When the pair is deployed, the attacker is the first liquidity provider. The attacker calls the add function, providing a small amount of base and fractional tokens. The attacker receives the corresponding amount of LP tokens.
2. The attacker transfers, directly to the Pair contract, a large amount of base and fractional tokens.
3. This deflates the LP tokens obtained in (1), making shares in the pair extremely valuable.
4. Now any following liquidity provider must provide a very large amount of base and fractional tokens to obtain a small amount of LP tokens.

This gives the first liquidity provider the possibility to make it very costly for all other LPs to enter the pool, and potentially control the vast majority of the liquidity in a pair, leaving out smaller players.

Tools Used

Manual review

Recommended Mitigation Steps

This is a long known issue in this kind of AMMs, and there's already a solution available, pioneered by Uniswap V2.

It's based on implementing a minimum liquidity threshold in the pair. Essentially, during the first liquidity provision, part of the minted LP tokens are burned (instead of being transferred to the provider). I suggest reading the discussion in the Uniswap v2 audit for more details.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L63 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L107 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L147 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L182 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L275 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L294 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L310 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L323

Vulnerability details

Impact

Neither liquidity provision nor trade orders executed in the Pair contract are time-bounded.

As a consequence, users can have their operations executed at unexpected times, when the market conditions are unfavorable. To avoid users Caviar be at the mercy of block builders, it's best to add an expiration timestamp to their orders, so that operations that are executed too late are automatically reverted.

The idea of expiring orders is not new in AMMs, and Uniswap and others already implement this feature to protect users.

Tools Used

Manual review

Recommended Mitigation Steps

I suggest either modifying the relevant functions of the Pair contract to allow users specify an expiration time for their orders. Alternatively, you could create a wrapper over the Pair contract that implements this safety measure. Similar to the periphery-core architecture implemented by Uniswap.

Lines of code

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

Vulnerability details

Impact

According to its docstrings, the price function of the Pair contract intends to return "The current price of one fractional token in base tokens with 18 decimals of precision.". The calculation is performed as follows:

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

This calculation has a fundamental flawed assumption: that all base tokens have 18 decimals like the fractional tokens. However, there's nothing in the contract enforcing base tokens to always have 18 decimals. As a consequence, when the base token has a different number of decimals than the fractional token, the price reported by the Pair contract will be miscalculated.

For example, let's say USDC is used as a base token. USDC has 6 decimals. If there's 100 USDC and 100 fractional tokens in reserve, one would expect the price reported by the price function to be 1e18. Because of the issue described, following the formula above:

100e6 * 1e18 / 100e18

the resulting price reported by price will actually be 1e6.

This can cause severe problems in any off-chain services or other contracts integrating with the Caviar protocol, as they will be receiving inaccurate price information.

Tools Used

Manual review

Recommended Mitigation Steps

Price calculation should be done in the same units for both tokens. The units may need to be scaled accordingly to match, and only then calculate the price. I also recommend including unit tests to have stronger assurances that the changes are correct and this issue is not re-introduced in the future.

Unused return value of _transferFrom function

The internal _transferFrom function of the Pair contract returns true on success. However, this value is never used when _transferFrom is called in lines 85, 125, 162 and 194.

These unused values may cause confusion in readers and contributors alike, so I recommend removing it to improve the code's quality. Alternatively, if the _transferFrom function is to be left unmodified, the code could include inline comments in the calls to _transferFrom to explicitly state why the return value is not used.

Erroneous casting

The _validateTokenIds function of the Pair contract returns early when the merkleRoot state variable is zero. The if clause that implements this logic contains an erroneous casting operation when comparing merkleRoot. In particular, the comparison is made against a bytes23 instead of bytes32. While this does not cause a problem, to favor correctness it's best to change the casting operation to a bytes32 type.

Potential out-of-bounds access in token ID validation

In the _validateTokenIds function of the Pair contract, there's a for loop that accesses the proofs and tokenIds parameters with the same index. The function implicitly assumes the arrays have the same length, yet it doesn't explicitly verify it. If the proofs array is shorter than the tokenIds array, execution will unexpectedly revert due to an out-of-bounds access.

It's best to always make function requirements explicit, so as to fail early and loudly. Include a require statement to ensure the two arrays have the same length.

Closed pair still accepts liquidity

In an emergency exit, the owner will trigger the close function on the Pair contract. In this scenario, it is expected for all users to withdraw their assets from the pair and unwrap their NFTs. However, there's nothing preventing users to (erroneously) continue providing liquidity via the add function.

Given this scenario is not tested, it is unclear whether this is an expected behavior or not. If it is expected, document and test it. If it's not, add a require statement in the add function to ensure users cannot supply more assets to a pair that is undergoing an emergency exit.