user can profit by reentrancy if erc777 is used as base token

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L154
https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L398-L400

Vulnerability details

Impact

This is a well known attack, openzepellin talks about it here https://blog.openzeppelin.com/exploiting-uniswap-from-reentrancy-to-actual-profit/.
When a erc777 token is used, user can reenter buy before the transfer of base token has taken place, allowing user to buy fractional token at a cheaper price because baseTokenBalance will stay the same.

Proof of Concept

User buys some fractional token. buyQuote is used to calculate the amount of baseToken needed for the amount of fractional token user want to buy.

    function buy(uint256 outputAmount, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {
        // *** Checks *** //


        // check that correct eth input was sent - if the baseToken equals address(0) then native ETH is used
        require(baseToken == address(0) ? msg.value == maxInputAmount : msg.value == 0, "Invalid ether input");


        // calculate required input amount using xyk invariant
        inputAmount = buyQuote(outputAmount);


        // check that the required amount of base tokens is less than the max amount
        require(inputAmount <= maxInputAmount, "Slippage: amount in");


        // *** Effects *** //


        // transfer fractional tokens to sender
        _transferFrom(address(this), msg.sender, outputAmount);


        // *** Interactions *** //


        if (baseToken == address(0)) {
            // refund surplus eth
            uint256 refundAmount = maxInputAmount - inputAmount;
            if (refundAmount > 0) msg.sender.safeTransferETH(refundAmount);
        } else {
            // transfer base tokens in
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), inputAmount);
        }


        emit Buy(inputAmount, outputAmount);
    }

In buyQuote, baseTokenReserves is divided by fractionalTokenReserves to get the price of fractional token.

    function buyQuote(uint256 outputAmount) public view returns (uint256) {
        return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
    }

However, user have the opportunity to reenter buy at safeTransferFrom if baseToken is a erc777 token. According to erc777 docs, the hook is called before the transfer of tokens. Hence, baseTokenReserves above will remain the same. This allows the user to buy fractional token at a cheaper price than usual by reentering the buy function repeatedly.

        } else {
            // transfer base tokens in
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), inputAmount);
        }

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend using ReentrancyGuard from OpenZepellin.

[c4-judge]

berndartmueller marked the issue as duplicate of #343

[c4-judge]

berndartmueller marked the issue as satisfactory