Merge pull request from GHSA-pcvp-3h5m-87rf

cygnusv · web-flow · commit f468a7d8a3aa · 2023-05-12T16:50:21.000+02:00
Advisory fix 1
diff --git a/contracts/staking/IStaking.sol b/contracts/staking/IStaking.sol
@@ -70,12 +70,6 @@ interface IStaking {
         address authorizer
     ) external;
 
-    /// @notice Refresh Keep stake owner. Can be called only by the old owner
-    ///         or their staking provider.
-    /// @dev The staking provider in T staking contract is the legacy KEEP
-    ///      staking contract operator.
-    function refreshKeepStakeOwner(address stakingProvider) external;
-
     /// @notice Allows the Governance to set the minimum required stake amount.
     ///         This amount is required to protect against griefing the staking
     ///         contract and individual applications are allowed to require
diff --git a/contracts/staking/TokenStaking.sol b/contracts/staking/TokenStaking.sol
@@ -214,6 +214,14 @@ contract TokenStaking is Initializable, IStaking, Checkpoints {
         _;
     }
 
+    modifier onlyOwnerOf(address stakingProvider) {
+        require(
+            stakingProviders[stakingProvider].owner == msg.sender,
+            "Caller is not owner"
+        );
+        _;
+    }
+
     /// @param _token Address of T token contract
     /// @param _keepStakingContract Address of Keep staking contract
     /// @param _nucypherStakingContract Address of NuCypher staking contract
@@ -400,28 +408,6 @@ contract TokenStaking is Initializable, IStaking, Checkpoints {
         );
     }
 
-    /// @notice Refresh Keep stake owner. Can be called only by the old owner
-    ///         or their staking provider.
-    /// @dev The staking provider in T staking contract is the legacy KEEP
-    ///      staking contract operator.
-    function refreshKeepStakeOwner(address stakingProvider)
-        external
-        override
-        onlyOwnerOrStakingProvider(stakingProvider)
-    {
-        StakingProviderInfo storage stakingProviderStruct = stakingProviders[
-            stakingProvider
-        ];
-        address newOwner = keepStake.resolveOwner(stakingProvider);
-
-        emit OwnerRefreshed(
-            stakingProvider,
-            stakingProviderStruct.owner,
-            newOwner
-        );
-        stakingProviderStruct.owner = newOwner;
-    }
-
     /// @notice Allows the Governance to set the minimum required stake amount.
     ///         This amount is required to protect against griefing the staking
     ///         contract and individual applications are allowed to require
@@ -480,6 +466,7 @@ contract TokenStaking is Initializable, IStaking, Checkpoints {
         address application,
         uint96 amount
     ) external override onlyAuthorizerOf(stakingProvider) {
+        require(amount > 0, "Parameters must be specified");
         ApplicationInfo storage applicationStruct = applicationInfo[
             application
         ];
@@ -763,7 +750,7 @@ contract TokenStaking is Initializable, IStaking, Checkpoints {
     function topUpNu(address stakingProvider)
         external
         override
-        onlyOwnerOrStakingProvider(stakingProvider)
+        onlyOwnerOf(stakingProvider)
     {
         StakingProviderInfo storage stakingProviderStruct = stakingProviders[
             stakingProvider
@@ -1420,14 +1407,11 @@ contract TokenStaking is Initializable, IStaking, Checkpoints {
         internal
         virtual
         override
+        onlyOwnerOf(stakingProvider)
     {
         StakingProviderInfo storage stakingProviderStruct = stakingProviders[
             stakingProvider
         ];
-        require(
-            stakingProviderStruct.owner == msg.sender,
-            "Caller is not owner"
-        );
         uint96 stakingProviderBalance = stakingProviderStruct.tStake +
             stakingProviderStruct.keepInTStake +
             stakingProviderStruct.nuInTStake;
diff --git a/contracts/test/KeepRegistryStub.sol b/contracts/test/KeepRegistryStub.sol
@@ -9,7 +9,7 @@ contract KeepRegistryStub is IKeepRegistry {
 
     event OperatorContractApproved(address operatorContract);
 
-    constructor() public {
+    constructor() {
         registryKeeper = msg.sender;
     }
 
diff --git a/docs/rfc-1-staking-contract.adoc b/docs/rfc-1-staking-contract.adoc
@@ -307,10 +307,6 @@ additionally appointing beneficiary and authorizer roles. Caches the amount
 staked in NU staking contract. Can be called only by the original delegation
 owner.
 
-==== `refreshKeepStakeOwner(address stakingProvider) external onlyOwnerOf(stakingProvider)`
-
-Refresh Keep stake owner. Can be called only by the old owner.
-
 ==== `setMinimumStakeAmount(uint96 amount) external onlyGovernance`
 
 Allows the governance to set the minimum required stake amount. This amount is
diff --git a/test/staking/TokenStaking.test.js b/test/staking/TokenStaking.test.js
@@ -852,152 +852,6 @@ describe("TokenStaking", () => {
     })
   })
 
-  describe("refreshKeepManagedGrantOwner", () => {
-    context("when staking provider has no delegated stake", () => {
-      it("should revert", async () => {
-        await expect(
-          tokenStaking
-            .connect(stakingProvider)
-            .refreshKeepStakeOwner(stakingProvider.address)
-        ).to.be.revertedWith("Not owner or provider")
-      })
-    })
-
-    context("when caller is neither old owner nor staking provider", () => {
-      it("should revert", async () => {
-        await tToken
-          .connect(staker)
-          .approve(tokenStaking.address, initialStakerBalance)
-        await tokenStaking
-          .connect(staker)
-          .stake(
-            stakingProvider.address,
-            beneficiary.address,
-            authorizer.address,
-            initialStakerBalance
-          )
-        await expect(
-          tokenStaking
-            .connect(authorizer)
-            .refreshKeepStakeOwner(stakingProvider.address)
-        ).to.be.revertedWith("Not owner or provider")
-      })
-    })
-
-    const contextRefreshKeepStakeOwner = (getCaller) => {
-      context("when grantee was not changed", () => {
-        let tx
-
-        beforeEach(async () => {
-          const createdAt = 1
-          await keepStakingMock.setOperator(
-            stakingProvider.address,
-            staker.address,
-            beneficiary.address,
-            authorizer.address,
-            createdAt,
-            0,
-            initialStakerBalance
-          )
-          await keepStakingMock.setEligibility(
-            stakingProvider.address,
-            tokenStaking.address,
-            true
-          )
-          await tokenStaking.stakeKeep(stakingProvider.address)
-
-          tx = await tokenStaking
-            .connect(getCaller())
-            .refreshKeepStakeOwner(stakingProvider.address)
-        })
-
-        it("should not update owner", async () => {
-          expect(
-            await tokenStaking.rolesOf(stakingProvider.address)
-          ).to.deep.equal([
-            staker.address,
-            beneficiary.address,
-            authorizer.address,
-          ])
-        })
-
-        it("should emit OwnerRefreshed", async () => {
-          await expect(tx)
-            .to.emit(tokenStaking, "OwnerRefreshed")
-            .withArgs(stakingProvider.address, staker.address, staker.address)
-        })
-      })
-
-      context("when grantee was changed", () => {
-        let tx
-
-        beforeEach(async () => {
-          const createdAt = 1
-          await keepStakingMock.setOperator(
-            stakingProvider.address,
-            otherStaker.address,
-            beneficiary.address,
-            authorizer.address,
-            createdAt,
-            0,
-            initialStakerBalance
-          )
-          await keepStakingMock.setEligibility(
-            stakingProvider.address,
-            tokenStaking.address,
-            true
-          )
-          await tokenStaking.stakeKeep(stakingProvider.address)
-
-          await keepStakingMock.setOperator(
-            stakingProvider.address,
-            staker.address,
-            beneficiary.address,
-            authorizer.address,
-            createdAt,
-            0,
-            initialStakerBalance
-          )
-          tx = await tokenStaking
-            .connect(otherStaker)
-            .refreshKeepStakeOwner(stakingProvider.address)
-        })
-
-        it("should update owner", async () => {
-          expect(
-            await tokenStaking.rolesOf(stakingProvider.address)
-          ).to.deep.equal([
-            staker.address,
-            beneficiary.address,
-            authorizer.address,
-          ])
-        })
-
-        it("should emit OwnerRefreshed", async () => {
-          await expect(tx)
-            .to.emit(tokenStaking, "OwnerRefreshed")
-            .withArgs(
-              stakingProvider.address,
-              otherStaker.address,
-              staker.address
-            )
-        })
-      })
-    }
-
-    context("when caller is the old owner", () => {
-      contextRefreshKeepStakeOwner(() => {
-        return staker
-      })
-    })
-
-    context("when caller is the staking provider", () => {
-      contextRefreshKeepStakeOwner(() => {
-        return stakingProvider
-      })
-    })
-  })
-
   describe("approveApplication", () => {
     context("when caller is not the governance", () => {
       it("should revert", async () => {
@@ -1526,7 +1380,7 @@ describe("TokenStaking", () => {
 
           await nucypherStakingMock.setStaker(staker.address, nuStake)
           await tokenStaking
-            .connect(stakingProvider)
+            .connect(staker)
             .topUpNu(stakingProvider.address)
 
           await tToken.connect(staker).approve(tokenStaking.address, tStake)
@@ -3514,12 +3368,12 @@ describe("TokenStaking", () => {
     context("when staking provider has no delegated stake", () => {
       it("should revert", async () => {
         await expect(
-          tokenStaking.connect(stakingProvider).topUpNu(stakingProvider.address)
-        ).to.be.revertedWith("Not owner or provider")
+          tokenStaking.connect(staker).topUpNu(stakingProvider.address)
+        ).to.be.revertedWith("Caller is not owner")
       })
     })
 
-    context("when caller is not owner or staking provider", () => {
+    context("when caller is not owner", () => {
       it("should revert", async () => {
         await tToken
           .connect(staker)
@@ -3533,8 +3387,8 @@ describe("TokenStaking", () => {
             initialStakerBalance
           )
         await expect(
-          tokenStaking.connect(authorizer).topUpNu(stakingProvider.address)
-        ).to.be.revertedWith("Not owner or provider")
+          tokenStaking.connect(stakingProvider).topUpNu(stakingProvider.address)
+        ).to.be.revertedWith("Caller is not owner")
       })
     })
 
@@ -3551,7 +3405,7 @@ describe("TokenStaking", () => {
             amount
           )
         await expect(
-          tokenStaking.connect(stakingProvider).topUpNu(stakingProvider.address)
+          tokenStaking.connect(staker).topUpNu(stakingProvider.address)
         ).to.be.revertedWith("Nothing to top-up")
       })
     })
@@ -3684,7 +3538,7 @@ describe("TokenStaking", () => {
           .connect(staker)
           .unstakeNu(stakingProvider.address, nuInTAmount)
         tx = await tokenStaking
-          .connect(stakingProvider)
+          .connect(staker)
           .topUpNu(stakingProvider.address)
       })
 
@@ -3804,7 +3658,7 @@ describe("TokenStaking", () => {
 
         await nucypherStakingMock.setStaker(staker.address, nuAmount)
         tx = await tokenStaking
-          .connect(stakingProvider)
+          .connect(staker)
           .topUpNu(stakingProvider.address)
       })
 

Original file line number	Diff line number	Diff line change
`@@ -9,7 +9,7 @@ contract KeepRegistryStub is IKeepRegistry {`
`9`	`9`
`10`	`10`	`event OperatorContractApproved(address operatorContract);`
`11`	`11`
`12`		`- constructor() public {`
	`12`	`+ constructor() {`
`13`	`13`	`registryKeeper = msg.sender;`
`14`	`14`	`}`
`15`	`15`