McToady - `Pair` contract incorrectly assumes the underlying tokens are not misbehaving leading to potential to break core AMM invariant

[sherlock-admin2]

McToady

Medium

Pair contract incorrectly assumes the underlying tokens are not misbehaving leading to potential to break core AMM invariant

Summary

Pair::swap does not follow the CEI pattern and performs its key invariant check (balance0 * balance1) >= (reserves0 * reserves1) before then making an external call that could change the value of balance0 and balance1 to something that would break the invariant.

Vulnerability Detail

The core invariant of AMM protocols is that the product of balances must be greater than or equal to the product of the reserves (overall liquidity remains the same or deepens after a swap). However Pair::swap checks for this core invariant before then updating _balance0 and _balance1 to new values as shown here:

        if (amount0In != 0) _balance0 = _balance0 - fee0;
        if (amount1In != 0) _balance1 = _balance1 - fee1;

        require(_k(_balance0, _balance1) >= _k(_reserve0, _reserve1), 'K'); // Pair: K
        }

        _balance0 = IERC20(token0).balanceOf(address(this)); 
        _balance1 = IERC20(token1).balanceOf(address(this));

There is no check that the invariant holds with these new values. Additionally, between setting the original values for _balance0 and _balance1 there is an external state changing call made to the potentially misbehaving token contract during ExternalBribe::notifyRewardAmount (called via _sendTokenFees) in which this misbehaviour could take place.

Given that this version of the protocol plans to allow for semi permissionless creation of pairs/gauges it is important the contract does not make assumptions that the tokens involved in these pairs will behave as expected.

Impact

A misbehaving token could break the balances >= reserves core invariant of AMMs. This would allow a malicious token to manipulate it's price by artificially lowering it's balance in the pool while bypassing the invariant check.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/Pair.sol#L329

Tool used

Manual Review

Recommendation

The invariant check that the product of current balances should be greater than the product of the reserves should be made after making the external call that updates _balance0 and _balance1. This way even if the token has unexpected behaviour the invariant will have to hold based on the contract's final state.

As shown here, the original Velodrome V1 Pair contract correctly makes the invariant check after proceeding with the final updates for balances. here

[nevillehuang]

Invalid, only fees are notified, which are not part of the reserves, there is no issue here