-
Notifications
You must be signed in to change notification settings - Fork 0
feat: fuzzing of staking invariants for ProtocolStaking contract with Foundry #50
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
base: main
Are you sure you want to change the base?
Changes from 33 commits
cc8b238
eb8322c
32dae45
e3d8529
3c55b59
f06d916
8d0e8ea
7ffc002
0e3e79e
49186e0
086ae97
af3703c
9c78c71
5830bf5
5142a29
da83054
2d2aa1b
de6e463
f05331a
c07b8cc
5a5eceb
f36f141
1b2a8a6
45197eb
036cebd
1ef6f04
97d102f
f1cd625
a21a879
987f8a4
61d738b
b0b880b
188ab6a
c505307
32ed0f5
6bc32da
931517a
c1d7182
18e7bc9
dece594
1bea63a
6a28a1f
5e54e3a
7394a67
14a9a04
ed53d1e
4e3b748
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 |
|---|---|---|
|
|
@@ -62,6 +62,7 @@ contracts-exposed | |
| # Foundry | ||
| /out | ||
| /cache_forge | ||
| /lib/forge-std | ||
|
|
||
| yarn.lock | ||
| fhevmTemp | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,22 @@ | ||
| [profile.default] | ||
| src = 'contracts' | ||
| libs = ['node_modules'] | ||
| out = 'out' | ||
| test = 'test/foundry' | ||
| cache_path = 'cache_forge' | ||
| libs = ['lib', 'node_modules'] | ||
| solc = '0.8.27' | ||
| optimizer_runs = 200 | ||
| optimizer = true | ||
| optimizer_runs = 800 | ||
| evm_version = 'cancun' | ||
|
|
||
| remappings = [ | ||
| 'forge-std/=lib/forge-std/src/', | ||
| '@openzeppelin/contracts/=node_modules/@openzeppelin/contracts/', | ||
| '@openzeppelin/contracts-upgradeable/=node_modules/@openzeppelin/contracts-upgradeable/', | ||
| 'token/=../token/', | ||
| ] | ||
|
|
||
| [invariant] | ||
| runs = 4096 | ||
| depth = 100 | ||
| fail_on_revert = true | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| // SPDX-License-Identifier: MIT | ||
| pragma solidity ^0.8.27; | ||
|
|
||
| import {Test} from "forge-std/Test.sol"; | ||
| import {ProtocolStakingHarness} from "./harness/ProtocolStakingHarness.sol"; | ||
| import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; | ||
| import {ZamaERC20} from "token/contracts/ZamaERC20.sol"; | ||
| import {ProtocolStakingHandler} from "./handlers/ProtocolStakingHandler.sol"; | ||
| import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | ||
|
|
||
| // Invariant fuzz test for ProtocolStaking | ||
| contract ProtocolStakingInvariantTest is Test { | ||
| ProtocolStakingHarness internal protocolStaking; | ||
| ZamaERC20 internal zama; | ||
| ProtocolStakingHandler internal handler; | ||
|
|
||
| address internal governor = address(1); | ||
|
||
| address internal manager = address(2); | ||
| address internal admin = address(3); | ||
|
|
||
| uint256 internal constant ACTOR_COUNT = 5; | ||
| uint256 internal constant INITIAL_TOTAL_SUPPLY = 1_000_000 ether; | ||
| uint256 internal constant INITIAL_REWARD_RATE = 1e18; // 1 token/second | ||
| uint48 internal constant INITIAL_UNSTAKE_COOLDOWN_PERIOD = 1 seconds; | ||
|
||
|
|
||
| function setUp() public { | ||
| address[] memory actorsList = new address[](ACTOR_COUNT); | ||
| for (uint256 i = 0; i < ACTOR_COUNT; i++) { | ||
| actorsList[i] = address(uint160(4 + i)); | ||
Seth-Schmidt marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| // Deploy ZamaERC20, mint to all actors, admin is DEFAULT_ADMIN | ||
| uint256 initialActorBalance = INITIAL_TOTAL_SUPPLY / ACTOR_COUNT; | ||
| address[] memory receivers = new address[](ACTOR_COUNT); | ||
| uint256[] memory amounts = new uint256[](ACTOR_COUNT); | ||
| for (uint256 i = 0; i < ACTOR_COUNT; i++) { | ||
| receivers[i] = actorsList[i]; | ||
| amounts[i] = initialActorBalance; | ||
| } | ||
|
|
||
| zama = new ZamaERC20("Zama", "ZAMA", receivers, amounts, admin); | ||
|
|
||
| // Deploy ProtocolStaking behind ERC1967 proxy | ||
| ProtocolStakingHarness impl = new ProtocolStakingHarness(); | ||
| bytes memory initData = abi.encodeCall( | ||
| protocolStaking.initialize, | ||
| ( | ||
| "Staked ZAMA", | ||
| "stZAMA", | ||
| "1", | ||
| address(zama), | ||
| governor, | ||
| manager, | ||
| INITIAL_UNSTAKE_COOLDOWN_PERIOD, | ||
| INITIAL_REWARD_RATE | ||
| ) | ||
| ); | ||
| ERC1967Proxy proxy = new ERC1967Proxy(address(impl), initData); | ||
| protocolStaking = ProtocolStakingHarness(address(proxy)); | ||
|
|
||
| // Grant MINTER_ROLE on Zama to ProtocolStaking | ||
| vm.startPrank(admin); | ||
| zama.grantRole(zama.MINTER_ROLE(), address(protocolStaking)); | ||
| vm.stopPrank(); | ||
|
|
||
| // Approve ProtocolStaking for all actors | ||
| for (uint256 i = 0; i < ACTOR_COUNT; i++) { | ||
| vm.prank(actorsList[i]); | ||
| zama.approve(address(protocolStaking), type(uint256).max); | ||
| } | ||
|
|
||
| // Deploy handler with actors list | ||
| handler = new ProtocolStakingHandler(protocolStaking, zama, manager, actorsList); | ||
| targetContract(address(handler)); | ||
|
|
||
| bytes4[] memory selectors = new bytes4[](12); | ||
| selectors[0] = ProtocolStakingHandler.warp.selector; | ||
| selectors[1] = ProtocolStakingHandler.setRewardRate.selector; | ||
| selectors[2] = ProtocolStakingHandler.addEligibleAccount.selector; | ||
| selectors[3] = ProtocolStakingHandler.removeEligibleAccount.selector; | ||
| selectors[4] = ProtocolStakingHandler.stake.selector; | ||
| selectors[5] = ProtocolStakingHandler.unstake.selector; | ||
| selectors[6] = ProtocolStakingHandler.claimRewards.selector; | ||
| selectors[7] = ProtocolStakingHandler.release.selector; | ||
| selectors[8] = ProtocolStakingHandler.unstakeThenWarp.selector; | ||
| selectors[9] = ProtocolStakingHandler.stakeEquivalenceScenario.selector; | ||
| selectors[10] = ProtocolStakingHandler.unstakeEquivalenceScenario.selector; | ||
| selectors[11] = ProtocolStakingHandler.setUnstakeCooldownPeriod.selector; | ||
| targetSelector(FuzzSelector({addr: address(handler), selectors: selectors})); | ||
|
||
| } | ||
|
Member
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. On the other hand, since Foundry will use random msg.sender addresses in invariant testing, maybe it would make sense to use targetSenders cheatcode here, and to pass the 5 actors to it (ie the eligible account) + few other random addresses, this is in order to reduce bias. Because my main concern here is that most calls would be done with current setup with uneligible accounts, thus reducing the coverage of the fuzzing campaign. Wdyt?
Member
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. Actually just noticed that you are using vm.prank calls in all functions of the handler contract to handle this correctly.
Contributor
Author
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 caller should always be a known actor through the combination of bounding the actor index
Contributor
Author
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.
Got it. You can ignore my previous reply, the comment had not refreshed yet. I think we came to the same conclusion on this.
Contributor
Author
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 actors have now been added as To enforce ineligibility, I made an |
||
|
|
||
| function invariant_TotalStakedWeightEqualsEligibleWeights() public view { | ||
|
Collaborator
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. nit: maybe best to keep same invariant order vs what you've described in the md file ! |
||
| assertEq( | ||
| protocolStaking.totalStakedWeight(), | ||
| handler.computeExpectedTotalWeight(), | ||
| "totalStakedWeight does not match sum of eligible weights" | ||
| ); | ||
| } | ||
|
|
||
| function invariant_TotalSupplyBoundedByRewardRate() public view { | ||
| assertLe( | ||
| zama.totalSupply(), | ||
| // TODO: Occasional Off-by-one error in the ghost total supply calculation, need to locate the source of the error | ||
|
||
| // adding small buffer of 1 wei to account for this for now | ||
| handler.ghost_initialTotalSupply() + handler.ghost_accumulatedRewardCapacity() + 1, | ||
| "totalSupply exceeds piecewise rewardRate bound" | ||
| ); | ||
| } | ||
|
|
||
| function invariant_RewardDebtConservation() public view { | ||
| uint256 tolerance = handler.REWARD_DEBT_CONSERVATION_TOLERANCE(); | ||
| int256 lhs = handler.computeRewardDebtLHS(); | ||
| // When the system is empty, net debt across all users should net out to 0 | ||
| // Σ _paid[account] + Σ earned(account) = 0 | ||
| // Using ApproxEqAbs per contract comment: "Accounting rounding may have a marginal impact on earned rewards (dust)." | ||
| if (protocolStaking.totalStakedWeight() == 0) { | ||
| assertApproxEqAbs(lhs, 0, tolerance, "Net reward debt must be 0 when no one is staked"); | ||
| return; | ||
| } | ||
| int256 rhs = handler.computeRewardDebtRHS(); | ||
| assertApproxEqAbs(lhs, rhs, tolerance, "reward debt conservation"); | ||
| } | ||
|
|
||
| function invariant_PendingWithdrawalsSolvency() public view { | ||
| address token = protocolStaking.stakingToken(); | ||
| uint256 balance = IERC20(token).balanceOf(address(protocolStaking)); | ||
| uint256 sumAwaitingRelease; | ||
| for (uint256 i = 0; i < handler.actorsLength(); i++) { | ||
| sumAwaitingRelease += protocolStaking.awaitingRelease(handler.actorAt(i)); | ||
| } | ||
| assertGe(balance, sumAwaitingRelease, "pending withdrawals solvency"); | ||
| } | ||
|
|
||
| function invariant_StakedFundsSolvency() public view { | ||
| for (uint256 i = 0; i < handler.actorsLength(); i++) { | ||
| address account = handler.actorAt(i); | ||
| uint256 totalStaked = handler.ghost_totalStaked(account); | ||
| uint256 balance = protocolStaking.balanceOf(account); | ||
| uint256 awaiting = protocolStaking.awaitingRelease(account); | ||
| uint256 released = handler.ghost_totalReleased(account); | ||
| assertEq(totalStaked, balance + awaiting + released, "staked funds solvency"); | ||
| } | ||
| } | ||
| } | ||
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.
I just tried to run fuzz tests locally and realised 4096 is actually taking a while on my machine.
Maybe it makes sense to go back to 256 runs here, and to integrate fuzz testing in the CI. If it runs regularly via CI it should be good enough to catch future (or past) bugs. Wdyt?
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 makes sense to me, and regular runs are preferable. My only concern with CI integration would be how the dust tolerances are handled so that they won't incorrectly interrupt a workflow. Finding a known bound as per #50 (comment) would prevent this.