Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove malice reports system #241

Merged
merged 5 commits into from
Aug 26, 2024
Merged

Remove malice reports system #241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8658be3
remove malice reports system
axel-muller Aug 12, 2024
c5569db
remove unused import
axel-muller Aug 12, 2024
45568c6
tests coverage increase
axel-muller Aug 18, 2024
951008b
unit tests coverage increase
axel-muller Aug 19, 2024
649c9b1
remove unused constant
axel-muller Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions contracts/BlockRewardHbbft.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,10 +475,6 @@ contract BlockRewardHbbft is
uint256 numRewardedValidators = 0;

for (uint256 i = 0; i < validators.length; ++i) {
if (validatorSetContract.isValidatorBanned(validators[i])) {
continue;
}

uint256 validatorStakeAmount = stakingContract.getPoolValidatorStakeAmount(
stakingEpoch,
validatorSetContract.stakingByMiningAddress(validators[i])
Expand Down
6 changes: 2 additions & 4 deletions contracts/ConnectivityTrackerHbbft.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,8 @@ contract ConnectivityTrackerHbbft is Initializable, OwnableUpgradeable, IConnect
uint256 epoch = currentEpoch();
uint256 currentScore = getValidatorConnectivityScore(epoch, validator);

if (currentScore != 0) {
// slither-disable-next-line unused-return
_reporters[epoch][validator].remove(msg.sender);
}
// slither-disable-next-line unused-return
_reporters[epoch][validator].remove(msg.sender);

if (currentScore == 1) {
// slither-disable-next-line unused-return
Expand Down
46 changes: 4 additions & 42 deletions contracts/StakingHbbft.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,6 @@ contract StakingHbbft is Initializable, OwnableUpgradeable, ReentrancyGuardUpgra
error PoolAbandoned(address pool);
error PoolCannotBeRemoved(address pool);
error PoolEmpty(address pool);
error PoolMiningBanned(address pool);
error PoolNotExist(address pool);
error PoolStakeLimitExceeded(address pool, address delegator);
error InitialStakingPoolsListEmpty();
Expand Down Expand Up @@ -693,12 +692,7 @@ contract StakingHbbft is Initializable, OwnableUpgradeable, ReentrancyGuardUpgra

address staker = msg.sender;

if (
!_isWithdrawAllowed(
validatorSetContract.miningByStakingAddress(_poolStakingAddress),
staker != _poolStakingAddress
)
) {
if (!areStakeAndWithdrawAllowed()) {
revert WithdrawNotAllowed();
}

Expand Down Expand Up @@ -792,12 +786,7 @@ contract StakingHbbft is Initializable, OwnableUpgradeable, ReentrancyGuardUpgra
revert CannotClaimWithdrawOrderYet(_poolStakingAddress, staker);
}

if (
!_isWithdrawAllowed(
validatorSetContract.miningByStakingAddress(_poolStakingAddress),
staker != _poolStakingAddress
)
) {
if (!areStakeAndWithdrawAllowed()) {
revert WithdrawNotAllowed();
}

Expand Down Expand Up @@ -994,10 +983,7 @@ contract StakingHbbft is Initializable, OwnableUpgradeable, ReentrancyGuardUpgra
function maxWithdrawAllowed(address _poolStakingAddress, address _staker) public view returns (uint256) {
address miningAddress = validatorSetContract.miningByStakingAddress(_poolStakingAddress);

if (
!_isWithdrawAllowed(miningAddress, _poolStakingAddress != _staker) ||
abandonedAndRemoved[_poolStakingAddress]
) {
if (!areStakeAndWithdrawAllowed() || abandonedAndRemoved[_poolStakingAddress]) {
return 0;
}

Expand Down Expand Up @@ -1029,7 +1015,7 @@ contract StakingHbbft is Initializable, OwnableUpgradeable, ReentrancyGuardUpgra
function maxWithdrawOrderAllowed(address _poolStakingAddress, address _staker) public view returns (uint256) {
address miningAddress = validatorSetContract.miningByStakingAddress(_poolStakingAddress);

if (!_isWithdrawAllowed(miningAddress, _poolStakingAddress != _staker)) {
if (!areStakeAndWithdrawAllowed()) {
return 0;
}

Expand Down Expand Up @@ -1320,10 +1306,6 @@ contract StakingHbbft is Initializable, OwnableUpgradeable, ReentrancyGuardUpgra
revert InsufficientStakeAmount(_poolStakingAddress, _staker);
}

if (validatorSetContract.isValidatorBanned(poolMiningAddress)) {
revert PoolMiningBanned(_poolStakingAddress);
}

if (abandonedAndRemoved[_poolStakingAddress]) {
revert PoolAbandoned(_poolStakingAddress);
}
Expand Down Expand Up @@ -1498,26 +1480,6 @@ contract StakingHbbft is Initializable, OwnableUpgradeable, ReentrancyGuardUpgra
return (false, 0);
}

/// @dev Returns `true` if withdrawal from the pool of the specified candidate/validator is allowed at the moment.
/// Used by all withdrawal functions.
/// @param _miningAddress The mining address of the validator's pool.
/// @param _isDelegator Whether the withdrawal is requested by a delegator, not by a candidate/validator.
function _isWithdrawAllowed(address _miningAddress, bool _isDelegator) private view returns (bool) {
if (_isDelegator) {
if (validatorSetContract.areDelegatorsBanned(_miningAddress)) {
// The delegator cannot withdraw from the banned validator pool until the ban is expired
return false;
}
} else {
if (validatorSetContract.isValidatorBanned(_miningAddress)) {
// The banned validator cannot withdraw from their pool until the ban is expired
return false;
}
}

return areStakeAndWithdrawAllowed();
}

function _delegatorRewardShare(
bool _minRewardPercentExceeded,
uint256 _totalStake,
Expand Down
84 changes: 31 additions & 53 deletions contracts/TxPermissionHbbft.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,19 @@ import { IKeyGenHistory } from "./interfaces/IKeyGenHistory.sol";
import { IValidatorSetHbbft } from "./interfaces/IValidatorSetHbbft.sol";
import { IConnectivityTrackerHbbft } from "./interfaces/IConnectivityTrackerHbbft.sol";

import { DEFAULT_BLOCK_GAS_LIMIT, DEFAULT_GAS_PRICE, MIN_BLOCK_GAS_LIMIT } from "./lib/Constants.sol";
import { DEFAULT_BLOCK_GAS_LIMIT, DEFAULT_GAS_PRICE } from "./lib/Constants.sol";
import { ZeroAddress } from "./lib/Errors.sol";

/// @dev Controls the use of zero gas price by validators in service transactions,
/// protecting the network against "transaction spamming" by malicious validators.
/// The protection logic is declared in the `allowedTxTypes` function.
contract TxPermissionHbbft is Initializable, OwnableUpgradeable, ITxPermission, ValueGuards {
struct AllowanceCheckResult {
uint32 mask;
bool knownFunc;
bool cache;
}

// =============================================== Storage ========================================================

// WARNING: since this contract is upgradeable, do not remove
Expand Down Expand Up @@ -61,9 +67,6 @@ contract TxPermissionHbbft is Initializable, OwnableUpgradeable, ITxPermission,

// Function signatures

// bytes4(keccak256("reportMalicious(address,uint256,bytes)"))
bytes4 public constant REPORT_MALICIOUS_SIGNATURE = 0xc476dd40;

// bytes4(keccak256("writePart(uint256,uint256,bytes)"))
bytes4 public constant WRITE_PART_SIGNATURE = 0x2d4de124;

Expand Down Expand Up @@ -220,13 +223,7 @@ contract TxPermissionHbbft is Initializable, OwnableUpgradeable, ITxPermission,
/// The limit can be changed by the owner (typical the DAO)
/// @param _value The new minimum gas price.
function setMinimumGasPrice(uint256 _value) public onlyOwner withinAllowedRange(_value) {
// currently, we do not allow to set the minimum gas price to 0,
// that would open pandoras box, and the consequences of doing that,
// requires deeper research.
if (_value == 0) {
revert InvalidMinGasPrice();
}

// param value validation is done in the {ValueGuards-withinAllowedRange} modifier
minimumGasPrice = _value;

emit GasPriceChanged(_value);
Expand All @@ -235,13 +232,7 @@ contract TxPermissionHbbft is Initializable, OwnableUpgradeable, ITxPermission,
/// @dev set's the block gas limit.
/// IN HBBFT, there must be consens about the block gas limit.
function setBlockGasLimit(uint256 _value) public onlyOwner withinAllowedRange(_value) {
// we make some check that the block gas limit can not be set to low,
// to prevent the chain to be completly inoperatable.
// this value is chosen arbitrarily
if (_value < MIN_BLOCK_GAS_LIMIT) {
revert InvalidBlockGasLimit();
}

// param value validation is done in the {ValueGuards-withinAllowedRange} modifier
blockGasLimit = _value;

emit BlockGasLimitChanged(_value);
Expand Down Expand Up @@ -316,24 +307,6 @@ contract TxPermissionHbbft is Initializable, OwnableUpgradeable, ITxPermission,
}

if (_to == address(validatorSetContract)) {
// The rules for the ValidatorSet contract
if (signature == REPORT_MALICIOUS_SIGNATURE) {
uint256 paramsSize = _data.length - 4 > 64 ? 64 : _data.length - 4;
bytes memory abiParams = _memcpy(_data, paramsSize, 4);

(address maliciousMiningAddress, uint256 blockNumber) = abi.decode(abiParams, (address, uint256));

// The `reportMalicious()` can only be called by the validator's mining address
// when the calling is allowed
// slither-disable-next-line unused-return
(bool callable, ) = validatorSetContract.reportMaliciousCallable(
_sender,
maliciousMiningAddress,
blockNumber
);
return (callable ? CALL : NONE, false);
}

if (signature == ANNOUNCE_AVAILABILITY_SIGNATURE) {
return (validatorSetContract.canCallAnnounceAvailability(_sender) ? CALL : NONE, false);
}
Expand Down Expand Up @@ -430,8 +403,9 @@ contract TxPermissionHbbft is Initializable, OwnableUpgradeable, ITxPermission,
}

if (_to == address(connectivityTracker)) {
if (signature == REPORT_MISSING_CONNECTIVITY_SELECTOR || signature == REPORT_RECONNECT_SELECTOR) {
return _handleCallToConnectivityTracker(_sender, signature, _data);
AllowanceCheckResult memory result = _handleCallToConnectivityTracker(_sender, signature, _data);
if (result.knownFunc) {
return (result.mask, result.cache);
}

// if there is another external call to ConnectivityTracker contracts.
Expand Down Expand Up @@ -503,35 +477,39 @@ contract TxPermissionHbbft is Initializable, OwnableUpgradeable, ITxPermission,
address sender,
bytes4 selector,
bytes memory _calldata
) internal view returns (uint32 typesMask, bool cache) {
) internal view returns (AllowanceCheckResult memory) {
// 3 x 32 bytes calldata args = 96 bytes
uint256 paramsSize = _calldata.length - 4 > 96 ? 96 : _calldata.length - 4;
bytes memory params = _memcpy(_calldata, paramsSize, 4);

(address validator, uint256 blockNumber, bytes32 blockHash) = abi.decode(params, (address, uint256, bytes32));
AllowanceCheckResult memory result = AllowanceCheckResult({ mask: NONE, knownFunc: true, cache: false });

if (selector == REPORT_MISSING_CONNECTIVITY_SELECTOR) {
uint32 mask = NONE;
(address validator, uint256 blockNumber, bytes32 blockHash) = abi.decode(
params,
(address, uint256, bytes32)
);

try connectivityTracker.checkReportMissingConnectivityCallable(sender, validator, blockNumber, blockHash) {
mask = CALL;
result.mask = CALL;
} catch {
mask = NONE;
result.mask = NONE;
}

return (mask, false);
}

if (selector == REPORT_RECONNECT_SELECTOR) {
uint32 mask = NONE;
} else if (selector == REPORT_RECONNECT_SELECTOR) {
(address validator, uint256 blockNumber, bytes32 blockHash) = abi.decode(
params,
(address, uint256, bytes32)
);

try connectivityTracker.checkReportReconnectCallable(sender, validator, blockNumber, blockHash) {
mask = CALL;
result.mask = CALL;
} catch {
mask = NONE;
result.mask = NONE;
}

return (mask, false);
} else {
result.knownFunc = false;
}

return result;
}
}
Loading
Loading