DeXit security record

The function `removeFromHighestValidatorList()` removes a highest validator from the array `highestValidators`. However, this function is `external` and no access control is enforced. Therefore, it can be called by anyone. Since validators in `highestValidator` are responsible for producing/replaying blocks and voting/executing proposals, if a malicious user removes the validators by repeatedly calling the function, the whole network may get unstable. **Exploit Scenario** - An attacker calls `removeFromHighestValidatorList()` repeatedly to remove the highest validator from the array `highestValidators`. - Finally, there will be only one "highest validator" left. - Therefore, if the attacker is able to control the left validator, the attacker will be able to manipulate the chain and cause devastating consequences.

The function `punish()` is used to record the count of the validator's missed block and jail the validator when it meets the conditions of punishment. However, this function is `external` and no access control is enforced. Therefore, it can be called by anyone. **Exploit Scenario** - An attacker repeatedly calls this function to punish all the validators multiple times, until all validators are jailed, which would cause a Denial-Of-Service.

In the `GoveHub.sol` contract, the `voteProposal()` function allows users to participant in the governance voting. If the voting meets the threshold, the proposal will be executed automatically. However, allowing voting and executing a proposal in the same transaction could be risky since if the voting is manipulated, the proposal will be immediately executed, thus causing actual damage to the project. Therefore, considering the users' unpredictable behavior, it is recommended to introduce a certain time of delay when performing governance actions. For example, in the case that the private keys of multiple governors are compromised, attackers could immediately perform the following actions to execute malicious proposals immediately: 1. Create a Malicious Proposal (or Global Proposal), 2. Cast the Vote, 3. Execute a Malicious proposal.

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. Two functions are vulnerable to re-entrancy attack: - `claimValidatorReward()` - `claimDelegatorReward()` In both cases, if `oldClaimTime` is different from 0, a re-entrancy will occur. \ Below is a summary of the performed calls for `claimDelegatorReward()` : ```solidity function claimDelegatorReward(address validator) (...) address payable delegator = msg.sender; (...) uint256 oldClaimTime = rewardClaimRecords[delegator]; (...) uint256 staking = stakeInfo.income; (...) if (oldClaimTime == 0) { (...) } else { uint256 endTime = oldClaimTime + RewardClaimingPeriod; require(block.number >= endTime, "Claim after 24 hours"); delegator.transfer(staking); rewardClaimRecords[delegator] = block.number; stakeInfo.income = 0; ``` If `delegator` is a contract, it could implement a fallback function that re-enters through `BSCValidatorSet.claimDelegatorReward()`, which would allow the `delegator` to siphon all funds from the `BSCValidatorSet` contract. \ The issue is tied to the fact that the value of `rewardClaimRecords[delegator]` and `stakeInfo.income` are updated after the function `delegator.transfer()` is called. This violates the check-effect-interaction pattern and allows a dangerous re-entrancy. **Exploit Scenario** - An attacker uses a contract to stake tokens to become a validator/delegator and get the block reward recorded in the `validatorInfo`/`stakeInfo`. - Then the attacker claims the reward once when the `oldClaimTime` is 0. - After this, in the next call to get rewards, the attacker can make a recursive call back to the original function, and siphon all funds from the `BSCValidatorSet` contract.

According to the `unstakeValidator()` function, the `highestValidators` array needs to have at least 4 elements. ```solidity require( highestValidators.length > 3 && this.isActiveValidator(staker), "Can't Unstake, Validator List Empty" ); ``` This means that, for Dexit to work correctly, at least 4 validators are required. However, the `removeFromHighestValidatorList()`, which deletes validators from the `highestValidators` array, does not check the same condition, and instead check that the `highestValidators` has at least 1 element. \ Indeed, if there are 2 validators (n=2), then the loop will be entered and one of the validators removed, leaving only one validator. ```solidity=823 function removeFromHighestValidatorList(address val) external override { uint256 n = highestValidators.length; for (uint256 k = 0; k < n && n > 1; k++) { ``` In the same way, the `removeFromCurrentValidatorList()` function does not ensure that there are at least 3 validators remaining: ```solidity=837 function removeFromCurrentValidatorList(address val) private { uint256 n = currentValidators.length; for (uint256 i = 0; i < n && n > 1; i++) ```

"The linked declarations do not conform to the [Solidity style guide](https://solidity.readthedocs.io/en/v0.7.3/style-guide.html#naming-conventions) with regards to its naming convention. Particularly: - `camelCase`: Should be applied to function names, argument names, local and state variable names, modifiers - `UPPER_CASE`: Should be applied to `constant` variables - `CapWords`: Should be applied to contract names, struct names, event names and enums"

The function `unstakeValidator()` is used for the validator to unstake their position. According to the following code, the condition will ensure that the last three validators can't unstake their position. Besides, the comment for this condition check is `Can't Unstake, Validator List Empty`, which is mismatch with the condition check. ```solidity require( highestValidators.length > 3 && this.isActiveValidator(staker), "Can't Unstake, Validator List Empty" ); ```

The function `deposit()` is used by `Coinbase` to distribute block rewards to the validator `valAddr` and the contracts `_contractArray`. ```solidity=145 if(valInfo.validator == address(0x0) && highestValidators.length == 0){ valInfo.validator = valAddr; valInfo.status = IBSCValidatorSet.Status.NotExist; valInfo.income = valInfo.income.add(value); valInfo.TotalIncome = valInfo.TotalIncome.add(value); return; } ``` Consider the following cases: * **Case 1**: highestValidators.length == 0 In this case, there is no data in the `highestValidators` and `currentValidators` arrays. As a result, for the first call of the function with the parameter address `valAddr`, the `valInfo.validator` should be address(0x0). \ This will distribute rewards to `valAddr`, all the block rewards will be recorded in the `valInfo` structure, and the current status of `valInfo` will be `NotExist`. \ Since `claimValidatorReward()` will check whether the status is `NotExist`, the call of function `claimValidatorReward()` by the `valInfo.validator` will fail and the block rewards can't be claimed. As a result, the information recorded in the `valInfo` seems to be useless unless the `valInfo.validator` stakes a sufficient amount to become a normal validator. ```solidity function claimValidatorReward() external zeroAddress returns (bool) {//@audit-ok (...) require( valInfo.status != Status.NotExist && valInfo.status != Status.Jailed ); // Check for Not Exist Or Jailed require(valInfo.income > 0, "No incomes yet."); ``` Besides, if the function with the same parameter address `valAddr` is called again before the `valInfo.validator` become a normal validator, the `valInfo.validator` has been the parameter address `valAddr`, part of the block rewards will still be accumulated to this `valInfo` in the subsequent logic, the `valInfo` is still useless like before. * **Case 2**: highestValidators.length > 0 When the `valInfo.validator` is not `address(0x0)`, it will be a normal call. However, when the `valInfo.validator` is `address(0x0)`, the function will perform multiple calls like in **Case 1**.

When a user stakes tokens to become a validator for the first time, the amount of `valInfo` equals 0 and the status is `NotExist`, The `valInfo`'s status is updated to `Created`. But the `if` statement at L257 checks whether `valInfo.status` is `Staked` and if not, update it to `Staked`. ```solidity=257 if (valInfo.status != IBSCValidatorSet.Status.Staked) { valInfo.status = IBSCValidatorSet.Status.Staked; } ``` The status `Created` seems to be a temporary status and won't take any effect. Is that designed as expected?

According to the following code, the variable `percentageToTransfer` is the percentage of validator's stake amount in total stake. As the multiplier used is 100, the value of `percentageToTransfer` will be 0 to 100. When less than one percent, the value will be 0 due to division truncation。 ```solidity uint256 percentageToTransfer = valInfo.amount.mul(100).div(UpdatedCoins); ``` As a result, the reward for validator will be zero. We would like to confirm with the client if the current implementation aligns with the original project design.