From 4c8af3c628c95e1a322c955d0ccc221454a0d89a Mon Sep 17 00:00:00 2001 From: Roshan Date: Wed, 14 Feb 2024 10:59:45 +0800 Subject: [PATCH 1/9] fix report 8 issue 1 --- contracts/SlashIndicator.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/SlashIndicator.sol b/contracts/SlashIndicator.sol index 611be0c2..b0943715 100644 --- a/contracts/SlashIndicator.sol +++ b/contracts/SlashIndicator.sol @@ -41,7 +41,7 @@ contract SlashIndicator is ISlashIndicator,System,IParamSubscriber, IApplication uint256 public felonySlashRewardRatio; bool public enableMaliciousVoteSlash; - uint256 public constant INIT_FELONY_SLASH_SCOPE = 86400; // 3 days + uint256 public constant INIT_FELONY_SLASH_SCOPE = 28800; // 1 days(block number) uint256 public felonySlashScope; From c990f51d17e8fd33d55e248d66ecc35fb08773c5 Mon Sep 17 00:00:00 2001 From: Roshan Date: Wed, 14 Feb 2024 11:50:14 +0800 Subject: [PATCH 2/9] fix report 8 issue 2 --- contracts/BSCValidatorSet.sol | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contracts/BSCValidatorSet.sol b/contracts/BSCValidatorSet.sol index 7a2e6a93..6e6e5993 100644 --- a/contracts/BSCValidatorSet.sol +++ b/contracts/BSCValidatorSet.sol @@ -934,7 +934,6 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica if (!BytesLib.equal(newVoteAddrs[i], validatorExtraSet[i].voteAddress)) { validatorExtraSet[i].voteAddress = newVoteAddrs[i]; } - currentValidatorSet[i].jailed = newValidatorSet[i].jailed; } } @@ -1122,6 +1121,12 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica address validator; bool isFelony; + for (uint i; i<_validatorSet.length; ++i) { + if (_validatorSet[i].jailed) { + ++numOfFelony; + } + } + // 1. validators exit maintenance uint256 i; // caution: it must calculate workingValidatorCount before _exitMaintenance loop From 74c0dcadfb581a330d66231f85565610625200ac Mon Sep 17 00:00:00 2001 From: Roshan Date: Wed, 14 Feb 2024 11:32:18 +0800 Subject: [PATCH 3/9] fix report 9 issue 2 --- contracts/BSCValidatorSet.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/BSCValidatorSet.sol b/contracts/BSCValidatorSet.sol index 6e6e5993..0ec25397 100644 --- a/contracts/BSCValidatorSet.sol +++ b/contracts/BSCValidatorSet.sol @@ -847,7 +847,7 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica } else if (Memory.compareStrings(key, "burnRatio")) { require(value.length == 32, "length of burnRatio mismatch"); uint256 newBurnRatio = BytesToTypes.bytesToUint256(32, value); - require(newBurnRatio + systemRewardRatio <= BLOCK_FEES_RATIO_SCALE, "the burnRatio plus systemRewardRatio must be no greater than 10000"); + require(newBurnRatio.add(systemRewardRatio) <= BLOCK_FEES_RATIO_SCALE, "the burnRatio plus systemRewardRatio must be no greater than 10000"); burnRatio = newBurnRatio; } else if (Memory.compareStrings(key, "maxNumOfMaintaining")) { require(value.length == 32, "length of maxNumOfMaintaining mismatch"); @@ -884,7 +884,7 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica } else if (Memory.compareStrings(key, "systemRewardRatio")) { require(value.length == 32, "length of systemRewardRatio mismatch"); uint256 newSystemRewardRatio = BytesToTypes.bytesToUint256(32, value); - require(newSystemRewardRatio + burnRatio <= BLOCK_FEES_RATIO_SCALE, "the systemRewardRatio plus burnRatio must be no greater than 10000"); + require(newSystemRewardRatio.add(burnRatio) <= BLOCK_FEES_RATIO_SCALE, "the systemRewardRatio plus burnRatio must be no greater than 10000"); systemRewardRatio = newSystemRewardRatio; } else { require(false, "unknown param"); @@ -1073,11 +1073,11 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica uint256 averageDistribute = income / rest; if (averageDistribute != 0) { for (uint i; i Date: Wed, 14 Feb 2024 11:53:31 +0800 Subject: [PATCH 4/9] fix report 9 issue 4 --- contracts/Staking.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/Staking.sol b/contracts/Staking.sol index 4f0d133e..43077a10 100644 --- a/contracts/Staking.sol +++ b/contracts/Staking.sol @@ -651,6 +651,7 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication { if (isAutoUndelegate) { delegated[recipient] = delegated[recipient].sub(amount); delegatedOfValidator[recipient][validator] = delegatedOfValidator[recipient][validator].sub(amount); + emit undelegateSuccess(recipient, validator, amount); } emit undelegatedReceived(recipient, validator, amount); From ffe4c8e19d6ddb11a32ebd6662a33f95b5a3672b Mon Sep 17 00:00:00 2001 From: Roshan Date: Wed, 14 Feb 2024 11:55:36 +0800 Subject: [PATCH 5/9] fix report 9 issue 7 --- contracts/BSCValidatorSet.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/BSCValidatorSet.sol b/contracts/BSCValidatorSet.sol index 0ec25397..2e2b6260 100644 --- a/contracts/BSCValidatorSet.sol +++ b/contracts/BSCValidatorSet.sol @@ -930,6 +930,7 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica validatorExtraSet[i].isMaintaining = false; validatorExtraSet[i].enterMaintenanceHeight = 0; } else { + currentValidatorSet[i].votingPower = newValidatorSet[i].votingPower; // update the vote address if it is different if (!BytesLib.equal(newVoteAddrs[i], validatorExtraSet[i].voteAddress)) { validatorExtraSet[i].voteAddress = newVoteAddrs[i]; @@ -967,7 +968,7 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica * Vote address is not considered */ function isSameValidator(Validator memory v1, Validator memory v2) private pure returns(bool) { - return v1.consensusAddress == v2.consensusAddress && v1.feeAddress == v2.feeAddress && v1.BBCFeeAddress == v2.BBCFeeAddress && v1.votingPower == v2.votingPower; + return v1.consensusAddress == v2.consensusAddress && v1.feeAddress == v2.feeAddress && v1.BBCFeeAddress == v2.BBCFeeAddress; } function getVoteAddresses(address[] memory validators) internal view returns(bytes[] memory) { From 197d78ab75e63d3ed32c0d641a057e4a3e301296 Mon Sep 17 00:00:00 2001 From: Roshan Date: Thu, 15 Feb 2024 11:40:19 +0800 Subject: [PATCH 6/9] fix ToB report issue 5 --- abi/bscgovernor.abi | 19 ++++++ abi/stakehub.abi | 19 ++++++ contracts/BC_fusion/extension/Protectable.sol | 2 + test/TokenRecoverPortal.t.sol | 11 ++-- test/utils/interface/IBSCGovernor.sol | 1 + test/utils/interface/IStakeHub.sol | 1 + test/utils/interface/ITokenRecoverPortal.sol | 63 ++++++++++++++----- 7 files changed, 95 insertions(+), 21 deletions(-) diff --git a/abi/bscgovernor.abi b/abi/bscgovernor.abi index 93313dbe..601cb50a 100644 --- a/abi/bscgovernor.abi +++ b/abi/bscgovernor.abi @@ -1652,6 +1652,25 @@ ], "anonymous": false }, + { + "type": "event", + "name": "ProtectorChanged", + "inputs": [ + { + "name": "oldProtector", + "type": "address", + "indexed": true, + "internalType": "address" + }, + { + "name": "newProtector", + "type": "address", + "indexed": true, + "internalType": "address" + } + ], + "anonymous": false + }, { "type": "event", "name": "QuorumNumeratorUpdated", diff --git a/abi/stakehub.abi b/abi/stakehub.abi index 6c50cca1..a5f5f02b 100644 --- a/abi/stakehub.abi +++ b/abi/stakehub.abi @@ -1266,6 +1266,25 @@ "inputs": [], "anonymous": false }, + { + "type": "event", + "name": "ProtectorChanged", + "inputs": [ + { + "name": "oldProtector", + "type": "address", + "indexed": true, + "internalType": "address" + }, + { + "name": "newProtector", + "type": "address", + "indexed": true, + "internalType": "address" + } + ], + "anonymous": false + }, { "type": "event", "name": "Redelegated", diff --git a/contracts/BC_fusion/extension/Protectable.sol b/contracts/BC_fusion/extension/Protectable.sol index bd949369..b7272803 100644 --- a/contracts/BC_fusion/extension/Protectable.sol +++ b/contracts/BC_fusion/extension/Protectable.sol @@ -21,6 +21,7 @@ abstract contract Protectable is Initializable { /*----------------- events -----------------*/ event Paused(); event Resumed(); + event ProtectorChanged(address indexed oldProtector, address indexed newProtector); event BlackListed(address indexed target); event UnBlackListed(address indexed target); @@ -96,6 +97,7 @@ abstract contract Protectable is Initializable { /*----------------- internal functions -----------------*/ function _setProtector(address protector) internal { + emit ProtectorChanged(_protector, protector); _protector = protector; } diff --git a/test/TokenRecoverPortal.t.sol b/test/TokenRecoverPortal.t.sol index 690bf931..2106d05e 100644 --- a/test/TokenRecoverPortal.t.sol +++ b/test/TokenRecoverPortal.t.sol @@ -32,7 +32,7 @@ contract TokenRecoverPortalTest is Deployer { address mockUser = address(0x2e9247B67ae885a8dcfBf77Eb6d0e93A32bea24C); bytes mockTokenOwner = hex"b713200f29effb427fb76a185b4ac73ea09a534b"; bytes32 testTokenSymbol = hex"424e420000000000000000000000000000000000000000000000000000000000"; - + address protector = address(0x5C7c4b3ee76D1eD8a4341Ab07D87a2a88d81454A); address approvalAddress = address(0xb26859a7321AB7B2025E5E6a425D697e2eacbFB1); bytes merkleRoot = hex"59bb94f7047904a8fdaec42e4785295167f7fd63742b309afeb84bd71f8e6554"; @@ -64,8 +64,10 @@ contract TokenRecoverPortalTest is Deployer { bytes32 tokenSymbol = testTokenSymbol; uint256 amount = 14188000000; bytes memory ownerPubKey = hex"036d5d41cd7da2e96d39bcbd0390bfed461a86382f7a2923436ff16c65cabc7719"; - bytes memory ownerSignature = hex"5f5391ba7f2b002b4746025f7e803a43e57a397ea66f3939d05302eb7851bbbc0773cda87aae0fbb1e2a29367b606209ed47dc5cba6d1a83f6b79cb70e56efdb"; - bytes memory approvalSignature = hex"52a0a5ca80beb068d82413cac31c1df0540dc6a61eddec9f31b94419e60b6c586e5342552f4c8034a00c876d640abea8c5ba9c4d72145d0e562fedd09fe1e00a01"; + bytes memory ownerSignature = + hex"5f5391ba7f2b002b4746025f7e803a43e57a397ea66f3939d05302eb7851bbbc0773cda87aae0fbb1e2a29367b606209ed47dc5cba6d1a83f6b79cb70e56efdb"; + bytes memory approvalSignature = + hex"52a0a5ca80beb068d82413cac31c1df0540dc6a61eddec9f31b94419e60b6c586e5342552f4c8034a00c876d640abea8c5ba9c4d72145d0e562fedd09fe1e00a01"; bytes32[] memory merkleProof = new bytes32[](17); merkleProof[0] = hex"03719d7863e4aba727d7030e7a1916b9be2245d447eb71fc683d3ac0ded5eecd"; merkleProof[1] = hex"7f9aa9d8246251cbab3cc642416dec81d074d39a85be6ca8326a05ac422e74ab"; @@ -86,8 +88,7 @@ contract TokenRecoverPortalTest is Deployer { merkleProof[16] = hex"a2d456e52facaa953bfbc79a5a6ed7647dda59872b9b35c20183887eeb4640eb"; // recover the token - tokenRecoverPortal.recover( - tokenSymbol, amount, ownerPubKey, ownerSignature, approvalSignature, merkleProof); + tokenRecoverPortal.recover(tokenSymbol, amount, ownerPubKey, ownerSignature, approvalSignature, merkleProof); // check if the token is recovered bytes32 node = keccak256(abi.encodePacked(mockTokenOwner, tokenSymbol, amount)); diff --git a/test/utils/interface/IBSCGovernor.sol b/test/utils/interface/IBSCGovernor.sol index 0efc7948..a0e77b63 100644 --- a/test/utils/interface/IBSCGovernor.sol +++ b/test/utils/interface/IBSCGovernor.sol @@ -46,6 +46,7 @@ interface BSCGovernor { event ProposalExtended(uint256 indexed proposalId, uint64 extendedDeadline); event ProposalQueued(uint256 proposalId, uint256 eta); event ProposalThresholdSet(uint256 oldProposalThreshold, uint256 newProposalThreshold); + event ProtectorChanged(address indexed oldProtector, address indexed newProtector); event QuorumNumeratorUpdated(uint256 oldQuorumNumerator, uint256 newQuorumNumerator); event Resumed(); event TimelockChange(address oldTimelock, address newTimelock); diff --git a/test/utils/interface/IStakeHub.sol b/test/utils/interface/IStakeHub.sol index 24cc0d0b..f2ce9c52 100644 --- a/test/utils/interface/IStakeHub.sol +++ b/test/utils/interface/IStakeHub.sol @@ -65,6 +65,7 @@ interface StakeHub { event MigrateSuccess(address indexed operatorAddress, address indexed delegator, uint256 shares, uint256 bnbAmount); event ParamChange(string key, bytes value); event Paused(); + event ProtectorChanged(address indexed oldProtector, address indexed newProtector); event Redelegated( address indexed srcValidator, address indexed dstValidator, diff --git a/test/utils/interface/ITokenRecoverPortal.sol b/test/utils/interface/ITokenRecoverPortal.sol index 0b83a167..d87e28d2 100644 --- a/test/utils/interface/ITokenRecoverPortal.sol +++ b/test/utils/interface/ITokenRecoverPortal.sol @@ -1,26 +1,57 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.10; -// TokenRecoverPortal anyone to recover a token if they exist in a Beacon Chain. interface TokenRecoverPortal { - // Returns the merkle root of the merkle tree containing account balances available to recover. - function merkleRoot() external view returns (bytes32); - // Returns the address of the contract that is allowed to confirm the recover. + error AlreadyPaused(); + error AlreadyRecovered(); + error ApprovalAddressNotInitialized(); + error InBlackList(); + error InvalidApprovalSignature(); + error InvalidOwnerPubKeyLength(); + error InvalidOwnerSignatureLength(); + error InvalidProof(); + error InvalidValue(string key, bytes value); + error MerkleRootAlreadyInitiated(); + error MerkleRootNotInitialized(); + error NotPaused(); + error OnlyCoinbase(); + error OnlyProtector(); + error OnlySystemContract(address systemContract); + error OnlyZeroGasPrice(); + error TokenRecoverPortalPaused(); + error UnknownParam(string key, bytes value); + + event BlackListed(address indexed target); + event Initialized(uint8 version); + event ParamChange(string key, bytes value); + event Paused(); + event ProtectorChanged(address indexed oldProtector, address indexed newProtector); + event Resumed(); + event TokenRecoverRequested(bytes32 tokenSymbol, address account, uint256 amount); + event UnBlackListed(address indexed target); + + function BC_FUSION_CHANNELID() external view returns (uint8); + function SOURCE_CHAIN_ID() external view returns (string memory); + function STAKING_CHANNELID() external view returns (uint8); + function addToBlackList(address account) external; function approvalAddress() external view returns (address); - // Returns the address of the contract that is allowed to pause the recover. - function assetProtector() external view returns (address); - // Returns true if the index has been marked recovered. - function isRecovered(bytes32 index) external view returns (bool); - // recover the given amount of the token to the given address. Reverts if the inputs are invalid. + function blackList(address) external view returns (bool); + function cancelTokenRecover(bytes32 tokenSymbol, address attacker) external; + function initialize() external; + function isPaused() external view returns (bool); + function isRecovered(bytes32 node) external view returns (bool); + function merkleRoot() external view returns (bytes32); + function merkleRootAlreadyInit() external view returns (bool); + function pause() external; function recover( bytes32 tokenSymbol, uint256 amount, - bytes calldata ownerPubKey, - bytes calldata ownerSignature, - bytes calldata approvalSignature, - bytes32[] calldata merkleProof + bytes memory ownerPubKey, + bytes memory ownerSignature, + bytes memory approvalSignature, + bytes32[] memory merkleProof ) external; - // Cancel the user token recover request by the assetProtector. - function cancelTokenRecover(bytes32 tokenSymbol, address recipient) external; - function initialize() external; + function removeFromBlackList(address account) external; + function resume() external; + function updateParam(string memory key, bytes memory value) external; } From 7d89be26212e989a4cfde4e8ee072373359fa37f Mon Sep 17 00:00:00 2001 From: Roshan Date: Fri, 16 Feb 2024 11:55:19 +0800 Subject: [PATCH 7/9] fix ToB report issue 1 --- contracts/BC_fusion/StakeHub.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/BC_fusion/StakeHub.sol b/contracts/BC_fusion/StakeHub.sol index 09553aaf..95e86e4a 100644 --- a/contracts/BC_fusion/StakeHub.sol +++ b/contracts/BC_fusion/StakeHub.sol @@ -351,7 +351,7 @@ contract StakeHub is System, Initializable, Protectable { ) revert InvalidCommission(); if (!_checkMoniker(description.moniker)) revert InvalidMoniker(); // proof-of-possession verify - if (!_checkVoteAddress(voteAddress, blsProof)) revert InvalidVoteAddress(); + if (!_checkVoteAddress(operatorAddress, voteAddress, blsProof)) revert InvalidVoteAddress(); // deploy stake credit proxy contract address creditContract = _deployStakeCredit(operatorAddress, description.moniker); @@ -458,12 +458,12 @@ contract StakeHub is System, Initializable, Protectable { bytes calldata blsProof ) external whenNotPaused notInBlackList validatorExist(msg.sender) { // proof-of-possession verify - if (!_checkVoteAddress(newVoteAddress, blsProof)) revert InvalidVoteAddress(); + address operatorAddress = msg.sender; + if (!_checkVoteAddress(operatorAddress, newVoteAddress, blsProof)) revert InvalidVoteAddress(); if (voteToOperator[newVoteAddress] != address(0) || _legacyVoteAddress[newVoteAddress]) { revert DuplicateVoteAddress(); } - address operatorAddress = msg.sender; Validator storage valInfo = _validators[operatorAddress]; if (valInfo.updateTime + BREATHE_BLOCK_INTERVAL > block.timestamp) revert UpdateTooFrequently(); @@ -1065,13 +1065,13 @@ contract StakeHub is System, Initializable, Protectable { return true; } - function _checkVoteAddress(bytes calldata voteAddress, bytes calldata blsProof) internal view returns (bool) { + function _checkVoteAddress(address operatorAddress, bytes calldata voteAddress, bytes calldata blsProof) internal view returns (bool) { if (voteAddress.length != BLS_PUBKEY_LENGTH || blsProof.length != BLS_SIG_LENGTH) { return false; } // get msg hash - bytes32 msgHash = keccak256(abi.encodePacked(voteAddress, block.chainid)); + bytes32 msgHash = keccak256(abi.encodePacked(operatorAddress, voteAddress, block.chainid)); bytes memory msgBz = new bytes(32); assembly { mstore(add(msgBz, 32), msgHash) From a454fa374b7ddf58ca012d5547fcb95ed4082515 Mon Sep 17 00:00:00 2001 From: Roshan Date: Fri, 16 Feb 2024 17:18:05 +0800 Subject: [PATCH 8/9] fix review comments --- contracts/BSCValidatorSet.sol | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/contracts/BSCValidatorSet.sol b/contracts/BSCValidatorSet.sol index 2e2b6260..719e7d7a 100644 --- a/contracts/BSCValidatorSet.sol +++ b/contracts/BSCValidatorSet.sol @@ -1122,6 +1122,7 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica address validator; bool isFelony; + // count the number of felony validators before forcing maintaining validators exit for (uint i; i<_validatorSet.length; ++i) { if (_validatorSet[i].jailed) { ++numOfFelony; @@ -1153,8 +1154,10 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica // record the jailed validator in validatorSet for (uint k; k<_validatorSet.length; ++k) { if (_validatorSet[k].consensusAddress == validator) { - _validatorSet[k].jailed = true; - ++numOfFelony; + if (!_validatorSet[k].jailed) { + _validatorSet[k].jailed = true; + ++numOfFelony; + } break; } } From e1bc6ce884292543f712bf26f05eceb2fa424447 Mon Sep 17 00:00:00 2001 From: Roshan Date: Fri, 16 Feb 2024 17:19:14 +0800 Subject: [PATCH 9/9] fix lint issue --- contracts/BC_fusion/StakeHub.sol | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/contracts/BC_fusion/StakeHub.sol b/contracts/BC_fusion/StakeHub.sol index 95e86e4a..b6da1111 100644 --- a/contracts/BC_fusion/StakeHub.sol +++ b/contracts/BC_fusion/StakeHub.sol @@ -1065,7 +1065,11 @@ contract StakeHub is System, Initializable, Protectable { return true; } - function _checkVoteAddress(address operatorAddress, bytes calldata voteAddress, bytes calldata blsProof) internal view returns (bool) { + function _checkVoteAddress( + address operatorAddress, + bytes calldata voteAddress, + bytes calldata blsProof + ) internal view returns (bool) { if (voteAddress.length != BLS_PUBKEY_LENGTH || blsProof.length != BLS_SIG_LENGTH) { return false; }