attacker steal funds because functions buy() shouldn't revert when user pays 0 amount and receive fractional token

[code423n4]

Lines of code

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

Vulnerability details

Impact

Function buy() buys fractional tokens from the pair, there is no check in those functions that what user pays or receives isn't 0 so it's possible for attacker to buy some fractional tokens and pay 0 base token. so liquidity providers would lose funds because of this.

Proof of Concept

This is buy() code:

    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);
    }

As you can see the amount user pays calculated in buyQuote() and the return value of that function has been used to transfer funds from user. there is no check that what user pays is not 0 and if the return value was 0 then user would pay 0 base token and receive some fractional tokens.
This is buyQuote() code:

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

so when outputAmount * 1000 * baseTokenReserves() < (fractionalTokenReserves() - outputAmount) * 997 then user would pay 0 amount but receives fractional tokens. for example if:

1. fractionalTokenReserves() would equal to 1000 * (1e18 +1)
2. baseTokenReserves() equal to 997 * 1e15
3. then the expression which makes inputAmount 0 would become outputAmount * 1000 * 997 * 1e15 < (1000 * (1e18 +1) - outputAmount) * 997
4. which equals to outputAmount * 997 * 1e18 < (1000 * (1e18 +1) - outputAmount) * 997
5. which equals to outputAmount * 1e18 < 1000 * (1e18 +1) - outputAmount
6. which equals outputAmount (1+1e18) < 1000 * (1e18 +1)
7. which equals to outputAmount < 1000.
8. so in this situation attacker can withdraw buy any amount less than 1000 of fractional tokens and he would pay 0 amount of base token.

the attack is more practical if those 1000 fractional token worth more than fee, but if the numbers changes the amount could be higher and attack could become practical, the severity is Medium and not High because the attack is not always practical. the real cause if this issue is rounding error when price (base token to fractional token) is too high or too low but contract should prevent this cases too because it's a permission-less protocol which wants to support all pairs of NFTs and base tokens.

Tools Used

VIM

Recommended Mitigation Steps

add more precision to fractional token and also revert if user is going to pay 0 amount of base token.

[c4-judge]

berndartmueller marked the issue as primary issue

[outdoteth]

This is a duplicate of #243

[c4-sponsor]

outdoteth requested judge review

[c4-judge]

berndartmueller marked the issue as duplicate of #243

[c4-judge]

berndartmueller marked the issue as satisfactory