fix: resolve review comments

bnb-chain · Dec 26, 2023 · 97111ff · 97111ff
1 parent e06f8d4
commit 97111ff
Show file tree

Hide file tree

Showing 23 changed files with 87 additions and 141 deletions.
diff --git a/abi/bscvalidatorset.abi b/abi/bscvalidatorset.abi
@@ -1062,19 +1062,6 @@
     ],
     "stateMutability": "view"
   },
-  {
-    "type": "function",
-    "name": "jailValidator",
-    "inputs": [
-      {
-        "name": "consensusAddress",
-        "type": "address",
-        "internalType": "address"
-      }
-    ],
-    "outputs": [],
-    "stateMutability": "nonpayable"
-  },
   {
     "type": "function",
     "name": "maintainSlashScale",

diff --git a/abi/slashindicator.abi b/abi/slashindicator.abi
@@ -865,25 +865,6 @@
     ],
     "anonymous": false
   },
-  {
-    "type": "event",
-    "name": "failedMaliciousVoteSlash",
-    "inputs": [
-      {
-        "name": "voteAddrSlice",
-        "type": "bytes32",
-        "indexed": true,
-        "internalType": "bytes32"
-      },
-      {
-        "name": "failReason",
-        "type": "bytes",
-        "indexed": false,
-        "internalType": "bytes"
-      }
-    ],
-    "anonymous": false
-  },
   {
     "type": "event",
     "name": "indicatorCleaned",

diff --git a/abi/stakehub.abi b/abi/stakehub.abi
@@ -1009,7 +1009,7 @@
         "internalType": "address"
       },
       {
-        "name": "commissionRate",
+        "name": "newCommissionRate",
         "type": "uint64",
         "indexed": false,
         "internalType": "uint64"

diff --git a/abi/systemreward.abi b/abi/systemreward.abi
@@ -148,7 +148,7 @@
   },
   {
     "type": "function",
-    "name": "MAX_REWARDS",
+    "name": "MAX_LARGE_REWARDS",
     "inputs": [],
     "outputs": [
       {
@@ -161,7 +161,7 @@
   },
   {
     "type": "function",
-    "name": "MAX_REWARDS_FOR_FINALITY",
+    "name": "MAX_REWARDS",
     "inputs": [],
     "outputs": [
       {
@@ -395,7 +395,7 @@
   },
   {
     "type": "function",
-    "name": "claimRewards",
+    "name": "claimLargeRewards",
     "inputs": [
       {
         "name": "to",
@@ -419,7 +419,7 @@
   },
   {
     "type": "function",
-    "name": "claimRewardsforFinality",
+    "name": "claimRewards",
     "inputs": [
       {
         "name": "to",

diff --git a/abi/tokenhub.abi b/abi/tokenhub.abi
@@ -728,15 +728,15 @@
   },
   {
     "type": "function",
-    "name": "claimRewards",
+    "name": "claimLargeRewards",
     "inputs": [
       {
-        "name": "to",
+        "name": "",
         "type": "address",
         "internalType": "address payable"
       },
       {
-        "name": "amount",
+        "name": "",
         "type": "uint256",
         "internalType": "uint256"
       }
@@ -752,15 +752,15 @@
   },
   {
     "type": "function",
-    "name": "claimRewardsforFinality",
+    "name": "claimRewards",
     "inputs": [
       {
-        "name": "",
+        "name": "to",
         "type": "address",
         "internalType": "address payable"
       },
       {
-        "name": "",
+        "name": "amount",
         "type": "uint256",
         "internalType": "uint256"
       }

diff --git a/contracts/BC_fusion/StakeHub.sol b/contracts/BC_fusion/StakeHub.sol
@@ -176,7 +176,7 @@ contract StakeHub is System, Initializable {
         bytes voteAddress
     );
     event ConsensusAddressEdited(address indexed operatorAddress, address indexed newConsensusAddress);
-    event CommissionRateEdited(address indexed operatorAddress, uint64 commissionRate);
+    event CommissionRateEdited(address indexed operatorAddress, uint64 newCommissionRate);
     event DescriptionEdited(address indexed operatorAddress);
     event VoteAddressEdited(address indexed operatorAddress, bytes newVoteAddress);
     event Delegated(address indexed operatorAddress, address indexed delegator, uint256 shares, uint256 bnbAmount);
@@ -994,8 +994,7 @@ contract StakeHub is System, Initializable {
         }
         if (IStakeCredit(valInfo.creditContract).getPooledBNB(operatorAddress) < minSelfDelegationBNB) {
             _jailValidator(valInfo, downtimeJailTime);
-            // need to inform BSCValidatorSet contract to remove the validator
-            IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).jailValidator(valInfo.consensusAddress);
+            IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).felony(valInfo.consensusAddress);
         }
     }
 

diff --git a/contracts/BC_fusion/interface/IBSCValidatorSet.sol b/contracts/BC_fusion/interface/IBSCValidatorSet.sol
@@ -2,5 +2,5 @@
 pragma solidity 0.8.17;
 
 interface IBSCValidatorSet {
-    function jailValidator(address consensusAddress) external;
+    function felony(address consensusAddress) external;
 }
diff --git a/contracts/BSCValidatorSet.sol b/contracts/BSCValidatorSet.sol
@@ -188,8 +188,7 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
   receive() external payable {}
 
   /*********************** Cross Chain App Implement **************************/
-  // TODO: add `onlyCrossChainContract`
-  function handleSynPackage(uint8, bytes calldata msgBytes) onlyInit initValidatorExtraSet external override returns(bytes memory responsePayload) {
+  function handleSynPackage(uint8, bytes calldata msgBytes) onlyInit onlyCrossChainContract initValidatorExtraSet external override returns(bytes memory responsePayload) {
     (IbcValidatorSetPackage memory validatorSetPackage, bool ok) = decodeValidatorSetSynPackage(msgBytes);
     if (!ok) {
       return CmnPkg.encodeCommonAckPackage(ERROR_FAIL_DECODE);
@@ -202,13 +201,18 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
         emit failReasonWithStr("length of jail validators must be one");
         resCode = ERROR_LEN_OF_VAL_MISMATCH;
       } else {
-        uint256 index = currentValidatorSetMap[validatorSetPackage.validatorSet[0].consensusAddress];
-        if (index==0 || currentValidatorSet[index-1].jailed) {
-          emit validatorEmptyJailed(validatorSetPackage.validatorSet[0].consensusAddress);
-          resCode = CODE_OK;
+        address validator = validatorSetPackage.validatorSet[0].consensusAddress;
+        uint256 index = currentValidatorSetMap[validator];
+        if (index == 0 || currentValidatorSet[index-1].jailed) {
+          emit validatorEmptyJailed(validator);
         } else {
-          resCode = _jailValidator(index);
+          // felony will failed if the validator is the only one in the validator set
+          bool success = _felony(validator, index-1);
+          if (!success) {
+            emit validatorEmptyJailed(validator);
+          }
         }
+        resCode = CODE_OK;
       }
     } else {
       resCode = ERROR_UNKNOWN_PACKAGE_TYPE;
@@ -357,36 +361,6 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
     }
   }
 
-  function jailValidator(address consensusAddress) external onlyStakeHub {
-    for (uint256 i; i < _tmpMigratedValidatorSet.length; ++i) {
-      if (_tmpMigratedValidatorSet[i].consensusAddress == consensusAddress) {
-        _tmpMigratedValidatorSet[i].jailed = true;
-        break;
-      }
-    }
-
-    uint256 index = currentValidatorSetMap[consensusAddress];
-    if (index==0 || currentValidatorSet[index-1].jailed) {
-      emit validatorEmptyJailed(consensusAddress);
-    } else {
-      _jailValidator(index);
-    }
-  }
-
-  function _jailValidator(uint256 index) internal returns (uint32) {
-    uint n = currentValidatorSet.length;
-    bool shouldKeep = (numOfJailed >= n-1);
-    // will not jail if it is the last valid validator
-    if (shouldKeep) {
-      emit validatorEmptyJailed(currentValidatorSet[index-1].consensusAddress);
-      return CODE_OK;
-    }
-    ++numOfJailed;
-    currentValidatorSet[index-1].jailed = true;
-    emit validatorJailed(currentValidatorSet[index-1].consensusAddress);
-    return CODE_OK;
-  }
-
   function updateValidatorSet(Validator[] memory validatorSet, bytes[] memory voteAddrs) internal returns (uint32) {
     {
       // do verify.
@@ -701,7 +675,7 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
       return;
     }
 
-    totalValue = ISystemReward(SYSTEM_REWARD_ADDR).claimRewardsforFinality(payable(address(this)), totalValue);
+    totalValue = ISystemReward(SYSTEM_REWARD_ADDR).claimLargeRewards(payable(address(this)), totalValue);
     if (totalValue == 0) {
       return;
     }
@@ -758,7 +732,9 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
     }
   }
 
-  function felony(address validator)external onlySlash initValidatorExtraSet override{
+  function felony(address validator) external initValidatorExtraSet override {
+    require(msg.sender == SLASH_CONTRACT_ADDR || msg.sender == STAKE_HUB_ADDR, "only slash or stakeHub contract");
+
     uint256 index = currentValidatorSetMap[validator];
     if (index <= 0) {
       return;
@@ -1064,6 +1040,8 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
       // should not happen, but still protect
       return index;
     }
+
+    // averageDistribute*rest may less than income, but it is ok, the dust income will go to system reward eventually.
     uint256 averageDistribute = income / rest;
     if (averageDistribute != 0) {
       for (uint i; i<index; ++i) {
@@ -1074,12 +1052,11 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
         currentValidatorSet[i].incoming = currentValidatorSet[i].incoming + averageDistribute;
       }
     }
-    // averageDistribute*rest may less than income, but it is ok, the dust income will go to system reward eventually.
 
     return index;
   }
 
-  function _felony(address validator, uint256 index) private returns (bool){
+  function _felony(address validator, uint256 index) private returns (bool) {
     uint256 income = currentValidatorSet[index].incoming;
     uint256 rest = currentValidatorSet.length - 1;
     if (getValidators().length <= 1) {

diff --git a/contracts/GovHub.sol b/contracts/GovHub.sol
@@ -27,8 +27,7 @@ contract GovHub is System, IApplication{
     address  target;
   }
 
-  //TODO: add onlyCrossChainContract
-  function handleSynPackage(uint8, bytes calldata msgBytes) external override returns(bytes memory responsePayload) {
+  function handleSynPackage(uint8, bytes calldata msgBytes) external onlyCrossChainContract override returns(bytes memory responsePayload) {
     (ParamChangePackage memory proposal, bool success) = decodeSynPackage(msgBytes);
     if (!success) {
       return CmnPkg.encodeCommonAckPackage(ERROR_FAIL_DECODE);

diff --git a/contracts/SlashIndicator.sol b/contracts/SlashIndicator.sol
@@ -19,11 +19,8 @@ import "./lib/RLPEncode.sol";
 contract SlashIndicator is ISlashIndicator,System,IParamSubscriber, IApplication{
   using RLPEncode for *;
 
-  // TODO: revert to 50 and 150
-  // uint256 public constant MISDEMEANOR_THRESHOLD = 50;
-  // uint256 public constant FELONY_THRESHOLD = 150;
-  uint256 public constant MISDEMEANOR_THRESHOLD = 5;
-  uint256 public constant FELONY_THRESHOLD = 10;
+  uint256 public constant MISDEMEANOR_THRESHOLD = 50;
+  uint256 public constant FELONY_THRESHOLD = 150;
   uint256 public constant DECREASE_RATE = 4;
 
   // State of the contract
@@ -58,7 +55,6 @@ contract SlashIndicator is ISlashIndicator,System,IParamSubscriber, IApplication
   event crashResponse();
 
   event failedFelony(address indexed validator, uint256 slashCount, bytes failReason);
-  event failedMaliciousVoteSlash(bytes32 indexed voteAddrSlice, bytes failReason);
 
   struct Indicator {
     uint256 height;
@@ -247,11 +243,12 @@ contract SlashIndicator is ISlashIndicator,System,IParamSubscriber, IApplication
       _verifyBLSSignature(_evidence.voteB, _evidence.voteAddr), "verify signature failed");
 
     // reward sender and felony validator if validator found
+    // TODO: after BC-fusion, we don't need to check if validator is living
     (address[] memory vals, bytes[] memory voteAddrs) = IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).getLivingValidators();
     for (uint i; i < voteAddrs.length; ++i) {
       if (BytesLib.equal(voteAddrs[i],  _evidence.voteAddr)) {
         uint256 amount = (address(SYSTEM_REWARD_ADDR).balance * felonySlashRewardRatio) / 100;
-        ISystemReward(SYSTEM_REWARD_ADDR).claimRewardsforFinality(msg.sender, amount);
+        ISystemReward(SYSTEM_REWARD_ADDR).claimLargeRewards(msg.sender, amount);
         IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).felony(vals[i]);
         break;
       }
@@ -261,12 +258,10 @@ contract SlashIndicator is ISlashIndicator,System,IParamSubscriber, IApplication
       IStakeHub(STAKE_HUB_ADDR).maliciousVoteSlash(_evidence.voteAddr);
     } else {
       // send slash msg to bc if the validator not migrated
-      bytes32 voteAddrSlice = BytesLib.toBytes32(_evidence.voteAddr,0);
-      try ICrossChain(CROSS_CHAIN_CONTRACT_ADDR).sendSynPackage(SLASH_CHANNELID, encodeVoteSlashPackage(_evidence.voteAddr), 0) {
-        emit maliciousVoteSlashed(voteAddrSlice);
-      } catch (bytes memory reason) {
-        emit failedMaliciousVoteSlash(voteAddrSlice, reason);
-      }
+      ICrossChain(CROSS_CHAIN_CONTRACT_ADDR).sendSynPackage(SLASH_CHANNELID, encodeVoteSlashPackage(_evidence.voteAddr), 0);
+
+      bytes32 voteAddrSlice = BytesLib.toBytes32(_evidence.voteAddr, 0);
+      emit maliciousVoteSlashed(voteAddrSlice);
     }
   }
 
@@ -304,18 +299,12 @@ contract SlashIndicator is ISlashIndicator,System,IParamSubscriber, IApplication
     require(IStakeHub(STAKE_HUB_ADDR).consensusToOperator(signer) != address(0), "validator not migrated");
     require(evidenceHeight + felonySlashScope >= block.number, "evidence too old");
 
-    // reward sender and felony validator if validator found
-    (address[] memory vals, ) = IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).getLivingValidators();
-    for (uint i; i < vals.length; ++i) {
-      if (signer == vals[i]) {
-        uint256 amount = (address(SYSTEM_REWARD_ADDR).balance * felonySlashRewardRatio) / 100;
-        ISystemReward(SYSTEM_REWARD_ADDR).claimRewards(msg.sender, amount);
-        IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).felony(vals[i]);
-        break;
-      }
-    }
-
+    // reward sender and felony validator
     IStakeHub(STAKE_HUB_ADDR).doubleSignSlash(signer);
+    IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).felony(signer);
+
+    uint256 amount = (address(SYSTEM_REWARD_ADDR).balance * felonySlashRewardRatio) / 100;
+    ISystemReward(SYSTEM_REWARD_ADDR).claimLargeRewards(msg.sender, amount);
   }
 
   /**

diff --git a/contracts/SystemReward.sol b/contracts/SystemReward.sol
@@ -7,7 +7,7 @@ import "./interface/ISystemReward.sol";
 
 contract SystemReward is System, IParamSubscriber, ISystemReward {
   uint256 public constant MAX_REWARDS/*_FOR_RELAYER*/ = 1e18;
-  uint256 public constant MAX_REWARDS_FOR_FINALITY = 5e18;
+  uint256 public constant MAX_LARGE_REWARDS = 5e18;
 
   uint public numOperator;
   mapping(address => bool) operators;
@@ -40,6 +40,9 @@ contract SystemReward is System, IParamSubscriber, ISystemReward {
     }
   }
 
+  /**
+    * @dev claim rewards for relayer
+    */
   function claimRewards(address payable to, uint256 amount) external override(ISystemReward) doInit onlyOperator returns (uint256) {
     uint256 actualAmount = amount < address(this).balance ? amount : address(this).balance;
     if (actualAmount > MAX_REWARDS) {
@@ -54,10 +57,13 @@ contract SystemReward is System, IParamSubscriber, ISystemReward {
     return actualAmount;
   }
 
-  function claimRewardsforFinality(address payable to, uint256 amount) external override(ISystemReward) doInit onlyOperator returns (uint256) {
+  /**
+  * @dev claim rewards for fast finality or felony slash
+  */
+  function claimLargeRewards(address payable to, uint256 amount) external override(ISystemReward) doInit onlyOperator returns (uint256) {
     uint256 actualAmount = amount < address(this).balance ? amount : address(this).balance;
-    if (actualAmount > MAX_REWARDS_FOR_FINALITY) {
-      actualAmount = MAX_REWARDS_FOR_FINALITY;
+    if (actualAmount > MAX_LARGE_REWARDS) {
+      actualAmount = MAX_LARGE_REWARDS;
     }
     if (actualAmount != 0) {
       to.transfer(actualAmount);