Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: fix value boundary #198

Merged
merged 1 commit into from
Feb 1, 2023
Merged

docs: fix value boundary #198

merged 1 commit into from
Feb 1, 2023

Conversation

ChmielewskiKamil
Copy link
Contributor

@ChmielewskiKamil ChmielewskiKamil commented Jan 21, 2023

Isn't the boundary val = val % token.balanceOf(address(this)) missing the maximal value of token.balanceOf(address(this))?

In the Learn how to fuzz like a pro: Intro to AMM's invariants, Justin creates a helper function to create a boundary on the amount.

It looks like this:

function _between(
        uint256 amount,
        uint256 low,
        uint256 high
    ) internal pure returns (uint256) {
        return (low + (amount % (high - low + 1)));
    }

+1 is added to include the maximal value that can be sent.

As explained in this video on the modulo operator by Golan Levin, the modulo operator:

mod remainder
5%5 0
6%5 1
7%5 2
8%5 3
9%5 4
10%5 0
11%5 1
...

and the cycle repeats

So in our case

mod remainder
val % token.balanceOf(address(this)) token.balanceOf(address(this)) -1

So we have to add +1 to cover the gap.

Using `val = val % token.balanceOf(address(this))` constrains the value to be from zero up to the `token.balanceOf(address(this))` (not included). It misses the maximal value of `token.balanceOf(address(this))`.
@ChmielewskiKamil
Copy link
Contributor Author

This simple assertion proves it:

function val_boundary(uint256 value) public returns (uint256) {
        uint256 balance = 1000;
        if (value <= balance) {
            value = value % balance;
            return value;
        }
    }

    function test_val_boundary(uint256 value) public {
        assert(val_boundary(value) != 1000);
    }

The value will never be equal to 1000.

@ggrieco-tob
Copy link
Member

This is correct, thanks for the fix!

@montyly montyly merged commit 8ef863c into crytic:master Feb 1, 2023
@ChmielewskiKamil ChmielewskiKamil deleted the patch-3 branch February 1, 2023 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants