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
3 changes: 1 addition & 2 deletions certora/harnesses/ERC20PermitHarness.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@

pragma solidity ^0.8.0;

import "../patched/token/ERC20/ERC20.sol";
import "../patched/token/ERC20/extensions/ERC20Permit.sol";

contract ERC20PermitHarness is ERC20, ERC20Permit {
contract ERC20PermitHarness is ERC20Permit {
constructor(string memory name, string memory symbol) ERC20(name, symbol) ERC20Permit(name) {}

function mint(address account, uint256 amount) external {
Expand Down
25 changes: 25 additions & 0 deletions certora/harnesses/ERC20WrapperHarness.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

import "../patched/token/ERC20/extensions/ERC20Wrapper.sol";

contract ERC20WrapperHarness is ERC20Wrapper {
constructor(IERC20 _underlying, string memory _name, string memory _symbol) ERC20(_name, _symbol) ERC20Wrapper(_underlying) {}

function underlyingTotalSupply() public view returns (uint256) {
return underlying().totalSupply();
}

function underlyingBalanceOf(address account) public view returns (uint256) {
return underlying().balanceOf(account);
}

function underlyingAllowanceToThis(address account) public view returns (uint256) {
return underlying().allowance(account, address(this));
}

function recover(address account) public returns (uint256) {
return _recover(account);
}
}
10 changes: 5 additions & 5 deletions certora/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const [reqSpec, reqContract] = request.split(':').reverse();

for (const { spec, contract, files, options = [] } of Object.values(specs)) {
if ((!reqSpec || reqSpec === spec) && (!reqContract || reqContract === contract)) {
limit(runCertora, spec, contract, files, [...options, ...extraOptions]);
limit(runCertora, spec, contract, files, [...options.flatMap(opt => opt.split(' ')), ...extraOptions]);
}
}

Expand All @@ -44,7 +44,7 @@ async function runCertora(spec, contract, files, options = []) {
const urls = data.toString('utf8').match(/https?:\S*/g);
for (const url of urls ?? []) {
if (url.includes('/jobStatus/')) {
console.error(`[${spec}] ${url}`);
console.error(`[${spec}] ${url.replace('/jobStatus/', '/output/')}`);
frangio marked this conversation as resolved.
Show resolved Hide resolved
stream.off('data', logStatusUrl);
break;
}
Expand All @@ -64,7 +64,7 @@ async function runCertora(spec, contract, files, options = []) {
stream.end();

// write results in markdown format
writeEntry(spec, contract, code || signal, (await output).match(/https:\S*/)[0]);
writeEntry(spec, contract, code || signal, (await output).match(/https:\S*/)?.[0]);

// write all details
console.error(`+ certoraRun ${args.join(' ')}\n` + (await output));
Expand Down Expand Up @@ -102,8 +102,8 @@ function writeEntry(spec, contract, success, url) {
spec,
contract,
success ? ':x:' : ':heavy_check_mark:',
`[link](${url})`,
`[link](${url.replace('/jobStatus/', '/output/')})`,
url ? `[link](${url})` : 'error',
url ? `[link](${url?.replace('/jobStatus/', '/output/')})` : 'error',
),
);
}
12 changes: 12 additions & 0 deletions certora/specs.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,17 @@
"certora/harnesses/ERC3156FlashBorrowerHarness.sol"
],
"options": ["--optimistic_loop"]
},
{
"spec": "ERC20Wrapper",
"contract": "ERC20WrapperHarness",
"files": [
"certora/harnesses/ERC20PermitHarness.sol",
"certora/harnesses/ERC20WrapperHarness.sol"
],
"options": [
"--link ERC20WrapperHarness:_underlying=ERC20PermitHarness",
"--optimistic_loop"
]
}
]
190 changes: 190 additions & 0 deletions certora/specs/ERC20Wrapper.spec
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
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.

underlying() returns(address) envfree
underlyingTotalSupply() returns(uint256) envfree
underlyingBalanceOf(address) returns(uint256) envfree
underlyingAllowanceToThis(address) returns(uint256) envfree

depositFor(address, uint256) returns(bool)
withdrawTo(address, uint256) returns(bool)
recover(address) returns(uint256)
}

use invariant totalSupplyIsSumOfBalances

/*
┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Helper: consequence of `totalSupplyIsSumOfBalances` applied to underlying │
└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
*/
function sumOfUnderlyingBalancesLowerThanUnderlyingSupply(address a, address b) {
frangio marked this conversation as resolved.
Show resolved Hide resolved
if (a != b) {
require underlyingBalanceOf(a) + underlyingBalanceOf(b) <= underlyingTotalSupply();
} else {
require underlyingBalanceOf(a) <= underlyingTotalSupply();
}
}

/*
┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Invariant: wrapped token can't be undercollateralized (solvency of the wrapper) │
└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
*/
invariant totalSupplyIsSmallerThanUnderlyingBalance()
totalSupply() <= underlyingBalanceOf(currentContract) &&
underlyingBalanceOf(currentContract) <= underlyingTotalSupply() &&
underlyingTotalSupply() <= max_uint256
{
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

}
preserved depositFor(address account, uint256 amount) with (env e) {
require e.msg.sender != currentContract;
frangio marked this conversation as resolved.
Show resolved Hide resolved
sumOfUnderlyingBalancesLowerThanUnderlyingSupply(e.msg.sender, currentContract);
}
}

/*
┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Rule: depositFor liveness and effects │
└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
*/
rule depositFor(env e) {
require nonpayable(e);

address sender = e.msg.sender;
address receiver;
address other;
uint256 amount;

// sanity
require currentContract != sender;
require currentContract != underlying();
requireInvariant totalSupplyIsSumOfBalances;
requireInvariant totalSupplyIsSmallerThanUnderlyingBalance;
sumOfUnderlyingBalancesLowerThanUnderlyingSupply(currentContract, sender);

uint256 balanceBefore = balanceOf(receiver);
uint256 supplyBefore = totalSupply();
uint256 senderUnderlyingBalanceBefore = underlyingBalanceOf(sender);
uint256 senderUnderlyingAllowanceBefore = underlyingAllowanceToThis(sender);
uint256 wrapperUnderlyingBalanceBefore = underlyingBalanceOf(currentContract);
uint256 underlyingSupplyBefore = underlyingTotalSupply();

uint256 otherBalanceBefore = balanceOf(other);
uint256 otherUnderlyingBalanceBefore = underlyingBalanceOf(other);

depositFor@withrevert(e, receiver, amount);
bool success = !lastReverted;

// liveness
assert success <=> (
sender != 0 && // invalid sender
receiver != 0 && // invalid receiver
amount <= senderUnderlyingBalanceBefore && // deposit doesn't exceed balance
amount <= senderUnderlyingAllowanceBefore // deposit doesn't exceed allowance
);

// effects
assert success => balanceOf(receiver) == balanceBefore + amount;
assert success => totalSupply() == supplyBefore + amount;
assert success => underlyingBalanceOf(currentContract) == wrapperUnderlyingBalanceBefore + amount;
assert success => underlyingBalanceOf(sender) == senderUnderlyingBalanceBefore - amount;
frangio marked this conversation as resolved.
Show resolved Hide resolved
frangio marked this conversation as resolved.
Show resolved Hide resolved

// no side effect
assert underlyingTotalSupply() == underlyingSupplyBefore;
assert balanceOf(other) != otherBalanceBefore => other == receiver;
assert underlyingBalanceOf(other) != otherUnderlyingBalanceBefore => (other == sender || other == currentContract);
}

/*
┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Rule: withdrawTo liveness and effects │
└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
*/
rule withdrawTo(env e) {
require nonpayable(e);

address sender = e.msg.sender;
address receiver;
address other;
uint256 amount;

// sanity
require currentContract != underlying();
requireInvariant totalSupplyIsSumOfBalances;
requireInvariant totalSupplyIsSmallerThanUnderlyingBalance;
sumOfUnderlyingBalancesLowerThanUnderlyingSupply(currentContract, receiver);

uint256 balanceBefore = balanceOf(sender);
uint256 supplyBefore = totalSupply();
uint256 receiverUnderlyingBalanceBefore = underlyingBalanceOf(receiver);
uint256 wrapperUnderlyingBalanceBefore = underlyingBalanceOf(currentContract);
uint256 underlyingSupplyBefore = underlyingTotalSupply();

uint256 otherBalanceBefore = balanceOf(other);
uint256 otherUnderlyingBalanceBefore = underlyingBalanceOf(other);

withdrawTo@withrevert(e, receiver, amount);
bool success = !lastReverted;

// liveness
assert success <=> (
sender != 0 && // invalid sender
receiver != 0 && // invalid receiver
amount <= balanceBefore // withdraw doesn't exceed balance
);

// effects
assert success => balanceOf(sender) == balanceBefore - amount;
assert success => totalSupply() == supplyBefore - amount;
assert success => underlyingBalanceOf(currentContract) == wrapperUnderlyingBalanceBefore - (currentContract != receiver ? amount : 0);
assert success => underlyingBalanceOf(receiver) == receiverUnderlyingBalanceBefore + (currentContract != receiver ? amount : 0);

// no side effect
assert underlyingTotalSupply() == underlyingSupplyBefore;
assert balanceOf(other) != otherBalanceBefore => other == sender;
assert underlyingBalanceOf(other) != otherUnderlyingBalanceBefore => (other == receiver || other == currentContract);
}

/*
┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Rule: recover liveness and effects │
└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
*/
rule recover(env e) {
require nonpayable(e);

address receiver;
address other;

// sanity
require currentContract != underlying();
requireInvariant totalSupplyIsSumOfBalances;
requireInvariant totalSupplyIsSmallerThanUnderlyingBalance;

uint256 value = underlyingBalanceOf(currentContract) - totalSupply();
uint256 supplyBefore = totalSupply();
uint256 balanceBefore = balanceOf(receiver);

uint256 otherBalanceBefore = balanceOf(other);
uint256 otherUnderlyingBalanceBefore = underlyingBalanceOf(other);

recover@withrevert(e, receiver);
bool success = !lastReverted;

// liveness
assert success <=> receiver != 0;

// effect
assert success => balanceOf(receiver) == balanceBefore + value;
assert success => totalSupply() == supplyBefore + value;
assert success => totalSupply() == underlyingBalanceOf(currentContract);

// no side effect
assert underlyingBalanceOf(other) == otherUnderlyingBalanceBefore;
assert balanceOf(other) != otherBalanceBefore => other == receiver;
}