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

Add FV specification for ERC20Wrapper #4100

Merged
merged 15 commits into from Mar 8, 2023
Merged

Add FV specification for ERC20Wrapper #4100

merged 15 commits into from Mar 8, 2023

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Mar 8, 2023

Certora specs for the ERC20 wrapper, rewritten almost from scratch

@Amxx Amxx added ignore-changeset formal-verification Enable FV run in a PR. labels Mar 8, 2023
@changeset-bot
Copy link

changeset-bot bot commented Mar 8, 2023

🦋 Changeset detected

Latest commit: 0c57f1b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

certora/run.js Show resolved Hide resolved
import "helpers.spec"
import "ERC20.spec"

methods {
Copy link
Contributor

Choose a reason for hiding this comment

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

When we specify an invariant, is it only checked for the methods listed in this block? What happens if we add a new function in the Solidity code and we don't add it here?

I just noticed this in the docs and I find it concerning!

It is possible for a method signature to appear in the methods block but not in the contract being verified. In this case, the Prover will skip any rules that mention the missing method, rather than reporting an error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AFAIK:

  • The invariant (and arbitrary methods) will check all functions in the contract's ABI, even if not here.
  • Putting them here allow to mark them envfree, of set a summarize policy
  • If a rule does a test on a function declared here, its absence from the contract will not break things and the tests will just be ignored.

certora/specs/ERC20Wrapper.spec Outdated Show resolved Hide resolved
{
preserved {
requireInvariant totalSupplyIsSumOfBalances;
require underlyingBalanceOf(currentContract) <= underlyingTotalSupply();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we write:

Suggested change
require underlyingBalanceOf(currentContract) <= underlyingTotalSupply();
requireInvariant totalSupplyIsSmallerThanUnderlyingBalance;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

since we are in the definition of totalSupplyIsSmallerThanUnderlyingBalance

that would be saying totalSupplyIsSmallerThanUnderlyingBalance holds assuming totalSupplyIsSmallerThanUnderlyingBalance. I hope the prover will reject that. It a bit like using a variable inside its own declaration.

Copy link
Collaborator Author

@Amxx Amxx Mar 8, 2023

Choose a reason for hiding this comment

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

we could do

sumOfUnderlyingBalancesLowerThanUnderlyingSupply(currentContract, 0);

Copy link
Contributor

Choose a reason for hiding this comment

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

It a bit like using a variable inside its own declaration.

No, that is not the case according to the docs on invariants. The preserve block is included as a precondition, so by using a "recursive" requireInvariant in its own preserve block, we're just stating that this is an inductive property.

Copy link
Collaborator Author

@Amxx Amxx Mar 8, 2023

Choose a reason for hiding this comment

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

Isn't the whole point of an invariant that its a precondition of itself?

  • [instace] we test that its holds a construction
  • [preserve] assuming it holds, execute any function and check that it still holds.

I don't understand what requiring it adds, since its already assumed to hold before the function call

certora/specs/ERC20Wrapper.spec Outdated Show resolved Hide resolved
certora/specs/ERC20Wrapper.spec Outdated Show resolved Hide resolved
certora/specs/ERC20Wrapper.spec Outdated Show resolved Hide resolved
frangio
frangio previously approved these changes Mar 8, 2023
@Amxx Amxx enabled auto-merge (squash) March 8, 2023 21:57
@Amxx Amxx merged commit 3214f6c into master Mar 8, 2023
@Amxx Amxx deleted the fv/ERC20Wrapper branch March 8, 2023 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants