Caviar contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/039095ee6b73709289d88cb99397e8b9028224c7/src/Pair.sol#L95 https://github.com/code-423n4/2022-12-caviar/blob/039095ee6b73709289d88cb99397e8b9028224c7/src/Pair.sol#L172 https://github.com/code-423n4/2022-12-caviar/blob/039095ee6b73709289d88cb99397e8b9028224c7/src/Pair.sol#L480

Vulnerability details

Impact

When calculating amounts within the functions buyQuote, sellQuote, addQuote, and removeQuote, the output of baseTokenReserves is used. This function calls _baseTokenReservers, which queries the balance of the contract for ERC20 tokens (and therefore also for ERC777 tokens, which are downward compatible with ERC20).

function _baseTokenReserves() internal view returns (uint256) {
        return baseToken == address(0)
            ? address(this).balance - msg.value // subtract the msg.value if the base token is ETH
            : ERC20(baseToken).balanceOf(address(this));
    }

This logic becomes problematic with ERC777 tokens. These tokens provide a tokensToSend hook before any state is updated, i.e. before the actual transfer is performed and the balance of the Pair is updated. However, the LP Tokens are already minted (in the case of add) or the fractional tokens are already transferred (in the case of buy) at this point. This can be exploited for price manipulations when reentering within this hook.

Proof Of Concept

Let's look at a concrete example for add. We assume that lpTokenSupply = 100, that baseTokenReserve() = 100, and that fractionalTokenReserve() = 100. The user calls add with a baseTokenAmount of 100 and a fractionalTokenAmount of 100. The lpTokenAmount is:

uint256 baseTokenShare = (100 * 100) / 100 = 100;
uint256 fractionalTokenShare = (100 * 100) / 100 = 100;
return Math.min(baseTokenShare, fractionalTokenShare) = 100;

The user now reenters within the tokensToSend hook that is triggered from the following transfer:

ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);

When reentering, we have lpTokenSupply = 200 (as 100 additional LP tokens were minted), baseTokenReserve() = 100 (because the base token transfer is still in progress within the hook), fractionalTokenReserve() = 200 (because of the fractional token transfer in add). We now assume that the user calls add with a baseTokenAmount of 100 and a fractionalTokenAmount of 200. The lpTokenAmount is:

uint256 baseTokenShare = (100 * 200) / 100 = 200;
uint256 fractionalTokenShare = (200 * 200) / 200 = 100;
return Math.min(baseTokenShare, fractionalTokenShare) = 200;

As we can see in this example, the user only had to provide 100 base tokens to get 200 LP tokens, although the correct amount (when we would not exploit the not-yet updated state with the reentrancy) for this amount of LP tokens would be 200 base tokens.

Recommended Mitigation Steps

Use reentrancy guards.