Caviar contest - 9svR6w'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#L95

Vulnerability details

Impact

Some ERC20 tokens implement the EIP 777 interface including the tokensToSend() hook, which performs a callback to the user from which tokens will be transferred before the tokens are transferred. A contract can be constructed to receive such a callback in (for example) Pair's buy() line 172, safeTransferFrom(). At the time of this callback the fractional tokens have been transferred out of the Pair but the base tokens have not been transferred into the Pair. At this moment, the xyk exchange rate between these assets will be incorrect. Because Pair does not employ reentrancy protection, the malicious contract can use the tokensToSend() callback to perform another transaction against the Pair to take advantage of this temporarily incorrect exchange rate. (It may in addition be possible to use Pair's add() to trigger tokensToSend() via safeTransferFrom() in a similar manner.)

The contest description states "It's assumed that all NFTs and base token contracts used to create new pairs are honest." I am noting this issue because an ERC777 base token may itself be honest, and users may wish to trade this token but be unaware of the possible exploit available to other users of the honest ERC777 contract due to lack of reentrancy protection in Pair.

Note that the same basic issue was identified in uniswap https://github.com/ConsenSys/Uniswap-audit-report-2018-12#31-liquidity-pool-can-be-stolen-in-some-tokens-eg-erc-777-29 https://github.com/OpenZeppelin/exploit-uniswap and reentrancy protection is now available as an option https://ethereum.stackexchange.com/a/113700

Proof of Concept

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

Tools Used

Recommended Mitigation Steps

Add reentrancy protection to Pair trading functions add(), remove(), buy(), sell().

Lines of code

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

Vulnerability details

Impact

A user of Pair calls add() supplying a baseTokenAmount and a fractionalTokenAmount they intend to use to purchase LP tokens. They also supply minLpTokenAmount, the minimum number of LP tokens they expect to receive. The add() function in turn calls addQuote(), which checks the exchange rates for both base tokens and fractional tokens and determines the number of LP tokens to issue, based on the exact exchange rate for either base tokens or fractional tokens. The selection of whether the exchange rate for base tokens or fractional tokens is to be used is based on which results in the smallest number of LP tokens being issued for the asset amounts provided by the caller of add(). This means that for the asset (base or fractional token) with an exchange rate that is not used, there may be an excess bid provided by the caller in that token. This excess bid is not refunded to the caller.

This means that if exchange rates change between when the caller of add() requests their transaction and when the transaction is committed, they will be charged based on the full amount of assets they provide, not on the exchange rate value (in the higher valued asset) at the time. Furthermore, in anticipation of volatile exchange rates, the caller may want to include some slack in providing baseTokenAmount, fractionalTokenAmount, and minLpTokenAmount. But because they will not receive a refund for any excess amount provided they are discouraged from providing too much slack. And in fact they may even be unaware of the no-refund policy in the first place and simply lose an arbitrary excess bid provided. Finally, while the excess assets (non refunded excess bids) will tend to accrue to the Pair pool when transactions are ordered arbitrarily, it may be possible for a miner to extract MEV by ordering (delaying) a call to add() until after a separate transaction that changes the base token or fractional token exchange rates with respect to LP tokens, and adding additional transactions to capture some of the excess bid assets that would normally be absorbed by the Pair pool.

Proof of Concept

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

Tools Used

Recommended Mitigation Steps

Return excess bid of baseTokenAmount or fractionalTokenAmount to the caller of add().