From 895ea8eded83bd587015019f32650b6af0b80ebc Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sat, 3 Aug 2024 09:43:23 -0300 Subject: [PATCH 01/17] Donation attack test --- test/DelegateStaking.t.sol | 114 ++++++++++++------------ test/Staking.t.sol | 3 +- test/helpers/DelegateStakingHarness.sol | 6 ++ test/helpers/StakingHarness.sol | 6 ++ 4 files changed, 72 insertions(+), 57 deletions(-) diff --git a/test/DelegateStaking.t.sol b/test/DelegateStaking.t.sol index 7f27709..79cf8ad 100644 --- a/test/DelegateStaking.t.sol +++ b/test/DelegateStaking.t.sol @@ -651,59 +651,63 @@ contract Stake is DelegateStakingTest { delegate.stake(_keyper, 0); } - // function test_DonationAttackNoRewards( - // address keyper, - // address bob, - // address alice, - // uint256 bobAmount - // ) public { - // vm.assume(bob != alice); - // rewardsDistributor.removeRewardConfiguration(address(delegate)); - - // _setKeyper(keyper, true); - - // bobAmount = _boundToRealisticStake(bobAmount); - - // // alice deposits 1 - // _mintGovToken(alice, 1); - // uint256 aliceStakeId = _stake(alice, keyper, 1); - - // // simulate donation - // govToken.mint(address(delegate), bobAmount); - - // // bob stake - // _mintGovToken(bob, bobAmount); - // uint256 bobStakeId = _stake(bob, keyper, bobAmount); - - // _jumpAhead(vm.getBlockTimestamp() + LOCK_PERIOD); - - // // alice withdraw rewards (bob stake) even when there is no rewards distributed - // vm.startPrank(alice); - // //delegate.unstake(aliceStakeId, 0); - // delegate.claimRewards(0); - // vm.stopPrank(); - - // uint256 aliceBalanceAfterAttack = govToken.balanceOf(alice); - - // // attack should not be profitable for alice - // assertGtDecimal( - // bobAmount + 1, // amount alice has spend in total - // aliceBalanceAfterAttack, - // 1e18, - // "Alice receive more than expend for the attack" - // ); - - // vm.startPrank(bob); - // delegate.unstake(bobStakeId, 0); - // delegate.claimRewards(0); - - // uint256 bobBalanceAfterAttack = govToken.balanceOf(bob); - - // // at the end Alice still earn less than bob - // assertGt( - // bobBalanceAfterAttack, - // aliceBalanceAfterAttack, - // "Alice earn more than Bob after the attack" - // ); - // } + function test_DonationAttackNoRewards( + address keyper, + address bob, + address alice, + uint256 bobAmount + ) public { + vm.assume(bob != alice); + rewardsDistributor.removeRewardConfiguration(address(delegate)); + + _setKeyper(keyper, true); + + bobAmount = _boundToRealisticStake(bobAmount); + + // alice deposits 1 + _mintGovToken(alice, 1); + _stake(alice, keyper, 1); + + // simulate donation + govToken.mint(address(delegate), bobAmount); + + // bob stake + _mintGovToken(bob, bobAmount); + uint256 bobStakeId = _stake(bob, keyper, bobAmount); + + _jumpAhead(vm.getBlockTimestamp() + LOCK_PERIOD); + + // alice withdraw rewards (bob stake) even when there is no rewards distributed + vm.startPrank(alice); + //delegate.unstake(aliceStakeId, 0); + uint256 aliceRewards = delegate.claimRewards(0); + vm.stopPrank(); + + uint256 aliceBalanceAfterAttack = govToken.balanceOf(alice); + + // attack should not be profitable for alice + assertGtDecimal( + bobAmount + 1, // amount alice has spend in total + aliceBalanceAfterAttack, + 1e18, + "Alice receive more than expend for the attack" + ); + + // as previewWithdraw rounds up, someone needs to stake again to have a dSHU total supply > 1 + // so bob can unstake + _mintGovToken(bob, aliceRewards + 10e18); + _stake(bob, keyper, aliceRewards + 10e18); + + vm.prank(bob); + delegate.unstake(bobStakeId, 0); + + uint256 bobBalanceAfterAttack = govToken.balanceOf(bob); + + // Alice earn less than bob + assertGt( + bobBalanceAfterAttack, + aliceBalanceAfterAttack, + "Alice earn more than Bob after the attack" + ); + } } diff --git a/test/Staking.t.sol b/test/Staking.t.sol index 520f4e0..fad6ef2 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -1259,7 +1259,7 @@ contract Unstake is StakingTest { _jumpAhead(_jump); uint256 unstakeAmount = _amount - MIN_STAKE; - uint256 shares = staking.previewWithdraw(unstakeAmount); + uint256 shares = staking.exposed_previewWithdraw(unstakeAmount); vm.expectEmit(); emit Staking.Unstaked(_depositor, unstakeAmount, shares); @@ -1589,7 +1589,6 @@ contract OwnableFunctions is StakingTest { ); vm.expectEmit(); - emit BaseStaking.NewRewardsDistributor(_newRewardsDistributor); staking.setRewardsDistributor(_newRewardsDistributor); assertEq( diff --git a/test/helpers/DelegateStakingHarness.sol b/test/helpers/DelegateStakingHarness.sol index 738570b..4ad1103 100644 --- a/test/helpers/DelegateStakingHarness.sol +++ b/test/helpers/DelegateStakingHarness.sol @@ -7,4 +7,10 @@ contract DelegateStakingHarness is DelegateStaking { function exposed_nextStakeId() external view returns (uint256) { return nextStakeId; } + + function exposed_previewWithdraw( + uint256 amount + ) external view returns (uint256) { + return _previewWithdraw(amount); + } } diff --git a/test/helpers/StakingHarness.sol b/test/helpers/StakingHarness.sol index 81e44a8..0a99125 100644 --- a/test/helpers/StakingHarness.sol +++ b/test/helpers/StakingHarness.sol @@ -14,4 +14,10 @@ contract StakingHarness is Staking { ) external view virtual returns (uint256) { return _maxWithdraw(keyper, unlockedAmount); } + + function exposed_previewWithdraw( + uint256 amount + ) external view returns (uint256) { + return _previewWithdraw(amount); + } } From cccbe737c57df6e273398f000a8e387aa5b8b060 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sat, 3 Aug 2024 11:14:08 -0300 Subject: [PATCH 02/17] improve vm.assume --- src/BaseStaking.sol | 7 ++++ test/DelegateStaking.t.sol | 3 +- test/Staking.t.sol | 76 -------------------------------------- 3 files changed, 9 insertions(+), 77 deletions(-) diff --git a/src/BaseStaking.sol b/src/BaseStaking.sol index 005a280..0b41806 100644 --- a/src/BaseStaking.sol +++ b/src/BaseStaking.sol @@ -181,6 +181,7 @@ abstract contract BaseStaking is OwnableUpgradeable, ERC20VotesUpgradeable { uint256 assets ) public view virtual returns (uint256) { // sum + 1 on both sides to prevent donation attack + // this is the same as OZ ERC4626 prevetion to inflation attack with decimal offset = 0 return assets.mulDivDown(totalSupply() + 1, _totalAssets() + 1); } @@ -190,6 +191,7 @@ abstract contract BaseStaking is OwnableUpgradeable, ERC20VotesUpgradeable { uint256 shares ) public view virtual returns (uint256) { // sum + 1 on both sides to prevent donation attack + // this is the same as OZ ERC4626 prevetion to inflation attack with decimal offset = 0 return shares.mulDivDown(_totalAssets() + 1, totalSupply() + 1); } @@ -215,6 +217,10 @@ abstract contract BaseStaking is OwnableUpgradeable, ERC20VotesUpgradeable { // Calculate the amount of shares to mint uint256 shares = convertToShares(amount); + // Shares may be 0 if a first deposit donation attack occurs. Even it will not profitable for the attacker, as he will spend more tokens than he will get back + // this attack can still be employed to make a DDOS attack against a specific user. Although the targeted user will still be able to withdraw the SHU, this will only hold true if someone mints to increase the total supply. + require(shares > 0, "Shares must be greater than 0"); + // Update the total locked amount totalLocked[user] += amount; @@ -253,6 +259,7 @@ abstract contract BaseStaking is OwnableUpgradeable, ERC20VotesUpgradeable { /// @param assets The amount of assets function _previewWithdraw(uint256 assets) internal view returns (uint256) { // sum + 1 on both sides to prevent donation attack + // this is the same as OZ ERC4626 prevetion to inflation attack with decimal offset = 0 return assets.mulDivUp(totalSupply() + 1, _totalAssets() + 1); } diff --git a/test/DelegateStaking.t.sol b/test/DelegateStaking.t.sol index 79cf8ad..ecd1330 100644 --- a/test/DelegateStaking.t.sol +++ b/test/DelegateStaking.t.sol @@ -138,7 +138,8 @@ contract DelegateStakingTest is Test { _user != address(this) && _user != address(delegate) && _user != ProxyUtils.getAdminAddress(address(delegate)) && - _user != address(rewardsDistributor) + _user != address(rewardsDistributor) && + _user != address(staking) ); vm.startPrank(_user); diff --git a/test/Staking.t.sol b/test/Staking.t.sol index fad6ef2..07456fc 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -657,81 +657,6 @@ contract Stake is StakingTest { vm.stopPrank(); } - // function test_DonationAttack(address bob, address alice) public { - // uint256 initialStake = MIN_STAKE; - // uint256 donationAmount = MIN_STAKE * 100; - // uint256 bobStake = MIN_STAKE * 100; - - // // first alice mints - // _mintGovToken(alice, initialStake); - // _setKeyper(alice, true); - // _stake(alice, initialStake); - - // assertEq(staking.totalSupply(), initialStake); - - // // simulate donation - // govToken.mint(address(staking), donationAmount); - - // assertEq(staking.totalSupply(), initialStake); - // assertEq( - // govToken.balanceOf(address(staking)), - // initialStake + donationAmount - // ); - - // // bob mints - // _mintGovToken(bob, bobStake); - // _setKeyper(bob, true); - // uint256 bobStakeId = _stake(bob, bobStake); - - // // bob shares - // uint256 bobShares = staking.balanceOf(bob); - // console.log("bob shares", bobShares); - - // //vm.prank(alice); - // // uint256 aliceUnstake = staking.unstake(alice, 1, 0); - // // assertEq(aliceUnstake, initialStake); - - // // alice claim rewards withdrawing donation - // vm.prank(alice); - // uint256 aliceRewards = staking.claimRewards(0); - - // // attacker cost is greater than expected gains - // assertGt( - // donationAmount, - // aliceRewards, - // "Alice receive more than expend for the attack" - // ); - - // _jumpAhead(vm.getBlockTimestamp() + LOCK_PERIOD); - // // bob unstake maximum he can unstake - // uint256 maxBobCanWithdraw = staking.exposed_maxWithdraw(bob, bobStake); - // vm.prank(bob); - // staking.unstake(bob, bobStakeId, maxBobCanWithdraw); - - // uint256 bobBalance = govToken.balanceOf(bob); - // uint256 aliceBalance = govToken.balanceOf(alice); - - // vm.prank(bob); - // uint256 bobRewards = staking.claimRewards(0); - // console.log("bob rewards", bobRewards); - - // // bob lost a small amount maximum lost is 1% - // assertApproxEqRel( - // bobBalance, - // bobStake + bobRewards, - // 0.01e18, - // "Bob lost more than 1%" - // ); - - // // at the end Alice still lost more than bob - // assertGtDecimal( - // donationAmount - aliceRewards, - // bobStake - bobBalance, - // 1e18, - // "Alice receive more than bob" - // ); - // } - function testFuzz_DonationAttackNoRewards( address bob, address alice, @@ -1588,7 +1513,6 @@ contract OwnableFunctions is StakingTest { _newRewardsDistributor != address(govToken) ); - vm.expectEmit(); staking.setRewardsDistributor(_newRewardsDistributor); assertEq( From 907887e450e777270681d1ce4924773789f01d33 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sat, 3 Aug 2024 11:24:22 -0300 Subject: [PATCH 03/17] skip IntegrationTests from coverage --- .github/workflows/ci.yml | 3 +- test/DelegateStaking.t.sol | 80 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4ca28d4..ad8b0f9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,7 +59,8 @@ jobs: )" >> $GITHUB_ENV - name: Run coverage - run: forge coverage --report summary --report lcov --ir-minimum + run: forge coverage --report summary --report lcov --ir-minimum \ + --nmc IntegrationTest # 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/DelegateStaking.t.sol b/test/DelegateStaking.t.sol index ecd1330..29ed6fd 100644 --- a/test/DelegateStaking.t.sol +++ b/test/DelegateStaking.t.sol @@ -711,4 +711,84 @@ contract Stake is DelegateStakingTest { "Alice earn more than Bob after the attack" ); } + + function testFuzz_KeyperCanDelegateToHimself( + address _keyper, + uint256 _amount + ) public { + _amount = _boundToRealisticStake(_amount); + + _mintGovToken(_keyper, _amount); + _setKeyper(_keyper, true); + + uint256 stakeId = _stake(_keyper, _keyper, _amount); + + (address keyper, , , ) = delegate.stakes(stakeId); + + assertEq(keyper, _keyper, "Wrong keyper"); + } +} + +contract ClaimRewards is DelegateStakingTest { + function testFuzz_UpdateStakerGovTokenBalanceWhenClaimingRewards( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundRealisticTimeAhead(_jump); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + uint256 stakeId = _stake(_depositor, _keyper, _amount); + + _jumpAhead(_jump); + + vm.startPrank(_depositor); + uint256 rewards = delegate.claimRewards(0); + + uint256 expectedRewards = REWARD_RATE * (_jump); + + // need to accept a small error due to the donation attack prevention + assertApproxEqAbs( + govToken.balanceOf(_depositor), + expectedRewards, + 1e18, + "Wrong balance" + ); + } + + function testFuzz_GovTokenBalanceUnchangedWhenClaimingRewardsOnlyStaker( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundRealisticTimeAhead(_jump); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + _stake(_depositor, _keyper, _amount); + + uint256 contractBalanceBefore = govToken.balanceOf(address(delegate)); + + _jumpAhead(_jump); + + vm.prank(_depositor); + delegate.claimRewards(0); + + uint256 contractBalanceAfter = govToken.balanceOf(address(delegate)); + + // small percentage lost to the vault due to the donation attack prevention + assertApproxEqAbs( + contractBalanceAfter - contractBalanceBefore, + 0, + 1e18, + "Wrong balance" + ); + } } From 77664e47937dfcfffb52e0dfa0fd3cf5506ff4de Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sat, 3 Aug 2024 11:25:23 -0300 Subject: [PATCH 04/17] skip branch from cov --- .github/workflows/ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ad8b0f9..52be327 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -74,7 +74,8 @@ jobs: run: | sudo apt update && sudo apt install -y lcov lcov --remove lcov.info 'test/*' 'script/*' 'src/libraries/*' \ - --output-file lcov.info --rc lcov_branch_coverage=1 + --output-file lcov.info --rc lcov_branch_coverage=0 + # This step posts a detailed coverage report as a comment and deletes previous comments on # each push. The below step is used to fail coverage if the specified coverage threshold is From 83b46704c6b4c1a3cd66e7ab773762fbc5717d12 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sat, 3 Aug 2024 11:28:49 -0300 Subject: [PATCH 05/17] indent ci.yml --- .github/workflows/ci.yml | 100 +++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 52 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 52be327..221cda8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,58 +43,54 @@ jobs: - name: Run tests run: forge test - coverage: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 - - - name: Install Foundry - uses: foundry-rs/foundry-toolchain@v1 - - # https://twitter.com/PaulRBerg/status/1611116650664796166 - - name: Generate fuzz seed with 1 day TTL - run: > - echo "FOUNDRY_FUZZ_SEED=$( - echo $(($EPOCHSECONDS - $EPOCHSECONDS % 86400)) - )" >> $GITHUB_ENV - - - name: Run coverage - run: forge coverage --report summary --report lcov --ir-minimum \ - --nmc IntegrationTest - - # 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, - # to include coverage in all directories, comment out this step. Note that because this - # filtering applies to the lcov file, the summary table generated in the previous step will - # still include all files and directories. - # Disable branch coverage temporarily due to https://github.com/foundry-rs/foundry/issues/8279 - # The `--rc lcov_branch_coverage=1` part keeps branch info in the filtered report, since lcov - # defaults to removing branch info. - - name: Filter directories - run: | - sudo apt update && sudo apt install -y lcov - lcov --remove lcov.info 'test/*' 'script/*' 'src/libraries/*' \ - --output-file lcov.info --rc lcov_branch_coverage=0 - - - # This step posts a detailed coverage report as a comment and deletes previous comments on - # each push. The below step is used to fail coverage if the specified coverage threshold is - # not met. The below step can post a comment (when it's `github-token` is specified) but it's - # not as useful, and this action cannot fail CI based on a minimum coverage threshold, which - # is why we use both in this way. - - name: Post coverage report - if: github.event_name == 'pull_request' # This action fails when ran outside of a pull request. - uses: romeovs/lcov-reporter-action@v0.3.1 - with: - delete-old-comments: true - lcov-file: ./lcov.info - github-token: ${{ secrets.GITHUB_TOKEN }} # Adds a coverage summary comment to the PR. - - - name: Verify minimum coverage - uses: zgosalvez/github-actions-report-lcov@v2 - with: - coverage-files: ./lcov.info - minimum-coverage: 97 +coverage: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + + - name: Install Foundry + uses: foundry-rs/foundry-toolchain@v1 + + # https://twitter.com/PaulRBerg/status/1611116650664796166 + - name: Generate fuzz seed with 1 day TTL + run: | + echo "FOUNDRY_FUZZ_SEED=$(( $(date +%s) - $(date +%s) % 86400 ))" >> $GITHUB_ENV + + - name: Run coverage + run: | + forge coverage --report summary --report lcov --nmc IntegrationTest --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, + # to include coverage in all directories, comment out this step. Note that because this + # filtering applies to the lcov file, the summary table generated in the previous step will + # still include all files and directories. + # Disable branch coverage temporarily due to https://github.com/foundry-rs/foundry/issues/8279 + # The `--rc lcov_branch_coverage=1` part keeps branch info in the filtered report, since lcov + # defaults to removing branch info. + - name: Filter directories + run: | + sudo apt update && sudo apt install -y lcov + lcov --remove lcov.info 'test/*' 'script/*' 'src/libraries/*' --output-file lcov.info --rc lcov_branch_coverage=0 + + # This step posts a detailed coverage report as a comment and deletes previous comments on + # each push. The below step is used to fail coverage if the specified coverage threshold is + # not met. The below step can post a comment (when it's `github-token` is specified) but it's + # not as useful, and this action cannot fail CI based on a minimum coverage threshold, which + # is why we use both in this way. + - name: Post coverage report + if: github.event_name == 'pull_request' # This action fails when ran outside of a pull request. + uses: romeovs/lcov-reporter-action@v0.3.1 + with: + delete-old-comments: true + lcov-file: ./lcov.info + github-token: ${{ secrets.GITHUB_TOKEN }} # Adds a coverage summary comment to the PR. + + - name: Verify minimum coverage + uses: zgosalvez/github-actions-report-lcov@v2 + with: + coverage-files: ./lcov.info + minimum-coverage: 97 lint: runs-on: ubuntu-latest From 204491c99608f4f825f9b49395e3c5ec5c156c2e Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sat, 3 Aug 2024 11:30:53 -0300 Subject: [PATCH 06/17] indent ci --- .github/workflows/ci.yml | 96 ++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 221cda8..4d1c650 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,54 +43,54 @@ jobs: - name: Run tests run: forge test -coverage: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 - - - name: Install Foundry - uses: foundry-rs/foundry-toolchain@v1 - - # https://twitter.com/PaulRBerg/status/1611116650664796166 - - name: Generate fuzz seed with 1 day TTL - run: | - echo "FOUNDRY_FUZZ_SEED=$(( $(date +%s) - $(date +%s) % 86400 ))" >> $GITHUB_ENV - - - name: Run coverage - run: | - forge coverage --report summary --report lcov --nmc IntegrationTest --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, - # to include coverage in all directories, comment out this step. Note that because this - # filtering applies to the lcov file, the summary table generated in the previous step will - # still include all files and directories. - # Disable branch coverage temporarily due to https://github.com/foundry-rs/foundry/issues/8279 - # The `--rc lcov_branch_coverage=1` part keeps branch info in the filtered report, since lcov - # defaults to removing branch info. - - name: Filter directories - run: | - sudo apt update && sudo apt install -y lcov - lcov --remove lcov.info 'test/*' 'script/*' 'src/libraries/*' --output-file lcov.info --rc lcov_branch_coverage=0 - - # This step posts a detailed coverage report as a comment and deletes previous comments on - # each push. The below step is used to fail coverage if the specified coverage threshold is - # not met. The below step can post a comment (when it's `github-token` is specified) but it's - # not as useful, and this action cannot fail CI based on a minimum coverage threshold, which - # is why we use both in this way. - - name: Post coverage report - if: github.event_name == 'pull_request' # This action fails when ran outside of a pull request. - uses: romeovs/lcov-reporter-action@v0.3.1 - with: - delete-old-comments: true - lcov-file: ./lcov.info - github-token: ${{ secrets.GITHUB_TOKEN }} # Adds a coverage summary comment to the PR. - - - name: Verify minimum coverage - uses: zgosalvez/github-actions-report-lcov@v2 - with: - coverage-files: ./lcov.info - minimum-coverage: 97 + coverage: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + + - name: Install Foundry + uses: foundry-rs/foundry-toolchain@v1 + + # https://twitter.com/PaulRBerg/status/1611116650664796166 + - name: Generate fuzz seed with 1 day TTL + run: | + echo "FOUNDRY_FUZZ_SEED=$(( $(date +%s) - $(date +%s) % 86400 ))" >> $GITHUB_ENV + + - name: Run coverage + run: | + forge coverage --report summary --report lcov --nmc IntegrationTest --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, + # to include coverage in all directories, comment out this step. Note that because this + # filtering applies to the lcov file, the summary table generated in the previous step will + # still include all files and directories. + # Disable branch coverage temporarily due to https://github.com/foundry-rs/foundry/issues/8279 + # The `--rc lcov_branch_coverage=1` part keeps branch info in the filtered report, since lcov + # defaults to removing branch info. + - name: Filter directories + run: | + sudo apt update && sudo apt install -y lcov + lcov --remove lcov.info 'test/*' 'script/*' 'src/libraries/*' --output-file lcov.info --rc lcov_branch_coverage=0 + + # This step posts a detailed coverage report as a comment and deletes previous comments on + # each push. The below step is used to fail coverage if the specified coverage threshold is + # not met. The below step can post a comment (when it's `github-token` is specified) but it's + # not as useful, and this action cannot fail CI based on a minimum coverage threshold, which + # is why we use both in this way. + - name: Post coverage report + if: github.event_name == 'pull_request' # This action fails when ran outside of a pull request. + uses: romeovs/lcov-reporter-action@v0.3.1 + with: + delete-old-comments: true + lcov-file: ./lcov.info + github-token: ${{ secrets.GITHUB_TOKEN }} # Adds a coverage summary comment to the PR. + + - name: Verify minimum coverage + uses: zgosalvez/github-actions-report-lcov@v2 + with: + coverage-files: ./lcov.info + minimum-coverage: 97 lint: runs-on: ubuntu-latest From d1e5745f04087f8861d59d52c88bd327103871a4 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sat, 3 Aug 2024 11:47:23 -0300 Subject: [PATCH 07/17] checkpoint --- .github/workflows/ci.yml | 64 +++++++++++++++++++------------------- test/DelegateStaking.t.sol | 1 + test/Staking.t.sol | 2 +- 3 files changed, 34 insertions(+), 33 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4d1c650..eb694c9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,35 +124,35 @@ jobs: echo "✅ Passed or warnings found" >> $GITHUB_STEP_SUMMARY 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" - - - 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 + # 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" + + # - 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 diff --git a/test/DelegateStaking.t.sol b/test/DelegateStaking.t.sol index 29ed6fd..4dc1a15 100644 --- a/test/DelegateStaking.t.sol +++ b/test/DelegateStaking.t.sol @@ -454,6 +454,7 @@ contract Stake is DelegateStakingTest { uint256 _amount, uint256 _jump ) public { + vm.assume(_depositor1 != _depositor2); _amount = _boundToRealisticStake(_amount); _jump = _boundRealisticTimeAhead(_jump); diff --git a/test/Staking.t.sol b/test/Staking.t.sol index 07456fc..1b8bdd5 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -844,7 +844,7 @@ contract ClaimRewards is StakingTest { assertApproxEqAbs(rewards, expectedRewards, 1e18, "Wrong rewards"); } - function testFuzz_claimRewardBurnShares( + function testFuzz_ClaimRewardBurnShares( address _depositor, uint256 _amount, uint256 _jump From 28ccad1b6499c01e925320ad80a91ced28465e89 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sat, 3 Aug 2024 11:59:36 -0300 Subject: [PATCH 08/17] vm.assume(_depositor1 != _depositor2) --- test/DelegateStaking.t.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/test/DelegateStaking.t.sol b/test/DelegateStaking.t.sol index 4dc1a15..5e87f41 100644 --- a/test/DelegateStaking.t.sol +++ b/test/DelegateStaking.t.sol @@ -423,6 +423,7 @@ contract Stake is DelegateStakingTest { address _depositor2, uint256 _amount ) public { + vm.assume(_depositor1 != _depositor2); _amount = _boundToRealisticStake(_amount); _mintGovToken(_depositor1, _amount); From f8be7a169b655b522d71a51ff553d8e1f7503011 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sat, 3 Aug 2024 13:37:15 -0300 Subject: [PATCH 09/17] claim rewards tests --- src/Staking.sol | 5 -- test/DelegateStaking.t.sol | 126 +++++++++++++++++++++++++++++++++++++ test/Staking.t.sol | 4 +- 3 files changed, 129 insertions(+), 6 deletions(-) diff --git a/src/Staking.sol b/src/Staking.sol index a571508..a69c373 100644 --- a/src/Staking.sol +++ b/src/Staking.sol @@ -115,11 +115,6 @@ contract Staking is BaseStaking { _; } - /// @notice Ensure logic contract is unusable - constructor() { - _disableInitializers(); - } - /// @notice Initialize the contract /// @param _owner The owner of the contract, i.e. the DAO contract address /// @param _stakingToken The address of the staking token, i.e. SHU diff --git a/test/DelegateStaking.t.sol b/test/DelegateStaking.t.sol index 5e87f41..3f4158f 100644 --- a/test/DelegateStaking.t.sol +++ b/test/DelegateStaking.t.sol @@ -7,6 +7,7 @@ import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IER import {FixedPointMathLib} from "src/libraries/FixedPointMathLib.sol"; import {Staking} from "src/Staking.sol"; +import {BaseStaking} from "src/BaseStaking.sol"; import {DelegateStaking} from "src/DelegateStaking.sol"; import {RewardsDistributor} from "src/RewardsDistributor.sol"; import {IRewardsDistributor} from "src/interfaces/IRewardsDistributor.sol"; @@ -793,4 +794,129 @@ contract ClaimRewards is DelegateStakingTest { "Wrong balance" ); } + + function testFuzz_EmitRewardsClaimedEventWhenClaimingRewards( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundRealisticTimeAhead(_jump); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + _stake(_depositor, _keyper, _amount); + + _jumpAhead(_jump); + + vm.expectEmit(true, true, false, false); + emit BaseStaking.RewardsClaimed(_depositor, REWARD_RATE * _jump); + + vm.prank(_depositor); + delegate.claimRewards(0); + } + + function testFuzz_ClaimAllRewardsOnlyStaker( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundRealisticTimeAhead(_jump); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + _stake(_depositor, _keyper, _amount); + + _jumpAhead(_jump); + + vm.prank(_depositor); + uint256 rewards = delegate.claimRewards(0); + + uint256 expectedRewards = REWARD_RATE * _jump; + + // need to accept a small error due to the donation attack prevention + assertApproxEqAbs(rewards, expectedRewards, 1e18, "Wrong rewards"); + } + + function testFuzz_ClaimRewardBurnShares( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundRealisticTimeAhead(_jump); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + _stake(_depositor, _keyper, _amount); + + uint256 sharesBefore = delegate.balanceOf(_depositor); + + _jumpAhead(_jump); + + uint256 expectedRewards = REWARD_RATE * _jump; + + uint256 burnShares = _previewWithdrawIncludeRewardsDistributed( + expectedRewards, + expectedRewards + ); + + vm.prank(_depositor); + delegate.claimRewards(0); + + uint256 sharesAfter = delegate.balanceOf(_depositor); + + // need to accept a small error due to the donation attack prevention + assertApproxEqAbs( + sharesBefore - sharesAfter, + burnShares, + 1, + "Wrong shares burned" + ); + } + + function testFuzz_UpdateTotalSupplyWhenClaimingRewards( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundRealisticTimeAhead(_jump); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + _stake(_depositor, _keyper, _amount); + + uint256 totalSupplyBefore = delegate.totalSupply(); + + _jumpAhead(_jump); + + uint256 expectedRewards = REWARD_RATE * _jump; + + uint256 burnShares = _previewWithdrawIncludeRewardsDistributed( + expectedRewards, + expectedRewards + ); + + vm.prank(_depositor); + delegate.claimRewards(0); + + uint256 totalSupplyAfter = delegate.totalSupply(); + + assertApproxEqAbs( + totalSupplyAfter, + totalSupplyBefore - burnShares, + 1, + "Wrong total supply" + ); + } } diff --git a/test/Staking.t.sol b/test/Staking.t.sol index 1b8bdd5..a8573f6 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -4,9 +4,10 @@ pragma solidity 0.8.26; import "@forge-std/Test.sol"; import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; -import {FixedPointMathLib} from "src/libraries/FixedPointMathLib.sol"; import {TransparentUpgradeableProxy, ITransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; + +import {FixedPointMathLib} from "src/libraries/FixedPointMathLib.sol"; import {Staking} from "src/Staking.sol"; import {BaseStaking} from "src/BaseStaking.sol"; import {RewardsDistributor} from "src/RewardsDistributor.sol"; @@ -439,6 +440,7 @@ contract Stake is StakingTest { uint256 _amount, uint256 _jump ) public { + vm.assume(_depositor1 != _depositor2); _amount = _boundToRealisticStake(_amount); _jump = _boundRealisticTimeAhead(_jump); From 60d90e335286d5074183c0fd7c047d18fecea69c Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sat, 3 Aug 2024 15:28:27 -0300 Subject: [PATCH 10/17] coverage increase --- src/BaseStaking.sol | 3 - src/DelegateStaking.sol | 3 + src/Staking.sol | 2 + test/DelegateStaking.t.sol | 192 ++++++++++++++++++++++++++++++++++++- test/Staking.t.sol | 4 +- 5 files changed, 196 insertions(+), 8 deletions(-) diff --git a/src/BaseStaking.sol b/src/BaseStaking.sol index 0b41806..dd7b888 100644 --- a/src/BaseStaking.sol +++ b/src/BaseStaking.sol @@ -71,9 +71,6 @@ abstract contract BaseStaking is OwnableUpgradeable, ERC20VotesUpgradeable { /// @notice Thrown when transfer/tranferFrom is called error TransferDisabled(); - /// @notice Thrown when a user has no shares - error UserHasNoShares(); - /// @notice Thrown when a user try to claim rewards but has no rewards to /// claim error NoRewardsToClaim(); diff --git a/src/DelegateStaking.sol b/src/DelegateStaking.sol index 8ea67c6..ce2cad8 100644 --- a/src/DelegateStaking.sol +++ b/src/DelegateStaking.sol @@ -89,6 +89,9 @@ contract DelegateStaking is BaseStaking { ERRORS //////////////////////////////////////////////////////////////*/ + /// @notice Thrown when a user has no shares + error UserHasNoShares(); + /// @notice Trown when amount is zero error ZeroAmount(); diff --git a/src/Staking.sol b/src/Staking.sol index a69c373..cbe58cd 100644 --- a/src/Staking.sol +++ b/src/Staking.sol @@ -84,6 +84,8 @@ contract Staking is BaseStaking { /*////////////////////////////////////////////////////////////// ERRORS //////////////////////////////////////////////////////////////*/ + /// @notice Thrown when a user has no shares + error UserHasNoShares(); /// @notice Thrown when a non-keyper attempts a call for which only keypers are allowed error OnlyKeyper(); diff --git a/test/DelegateStaking.t.sol b/test/DelegateStaking.t.sol index 3f4158f..220f19f 100644 --- a/test/DelegateStaking.t.sol +++ b/test/DelegateStaking.t.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.26; import "@forge-std/Test.sol"; +import {console} from "@forge-std/console.sol"; import {TransparentUpgradeableProxy, ITransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import {FixedPointMathLib} from "src/libraries/FixedPointMathLib.sol"; @@ -42,6 +43,7 @@ contract DelegateStakingTest is Test { // deploy staking address stakingImpl = address(new Staking()); + vm.label(stakingImpl, "stakingImpl"); staking = Staking( address( @@ -59,13 +61,14 @@ contract DelegateStakingTest is Test { ); address delegateImpl = address(new DelegateStakingHarness()); + vm.label(delegateImpl, "delegateImpl"); delegate = DelegateStakingHarness( address( new TransparentUpgradeableProxy(delegateImpl, address(this), "") ) ); - vm.label(address(delegate), "staking"); + vm.label(address(delegate), "delegate"); delegate.initialize( address(this), // owner @@ -745,12 +748,12 @@ contract ClaimRewards is DelegateStakingTest { _mintGovToken(_depositor, _amount); _setKeyper(_keyper, true); - uint256 stakeId = _stake(_depositor, _keyper, _amount); + _stake(_depositor, _keyper, _amount); _jumpAhead(_jump); vm.startPrank(_depositor); - uint256 rewards = delegate.claimRewards(0); + delegate.claimRewards(0); uint256 expectedRewards = REWARD_RATE * (_jump); @@ -919,4 +922,187 @@ contract ClaimRewards is DelegateStakingTest { "Wrong total supply" ); } + + function testFuzz_Depositor1GetsMoreRewardsThanDepositor2WhenStakingFirst( + address _keyper, + address _depositor1, + address _depositor2, + uint256 _amount, + uint256 _jump1, + uint256 _jump2 + ) public { + _amount = _boundToRealisticStake(_amount); + _jump1 = _boundRealisticTimeAhead(_jump1); + _jump2 = _boundRealisticTimeAhead(_jump2); + + vm.assume(_depositor1 != _depositor2); + + _mintGovToken(_depositor1, _amount); + _mintGovToken(_depositor2, _amount); + + _setKeyper(_keyper, true); + + _stake(_depositor1, _keyper, _amount); + + _jumpAhead(_jump1); + + _stake(_depositor2, _keyper, _amount); + + _jumpAhead(_jump2); + + vm.prank(_depositor1); + uint256 rewards1 = delegate.claimRewards(0); + + vm.prank(_depositor2); + uint256 rewards2 = delegate.claimRewards(0); + + assertGt(rewards1, rewards2, "Wrong rewards"); + } + + function testFuzz_DepositorsGetApproxSameRewardAmountWhenStakingSameAmountInSameBlock( + address _keyper, + address _depositor1, + address _depositor2, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundRealisticTimeAhead(_jump); + + vm.assume(_depositor1 != _depositor2); + + _mintGovToken(_depositor1, _amount); + _mintGovToken(_depositor2, _amount); + + _setKeyper(_keyper, true); + + _stake(_depositor1, _keyper, _amount); + + _stake(_depositor2, _keyper, _amount); + + _jumpAhead(_jump); + + vm.prank(_depositor1); + uint256 rewards1 = delegate.claimRewards(0); + + vm.prank(_depositor2); + uint256 rewards2 = delegate.claimRewards(0); + + assertApproxEqAbs(rewards1, rewards2, 1e18, "Wrong rewards"); + } + + function testFuzz_DepositorGetExactSpecifiedAmountWhenClaimingRewards( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundRealisticTimeAhead(_jump); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + _stake(_depositor, _keyper, _amount); + + _jumpAhead(_jump); + + uint256 expectedRewards = REWARD_RATE * _jump; + vm.prank(_depositor); + uint256 rewards = delegate.claimRewards(expectedRewards / 2); + + assertEq(rewards, expectedRewards / 2, "Wrong rewards"); + } + + function testFuzz_OnlyBurnTheCorrespondedAmountOfSharesSpecifiedWhenClaimingRewards( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundRealisticTimeAhead(_jump); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + _stake(_depositor, _keyper, _amount); + + uint256 sharesBefore = delegate.balanceOf(_depositor); + + _jumpAhead(_jump); + + uint256 expectedRewards = REWARD_RATE * _jump; + uint256 burnShares = _previewWithdrawIncludeRewardsDistributed( + expectedRewards / 2, + expectedRewards + ); + + vm.prank(_depositor); + delegate.claimRewards(expectedRewards / 2); + + uint256 sharesAfter = delegate.balanceOf(_depositor); + + assertEq(sharesBefore - sharesAfter, burnShares, "Wrong shares burned"); + } + + function testFuzz_RevertIf_NoRewardsToClaim( + address _keyper, + address _depositor, + uint256 _amount + ) public { + _amount = _boundToRealisticStake(_amount); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + _stake(_depositor, _keyper, _amount); + + vm.prank(_depositor); + vm.expectRevert(BaseStaking.NoRewardsToClaim.selector); + delegate.claimRewards(0); + } + + function testFuzz_RevertIf_UserHasNoShares(address _depositor) public { + vm.assume( + _depositor != address(0) && + _depositor != ProxyUtils.getAdminAddress(address(staking)) + ); + + vm.prank(_depositor); + vm.expectRevert(DelegateStaking.UserHasNoShares.selector); + staking.claimRewards(0); + } + + function testFuzz_RevertIf_NoRewardsToClaimForThatUser( + address _keyper, + address _depositor1, + address _depositor2, + uint256 _amount1, + uint256 _amount2, + uint256 _jump + ) public { + _amount1 = _boundToRealisticStake(_amount1); + _amount2 = _boundToRealisticStake(_amount2); + _jump = _boundRealisticTimeAhead(_jump); + + vm.assume(_depositor1 != _depositor2); + + _mintGovToken(_depositor1, _amount1); + _mintGovToken(_depositor2, _amount2); + + _setKeyper(_keyper, true); + + _stake(_depositor1, _keyper, _amount1); + + _jumpAhead(_jump); + + _stake(_depositor2, _keyper, _amount2); + + vm.prank(_depositor2); + vm.expectRevert(BaseStaking.NoRewardsToClaim.selector); + delegate.claimRewards(0); + } } + +contract Unstake is DelegateStaking {} diff --git a/test/Staking.t.sol b/test/Staking.t.sol index a8573f6..a5d85bd 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -1072,7 +1072,7 @@ contract ClaimRewards is StakingTest { ); vm.prank(_depositor); - vm.expectRevert(BaseStaking.UserHasNoShares.selector); + vm.expectRevert(Staking.UserHasNoShares.selector); staking.claimRewards(0); } @@ -1640,7 +1640,7 @@ contract ViewFunctions is StakingTest { function testFuzz_Revertif_MaxWithdrawDepositorHasNoStakes( address _depositor ) public { - vm.expectRevert(BaseStaking.UserHasNoShares.selector); + vm.expectRevert(Staking.UserHasNoShares.selector); staking.maxWithdraw(_depositor); } From efabd5fa150aa30f3696049cddc4ce336488ea7b Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Tue, 6 Aug 2024 11:05:36 -0300 Subject: [PATCH 11/17] unstake --- src/DelegateStaking.sol | 2 +- src/Staking.sol | 6 +- test/DelegateStaking.t.sol | 305 ++++++++++++++++++++++++++++++++++++- test/Staking.t.sol | 37 +++-- 4 files changed, 335 insertions(+), 15 deletions(-) diff --git a/src/DelegateStaking.sol b/src/DelegateStaking.sol index ce2cad8..5e4abe9 100644 --- a/src/DelegateStaking.sol +++ b/src/DelegateStaking.sol @@ -186,7 +186,7 @@ contract DelegateStaking is BaseStaking { function unstake( uint256 stakeId, uint256 _amount - ) external returns (uint256 amount) { + ) external updateRewards returns (uint256 amount) { address user = msg.sender; require(userStakes[user].contains(stakeId), StakeDoesNotBelongToUser()); Stake memory userStake = stakes[stakeId]; diff --git a/src/Staking.sol b/src/Staking.sol index cbe58cd..ccf9032 100644 --- a/src/Staking.sol +++ b/src/Staking.sol @@ -99,7 +99,7 @@ contract Staking is BaseStaking { /// @notice Thrown when someone try to unstake a stake that doesn't belong /// to the keyper in question - error StakeDoesNotBelongToKeyper(); + error StakeDoesNotBelongToUser(); /// @notice Thrown when someone try to unstake a stake that doesn't exist error StakeDoesNotExist(); @@ -208,10 +208,10 @@ contract Staking is BaseStaking { address keyper, uint256 stakeId, uint256 _amount - ) external returns (uint256 amount) { + ) external updateRewards returns (uint256 amount) { require( userStakes[keyper].contains(stakeId), - StakeDoesNotBelongToKeyper() + StakeDoesNotBelongToUser() ); Stake memory keyperStake = stakes[stakeId]; diff --git a/test/DelegateStaking.t.sol b/test/DelegateStaking.t.sol index 220f19f..e62d14e 100644 --- a/test/DelegateStaking.t.sol +++ b/test/DelegateStaking.t.sol @@ -1105,4 +1105,307 @@ contract ClaimRewards is DelegateStakingTest { } } -contract Unstake is DelegateStaking {} +contract Unstake is DelegateStakingTest { + function testFuzz_UnstakeUpdateStakerGovTokenBalanceWhenUnstaking( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundUnlockedTime(_jump); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + uint256 stakeId = _stake(_depositor, _keyper, _amount); + + _jumpAhead(_jump); + + vm.prank(_depositor); + delegate.unstake(stakeId, 0); + + assertEq(govToken.balanceOf(_depositor), _amount, "Wrong balance"); + } + + function testFuzz_UpdateTotalSupplyWhenUnstaking( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundUnlockedTime(_jump); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + uint256 stakeId = _stake(_depositor, _keyper, _amount); + + uint256 totalSupplyBefore = delegate.totalSupply(); + + _jumpAhead(_jump); + + uint256 expectedRewards = REWARD_RATE * _jump; + + uint256 sharesToBurn = _previewWithdrawIncludeRewardsDistributed( + _amount, + expectedRewards + ); + + vm.prank(_depositor); + delegate.unstake(stakeId, 0); + + assertEq( + delegate.totalSupply(), + totalSupplyBefore - sharesToBurn, + "Wrong total supply" + ); + + uint256 expectedSharesRemaining = delegate.convertToShares( + expectedRewards + ); + + assertEq(delegate.totalSupply(), expectedSharesRemaining); + } + + function testFuzz_UnstakeShouldNotTransferRewards( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundUnlockedTime(_jump); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + uint256 stakeId = _stake(_depositor, _keyper, _amount); + + _jumpAhead(_jump); + + uint256 expectedRewards = REWARD_RATE * _jump; + + vm.prank(_depositor); + uint256 unstakeAmount = delegate.unstake(stakeId, 0); + + assertEq( + govToken.balanceOf(address(delegate)), + expectedRewards, + "Wrong balance" + ); + assertEq( + govToken.balanceOf(_depositor), + unstakeAmount, + "Wrong balance" + ); + } + + function testFuzz_EmitUnstakeEventWhenUnstaking( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = _boundUnlockedTime(_jump); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + uint256 stakeId = _stake(_depositor, _keyper, _amount); + + _jumpAhead(_jump); + + uint256 shares = _previewWithdrawIncludeRewardsDistributed( + _amount, + REWARD_RATE * _jump + ); + vm.expectEmit(); + emit Staking.Unstaked(_depositor, _amount, shares); + + vm.prank(_depositor); + delegate.unstake(stakeId, 0); + } + + function testFuzz_DepositorHasMultipleStakesUnstakeCorrectStake( + address _keyper, + address _depositor, + uint256 _amount1, + uint256 _amount2, + uint256 _jump + ) public { + _amount1 = _boundToRealisticStake(_amount1); + _amount2 = _boundToRealisticStake(_amount2); + _jump = _boundUnlockedTime(_jump); + + _mintGovToken(_depositor, _amount1 + _amount2); + _setKeyper(_keyper, true); + + uint256 stakeId1 = _stake(_depositor, _keyper, _amount1); + uint256 stakeId2 = _stake(_depositor, _keyper, _amount2); + assertEq(govToken.balanceOf(_depositor), 0, "Wrong balance"); + + _jumpAhead(_jump); + + vm.prank(_depositor); + delegate.unstake(stakeId1, 0); + + assertEq(govToken.balanceOf(_depositor), _amount1, "Wrong balance"); + + vm.prank(_depositor); + delegate.unstake(stakeId2, 0); + + assertEq( + govToken.balanceOf(_depositor), + _amount1 + _amount2, + "Wrong balance" + ); + } + + function testFuzz_UnstakeOnlyAmountSpecified( + address _keyper, + address _depositor, + uint256 _amount1, + uint256 _amount2, + uint256 _jump + ) public { + _amount1 = _boundToRealisticStake(_amount1); + _amount2 = _boundToRealisticStake(_amount2); + _jump = _boundUnlockedTime(_jump); + + vm.assume(_amount1 > _amount2); + _jump = _boundUnlockedTime(_jump); + + _mintGovToken(_depositor, _amount1); + + _setKeyper(_keyper, true); + + uint256 stakeId = _stake(_depositor, _keyper, _amount1); + assertEq(govToken.balanceOf(_depositor), 0, "Wrong balance"); + + _jumpAhead(_jump); + + vm.prank(_depositor); + delegate.unstake(stakeId, _amount2); + + assertEq(govToken.balanceOf(_depositor), _amount2, "Wrong balance"); + + uint256[] memory stakeIds = delegate.getUserStakeIds(_depositor); + assertEq(stakeIds.length, 1, "Wrong stake ids"); + + (, uint256 amount, , ) = delegate.stakes(stakeIds[0]); + + assertEq(amount, _amount1 - _amount2, "Wrong amount"); + } + + function testFuzz_RevertIf_StakeIsStillLocked( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = bound(_jump, vm.getBlockTimestamp(), LOCK_PERIOD); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + uint256 stakeId = _stake(_depositor, _keyper, _amount); + + _jumpAhead(_jump); + + vm.prank(_depositor); + vm.expectRevert(DelegateStaking.StakeIsStillLocked.selector); + delegate.unstake(stakeId, 0); + } + + function testFuzz_RevertIf_StakeIsStillLockedAfterLockPeriodChangedToLessThanCurrent( + address _keyper, + address _depositor, + uint256 _amount, + uint256 _jump + ) public { + _amount = _boundToRealisticStake(_amount); + _jump = bound(_jump, vm.getBlockTimestamp(), LOCK_PERIOD - 1); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + uint256 stakeId = _stake(_depositor, _keyper, _amount); + + delegate.setLockPeriod(_jump); + + _jumpAhead(_jump); + + vm.prank(_depositor); + vm.expectRevert(DelegateStaking.StakeIsStillLocked.selector); + delegate.unstake(stakeId, 0); + } + + function testFuzz_RevertIf_StakeDoesNotBelongToUser( + address _keyper, + address _depositor1, + address _depositor2, + uint256 _amount1 + ) public { + vm.assume(_depositor1 != _depositor2); + vm.assume( + _depositor1 != address(0) && + _depositor1 != ProxyUtils.getAdminAddress(address(delegate)) + ); + vm.assume( + _depositor2 != address(0) && + _depositor2 != ProxyUtils.getAdminAddress(address(delegate)) + ); + _amount1 = _boundToRealisticStake(_amount1); + + _mintGovToken(_depositor1, _amount1); + + _setKeyper(_keyper, true); + + uint256 stakeId = _stake(_depositor1, _keyper, _amount1); + + vm.prank(_depositor2); + vm.expectRevert(DelegateStaking.StakeDoesNotBelongToUser.selector); + delegate.unstake(stakeId, 0); + } + + function testFuzz_RevertIf_AmountGreaterThanStakeAmount( + address _keyper, + address _depositor, + uint256 _amount + ) public { + _amount = _boundToRealisticStake(_amount); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + uint256 stakeId = _stake(_depositor, _keyper, _amount); + + vm.prank(_depositor); + vm.expectRevert(BaseStaking.WithdrawAmountTooHigh.selector); + staking.unstake(stakeId, _amount + 1); + } +} + +contract OwnableFunctions is DelegateStaking { + function testFuzz_setRewardsDistributor( + address _newRewardsDistributor + ) public { + vm.assume( + _newRewardsDistributor != address(0) && + _newRewardsDistributor != address(delegate) && + _newRewardsDistributor != address(govToken) + ); + + delegate.setRewardsDistributor(_newRewardsDistributor); + + assertEq( + address(delegate.rewardsDistributor()), + _newRewardsDistributor, + "Wrong rewards distributor" + ); + } +} diff --git a/test/Staking.t.sol b/test/Staking.t.sol index a5d85bd..f8ae633 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -1186,7 +1186,10 @@ contract Unstake is StakingTest { _jumpAhead(_jump); uint256 unstakeAmount = _amount - MIN_STAKE; - uint256 shares = staking.exposed_previewWithdraw(unstakeAmount); + uint256 shares = _previewWithdrawIncludeRewardsDistributed( + unstakeAmount, + REWARD_RATE * _jump + ); vm.expectEmit(); emit Staking.Unstaked(_depositor, unstakeAmount, shares); @@ -1232,23 +1235,37 @@ contract Unstake is StakingTest { _mintGovToken(_depositor, _amount + MIN_STAKE); _setKeyper(_depositor, true); - _stake(_depositor, MIN_STAKE); - uint256 stakeId = _stake(_depositor, _amount); + uint256 totalSupplyBefore = staking.totalSupply(); + _jumpAhead(_jump); + uint256 expectedRewards = REWARD_RATE * _jump; + uint256 sharesToBurn = _previewWithdrawIncludeRewardsDistributed( + _amount, + expectedRewards + ); + vm.prank(_depositor); staking.unstake(_depositor, stakeId, 0); - uint256 expectedSharesRemaining = staking.convertToShares(MIN_STAKE); + assertEq( + staking.totalSupply(), + totalSupplyBefore - sharesToBurn, + "Wrong total supply" + ); - uint256 totalSupplyAfter = staking.totalSupply(); + uint256 expectedSharesRemaining = staking.convertToShares( + MIN_STAKE + expectedRewards + ); - assertEq( - totalSupplyAfter, + // TODO review this + assertApproxEqRel( + staking.totalSupply(), expectedSharesRemaining, - "Wrong total supply" + 0.1e18, + "Wrong total supply with remaing shares" ); } @@ -1433,7 +1450,7 @@ contract Unstake is StakingTest { staking.unstake(depositor, stakeId, MIN_STAKE); } - function testFuzz_RevertIf_StakeDoesNotBelongToKeyper( + function testFuzz_RevertIf_StakeDoesNotBelongToUser( address _depositor1, address _depositor2, uint256 _amount1 @@ -1456,7 +1473,7 @@ contract Unstake is StakingTest { uint256 stakeId = _stake(_depositor1, _amount1); vm.prank(_depositor2); - vm.expectRevert(Staking.StakeDoesNotBelongToKeyper.selector); + vm.expectRevert(Staking.StakeDoesNotBelongToUser.selector); staking.unstake(_depositor2, stakeId, 0); } From 8dd76f27ad81a5390c34a77f058b62b71d7013d0 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Tue, 6 Aug 2024 11:19:13 -0300 Subject: [PATCH 12/17] ownable functions test --- test/DelegateStaking.t.sol | 106 ++++++++++++++++++++++++++++++++++++- 1 file changed, 104 insertions(+), 2 deletions(-) diff --git a/test/DelegateStaking.t.sol b/test/DelegateStaking.t.sol index e62d14e..014ca4b 100644 --- a/test/DelegateStaking.t.sol +++ b/test/DelegateStaking.t.sol @@ -4,7 +4,9 @@ pragma solidity 0.8.26; import "@forge-std/Test.sol"; import {console} from "@forge-std/console.sol"; import {TransparentUpgradeableProxy, ITransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; +import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; + import {FixedPointMathLib} from "src/libraries/FixedPointMathLib.sol"; import {Staking} from "src/Staking.sol"; @@ -1386,11 +1388,11 @@ contract Unstake is DelegateStakingTest { vm.prank(_depositor); vm.expectRevert(BaseStaking.WithdrawAmountTooHigh.selector); - staking.unstake(stakeId, _amount + 1); + delegate.unstake(stakeId, _amount + 1); } } -contract OwnableFunctions is DelegateStaking { +contract OwnableFunctions is DelegateStakingTest { function testFuzz_setRewardsDistributor( address _newRewardsDistributor ) public { @@ -1408,4 +1410,104 @@ contract OwnableFunctions is DelegateStaking { "Wrong rewards distributor" ); } + + function testFuzz_setLockPeriod(uint256 _newLockPeriod) public { + vm.expectEmit(); + + emit BaseStaking.NewLockPeriod(_newLockPeriod); + + delegate.setLockPeriod(_newLockPeriod); + + assertEq(delegate.lockPeriod(), _newLockPeriod, "Wrong lock period"); + } + + function testFuzz_setStakingContract(address _newStaking) public { + vm.assume( + _newStaking != address(0) && + _newStaking != address(delegate) && + _newStaking != address(govToken) + ); + + vm.expectEmit(); + emit DelegateStaking.NewStakingContract(_newStaking); + delegate.setStakingContract(_newStaking); + + assertEq( + address(delegate.staking()), + _newStaking, + "Wrong staking contract" + ); + } + + function testFuzz_RevertIf_NonOwnerSetRewardsDistributor( + address _newRewardsDistributor, + address _nonOwner + ) public { + vm.assume( + _newRewardsDistributor != address(0) && + _newRewardsDistributor != address(delegate) && + _newRewardsDistributor != address(govToken) + ); + + vm.assume( + _nonOwner != address(0) && + _nonOwner != ProxyUtils.getAdminAddress(address(delegate)) && + _nonOwner != address(this) + ); + + vm.prank(_nonOwner); + vm.expectRevert( + abi.encodeWithSelector( + Ownable.OwnableUnauthorizedAccount.selector, + _nonOwner + ) + ); + delegate.setRewardsDistributor(_newRewardsDistributor); + } + + function testFuzz_RevertIf_NonOwnerSetLockPeriod( + uint256 _newLockPeriod, + address _nonOwner + ) public { + vm.assume( + _nonOwner != address(0) && + _nonOwner != ProxyUtils.getAdminAddress(address(delegate)) && + _nonOwner != address(this) + ); + + vm.prank(_nonOwner); + vm.expectRevert( + abi.encodeWithSelector( + Ownable.OwnableUnauthorizedAccount.selector, + _nonOwner + ) + ); + delegate.setLockPeriod(_newLockPeriod); + } + + function testFuzz_RevertIf_NonOwnerSetStakingContract( + address _newStaking, + address _nonOwner + ) public { + vm.assume( + _newStaking != address(0) && + _newStaking != address(delegate) && + _newStaking != address(govToken) + ); + + vm.assume( + _nonOwner != address(0) && + _nonOwner != ProxyUtils.getAdminAddress(address(delegate)) && + _nonOwner != address(this) + ); + + vm.prank(_nonOwner); + vm.expectRevert( + abi.encodeWithSelector( + Ownable.OwnableUnauthorizedAccount.selector, + _nonOwner + ) + ); + delegate.setStakingContract(_newStaking); + } } From 60d0f54fd012daea34a7cc4b58990c8d28aa331d Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Tue, 6 Aug 2024 11:24:01 -0300 Subject: [PATCH 13/17] view functions --- test/DelegateStaking.t.sol | 51 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/test/DelegateStaking.t.sol b/test/DelegateStaking.t.sol index 014ca4b..98d5ad6 100644 --- a/test/DelegateStaking.t.sol +++ b/test/DelegateStaking.t.sol @@ -1511,3 +1511,54 @@ contract OwnableFunctions is DelegateStakingTest { delegate.setStakingContract(_newStaking); } } + +contract ViewFunctions is DelegateStakingTest { + function testFuzz_Revertif_MaxWithdrawDepositorHasNoStakes( + address _depositor + ) public { + vm.expectRevert(DelegateStaking.UserHasNoShares.selector); + delegate.maxWithdraw(_depositor); + } + + function testFuzz_MaxWithdrawDepositorHasLockedStakeNoRewards( + address _keyper, + address _depositor, + uint256 _amount + ) public { + _amount = _boundToRealisticStake(_amount); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + _stake(_depositor, _keyper, _amount); + + uint256 maxWithdraw = delegate.maxWithdraw(_depositor); + assertEq(maxWithdraw, 0, "Wrong max withdraw"); + } + + function testFuzz_MaxWithdrawDepositorHasLockedStakeAndReward( + address _keyper, + address _depositor1, + uint256 _amount1, + uint256 _jump + ) public { + _amount1 = _boundToRealisticStake(_amount1); + + _jump = _boundUnlockedTime(_jump); + + _mintGovToken(_depositor1, _amount1); + _setKeyper(_keyper, true); + + _stake(_depositor1, _keyper, _amount1); + + _jumpAhead(_jump); + + rewardsDistributor.collectRewardsTo(address(delegate)); + + uint256 rewards = REWARD_RATE * _jump; + + uint256 maxWithdraw = delegate.maxWithdraw(_depositor1); + + assertApproxEqAbs(maxWithdraw, rewards, 0.1e18, "Wrong max withdraw"); + } +} From 2282444c7cef80ac950b5240d3fa58a42b878397 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Wed, 7 Aug 2024 08:21:17 -0300 Subject: [PATCH 14/17] totalDelegated --- src/BaseStaking.sol | 14 +++++++++++--- src/DelegateStaking.sol | 9 +++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/BaseStaking.sol b/src/BaseStaking.sol index dd7b888..f220352 100644 --- a/src/BaseStaking.sol +++ b/src/BaseStaking.sol @@ -78,6 +78,9 @@ abstract contract BaseStaking is OwnableUpgradeable, ERC20VotesUpgradeable { /// @notice Thrown when the argument is the zero address error AddressZero(); + /// @notice Thrown when the amount of shares is 0 + error SharesMustBeGreaterThanZero(); + /*////////////////////////////////////////////////////////////// MODIFIERS //////////////////////////////////////////////////////////////*/ @@ -214,9 +217,14 @@ abstract contract BaseStaking is OwnableUpgradeable, ERC20VotesUpgradeable { // Calculate the amount of shares to mint uint256 shares = convertToShares(amount); - // Shares may be 0 if a first deposit donation attack occurs. Even it will not profitable for the attacker, as he will spend more tokens than he will get back - // this attack can still be employed to make a DDOS attack against a specific user. Although the targeted user will still be able to withdraw the SHU, this will only hold true if someone mints to increase the total supply. - require(shares > 0, "Shares must be greater than 0"); + // A first deposit donation attack may result in shares being 0 if the + // contract has very high assets balance but a very low total supply. + // Although this attack is not profitable for the attacker, as they will + // spend more tokens than they will receive, it can still be used to perform a DDOS attack + // against a specific user. The targeted user can still withdraw their SHU, + // but this is only guaranteed if someone mints to increase the total supply of shares, + // because previewWithdraw rounds up and their shares will be less than the burn amount. + require(shares > 0, SharesMustBeGreaterThanZero()); // Update the total locked amount totalLocked[user] += amount; diff --git a/src/DelegateStaking.sol b/src/DelegateStaking.sol index 5e4abe9..ae98ef7 100644 --- a/src/DelegateStaking.sol +++ b/src/DelegateStaking.sol @@ -67,6 +67,9 @@ contract DelegateStaking is BaseStaking { /// @notice stores the metadata associated with a given stake mapping(uint256 id => Stake _stake) public stakes; + /// @notice stores the amount delegated to a keyper + mapping(address keyper => uint256 totalDelegated) public totalDelegated; + /*////////////////////////////////////////////////////////////// EVENTS //////////////////////////////////////////////////////////////*/ @@ -163,6 +166,9 @@ contract DelegateStaking is BaseStaking { stakes[stakeId].timestamp = block.timestamp; stakes[stakeId].lockPeriod = lockPeriod; + // Increase the keyper total delegated amount + totalDelegated[keyper] += amount; + _deposit(user, amount); emit Staked(user, keyper, amount, lockPeriod); @@ -211,6 +217,9 @@ contract DelegateStaking is BaseStaking { // Decrease the amount from the stake stakes[stakeId].amount -= amount; + // Decrease the total delegated amount + totalDelegated[userStake.keyper] -= amount; + // If the stake is empty, remove it if (stakes[stakeId].amount == 0) { // Remove the stake from the stakes mapping From 2f67e534e397e64387336bbfaa855de6841b55c7 Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Wed, 7 Aug 2024 08:30:22 -0300 Subject: [PATCH 15/17] natspec --- src/DelegateStaking.sol | 7 +++++++ src/Staking.sol | 10 +++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/DelegateStaking.sol b/src/DelegateStaking.sol index ae98ef7..7223987 100644 --- a/src/DelegateStaking.sol +++ b/src/DelegateStaking.sol @@ -25,11 +25,18 @@ interface IStaking { * A user's SHU balance is calculated as: * balanceOf(user) * totalSupply() / totalShares() * + * Staking, unstaking, and claiming rewards are based on shares, not the balance directly. + * This method ensures the balance can change over time without needing too many storage updates. + * * When staking, you must specify a keyper address. This symbolically demonstrates your support * for that keyper. The keyper address must be a valid keyper in the staking contract. * Staking, unstaking, and claiming rewards are based on shares rather than the balance directly. * This method ensures the balance can change over time without needing too many storage updates. * + * @dev SHU tokens transferred into the contract without using the `stake` function will be included + * in the rewards distribution and shared among all stakers. This contract only supports SHU + * tokens. Any non-SHU tokens transferred into the contract will be permanently lost. + * */ contract DelegateStaking is BaseStaking { /*////////////////////////////////////////////////////////////// diff --git a/src/Staking.sol b/src/Staking.sol index ccf9032..7ae6fae 100644 --- a/src/Staking.sol +++ b/src/Staking.sol @@ -27,8 +27,13 @@ import {IRewardsDistributor} from "./interfaces/IRewardsDistributor.sol"; * Please be aware that the contract's Owner can change the minimum stake amount. * If the Owner is compromised, they could set the minimum stake amount very high, * making it impossible for keypers to unstake their SHU. - * The Owner of this contract is the Shutter DAO multisig. By staking SHU, you trust - * the Owner not to set the minimum stake amount to an unreasonably high value. + * The Owner of this contract is the Shutter DAO multisig with a Azorius module. + * By staking SHU, you trust the Owner not to set the minimum stake amount to + * an unreasonably high value. + * + * @dev SHU tokens transferred into the contract without using the `stake` function will be included + * in the rewards distribution and shared among all stakers. This contract only supports SHU + * tokens. Any non-SHU tokens transferred into the contract will be permanently lost. * */ contract Staking is BaseStaking { @@ -230,7 +235,6 @@ contract Staking is BaseStaking { // must be locked for the lock period // If the global lock period is greater than the stake lock period, // the stake must be locked for the stake lock period - uint256 lock = keyperStake.lockPeriod > lockPeriod ? lockPeriod : keyperStake.lockPeriod; From 1aa8bf47fa1ffc40b44264896822febdcd92e6bf Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sun, 11 Aug 2024 11:18:09 -0300 Subject: [PATCH 16/17] fuzzing: view functions --- src/DelegateStaking.sol | 2 - src/Staking.sol | 3 +- test/DelegateStaking.t.sol | 116 ++++++++++++++++++++++++++++++++++ test/RewardsDistributor.t.sol | 1 + test/Staking.t.sol | 2 +- 5 files changed, 119 insertions(+), 5 deletions(-) diff --git a/src/DelegateStaking.sol b/src/DelegateStaking.sol index 7223987..b9e9767 100644 --- a/src/DelegateStaking.sol +++ b/src/DelegateStaking.sol @@ -30,8 +30,6 @@ interface IStaking { * * When staking, you must specify a keyper address. This symbolically demonstrates your support * for that keyper. The keyper address must be a valid keyper in the staking contract. - * Staking, unstaking, and claiming rewards are based on shares rather than the balance directly. - * This method ensures the balance can change over time without needing too many storage updates. * * @dev SHU tokens transferred into the contract without using the `stake` function will be included * in the rewards distribution and shared among all stakers. This contract only supports SHU diff --git a/src/Staking.sol b/src/Staking.sol index 7ae6fae..9124c4f 100644 --- a/src/Staking.sol +++ b/src/Staking.sol @@ -281,7 +281,6 @@ contract Staking is BaseStaking { /// @param _minStake The minimum stake amount function setMinStake(uint256 _minStake) external onlyOwner { minStake = _minStake; - emit NewMinStake(_minStake); } @@ -321,7 +320,7 @@ contract Staking is BaseStaking { /// - if the keyper sSHU balance is less or equal than the minimum /// stake or the total locked amount, the function will return 0 /// @param keyper The keyper address - /// @param unlockedAmount The amount of assets to unlock + /// @param unlockedAmount The amount of unlocked assets /// @return amount The maximum amount of assets that a keyper can withdraw after unlocking a certain amount function _maxWithdraw( address keyper, diff --git a/test/DelegateStaking.t.sol b/test/DelegateStaking.t.sol index 98d5ad6..d055646 100644 --- a/test/DelegateStaking.t.sol +++ b/test/DelegateStaking.t.sol @@ -1561,4 +1561,120 @@ contract ViewFunctions is DelegateStakingTest { assertApproxEqAbs(maxWithdraw, rewards, 0.1e18, "Wrong max withdraw"); } + + function testFuzz_MaxWithdrawDepositorHasMultipleLockedStakes( + address _keyper, + address _depositor, + uint256 _amount1, + uint256 _amount2, + uint256 _jump + ) public { + _amount1 = _boundToRealisticStake(_amount1); + _amount2 = _boundToRealisticStake(_amount2); + _jump = _boundUnlockedTime(_jump); + + _mintGovToken(_depositor, _amount1 + _amount2); + _setKeyper(_keyper, true); + + _stake(_depositor, _keyper, _amount1); + _stake(_depositor, _keyper, _amount2); + + uint256 maxWithdraw = delegate.maxWithdraw(_depositor); + assertEq(maxWithdraw, 0, "Wrong max withdraw"); + } + + function testFuzz_convertToSharesNoSupply(uint256 assets) public view { + assertEq(delegate.convertToShares(assets), assets); + } + + function testFuzz_ConvertToSharesHasSupplySameBlock( + address _keyper, + address _depositor, + uint256 _assets + ) public { + _assets = _boundToRealisticStake(_assets); + + _mintGovToken(_depositor, _assets); + _setKeyper(_keyper, true); + + _stake(_depositor, _keyper, _assets); + + uint256 shares = delegate.convertToShares(_assets); + + assertEq(shares, _assets, "Wrong shares"); + } + + function testFuzz_ConvertToAssetsHasSupplySameBlock( + address _keyper, + address _depositor, + uint256 _assets + ) public { + _assets = _boundToRealisticStake(_assets); + + _mintGovToken(_depositor, _assets); + _setKeyper(_keyper, true); + + _stake(_depositor, _keyper, _assets); + + uint256 shares = delegate.convertToShares(_assets); + uint256 assets = delegate.convertToAssets(shares); + + assertEq(assets, _assets, "Wrong assets"); + } + + function testFuzz_GetUserStakeIds( + address _keyper, + address _depositor, + uint256 _amount1, + uint256 _amount2 + ) public { + _amount1 = _boundToRealisticStake(_amount1); + _amount2 = _boundToRealisticStake(_amount2); + + _mintGovToken(_depositor, _amount1 + _amount2); + _setKeyper(_keyper, true); + + uint256 stakeId1 = _stake(_depositor, _keyper, _amount1); + uint256 stakeId2 = _stake(_depositor, _keyper, _amount2); + + uint256[] memory stakeIds = delegate.getUserStakeIds(_depositor); + + assertEq(stakeIds.length, 2, "Wrong stake ids"); + assertEq(stakeIds[0], stakeId1, "Wrong stake id"); + assertEq(stakeIds[1], stakeId2, "Wrong stake id"); + } +} + +contract Transfer is DelegateStakingTest { + function testFuzz_RevertWith_transferDisabled( + address _from, + address _to, + uint256 _amount + ) public { + _amount = _boundToRealisticStake(_amount); + + _mintGovToken(_from, _amount); + _setKeyper(_from, true); + + _stake(_from, _from, _amount); + + vm.expectRevert(BaseStaking.TransferDisabled.selector); + delegate.transfer(_to, _amount); + } + + function testFuzz_RevertWith_transferFromDisabled( + address _from, + address _to, + uint256 _amount + ) public { + _amount = _boundToRealisticStake(_amount); + + _mintGovToken(_from, _amount); + _setKeyper(_from, true); + + _stake(_from, _from, _amount); + + vm.expectRevert(BaseStaking.TransferDisabled.selector); + delegate.transferFrom(_from, _to, _amount); + } } diff --git a/test/RewardsDistributor.t.sol b/test/RewardsDistributor.t.sol index 778962c..1170d10 100644 --- a/test/RewardsDistributor.t.sol +++ b/test/RewardsDistributor.t.sol @@ -136,6 +136,7 @@ contract OwnableFunctions is RewardsDistributorTest { function testFuzz_RevertIf_SetRewardConfigurationEmissionRateZero( address _receiver ) public { + vm.assume(_receiver != address(0)); vm.expectRevert(RewardsDistributor.EmissionRateZero.selector); rewardsDistributor.setRewardConfiguration(_receiver, 0); } diff --git a/test/Staking.t.sol b/test/Staking.t.sol index f8ae633..29c0765 100644 --- a/test/Staking.t.sol +++ b/test/Staking.t.sol @@ -1771,7 +1771,7 @@ contract ViewFunctions is StakingTest { assertEq(assets, _assets, "Wrong assets"); } - function testFuzz_GetKeyperStakeIds( + function testFuzz_GetUserStakeIds( address _depositor, uint256 _amount1, uint256 _amount2 From 42ead22673482b4de66d254150416dad333e0bbd Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Sun, 11 Aug 2024 11:24:09 -0300 Subject: [PATCH 17/17] missing branche --- test/DelegateStaking.t.sol | 22 +++++++++++++++++++++- test/helpers/DelegateStakingHarness.sol | 7 +++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/test/DelegateStaking.t.sol b/test/DelegateStaking.t.sol index d055646..44a5705 100644 --- a/test/DelegateStaking.t.sol +++ b/test/DelegateStaking.t.sol @@ -1583,7 +1583,7 @@ contract ViewFunctions is DelegateStakingTest { assertEq(maxWithdraw, 0, "Wrong max withdraw"); } - function testFuzz_convertToSharesNoSupply(uint256 assets) public view { + function testFuzz_ConvertToSharesNoSupply(uint256 assets) public view { assertEq(delegate.convertToShares(assets), assets); } @@ -1643,6 +1643,26 @@ contract ViewFunctions is DelegateStakingTest { assertEq(stakeIds[0], stakeId1, "Wrong stake id"); assertEq(stakeIds[1], stakeId2, "Wrong stake id"); } + + function testFuzz_CalculateWithdrawAmountReturnsAmount( + address _keyper, + address _depositor, + uint256 _amount + ) public { + _amount = _boundToRealisticStake(_amount); + + _mintGovToken(_depositor, _amount); + _setKeyper(_keyper, true); + + uint256 stakeId = _stake(_depositor, _keyper, _amount); + + uint256 withdrawAmount = delegate.exposed_calculateWithdrawAmount( + _amount / 2, + _amount + ); + + assertEq(withdrawAmount, _amount / 2, "Wrong withdraw amount"); + } } contract Transfer is DelegateStakingTest { diff --git a/test/helpers/DelegateStakingHarness.sol b/test/helpers/DelegateStakingHarness.sol index 4ad1103..b1348f5 100644 --- a/test/helpers/DelegateStakingHarness.sol +++ b/test/helpers/DelegateStakingHarness.sol @@ -13,4 +13,11 @@ contract DelegateStakingHarness is DelegateStaking { ) external view returns (uint256) { return _previewWithdraw(amount); } + + function exposed_calculateWithdrawAmount( + uint256 _amount, + uint256 _maxWithdrawAmount + ) external view returns (uint256) { + return _calculateWithdrawAmount(_amount, _maxWithdrawAmount); + } }