Removed unsafe unchecked statements

[FinagleLord]

Particularly unsafe in the swap method, _getAmountOut() should be checked. Additionally, I'd also require() that the invariant is growing proportionally to the fee taken even though it's kinda implicit within the _getAmountOut() method. 🥄

[boringcrypto]

Great! I'm of the opinion that unchecked should only be used in places where you expect and want an over/underflow. Not for gas savings even if you (think you) know it's safe. These gas savings aren't worthwhile compared to the potential losses if you make a mistake.

[FinagleLord]

Great! I'm of the opinion that unchecked should only be used in places where you expect and want an over/underflow. Not for gas savings even if you (think you) know it's safe. These gas savings aren't worthwhile compared to the potential losses if you make a mistake.

I agree for the most part, only time I would disagree is when incrementing a uint256 by one. In regard to this contract, I think most, if not all, of these unchecked statements need to be removed. Additionally, the added convenience methods should probably follow the same rigorous checks that the original methods do.

P.S I'm a big fan @boringcrypto

[matthewlilley]

The argument is that the previous implemenation used unsafe math.

https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L176

The real question is how much gas is saved here?

[FinagleLord]

The argument is that the previous implementation used unsafe math.

https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L176

The real question is how much gas is saved here?

uint amount0In = balance0 > _reserve0 - amount0Out ? balance0 - (_reserve0 - amount0Out) : 0;
uint amount1In = balance1 > _reserve1 - amount1Out ? balance1 - (_reserve1 - amount1Out) : 0;

These ternary statements are checked in the require statements a few lines above so not much, you could however save some gas by changing them to if statements, since nothing is happening in the second/else case

uint amount0In
if (balance0 > _reserve0 - amount0Out) amount0In = balance0 - (_reserve0 - amount0Out); 

[gasperbr]

We can make two assumptions. One is that balances will always be greater or equal than reserves and the other is that getAmountOut function never returns an amountOut greater than reserveAmountOut. That's why the unchecked {} in the swap method is fine to stay. The same applies to the burnSingle function. Not sure if you're implying this in your first comment but the actual _getAmountOut method remains checked even if called within an unchecked scope. I think the current implementation is fine.

[boringcrypto]

The assumption that balances are always greater or equal to the reserves is a pretty big call. They SHOULD be, but are you so sure you would gamble everything you own on this? There are no weird rounding issues that can make it go off by 1? The balances come from an external contract (BentoBox), it's compiled with a somewhat untested solidity compiler. You're probably right, but I've made the decision to no longer safe a few gas, with the risk of adding a mistake multiplier. I now only use unchecked if it NEEDS to be unchecked because overflow/underflow is needed. Writing safe contracts is hard enough, why add extra risk?

[gasperbr]

Balances and reserves are read from bentobox shares, no calculation happens so there aren't any rounding issue concerns. The gas costs increase is roughly 130 gas for the swap function and 180 for the burnSingle function. If it gives everyone ease of mind I'm ok with merging this. But then also remove the 4 other unchecked scopes which fall in the same category.