From a4bff49b0f591a41ad1fff284f1c5ebf8f98ef4b Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sun, 23 Jun 2024 19:23:02 -0300 Subject: [PATCH 01/10] fmt ci.yml --- .github/workflows/ci.yml | 62 ++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3248848..e86b8e1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -125,34 +125,34 @@ jobs: fi slither-analyze: - runs-on: "ubuntu-latest" - permissions: - actions: "read" - contents: "read" - security-events: "write" - steps: - - name: "Check out the repo" - uses: "actions/checkout@v4" - - - name: "Install Bun" - uses: "oven-sh/setup-bun@v1" - - - name: "Install the Node.js dependencies" - run: "bun install --frozen-lockfile" - - - name: "Run Slither analysis" - uses: "crytic/slither-action@v0.3.0" - id: "slither" - with: - fail-on: "none" - sarif: "results.sarif" - - - name: "Upload SARIF file to GitHub code scanning" - uses: "github/codeql-action/upload-sarif@v2" - with: - sarif_file: ${{ steps.slither.outputs.sarif }} - - - name: "Add summary" - run: | - echo "## Slither result" >> $GITHUB_STEP_SUMMARY - echo "✅ Uploaded to GitHub code scanning" >> $GITHUB_STEP_SUMMARY + runs-on: "ubuntu-latest" + permissions: + actions: "read" + contents: "read" + security-events: "write" + steps: + - name: "Check out the repo" + uses: "actions/checkout@v4" + + - name: "Install Bun" + uses: "oven-sh/setup-bun@v1" + + - name: "Install the Node.js dependencies" + run: "bun install --frozen-lockfile" + + - name: "Run Slither analysis" + uses: "crytic/slither-action@v0.3.0" + id: "slither" + with: + fail-on: "none" + sarif: "results.sarif" + + - name: "Upload SARIF file to GitHub code scanning" + uses: "github/codeql-action/upload-sarif@v2" + with: + sarif_file: ${{ steps.slither.outputs.sarif }} + + - name: "Add summary" + run: | + echo "## Slither result" >> $GITHUB_STEP_SUMMARY + echo "✅ Uploaded to GitHub code scanning" >> $GITHUB_STEP_SUMMARY From de94c4d426887ef5cf7d48e7ac795f688beb72af Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sun, 23 Jun 2024 19:24:45 -0300 Subject: [PATCH 02/10] remove --frozen-lockfile --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e86b8e1..f07d50a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -138,7 +138,7 @@ jobs: uses: "oven-sh/setup-bun@v1" - name: "Install the Node.js dependencies" - run: "bun install --frozen-lockfile" + run: "bun install" - name: "Run Slither analysis" uses: "crytic/slither-action@v0.3.0" From b48b3dec12e1b009b48e1c6dafb5465fd845f2c9 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sun, 23 Jun 2024 20:46:36 -0300 Subject: [PATCH 03/10] use custom errors --- foundry.toml | 3 +- src/RewardsDistributor.sol | 2 +- src/Staking.sol | 119 ++++++++++++++++--------- src/interfaces/IRewardsDistributor.sol | 2 +- test/Staking.t.sol | 2 +- test/helper/ProxyUtils.sol | 2 +- test/mocks/MockGovToken.sol | 2 +- 7 files changed, 84 insertions(+), 48 deletions(-) diff --git a/foundry.toml b/foundry.toml index 2782732..9445c55 100644 --- a/foundry.toml +++ b/foundry.toml @@ -2,7 +2,8 @@ evm_version = "paris" optimizer = true optimizer_runs = 10_000_000 - solc_version = "0.8.25" + via_ir = true + solc_version = "0.8.26" verbosity = 3 [profile.ci] diff --git a/src/RewardsDistributor.sol b/src/RewardsDistributor.sol index 963cabc..45d2016 100644 --- a/src/RewardsDistributor.sol +++ b/src/RewardsDistributor.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity 0.8.25; +pragma solidity 0.8.26; import {console} from "@forge-std/console.sol"; import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; diff --git a/src/Staking.sol b/src/Staking.sol index a31496f..ba14654 100644 --- a/src/Staking.sol +++ b/src/Staking.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity 0.8.25; +pragma solidity 0.8.26; import {console} from "@forge-std/console.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; @@ -62,7 +62,7 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { } /*////////////////////////////////////////////////////////////// - MAPPINGS/ARRAYS + MAPPINGS //////////////////////////////////////////////////////////////*/ /// @notice stores the metadata associated with a given stake @@ -83,23 +83,65 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { EVENTS //////////////////////////////////////////////////////////////*/ + /// @notice Emitted when a keyper stakes SHU event Staked( address indexed user, uint256 indexed amount, uint256 indexed shares, uint256 lockPeriod ); + + /// @notice Emitted when a keyper unstakes SHU event Unstaked(address user, uint256 amount, uint256 shares); + + /// @notice Emitted when a keyper claims rewards event RewardsClaimed(address user, uint256 rewards); + + /// @notice Emitted when a keyper is added or removed event KeyperSet(address keyper, bool isKeyper); + /*////////////////////////////////////////////////////////////// + ERRORS + //////////////////////////////////////////////////////////////*/ + + /// @notice Thrown when a non-keyper attempts a call for which only keypers are allowed + error OnlyKeyper(); + + /// @notice Thrown when transfer/tranferFrom is called + error TransferDisabled(); + + /// @notice Thrown when a keyper has no shares + error KeyperHasNoShares(); + + /// @notice Thrown when a keyper has staking for the first time and the + /// amount is less than the minimum stake set by the DAO + error FirstStakeLessThanMinStake(); + + /// @notice Thrown when someone try to unstake a stake that doesn't belong + /// to the keyper in question + error StakeDoesNotBelongToKeyper(); + + /// @notice Thrown when someone try to unstake a stake that doesn't exist + error StakeDoesNotExist(); + + /// @notice Thrown when someone try to unstake a amount that is greater than + /// the stake amount belonging to the stake id + error WithdrawAmountTooHigh(); + + /// @notice Thrown when someone try to unstake a stake that is still locked + error StakeIsStillLocked(); + + /// @notice Thrown when a keyper try to claim rewards but has no rewards to + /// claim + error NoRewardsToClaim(); + /*////////////////////////////////////////////////////////////// MODIFIERS //////////////////////////////////////////////////////////////*/ /// @notice Ensure only keypers can stake modifier onlyKeyper() { - require(keypers[msg.sender], "Only keyper"); + require(keypers[msg.sender], OnlyKeyper()); _; } @@ -165,10 +207,7 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { // If the keyper has no stakes, the first stake must be at least the minimum stake if (stakesIds.length() == 0) { - require( - amount >= minStake, - "The first stake must be at least the minimum stake" - ); + require(amount >= minStake, FirstStakeLessThanMinStake()); } /////////////////////////// EFFECTS /////////////////////////////// @@ -231,28 +270,32 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { /////////////////////////// CHECKS /////////////////////////////// require( keyperStakes[keyper].contains(stakeId), - "Stake does not belong to keyper" + StakeDoesNotBelongToKeyper() ); + Stake memory keyperStake = stakes[stakeId]; - require(stakes[stakeId].amount > 0, "Stake does not exist"); - - // Gets the stake - Stake storage keyperStake = stakes[stakeId]; + require(keyperStake.amount > 0, StakeDoesNotExist()); - uint256 maxWithdrawAmount; + // If caller doesn't specify the amount, the contract will transfer the + // stake amount for the stakeId + if (amount == 0) { + amount = keyperStake.amount; + } else { + require(amount <= keyperStake.amount, WithdrawAmountTooHigh()); + } // Checks below only apply if keyper is still a keyper // if keyper is not a keyper anymore, anyone can unstake, lock period is // ignored and minStake is not enforced if (keypers[keyper]) { - require(msg.sender == keyper, "Only keyper can unstake"); + require(msg.sender == keyper, OnlyKeyper()); // If the lock period is less than the global lock period, the stake // must be locked for the lock period if (lockPeriod < keyperStake.lockPeriod) { require( keyperStake.timestamp + lockPeriod <= block.timestamp, - "Stake is still locked" + StakeIsStillLocked() ); } else { // If the global lock period is greater than the stake lock period, @@ -260,33 +303,24 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { require( keyperStake.timestamp + keyperStake.lockPeriod <= block.timestamp, - "Stake is still locked" + StakeIsStillLocked() ); } - maxWithdrawAmount = maxWithdraw(keyper, keyperStake.amount); + require( + maxWithdraw(keyper, keyperStake.amount) >= amount, + WithdrawAmountTooHigh() + ); } else { - // doesn't exclude the min stake and locked staked as the keyper is not a keyper anymore - maxWithdrawAmount = convertToAssets(balanceOf(keyper)); - } - - require(maxWithdrawAmount > 0, "Keyper has no stake"); - - // If the amount is still greater than the stake amount for the specified stake index - // the contract will transfer the stake amount not the requested amount - // If amount specified by user is 0 transfer the stake amount - if (amount > keyperStake.amount || amount == 0) { - amount = keyperStake.amount; - } - - // If the amount is greater than the max withdraw amount, the contract - // will transfer the maximum amount available not the requested amount - // TODO I think this is never going to happen - if (amount > maxWithdrawAmount) { - amount = maxWithdrawAmount; + // doesn't include the min stake and locked staked as the keyper is not a keyper anymore + require( + convertToAssets(balanceOf(keyper)) >= amount, + WithdrawAmountTooHigh() + ); } /////////////////////////// EFFECTS /////////////////////////////// + // Calculates the amounf of shares to burn uint256 shares = convertToShares(amount); @@ -294,13 +328,13 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { _burn(keyper, shares); // Decrease the amount from the stake - keyperStake.amount -= amount; + stakes[stakeId].amount -= amount; // Decrease the amount from the total locked totalLocked[keyper] -= amount; // If the stake is empty, remove it - if (keyperStake.amount == 0) { + if (stakes[stakeId].amount == 0) { // Remove the stake from the stakes mapping delete stakes[stakeId]; @@ -329,7 +363,7 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { /// @param amount The amount of rewards to claim function claimRewards( uint256 amount - ) external onlyKeyper updateRewards returns (uint256 rewards) { + ) external updateRewards returns (uint256 rewards) { address keyper = msg.sender; // Prevents the keyper from claiming more than they should @@ -418,7 +452,8 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { /// @return The maximum amount of assets that a keyper can withdraw function maxWithdraw(address keyper) public view virtual returns (uint256) { uint256 shares = balanceOf(keyper); - require(shares > 0, "Keyper has no shares"); + require(shares > 0, KeyperHasNoShares()); + uint256 assets = convertToAssets(shares); uint256 compare = totalLocked[keyper] >= minStake @@ -438,7 +473,7 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { uint256 unlockedAmount ) public view virtual returns (uint256) { uint256 shares = balanceOf(keyper); - require(shares > 0, "Keyper has no shares"); + require(shares > 0, KeyperHasNoShares()); uint256 assets = convertToAssets(shares); @@ -459,7 +494,7 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { /// @notice Transfer is disabled function transfer(address, uint256) public pure override returns (bool) { - revert("Transfer is disabled"); + revert TransferDisabled(); } /// @notice Transfer is disabled @@ -468,7 +503,7 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { address, uint256 ) public pure override returns (bool) { - revert("Transfer is disabled"); + revert TransferDisabled(); } /*////////////////////////////////////////////////////////////// diff --git a/src/interfaces/IRewardsDistributor.sol b/src/interfaces/IRewardsDistributor.sol index b5b2b52..7d9a8ef 100644 --- a/src/interfaces/IRewardsDistributor.sol +++ b/src/interfaces/IRewardsDistributor.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity 0.8.25; +pragma solidity 0.8.26; interface IRewardsDistributor { function setRewardConfiguration( diff --git a/test/Staking.t.sol b/test/Staking.t.sol index 58fb259..d116a35 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity 0.8.25; +pragma solidity 0.8.26; import "@forge-std/Test.sol"; diff --git a/test/helper/ProxyUtils.sol b/test/helper/ProxyUtils.sol index c1906f0..42a04d8 100644 --- a/test/helper/ProxyUtils.sol +++ b/test/helper/ProxyUtils.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity 0.8.25; +pragma solidity 0.8.26; import {Vm} from "@forge-std/Vm.sol"; import {ERC1967Utils} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Utils.sol"; diff --git a/test/mocks/MockGovToken.sol b/test/mocks/MockGovToken.sol index f864ad4..62ebfaf 100644 --- a/test/mocks/MockGovToken.sol +++ b/test/mocks/MockGovToken.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity 0.8.25; +pragma solidity 0.8.26; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; From 820fa495e7f712a41032ff556be517f9b21adbc3 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sun, 23 Jun 2024 21:12:26 -0300 Subject: [PATCH 04/10] unhappy paths --- src/Staking.sol | 2 +- test/Staking.t.sol | 103 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/src/Staking.sol b/src/Staking.sol index ba14654..b8348e4 100644 --- a/src/Staking.sol +++ b/src/Staking.sol @@ -378,7 +378,7 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { rewards = amount; } - require(rewards > 0, "No rewards to claim"); + require(rewards > 0, NoRewardsToClaim()); // Calculates the amount of shares to burn uint256 shares = convertToShares(rewards); diff --git a/test/Staking.t.sol b/test/Staking.t.sol index d116a35..113bcd0 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -463,7 +463,67 @@ contract Stake is StakingTest { assertEq(timestamp2, block.timestamp, "Wrong timestamp"); } - function testFuzz_increaseDepositorTotalLockedWhenStaking() public {} + function testFuzz_increaseTotalLockedWhenStaking( + address _depositor, + uint256 _amount + ) public { + _amount = _boundToRealisticStake(_amount); + + _mintGovToken(_depositor, _amount); + _setKeyper(_depositor, true); + + vm.assume(_depositor != address(0)); + + uint256 totalLockedBefore = staking.totalLocked(_depositor); + + _stake(_depositor, _amount); + + uint256 totalLockedAfter = staking.totalLocked(_depositor); + + assertEq( + totalLockedAfter - totalLockedBefore, + _amount, + "Wrong total locked" + ); + } + + function testFuzz_RevertIf_StakingLessThanMinStake( + address _depositor + ) public { + uint256 amount = MIN_STAKE - 1; + + _mintGovToken(_depositor, amount); + _setKeyper(_depositor, true); + + vm.assume(_depositor != address(0)); + + vm.startPrank(_depositor); + govToken.approve(address(staking), amount); + + vm.expectRevert(Staking.FirstStakeLessThanMinStake.selector); + staking.stake(amount); + + vm.stopPrank(); + } + + function testFuzz_RevertIf_DepositorIsNotAKeyper( + address _depositor, + uint256 _amount + ) public { + _amount = _boundToRealisticStake(_amount); + + _mintGovToken(_depositor, _amount); + + vm.assume(_depositor != address(0)); + + vm.startPrank(_depositor); + govToken.approve(address(staking), _amount); + + vm.expectRevert(Staking.OnlyKeyper.selector); + staking.stake(_amount); + + vm.stopPrank(); + } } contract ClaimRewards is StakingTest { @@ -769,6 +829,29 @@ contract ClaimRewards is StakingTest { assertEq(sharesBefore - sharesAfter, burnShares, "Wrong shares burned"); } + + function testFuzz_RevertIf_NoRewardsToClaim( + address _depositor, + uint256 _amount + ) public { + _amount = _boundToRealisticStake(_amount); + + _mintGovToken(_depositor, _amount); + _setKeyper(_depositor, true); + + _stake(_depositor, _amount); + + vm.prank(_depositor); + + vm.expectRevert(Staking.NoRewardsToClaim.selector); + staking.claimRewards(0); + } + + function testFuzz_RevertIf_KeyperHasNoSHares(address _depositor) public { + vm.prank(_depositor); + vm.expectRevert(Staking.KeyperHasNoShares.selector); + staking.claimRewards(0); + } } contract Unstake is StakingTest { @@ -972,4 +1055,22 @@ contract Unstake is StakingTest { "Wrong balance" ); } + + function testFuzz_RevertIf_StakeDoesNotBelongToKeyper( + address _depositor1, + address _depositor2, + uint256 _amount1 + ) public { + _amount1 = _boundToRealisticStake(_amount1); + + _mintGovToken(_depositor1, _amount1); + + _setKeyper(_depositor1, true); + + uint256 stakeId = _stake(_depositor1, _amount1); + + vm.prank(_depositor2); + vm.expectRevert(Staking.StakeDoesNotBelongToKeyper.selector); + staking.unstake(_depositor2, stakeId, 0); + } } From ef57bedfadedf3b3d9602ef048e9cbc12f014427 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sun, 23 Jun 2024 21:20:18 -0300 Subject: [PATCH 05/10] add --ir-minimum to forge coverage --- .github/workflows/ci.yml | 2 +- test/Staking.t.sol | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f07d50a..477982d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,7 +59,7 @@ jobs: )" >> $GITHUB_ENV - name: Run coverage - run: forge coverage --report summary --report lcov + run: forge coverage --report summary --report lcov --ir-minimum # To ignore coverage for certain directories modify the paths in this step as needed. The # below default ignores coverage results for the test and script directories. Alternatively, diff --git a/test/Staking.t.sol b/test/Staking.t.sol index 113bcd0..54f0355 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -1073,4 +1073,38 @@ contract Unstake is StakingTest { vm.expectRevert(Staking.StakeDoesNotBelongToKeyper.selector); staking.unstake(_depositor2, stakeId, 0); } + + function testFuzz_RevertIf_AmountGreaterThanStakeAmount( + address _depositor, + uint256 _amount + ) public { + _amount = _boundToRealisticStake(_amount); + + _mintGovToken(_depositor, _amount); + _setKeyper(_depositor, true); + + uint256 stakeIndex = _stake(_depositor, _amount); + + vm.prank(_depositor); + vm.expectRevert(Staking.WithdrawAmountTooHigh.selector); + staking.unstake(_depositor, stakeIndex, _amount + 1); + } + + function testFuzz_RevertIf_NonKeyperTryToUnstake( + address _depositor, + address _anyone, + uint256 _amount + ) public { + _amount = _boundToRealisticStake(_amount); + + _mintGovToken(_depositor, _amount); + + _setKeyper(_depositor, true); + + uint256 stakeId = _stake(_depositor, _amount); + + vm.prank(_anyone); + vm.expectRevert(Staking.OnlyKeyper.selector); + staking.unstake(_depositor, stakeId, 0); + } } From afbcaf7081f0f8c7aba84ca3079046c96eda27a2 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Mon, 24 Jun 2024 08:14:09 -0300 Subject: [PATCH 06/10] improve fuzz assumption --- test/Staking.t.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/Staking.t.sol b/test/Staking.t.sol index 54f0355..2aac13f 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -719,6 +719,8 @@ contract ClaimRewards is StakingTest { _jump1 = _boundRealisticTimeAhead(_jump1); _jump2 = _boundRealisticTimeAhead(_jump2); + vm.assume(_depositor1 != _depositor2); + _mintGovToken(_depositor1, _amount); _mintGovToken(_depositor2, _amount); From f95d4ac19732826bdf87cc78824983df35f0cb92 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Mon, 24 Jun 2024 08:35:08 -0300 Subject: [PATCH 07/10] fix vm.warp() issue when running compiler with via-ir --- test/Staking.t.sol | 55 +++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/test/Staking.t.sol b/test/Staking.t.sol index 2aac13f..646e2a3 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -76,7 +76,7 @@ contract StakingTest is Test { } function _jumpAhead(uint256 _seconds) public { - vm.warp(block.timestamp + _seconds); + vm.warp(vm.getBlockTimestamp() + _seconds); } function _boundMintAmount(uint96 _amount) internal pure returns (uint256) { @@ -90,7 +90,7 @@ contract StakingTest is Test { } function _boundUnlockedTime(uint256 _time) internal view returns (uint256) { - return bound(_time, block.timestamp + LOCK_PERIOD, 105 weeks); + return bound(_time, vm.getBlockTimestamp() + LOCK_PERIOD, 105 weeks); } function _mintGovToken(address _to, uint256 _amount) internal { @@ -285,14 +285,14 @@ contract Stake is StakingTest { uint256 _shares1 = staking.convertToShares(_amount1); - uint256 timestampBefore = block.timestamp; + uint256 timestampBefore = vm.getBlockTimestamp(); _stake(_depositor, _amount1); _jumpAhead(_jump); uint256 _shares2 = _convertToSharesIncludeRewardsDistributed( _amount2, - REWARD_RATE * (block.timestamp - timestampBefore) + REWARD_RATE * (vm.getBlockTimestamp() - timestampBefore) ); _stake(_depositor, _amount2); @@ -413,7 +413,7 @@ contract Stake is StakingTest { (, uint256 timestamp, ) = staking.stakes(stakeId); - assertEq(timestamp, block.timestamp, "Wrong timestamp"); + assertEq(timestamp, vm.getBlockTimestamp(), "Wrong timestamp"); } function testFuzz_trackLockPeriodWhenStaking( @@ -459,8 +459,8 @@ contract Stake is StakingTest { assertEq(amount1, _amount1, "Wrong amount"); assertEq(amount2, _amount2, "Wrong amount"); - assertEq(timestamp, block.timestamp - 1, "Wrong timestamp"); - assertEq(timestamp2, block.timestamp, "Wrong timestamp"); + assertEq(timestamp, vm.getBlockTimestamp() - 1, "Wrong timestamp"); + assertEq(timestamp2, vm.getBlockTimestamp(), "Wrong timestamp"); } function testFuzz_increaseTotalLockedWhenStaking( @@ -540,7 +540,7 @@ contract ClaimRewards is StakingTest { _stake(_depositor, _amount); - uint256 timestampBefore = block.timestamp; + uint256 timestampBefore = vm.getBlockTimestamp(); _jumpAhead(_jump); @@ -548,7 +548,7 @@ contract ClaimRewards is StakingTest { staking.claimRewards(0); uint256 expectedRewards = REWARD_RATE * - (block.timestamp - timestampBefore); + (vm.getBlockTimestamp() - timestampBefore); assertEq( govToken.balanceOf(_depositor), @@ -599,7 +599,7 @@ contract ClaimRewards is StakingTest { _stake(_depositor, _amount); - uint256 timestampBefore = block.timestamp; + uint256 timestampBefore = vm.getBlockTimestamp(); _jumpAhead(_jump); @@ -607,7 +607,7 @@ contract ClaimRewards is StakingTest { vm.expectEmit(); emit Staking.RewardsClaimed( _depositor, - REWARD_RATE * (block.timestamp - timestampBefore) + REWARD_RATE * (vm.getBlockTimestamp() - timestampBefore) ); staking.claimRewards(0); @@ -626,7 +626,7 @@ contract ClaimRewards is StakingTest { _stake(_depositor, _amount); - uint256 timestampBefore = block.timestamp; + uint256 timestampBefore = vm.getBlockTimestamp(); _jumpAhead(_jump); @@ -634,7 +634,7 @@ contract ClaimRewards is StakingTest { uint256 rewards = staking.claimRewards(0); uint256 expectedRewards = REWARD_RATE * - (block.timestamp - timestampBefore); + (vm.getBlockTimestamp() - timestampBefore); assertEq(rewards, expectedRewards, "Wrong rewards"); } @@ -652,13 +652,13 @@ contract ClaimRewards is StakingTest { _stake(_depositor, _amount); - uint256 timestampBefore = block.timestamp; + uint256 timestampBefore = vm.getBlockTimestamp(); uint256 sharesBefore = staking.balanceOf(_depositor); _jumpAhead(_jump); uint256 expectedRewards = REWARD_RATE * - (block.timestamp - timestampBefore); + (vm.getBlockTimestamp() - timestampBefore); uint256 burnShares = _convertToSharesIncludeRewardsDistributed( expectedRewards, @@ -686,12 +686,12 @@ contract ClaimRewards is StakingTest { _stake(_depositor, _amount); - uint256 timestampBefore = block.timestamp; + uint256 timestampBefore = vm.getBlockTimestamp(); _jumpAhead(_jump); uint256 expectedRewards = REWARD_RATE * - (block.timestamp - timestampBefore); + (vm.getBlockTimestamp() - timestampBefore); uint256 burnShares = _convertToSharesIncludeRewardsDistributed( expectedRewards, @@ -784,12 +784,12 @@ contract ClaimRewards is StakingTest { _stake(_depositor, _amount); - uint256 timestampBefore = block.timestamp; + uint256 timestampBefore = vm.getBlockTimestamp(); _jumpAhead(_jump); uint256 expectedRewards = REWARD_RATE * - (block.timestamp - timestampBefore); + (vm.getBlockTimestamp() - timestampBefore); vm.prank(_depositor); uint256 rewards = staking.claimRewards(expectedRewards); @@ -810,13 +810,13 @@ contract ClaimRewards is StakingTest { _stake(_depositor, _amount); - uint256 timestampBefore = block.timestamp; + uint256 timestampBefore = vm.getBlockTimestamp(); uint256 sharesBefore = staking.balanceOf(_depositor); _jumpAhead(_jump); uint256 expectedRewards = REWARD_RATE * - (block.timestamp - timestampBefore); + (vm.getBlockTimestamp() - timestampBefore); uint256 rewardsToClaim = expectedRewards / 2; uint256 burnShares = _convertToSharesIncludeRewardsDistributed( @@ -893,12 +893,12 @@ contract Unstake is StakingTest { uint256 stakeIndex = _stake(_depositor, _amount); - uint256 timestampBefore = block.timestamp; + uint256 timestampBefore = vm.getBlockTimestamp(); _jumpAhead(_jump); uint256 expectedRewards = REWARD_RATE * - (block.timestamp - timestampBefore); + (vm.getBlockTimestamp() - timestampBefore); vm.prank(_depositor); staking.unstake(_depositor, stakeIndex, 0); @@ -925,13 +925,13 @@ contract Unstake is StakingTest { uint256 stakeIndex = _stake(_depositor, _amount); - uint256 timestampBefore = block.timestamp; + uint256 timestampBefore = vm.getBlockTimestamp(); _jumpAhead(_jump); uint256 shares = _convertToSharesIncludeRewardsDistributed( _amount, - REWARD_RATE * (block.timestamp - timestampBefore) + REWARD_RATE * (vm.getBlockTimestamp() - timestampBefore) ); vm.expectEmit(); emit Staking.Unstaked(_depositor, _amount, shares); @@ -974,11 +974,12 @@ contract Unstake is StakingTest { uint256 stakeIndex = _stake(_depositor, _amount); - uint256 timestampBefore = block.timestamp; + uint256 timestampBefore = vm.getBlockTimestamp(); _jumpAhead(_jump); - uint256 rewards = REWARD_RATE * (block.timestamp - timestampBefore); + uint256 rewards = REWARD_RATE * + (vm.getBlockTimestamp() - timestampBefore); vm.prank(_depositor); staking.unstake(_depositor, stakeIndex, 0); From ef43bc1b9b3ef3ba3478fd2b2901f55a7c146fc4 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Mon, 24 Jun 2024 08:37:42 -0300 Subject: [PATCH 08/10] improve assumptions --- src/Staking.sol | 1 + test/Staking.t.sol | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/src/Staking.sol b/src/Staking.sol index b8348e4..b7ef4e4 100644 --- a/src/Staking.sol +++ b/src/Staking.sol @@ -368,6 +368,7 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { // Prevents the keyper from claiming more than they should uint256 maxWithdrawAmount = maxWithdraw(keyper); + console.log("maxWithdrawAmount", maxWithdrawAmount); // If the amount is greater than the max withdraw amount, the contract // will transfer the maximum amount available not the requested amount diff --git a/test/Staking.t.sol b/test/Staking.t.sol index 646e2a3..31f87e4 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -752,6 +752,8 @@ contract ClaimRewards is StakingTest { _amount = _boundToRealisticStake(_amount); _jump = _boundRealisticTimeAhead(_jump); + vm.assume(_depositor1 != _depositor2); + _mintGovToken(_depositor1, _amount); _mintGovToken(_depositor2, _amount); @@ -850,6 +852,11 @@ contract ClaimRewards is StakingTest { } function testFuzz_RevertIf_KeyperHasNoSHares(address _depositor) public { + vm.assume( + _depositor != address(0) && + _depositor != ProxyUtils.getAdminAddress(address(staking)) + ); + vm.prank(_depositor); vm.expectRevert(Staking.KeyperHasNoShares.selector); staking.claimRewards(0); From e012410bb64b874028193226c8b60077fdfbe7c2 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Mon, 24 Jun 2024 08:40:38 -0300 Subject: [PATCH 09/10] assumptions on unstake tests --- test/Staking.t.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/Staking.t.sol b/test/Staking.t.sol index 31f87e4..d1114cc 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -1071,6 +1071,7 @@ contract Unstake is StakingTest { address _depositor2, uint256 _amount1 ) public { + vm.assume(_depositor1 != _depositor2); _amount1 = _boundToRealisticStake(_amount1); _mintGovToken(_depositor1, _amount1); @@ -1105,6 +1106,8 @@ contract Unstake is StakingTest { address _anyone, uint256 _amount ) public { + vm.assume(_depositor != _anyone); + _amount = _boundToRealisticStake(_amount); _mintGovToken(_depositor, _amount); From dd7956e30f3c2e5492e7d2e1bd3454b98981e1ea Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Mon, 24 Jun 2024 08:44:32 -0300 Subject: [PATCH 10/10] fuzz assumptions on unstake with different addresses --- src/Staking.sol | 2 -- test/Staking.t.sol | 16 ++++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/Staking.sol b/src/Staking.sol index b7ef4e4..2f4838b 100644 --- a/src/Staking.sol +++ b/src/Staking.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.26; -import {console} from "@forge-std/console.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; @@ -368,7 +367,6 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable { // Prevents the keyper from claiming more than they should uint256 maxWithdrawAmount = maxWithdraw(keyper); - console.log("maxWithdrawAmount", maxWithdrawAmount); // If the amount is greater than the max withdraw amount, the contract // will transfer the maximum amount available not the requested amount diff --git a/test/Staking.t.sol b/test/Staking.t.sol index d1114cc..19c7836 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -1072,6 +1072,14 @@ contract Unstake is StakingTest { uint256 _amount1 ) public { vm.assume(_depositor1 != _depositor2); + vm.assume( + _depositor1 != address(0) && + _depositor1 != ProxyUtils.getAdminAddress(address(staking)) + ); + vm.assume( + _depositor2 != address(0) && + _depositor2 != ProxyUtils.getAdminAddress(address(staking)) + ); _amount1 = _boundToRealisticStake(_amount1); _mintGovToken(_depositor1, _amount1); @@ -1106,6 +1114,14 @@ contract Unstake is StakingTest { address _anyone, uint256 _amount ) public { + vm.assume( + _depositor != address(0) && + _depositor != ProxyUtils.getAdminAddress(address(staking)) + ); + vm.assume( + _anyone != address(0) && + _anyone != ProxyUtils.getAdminAddress(address(staking)) + ); vm.assume(_depositor != _anyone); _amount = _boundToRealisticStake(_amount);