patch StakingStakeModule to fix VP reverting vulnerability

DistributedCollective · May 26, 2023 · e3b14b4 · e3b14b4
1 parent ad7c9c0
commit e3b14b4
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 2 deletions.
diff --git a/contracts/governance/Staking/modules/StakingStakeModule.sol b/contracts/governance/Staking/modules/StakingStakeModule.sol
@@ -151,15 +151,19 @@ contract StakingStakeModule is IFunctionsList, StakingShared, CheckpointsShared,
         //		if  first stake was withdrawn completely and stake was delegated to the staker
         //		(no delegation to another address).
         address previousDelegatee = delegates[stakeFor][until];
+
         if (previousDelegatee != delegatee) {
             // @dev only the user that stakes for himself is allowed to delegate VP to another address
             // which works with vesting stakes and prevents vulnerability of delegating VP to an arbitrary address from
             // any address
+
             if (delegatee != stakeFor) {
                 require(
                     stakeFor == sender,
                     "Only stakeFor account is allowed to change delegatee"
                 );
+            } else if (sender != stakeFor && previousDelegatee != address(0)) {
+                require(stakeFor == sender, "Only sender is allowed to change delegatee");
             }
 
             /// @dev Update delegatee.

diff --git a/tests/staking/ExtendedStakingTest.js b/tests/staking/ExtendedStakingTest.js
@@ -90,7 +90,7 @@ const ZERO_ADDRESS = "0x0000000000000000000000000000000000000000";
 const maxWithdrawIterations = 10;
 
 contract("Staking", (accounts) => {
-    let root, account1, account2;
+    let root, account1, account2, account3;
     let token, SUSD, WRBTC, staking;
     let sovryn;
     let loanTokenLogic, loanToken;
@@ -178,7 +178,7 @@ contract("Staking", (accounts) => {
     }
 
     before(async () => {
-        [root, account1, account2, ...accounts] = accounts;
+        [root, account1, account2, account3, ...accounts] = accounts;
     });
 
     beforeEach(async () => {
@@ -333,6 +333,32 @@ contract("Staking", (accounts) => {
             );
         });
 
+        it("Should not revert delagated VP by staking for the delegator", async () => {
+            let amount = "1000";
+            let duration = new BN(TWO_WEEKS).mul(new BN(2));
+            let lockedTS = await getTimeFromKickoff(duration);
+
+            await token.transfer(account3, 1000);
+            await token.approve(staking.address, TOTAL_SUPPLY, { from: account3 });
+
+            // root user stakes and delegates voting to account1
+            await staking.stake(amount, lockedTS, root, account1);
+
+            let delegatee = await staking.delegates.call(root, lockedTS);
+            expect(delegatee).to.be.equal(account1);
+
+            // another user stakes for the root user (only able to delegate to the user being staked for)
+            // await staking.stake("1", lockedTS, root, root, { from: account3 });
+            await expectRevert(
+                staking.stake("1", lockedTS, root, root, { from: account3 }),
+                "Only sender is allowed to change delegatee"
+            );
+
+            // now the root user's delegatee is NOT reset back to the root user
+            delegatee = await staking.delegates.call(root, lockedTS);
+            expect(delegatee).to.be.equal(account1);
+        });
+
         it("Should be able to stake and delegate for another person", async () => {
             let amount = "1000";
             let duration = new BN(TWO_WEEKS).mul(new BN(2));