Protocol should maintain the reserve amount in storage and use that for buy/sell interactions

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L477
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L398
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L406
https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L390

Vulnerability details

Impact

The invariant is easily broken and liquidity providers do not get the fees.

Proof of Concept

The protocol resembles Uniswap and intends to benefit LPs by charging fees for buy/sell transactions.

Pair.sol
402:     /// @notice The amount of base tokens received for selling a given amount of fractional tokens.
403:     /// @dev Calculated using the xyk invariant and a 30bps fee.
404:     /// @param inputAmount The amount of fractional tokens to sell.
405:     /// @return outputAmount The amount of base tokens received.
406:     function sellQuote(uint256 inputAmount) public view returns (uint256) {
407:         uint256 inputAmountWithFee = inputAmount * 997;
408:         return (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee);
409:     }

As we can see in the functions _baseTokenReserves() and fractionalTokenReserves(), the protocol uses the total balance as reserve amounts. So the protocol relies on the balance of base token and fractional token while buy/sell interactions. (and price() view as well)

Pair.sol
383:     function fractionalTokenReserves() public view returns (uint256) {
384:         return balanceOf[address(this)];
385:     }
...
474:     /// @dev Returns the current base token reserves. If the base token is ETH then it ignores
475:     ///      the msg.value that is being sent in the current call context - this is to ensure the
476:     ///      xyk math is correct in the buy() and add() functions.
477:     function _baseTokenReserves() internal view returns (uint256) {
478:         return baseToken == address(0)
479:             ? address(this).balance - msg.value // subtract the msg.value if the base token is ETH
480:             : ERC20(baseToken).balanceOf(address(this));
481:     }

This can lead to various problems.

1. Because the fee is included in the price calculation, we can't say liquidity providers take the expected fee.
2. Anyone can break the invariant and manipulate the price by sending base token (or even fractional token). It is arguable if this kind of manipulation will benefit the attackers but there can be various scenarios.

Relying on balance is not the best practice and other AMMs stores the reserve amounts as state variables and only use that for calculations.

ElasticSwap was exploited due to the similar vulnerability on the 12th of December, just a few days ago! Check here.

Tools Used

Manual Review

Recommended Mitigation Steps

Add new state variables like reserveBase and reserveFractional and use them for buy/sell interactions.

[c4-judge]

berndartmueller marked the issue as primary issue

[outdoteth]

Does not provide a direct exploit that can lead to loss of funds

[c4-sponsor]

outdoteth marked the issue as disagree with severity

[c4-sponsor]

outdoteth marked the issue as sponsor acknowledged

[berndartmueller]

Marking #383 as the primary report.

The wardens pointed out a possible price manipulation issue due to reliance on direct ETH and token contract balances. I consider Medium severity to be appropriate.

[c4-judge]

berndartmueller marked the issue as satisfactory

[outdoteth]

Should this be tagged as duplicate of #383 ?

[C4-Staff]

CloudEllie marked the issue as duplicate of #383