Skip to content

Conversation

@amusingaxl
Copy link
Contributor

No description provided.

oddaf and others added 30 commits February 14, 2025 00:04
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Signed-off-by: oddaf <106770775+oddaf@users.noreply.github.com>
@amusingaxl amusingaxl marked this pull request as ready for review March 28, 2025 13:32
pedrobergamini
pedrobergamini previously approved these changes Mar 28, 2025
Copy link
Member

@pedrobergamini pedrobergamini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Just commented an idea on Certora's GH actions workflow

pedrobergamini
pedrobergamini previously approved these changes Apr 2, 2025
revert7 || revert8 || revert9 ||
revert10,
"set revert rules failed";
revert7 || revert8,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on this change

@sunbreak1211
Copy link
Collaborator

Maybe an interesting rule/invariant to do, if you end up having some extra time, would be to prove that after any call to set, the values updated can not end up being outside the min-max range and always respect be restricted by the step.
Then the fix ordering can be roll back and check that the rule would have caught that. In either case those things are also indirectly proved with the current rules so not really mandatory, but I guess it would be a more high level rule that helps guaranteeing those invariants.

@amusingaxl
Copy link
Contributor Author

Maybe an interesting rule/invariant to do, if you end up having some extra time, would be to prove that after any call to set, the values updated can not end up being outside the min-max range and always respect be restricted by the step.

This was added in 67d5fbc.

Then the fix ordering can be roll back and check that the rule would have caught that.

Not entirely sure if I get what you're suggesting here.

Base automatically changed from initial-implementation to master April 22, 2025 12:50
@amusingaxl amusingaxl dismissed pedrobergamini’s stale review April 22, 2025 12:50

The base branch was changed.

Copy link
Member

@pedrobergamini pedrobergamini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few questions about the mocks, also, after reviewing the updated rules I thought it could be useful if we added the following one to ensure the config is always consistent:

rule config_min_less_than_max(bytes32 id) {
    uint16 min; uint16 max; uint16 step;
    min, max, step = cfgs(id);
    
    assert min <= max, "Configuration min should always be <= max";
}

// NOTE: using mocked `drip` function because this method is not a subject of the formal verification
// NOTE: this line is needed to prevent `file` above from reverting
rho = uint64(block.timestamp);
// (uint256 chi_, uint256 rho_) = (chi, rho);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we return chi here? As right now nChi is always == 0?

nChi = uint256(chi)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, the return value is irrelevant to the main subject of the formal verification.
We just need to ensure that the calls to drip() and file() in the underlying contracts don't fail for reasons that are not relevant.

// NOTE: this line is needed to prevent `file` above from reverting
ilks[ilk].rho = now;
// require(now >= ilks[ilk].rho, "Jug/invalid-now");
// (, uint prev) = vat.ilks(ilk);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better if we returned the current duty instead of 0 here?

function drip(bytes32 ilk) external note returns (uint256 rate) {
    ilks[ilk].rho = now;
    rate = ilks[ilk].duty;  // Return current duty instead of 0
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

// uint chi_ = sub(tmp, chi);
// chi = tmp;
rho = now;
// rho = now;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as Jug::drip, would it be better if we returned the dsr value here?

function drip() external note returns (uint256 tmp) {
    rho = now;
    tmp = dsr;  // Return current dsr instead of 0
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Member

@pedrobergamini pedrobergamini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@amusingaxl amusingaxl requested a review from sunbreak1211 June 12, 2025 22:37
@amusingaxl amusingaxl merged commit bc88579 into master Aug 4, 2025
4 checks passed
@amusingaxl amusingaxl deleted the feat/certora-spec branch August 4, 2025 13:43
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.

5 participants