Reentrancy issue #2: Functions remove() and sell() are vulnerable to reentrancy attack if baseToken implement a call to receiver in _beforeTokenTransfer().

[code423n4]

Lines of code

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

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 remove(uint256 lpTokenAmount, uint256 minBaseTokenOutputAmount, uint256 minFractionalTokenOutputAmount)
    public
    returns (uint256 baseTokenOutputAmount, uint256 fractionalTokenOutputAmount)
{
    ...
    (baseTokenOutputAmount, fractionalTokenOutputAmount) = removeQuote(lpTokenAmount);
    ...
    // transfer fractional tokens to sender
    _transferFrom(address(this), msg.sender, fractionalTokenOutputAmount);

    // *** Interactions *** //

    // burn lp tokens from sender
    lpToken.burn(msg.sender, lpTokenAmount);

    if (baseToken == address(0)) {
        // if base token is native ETH then send ether to sender
        msg.sender.safeTransferETH(baseTokenOutputAmount);
    } else {
        // transfer base tokens to sender
        ERC20(baseToken).safeTransfer(msg.sender, baseTokenOutputAmount); 
        // @audit reentrancy from _beforeTokenTransfer() hook
    }

    emit Remove(baseTokenOutputAmount, fractionalTokenOutputAmount, lpTokenAmount);
}

However, if baseToken is an ERC20 token that implemented _beforeTokenTransfer() and this function has a call hook to receiver, it could be exploited because this function is called before state changes.

Proof of Concept

Check out a similar issue from VTVL contest.
code-423n4/2022-09-vtvl-findings#362

Consider the scenario:

1. Firstly, liquidity pool has 1000 XXX - 1000 FracToken and total supply of LP token is 1000.
2. Alice (attacker contract) calls sell() to swap from 100 FracToken to XXX. She will receive

inputAmountWithFee = inputAmount * 997 = 99700

outputAmount = (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee)
outputAmount = (99700 * 1000) / ((1000 * 1000) + 99700) = 90

4. After minting, before XXX is transferred out, liquidity has 1000 XXX - 1100 FracToken.

5. During safeTransferFrom() calls, _beforeTokenTransfer() does a call to receiver (Alice) before balance update. Alice used it to call sell() again to swap 100 FracToken. At this moment, the outputAmount is calculated as

inputAmountWithFee = inputAmount * 997 = 99700

outputAmount = (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee)
outputAmount = (99700 * 1000) / ((1100 * 1000) + 99700) = 83

After all, Alice swapped 200 FracToken and get 90 + 83 = 173 XXX. And now, liquidity pool has 827 XXX - 1200 FracToken.

Using xyk variant to check, we can notice that at first xy = 1000000. After Alice swaps, xy = 992400 < 1000000.

Tools Used

Manual Review

Recommended Mitigation Steps

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

[Shungy]

I think this is low risk based on this acknowledgement by sponsor: https://github.com/code-423n4/2022-12-caviar#malicious-base-token-or-nft-contracts

[minhquanym]

Thank you for your feedback. However, I don't think this report is implying that the token is malicious in nature; rather, it is just how it operates.

[berndartmueller]

Closing in favor of the warden's other submission #445

This submission demonstrates the same underlying issue of missing reentrancy protection, just with a different token standard (ERC-20 with a custom pre-transfer hook) instead of ERC-777. The underlying issue is the same.

[c4-judge]

berndartmueller marked the issue as nullified