fix: array out-of-bounds issue in _exitMaintenance (#565)

bnb-chain · Jun 25, 2024 · b743ce3 · b743ce3
1 parent 864d923
commit b743ce3
Show file tree

Hide file tree

Showing 13 changed files with 1,807 additions and 1,618 deletions.
diff --git a/abi/slashindicator.abi b/abi/slashindicator.abi
@@ -461,6 +461,11 @@
         "name": "count",
         "type": "uint256",
         "internalType": "uint256"
+      },
+      {
+        "name": "shouldRevert",
+        "type": "bool",
+        "internalType": "bool"
       }
     ],
     "outputs": [],

diff --git a/contracts/BSCValidatorSet.sol b/contracts/BSCValidatorSet.sol
@@ -680,7 +680,7 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
         // jailed validators are allowed to exit maintenance
         require(validatorExtraSet[index].isMaintaining, "not in maintenance");
         uint256 miningValidatorCount = getWorkingValidatorCount();
-        _exitMaintenance(msg.sender, index, miningValidatorCount);
+        _exitMaintenance(msg.sender, index, miningValidatorCount, true);
     }
 
     /*----------------- Param update -----------------*/
@@ -1202,7 +1202,7 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
             validator = currentValidatorSet[i].consensusAddress;
 
             // exit maintenance
-            isFelony = _exitMaintenance(validator, i, miningValidatorCount);
+            isFelony = _exitMaintenance(validator, i, miningValidatorCount, false);
             if (!isFelony) {
                 continue;
             }
@@ -1267,25 +1267,32 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
     function _exitMaintenance(
         address validator,
         uint256 index,
-        uint256 miningValidatorCount
+        uint256 miningValidatorCount,
+        bool shouldRevert
     ) private returns (bool isFelony) {
         if (maintainSlashScale == 0 || miningValidatorCount == 0 || numOfMaintaining == 0) {
             // should not happen, still protect
             return false;
         }
 
+        // step 0: modify numOfMaintaining
+        --numOfMaintaining;
+
         // step 1: calculate slashCount
         uint256 slashCount = block.number.sub(validatorExtraSet[index].enterMaintenanceHeight).div(miningValidatorCount)
             .div(maintainSlashScale);
 
-        // step2: slash the validator
+        // step 2: clear isMaintaining info
+        validatorExtraSet[index].isMaintaining = false;
+
+        // step 3: slash the validator
         (uint256 misdemeanorThreshold, uint256 felonyThreshold) =
             ISlashIndicator(SLASH_CONTRACT_ADDR).getSlashThresholds();
         isFelony = false;
         if (slashCount >= felonyThreshold) {
             _felony(validator, index);
             if (IStakeHub(STAKE_HUB_ADDR).consensusToOperator(validator) != address(0)) {
-                ISlashIndicator(SLASH_CONTRACT_ADDR).downtimeSlash(validator, slashCount);
+                ISlashIndicator(SLASH_CONTRACT_ADDR).downtimeSlash(validator, slashCount, shouldRevert);
             } else {
                 ISlashIndicator(SLASH_CONTRACT_ADDR).sendFelonyPackage(validator);
             }
@@ -1294,10 +1301,6 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
             _misdemeanor(validator);
         }
 
-        // step 3: modify global storage
-        --numOfMaintaining;
-        validatorExtraSet[index].isMaintaining = false;
-
         emit validatorExitMaintenance(validator);
     }
 

diff --git a/contracts/SlashIndicator.sol b/contracts/SlashIndicator.sol
@@ -135,7 +135,7 @@ contract SlashIndicator is ISlashIndicator, System, IParamSubscriber, IApplicati
             indicator.count = 0;
             IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).felony(validator);
             if (IStakeHub(STAKE_HUB_ADDR).consensusToOperator(validator) != address(0)) {
-                _downtimeSlash(validator, indicator.count);
+                _downtimeSlash(validator, indicator.count, false);
             } else {
                 // send slash msg to bc if validator is not migrated
                 try ICrossChain(CROSS_CHAIN_CONTRACT_ADDR).sendSynPackage(
@@ -205,16 +205,24 @@ contract SlashIndicator is ISlashIndicator, System, IParamSubscriber, IApplicati
         emit indicatorCleaned();
     }
 
-    function downtimeSlash(address validator, uint256 count) external override onlyValidatorContract {
-        _downtimeSlash(validator, count);
+    function downtimeSlash(
+        address validator,
+        uint256 count,
+        bool shouldRevert
+    ) external override onlyValidatorContract {
+        _downtimeSlash(validator, count, shouldRevert);
     }
 
-    function _downtimeSlash(address validator, uint256 count) internal {
-        try IStakeHub(STAKE_HUB_ADDR).downtimeSlash(validator) { }
-        catch Error(string memory reason) {
-            emit failedFelony(validator, count, bytes(reason));
-        } catch (bytes memory lowLevelData) {
-            emit failedFelony(validator, count, lowLevelData);
+    function _downtimeSlash(address validator, uint256 count, bool shouldRevert) internal {
+        if (shouldRevert) {
+            IStakeHub(STAKE_HUB_ADDR).downtimeSlash(validator);
+        } else {
+            try IStakeHub(STAKE_HUB_ADDR).downtimeSlash(validator) { }
+            catch Error(string memory reason) {
+                emit failedFelony(validator, count, bytes(reason));
+            } catch (bytes memory lowLevelData) {
+                emit failedFelony(validator, count, lowLevelData);
+            }
         }
     }
 

diff --git a/contracts/interface/ISlashIndicator.sol b/contracts/interface/ISlashIndicator.sol
@@ -2,7 +2,7 @@ pragma solidity 0.6.4;
 
 interface ISlashIndicator {
     function clean() external;
-    function downtimeSlash(address validator, uint256 count) external;
+    function downtimeSlash(address validator, uint256 count, bool shouldRevert) external;
     function sendFelonyPackage(address validator) external;
     function getSlashThresholds() external view returns (uint256, uint256);
 }