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

[Certora] Handle overflows #105

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion certora/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ The [`certora/specs`](specs) folder contains the following files:
- [`ExternalCalls.spec`](specs/ExternalCalls.spec) checks that the Morpho token implementation is reentrancy safe by ensuring that no function is making and external calls and, that the implementation is immutable as it doesn't perform any delegate call;
- [`ERC20Invariants.spec`](specs/ERC20Invariants.spec) common hooks and invariants to be shared in different specs;
- [`ERC20.spec`](specs/ERC20.spec) ensures that the Morpho token is compliant with the [ERC20](https://eips.ethereum.org/EIPS/eip-20) specification, we also check Morpho token `burn` and `mint` functions in [`MintBurnEthereum`](specs/MintBurnEthereum.spec) and [`MintBurnOptimism`](specs/MintBurnOptimism.spec);
- [`Delegation.spec`](specs/Delegation.spec) checks the logic for voting power delegation.
- [`Delegation.spec`](specs/Delegation.spec) checks the logic for voting power delegation;
- [`RevertsERC20.spec`](specs/RevertsERC20.spec), [`RevertsMintBurnEthereum.spec`](specs/RevertsMintBurnEthereum.spec) and [`RevertsMintBurnOptimism.spec`](specs/RevertsMintBurnOptimism.spec) check that conditions for reverts and inputs are correctly validated.

The [`certora/confs`](confs) folder contains a configuration file for each corresponding specification file for both the Ethereum and the Optimism version.
Expand Down
18 changes: 12 additions & 6 deletions certora/specs/ERC20.spec
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this spec useful for now that we have ERC20Reverts and ERC20Invariants?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the spec from OZ, with some liveness and safety properties that are specific to functions and that are not invariants. For example there is a rule on transfer specifically

Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,17 @@ rule transfer(env e) {
uint256 recipientVotingPowerBefore = delegatedVotingPower(delegatee(recipient));
uint256 otherBalanceBefore = balanceOf(other);

// Safe require as it is verified in delegatedLTEqDelegateeVP.
require holderBalanceBefore <= holderVotingPowerBefore;
colin-morpho marked this conversation as resolved.
Show resolved Hide resolved
// Safe require as it is verified in totalSupplyGTEqSumOfVotingPower.
require delegatee(holder) != delegatee(recipient) => holderBalanceBefore + recipientVotingPowerBefore <= totalSupply();

// run transaction
transfer@withrevert(e, recipient, amount);

// check outcome
if (lastReverted) {
assert holder == 0 || recipient == 0 || amount > holderBalanceBefore ||
// Handle overflows in delegation, should not be possible.
recipientVotingPowerBefore + amount > max_uint256 || holderVotingPowerBefore < amount ;
assert holder == 0 || recipient == 0 || amount > holderBalanceBefore;
} else {
// balances of holder and recipient are updated
assert to_mathint(balanceOf(holder)) == holderBalanceBefore - (holder == recipient ? 0 : amount);
Expand Down Expand Up @@ -129,14 +132,17 @@ rule transferFrom(env e) {
uint256 recipientVotingPowerBefore = delegatedVotingPower(delegatee(recipient));
uint256 otherBalanceBefore = balanceOf(other);

// Safe require as it is verified in delegatedLTEqDelegateeVP.
require holderBalanceBefore <= holderVotingPowerBefore;
colin-morpho marked this conversation as resolved.
Show resolved Hide resolved
// Safe require as it is verified in totalSupplyGTEqSumOfVotingPower.
require delegatee(holder) != delegatee(recipient) => holderBalanceBefore + recipientVotingPowerBefore <= totalSupply();

// run transaction
transferFrom@withrevert(e, holder, recipient, amount);

// check outcome
if (lastReverted) {
assert holder == 0 || recipient == 0 || spender == 0 || amount > holderBalanceBefore || amount > allowanceBefore
// Handle overflows in delegation, should not be possible.
|| amount + recipientVotingPowerBefore > max_uint256 || holderVotingPowerBefore < amount ;
assert holder == 0 || recipient == 0 || spender == 0 || amount > holderBalanceBefore || amount > allowanceBefore;
} else {
// allowance is valid & updated
assert allowanceBefore >= amount;
Expand Down
4 changes: 1 addition & 3 deletions certora/specs/MintBurnEthereum.spec
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ rule mint(env e) {

// cache state
uint256 toBalanceBefore = balanceOf(to);
uint256 toVotingPowerBefore = delegatedVotingPower(delegatee(to));
uint256 otherBalanceBefore = balanceOf(other);
uint256 totalSupplyBefore = totalSupply();

Expand All @@ -76,8 +75,7 @@ rule mint(env e) {

// check outcome
if (lastReverted) {
assert e.msg.sender != owner() || to == 0 || totalSupplyBefore + amount > max_uint256 ||
toVotingPowerBefore + amount > max_uint256;
assert e.msg.sender != owner() || to == 0 || totalSupplyBefore + amount > max_uint256 ;
} else {
Comment on lines 77 to 79
Copy link
Contributor

Choose a reason for hiding this comment

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

could we avoid this pattern? (and have a rule for checking reverts, and one rule for checking the non revert case's effects)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are still following OZ's specs (and actually getting closer to it in this PR)

Copy link
Contributor Author

@colin-morpho colin-morpho Feb 11, 2025

Choose a reason for hiding this comment

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

Yes, we did mention the idea of a more generic framework on ERC20 at some point but we didn't find a clever way to make this happen to decided to continue this route, if I'm not mistaken.

// updates balance and totalSupply
assert e.msg.sender == owner();
Expand Down
3 changes: 1 addition & 2 deletions certora/specs/MintBurnOptimism.spec
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ rule mint(env e) {

// cache state
uint256 toBalanceBefore = balanceOf(to);
uint256 toVotingPowerBefore = delegatedVotingPower(delegatee(to));
uint256 otherBalanceBefore = balanceOf(other);
uint256 totalSupplyBefore = totalSupply();

Expand All @@ -78,7 +77,7 @@ rule mint(env e) {
// check outcome
if (lastReverted) {
assert e.msg.sender != owner() || to == 0 || totalSupplyBefore + amount > max_uint256 ||
toVotingPowerBefore + amount > max_uint256 || e.msg.sender != currentContract.bridge;
e.msg.sender != currentContract.bridge;
} else {
// updates balance and totalSupply
assert e.msg.sender == currentContract.bridge;
Expand Down
Loading