Quote manipulation via reentrant trading

[code423n4]

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.

[c4-judge]

berndartmueller marked the issue as duplicate of #343

[c4-judge]

berndartmueller marked the issue as satisfactory