Reentrancy issue #1: Functions buy() and add() are vulnerable to reentrancy attack through tokensToSend() hook of ERC777

[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#L95

Vulnerability details

It is important to be aware that I have reported two reentrancy bugs. Each of these have different ways of being activated and can be found in separate functions.

Impact

All calculations done in Caviar Pair are using token balance directly. For example, when users add liquidity, it will use baseToken.balanceOf(address(this)) to calculate the amount of LP token should be minted.

function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
    public
    payable
    returns (uint256 lpTokenAmount)
{
    ...
    // calculate the lp token shares to mint
    uint lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);
    ...
    // mint lp tokens to sender
    lpToken.mint(msg.sender, lpTokenAmount);

    // transfer base tokens in if the base token is not ETH
    if (baseToken != address(0)) {
        // transfer base tokens in
        ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount); 
        // @audit ERC777 token reentrancy.
    }
    ...
}

However, if baseToken is ERC777 that implemented tokensToSend(), it could be exploited because the tokensToSend() hook is called before balance update.

Proof of Concept

Assuming baseToken is XXX - an ERC777 that implemnted tokensToSend(). This hook is called before balance update (Implementation Requirement from EIP-777)

• The token contract MUST call the tokensToSend hook before updating the state.

Consider the scenario:

1. At first, liquidity pool has 1000 XXX - 1000 FracToken and total supply of LP token is 1000.
2. Alice (attacker contract) calls add() to add 100 XXX and 100 FracToken. The calculate and mint lpTokenAmount = 100 to Alice.
   At this moment, liquidity pool has 1000 XXX (because baseToken is not transferred in yet) and 1100 FracToken and total supply of LP token is 1100.
3. After minting, during safeTransferFrom() calls, Alice received a tokensToSend() hook. She used this to call add() again with 100 XXX and 110 FracToken. Now lpTokenAmount is calculated as

baseTokenShare = 100 * 1100 / 1000 = 110
fractionalTokenShare = 110 * 1100 / 1100 = 110
lpAmount = min(baseTokenShare, fractionalTokenShare) = 110

So totally, Alice added 200 XXX and 210 FracToken and she received 210 LP token while she is expected to only receive 200 LP token normally.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding nonReentrant modifier to these functions to protect them from reentrancy issue.

[c4-judge]

berndartmueller marked the issue as duplicate of #343

[c4-judge]

berndartmueller marked the issue as satisfactory