-
Notifications
You must be signed in to change notification settings - Fork 52
Niv/fix sanity failure #183
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
Changes from 4 commits
0b8b272
544ce94
67d87e2
1df0132
2fd6b92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,3 +10,4 @@ | |
| **/emv* | ||
| **/collect.json | ||
|
|
||
| .vscode/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,5 +4,6 @@ | |
| ], | ||
| "verify": "MockMutexer:./Mutexer.spec", | ||
| "solc": "solc8.24", | ||
| "solc_evm_version": "cancun" | ||
| "solc_evm_version": "cancun", | ||
| "prover_args": ["-enableStorageSplitting false"] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was needed here as we got storage splitting failures otherwise. @christiane-certora do you know if this is expected here? The contract defines a constant slot via
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the contract uses ALLTLOAD or ALLTSTORE, you should get an error that storage splitting must be disabled, these hook types are not compatible with it (same as the non-transient versions of them).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So should we instead rather use TLOAD and TSTORE here directly (now that we support them?)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like we could, yeah, although the currently used way of course also still works.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But maybe it is not so bad to leave this example, since there are still a lot of patterns with transient storage where we will fail the storage analysis (when the slot accessed is just a parameter, typically), so in that case it could be useful to still see how it can be worked with without. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,12 @@ | ||
| invariant isUnlocked(env e) | ||
| getLock(e) == 0; | ||
| invariant isUnlocked(env e) | ||
| getLock(e) == 0 | ||
| filtered { | ||
| // The functions repay and borrow use the modifier onlyLocked therefore excluding them here, as it causes vacuity with the invariant expression. | ||
| f -> f.selector != sig:repay(int256).selector | ||
| && f.selector != sig:borrow(int256).selector | ||
| // The function callback calls repay and borrow, thus same applied as above. | ||
| && f.selector != sig:CallBacker.callback().selector | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what was the sanity failure? we prefer invariant as the requireinvariant is safer
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue here was that a few functions (repay, borrow, callback) use a modifier |
||
| invariant deltaZeroWhenUnlocked(env e) | ||
| getLock(e) == 0 => getDelta(e) == 0; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,12 +129,6 @@ hook Sstore _customers[KEY address user].accounts.length uint256 newLength { | |
| numOfAccounts[user] = newLength; | ||
| } | ||
|
|
||
| /** | ||
| An internal step check to verify that our ghost works as expected, it should mirror the number of accounts. | ||
| Once the sload is defined, this invariant becomes a tautology | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is sload now defined nowadays, so is this why we see the sanity failure for trivial post condition of the invariant?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After reviewing, I suggest we indeed remove this invariant as it fails on SANITY due to the tautology check. |
||
| */ | ||
| invariant checkNumOfAccounts(address user) | ||
| numOfAccounts[user] == bank.getNumberOfAccounts(user); | ||
|
|
||
| /// This Sload is required in order to eliminate adding unintializaed account balance to sumBalances. | ||
| hook Sload uint256 length _customers[KEY address user].accounts.length { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should actually be
_tload()which is private in the parent contract and cannot be called here.