Add testInitialLiquidity to part 3 tests

[ardislu]

Thank you Justin for the stream today, very insightful.

The purpose of this test is to check that this logic in the mint function of UniswapV2Pair is working as expected:

if (_totalSupply == 0) {
    liquidity = Math.sqrt(amount0.mul(amount1)).sub(MINIMUM_LIQUIDITY);
    _mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
}

// ...

require(liquidity > 0, 'UniswapV2: INSUFFICIENT_LIQUIDITY_MINTED');

The coverage report says the new asserts are getting hit, so I think the test is working.

I'm wondering if this is a good invariant to test though? During the stream I know you set the minimum amounts for testProvideLiquidity to 1000. My reasoning for adding this test is that because Math is a custom library, we'd want to confirm the sqrt logic works. Or is it safe to assume this library does what it's supposed to?

Appreciate any feedback and thank you all again for the great series!

[technovision99]

Hey @ardislu, thank you so much for the PR! We are so happy to see that people are learning from the workshops and acting upon it.

This is a very good invariant to test. The truth you are assessing is that "one should not be able to provide less than the initial minimum liquidity required by the protocol". Your format of the test and the use of coverage are all great.

Here are some points to consider:

1. Try to think if you can test the same truth in the testLiquidity function in the not-so-happy path (where success = false). Could you assert something around "if I fail to provide liquidity, it should probably be because I didn't provide enough tokens OR something else OR something else". Note that you will have to change the bounds on amount1 and amount2 using the _between function (the minimum shouldn't be 1000 anymore). Note that what you have is fantastic and you can do it like that or also include it in the test provided. It truly comes down to what you think is best, there is no right answer :)
2. Regarding the sqrt function - never assume anything works! So yes, you should absolutely test it. Our jobs as auditors is to try to validate / invalidate the "truths" that the development teams believes. Recall from part 2 that you should test that sqrt function by itself using arithmetic properties of the square root function in addition to doing something more indirect, like what you are doing here.

Hope this helps and thanks again for the PR.

[ardislu]

Thank you for the feedback!

On point 1, your suggestion looks like a much more elegant/efficient way to implement the testInitialLiquidity test. I think it could be implemented with a simple else statement on success3:

// If any failure condition is met, user should not be awarded any LP tokens
else {
  assert(pair.balanceOf(address(user)) == 0);
}

I observed that this assert was getting hit even when I didn't change the _between arguments. Digging into that, I saw that this was happening because the maximum amount passed to _between is uint256(-1) while UniswapV2 only supports a maximum reserve of uint112(-1). So I added another assert to confirm the reason for failing:

// success3 should be false if:
// - the minimimum liquidity was not met, OR
// - either token amount transferred is more than 2**112 - 1 (the maximum UniswapV2 reserve)
assert(amount1 > 2**112 - 1 || amount2 > 2**112 - 1 || Math.sqrt(amount1 * amount2) < 10**3);

On point 2, I added testSqrt to validate the product and quotient properties of square roots:

assert(Math.sqrt(a).mul(Math.sqrt(b)) == Math.sqrt(a.mul(b)));
assert(Math.sqrt(a) / Math.sqrt(b) == Math.sqrt(a / b));

After running this test I can confirm that these properties do not hold when the inputs are small numbers (<100) due to integer rounding in Solidity. However, since the UniswapV2 contract always multiplies the inputs together before passing them to sqrt, AND any square roots below the minimum liquidity are already reverted, I think this problem is mitigated here.

Still good to know about this behavior for future implementations though!

Thanks again for the feedback, looking forward to the next stream.

[aviggiano]

Hi @ardislu

Thanks for this update.
I'm running echidna with your new assertions and discovered other cases where providing liquidity fails.

Here's my updated version:

                // amounts overflow max reserve balance
                assert(amount1 > type(uint112).max ||
                    amount2 > type(uint112).max ||
                    // amounts do not pass the minimum initial liquidity check
                    Math.sqrt(amount1 * amount2) <= pair.MINIMUM_LIQUIDITY() ||
                    // amounts would mint zero liquidity
                    Math.min(
                        ((balance1Before - reserve1Before) *
                            (pairTotalSupplyBefore)) / reserve1Before,
                        ((balance2Before - reserve2Before) *
                            (pairTotalSupplyBefore)) / reserve2Before
                    ) ==
                    0);