Merge pull request #15 from blockful-io/staking-test

Unhappy path on Staking tests

.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,
@@ -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/[email protected]"
-          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"
+
+            - name: "Run Slither analysis"
+              uses: "crytic/[email protected]"
+              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

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]

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";

src/Staking.sol

@@ -1,7 +1,6 @@
 // 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";
 import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
 import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
@@ -62,7 +61,7 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable {
     }
 
     /*//////////////////////////////////////////////////////////////
-                             MAPPINGS/ARRAYS
+                                MAPPINGS
     //////////////////////////////////////////////////////////////*/
 
     /// @notice stores the metadata associated with a given stake
@@ -83,23 +82,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 +206,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,76 +269,71 @@ 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,
                 // the stake must be locked for the stake lock period
                 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);
 
         // Burn the shares
         _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 +362,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
@@ -344,7 +377,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);
@@ -418,7 +451,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 +472,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 +493,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 +502,7 @@ contract Staking is ERC20VotesUpgradeable, Ownable2StepUpgradeable {
         address,
         uint256
     ) public pure override returns (bool) {
-        revert("Transfer is disabled");
+        revert TransferDisabled();
     }
 
     /*//////////////////////////////////////////////////////////////

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(