Griefer can transfer baseToken directly into pool and lead to loss of user funds

[code423n4]

Lines of code

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

Vulnerability details

Impact

Users might lose funds due to the miscalculation of lpToken they should get.

Proof of Concept

Attacker can transfer ERC20 token directly into contract or use selfdestruct to force sending ETH into the contract.

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

    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 would make baseTokenShare return a lower value than what it should.

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

    function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        if (lpTokenSupply > 0) {
            // calculate amount of lp tokens as a fraction of existing reserves
            uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
            uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();

            return Math.min(baseTokenShare, fractionalTokenShare);
        } else {
            // if there is no liquidity then init
            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        } 

Example:
For simplicity assume lpTokenSupply == 1.

• Attacker deposits 1000 baseToken directly into the pair contract.
• User calls add() with 100 baseToken and fractionalToken
  Because of attacker's direct transfer baseTokenShare is now 0 instead of 100

baseTokenShare = 100 * 1 / 1001 = 0 (do to rounding down)

• Instead of receiving 100 lpToken user receives 0, making them unable to burn lpToken for base tokens or fractional tokens.

Recommended Mitigation Steps

Use internal accounting to not be affected by tokens directly deposited into the contract.

[c4-judge]

berndartmueller marked the issue as duplicate of #353

[berndartmueller]

Closing as invalid as the add function has slippage control in place and the mentioned issue only applies if the user supplied a value of 0 as the slippage parameter minLpTokenAmount.

[c4-judge]

berndartmueller marked the issue as not a duplicate

[c4-judge]

berndartmueller marked the issue as unsatisfactory:
Invalid