diff --git a/script/interfaces/ISharedArgument.sol b/script/interfaces/ISharedArgument.sol index 37971d4d..2d4e7374 100644 --- a/script/interfaces/ISharedArgument.sol +++ b/script/interfaces/ISharedArgument.sol @@ -18,6 +18,7 @@ interface ISharedArgument is IGeneralConfig { uint96[] voteWeights; GlobalProposal.TargetOption[] targetOptions; address[] targets; + uint256 minRequiredGovernor; } struct MainchainGatewayV3Param { diff --git a/src/extensions/bridge-operator-governance/BridgeManager.sol b/src/extensions/bridge-operator-governance/BridgeManager.sol index a1541f33..f6bbb8ff 100644 --- a/src/extensions/bridge-operator-governance/BridgeManager.sol +++ b/src/extensions/bridge-operator-governance/BridgeManager.sol @@ -28,6 +28,8 @@ abstract contract BridgeManager is IBridgeManager, HasContracts, BridgeManagerQu mapping(address operator => uint96 weight) _operatorWeight; /// @dev Total weight of all governors / operators. uint256 _totalWeight; + /// @dev The minimum number of governors that must exist in the contract, to avoid the contract become non-accessible. + uint256 _minRequiredGovernor; } // keccak256(abi.encode(uint256(keccak256("ronin.storage.BridgeManagerStorageLocation")) - 1)) & ~bytes32(uint256(0xff)) @@ -71,6 +73,7 @@ abstract contract BridgeManager is IBridgeManager, HasContracts, BridgeManagerQu ); _addBridgeOperators(voteWeights, governors, bridgeOperators); + _setMinRequiredGovernor(3); } // ===================== CONFIG ======================== @@ -83,6 +86,20 @@ abstract contract BridgeManager is IBridgeManager, HasContracts, BridgeManagerQu _setContract(contractType, addr); } + /** + * @inheritdoc IBridgeManager + */ + function setMinRequiredGovernor(uint min) external override onlySelfCall { + _setMinRequiredGovernor(min); + } + + function _setMinRequiredGovernor(uint min) internal { + if (min < 3) revert ErrInvalidInput(); + BridgeManagerStorage storage $ = _getBridgeManagerStorage(); + $._minRequiredGovernor = min; + emit MinRequiredGovernorUpdated(min); + } + /** * @dev Internal function to require that the caller has governor role access. */ @@ -289,6 +306,9 @@ abstract contract BridgeManager is IBridgeManager, HasContracts, BridgeManagerQu // simply skip remove operations if inputs are empty. if (length == 0) return removeds; + if ($._governors.length - length < $._minRequiredGovernor) { + revert ErrBelowMinRequiredGovernors(); + } address iGovernor; address iOperator; diff --git a/src/interfaces/bridge/IBridgeManager.sol b/src/interfaces/bridge/IBridgeManager.sol index ccbe7d16..aa057dac 100644 --- a/src/interfaces/bridge/IBridgeManager.sol +++ b/src/interfaces/bridge/IBridgeManager.sol @@ -14,6 +14,10 @@ interface IBridgeManager is IBridgeManagerEvents { error ErrGovernorNotFound(address governor); /// @notice Error indicating that the msg.sender is not match the required governor error ErrGovernorNotMatch(address required, address sender); + /// @notice Error indicating that the governors list will go below minimum number of required governor. + error ErrBelowMinRequiredGovernors(); + /// @notice Common invalid input error + error ErrInvalidInput(); /** * @dev The domain separator used for computing hash digests in the contract. @@ -187,4 +191,10 @@ interface IBridgeManager is IBridgeManagerEvents { * @param newOperator The new address of the operator. */ function updateBridgeOperator(address currOperator, address newOperator) external; + + /** + * @dev Self-call to update the minimum required governor. + * @param min The minimum number, this must not less than 3. + */ + function setMinRequiredGovernor(uint min) external; } diff --git a/src/interfaces/bridge/events/IBridgeManagerEvents.sol b/src/interfaces/bridge/events/IBridgeManagerEvents.sol index 163d5dea..75748f6a 100644 --- a/src/interfaces/bridge/events/IBridgeManagerEvents.sol +++ b/src/interfaces/bridge/events/IBridgeManagerEvents.sol @@ -26,4 +26,9 @@ interface IBridgeManagerEvents { * @dev Emitted when a bridge operator is updated. */ event BridgeOperatorUpdated(address indexed governor, address indexed fromBridgeOperator, address indexed toBridgeOperator); + + /** + * @dev Emitted when the minimum number of required governors is updated. + */ + event MinRequiredGovernorUpdated(uint min); } diff --git a/src/libraries/AddressArrayUtils.sol b/src/libraries/AddressArrayUtils.sol index 9def728d..60aa7089 100644 --- a/src/libraries/AddressArrayUtils.sol +++ b/src/libraries/AddressArrayUtils.sol @@ -52,13 +52,13 @@ library AddressArrayUtils { c = new address[](lengthA + lengthB); } uint256 i; - for (; i < lengthA; ) { + for (; i < lengthA;) { c[i] = a[i]; unchecked { ++i; } } - for (uint256 j; j < lengthB; ) { + for (uint256 j; j < lengthB;) { c[i] = b[j]; unchecked { ++i; diff --git a/src/libraries/Uint96ArrayUtils.sol b/src/libraries/Uint96ArrayUtils.sol new file mode 100644 index 00000000..18f8d639 --- /dev/null +++ b/src/libraries/Uint96ArrayUtils.sol @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: UNLICENSED + +pragma solidity ^0.8.0; + +library Uint96ArrayUtils { + function extend(uint96[] memory a, uint96[] memory b) internal pure returns (uint96[] memory c) { + uint256 lengthA = a.length; + uint256 lengthB = b.length; + unchecked { + c = new uint96[](lengthA + lengthB); + } + uint256 i; + for (; i < lengthA;) { + c[i] = a[i]; + unchecked { + ++i; + } + } + for (uint256 j; j < lengthB;) { + c[i] = b[j]; + unchecked { + ++i; + ++j; + } + } + } +} diff --git a/src/mainchain/MainchainBridgeManager.sol b/src/mainchain/MainchainBridgeManager.sol index ff9934d5..fbd2c526 100644 --- a/src/mainchain/MainchainBridgeManager.sol +++ b/src/mainchain/MainchainBridgeManager.sol @@ -29,7 +29,7 @@ contract MainchainBridgeManager is BridgeManager, GovernanceRelay, GlobalGoverna CoreGovernance(DEFAULT_EXPIRY_DURATION) GlobalCoreGovernance(targetOptions, targets) BridgeManager(num, denom, roninChainId, bridgeContract, callbackRegisters, bridgeOperators, governors, voteWeights) - {} + { } /** * @dev See `GovernanceRelay-_relayProposal`. @@ -56,13 +56,7 @@ contract MainchainBridgeManager is BridgeManager, GovernanceRelay, GlobalGoverna Ballot.VoteType[] calldata supports_, Signature[] calldata signatures ) external onlyGovernor { - _relayGlobalProposal({ - globalProposal: globalProposal, - supports_: supports_, - signatures: signatures, - domainSeparator: DOMAIN_SEPARATOR, - creator: msg.sender - }); + _relayGlobalProposal({ globalProposal: globalProposal, supports_: supports_, signatures: signatures, domainSeparator: DOMAIN_SEPARATOR, creator: msg.sender }); } /** diff --git a/src/ronin/gateway/RoninBridgeManager.sol b/src/ronin/gateway/RoninBridgeManager.sol index 7c4084c7..3e1452bf 100644 --- a/src/ronin/gateway/RoninBridgeManager.sol +++ b/src/ronin/gateway/RoninBridgeManager.sol @@ -3,7 +3,11 @@ pragma solidity ^0.8.0; import { ContractType, RoleAccess, ErrUnauthorized, BridgeManager } from "../../extensions/bridge-operator-governance/BridgeManager.sol"; import { Ballot, GlobalProposal, Proposal, GovernanceProposal } from "../../extensions/sequential-governance/governance-proposal/GovernanceProposal.sol"; -import { CoreGovernance, GlobalCoreGovernance, GlobalGovernanceProposal } from "../../extensions/sequential-governance/governance-proposal/GlobalGovernanceProposal.sol"; +import { + CoreGovernance, + GlobalCoreGovernance, + GlobalGovernanceProposal +} from "../../extensions/sequential-governance/governance-proposal/GlobalGovernanceProposal.sol"; import { VoteStatusConsumer } from "../../interfaces/consumers/VoteStatusConsumer.sol"; import { ErrQueryForEmptyVote } from "../../utils/CommonErrors.sol"; @@ -25,7 +29,7 @@ contract RoninBridgeManager is BridgeManager, GovernanceProposal, GlobalGovernan CoreGovernance(expiryDuration) GlobalCoreGovernance(targetOptions, targets) BridgeManager(num, denom, roninChainId, bridgeContract, callbackRegisters, bridgeOperators, governors, voteWeights) - {} + { } /** * CURRENT NETWORK @@ -101,21 +105,14 @@ contract RoninBridgeManager is BridgeManager, GovernanceProposal, GlobalGovernan * - The method caller is governor. * */ - function castProposalVoteForCurrentNetwork( - Proposal.ProposalDetail calldata proposal, - Ballot.VoteType support - ) external onlyGovernor { + function castProposalVoteForCurrentNetwork(Proposal.ProposalDetail calldata proposal, Ballot.VoteType support) external onlyGovernor { _castProposalVoteForCurrentNetwork(msg.sender, proposal, support); } /** * @dev See `GovernanceProposal-_castProposalBySignatures`. */ - function castProposalBySignatures( - Proposal.ProposalDetail calldata proposal, - Ballot.VoteType[] calldata supports_, - Signature[] calldata signatures - ) external { + function castProposalBySignatures(Proposal.ProposalDetail calldata proposal, Ballot.VoteType[] calldata supports_, Signature[] calldata signatures) external { _castProposalBySignatures(proposal, supports_, signatures, DOMAIN_SEPARATOR); } @@ -176,12 +173,7 @@ contract RoninBridgeManager is BridgeManager, GovernanceProposal, GlobalGovernan Ballot.VoteType[] calldata supports_, Signature[] calldata signatures ) external { - _castGlobalProposalBySignatures({ - globalProposal: globalProposal, - supports_: supports_, - signatures: signatures, - domainSeparator: DOMAIN_SEPARATOR - }); + _castGlobalProposalBySignatures({ globalProposal: globalProposal, supports_: supports_, signatures: signatures, domainSeparator: DOMAIN_SEPARATOR }); } /** diff --git a/test/bridge/integration/BaseIntegration.t.sol b/test/bridge/integration/BaseIntegration.t.sol index 0be51cd2..858140dd 100644 --- a/test/bridge/integration/BaseIntegration.t.sol +++ b/test/bridge/integration/BaseIntegration.t.sol @@ -28,6 +28,7 @@ import { SignatureConsumer } from "@ronin/contracts/interfaces/consumers/Signatu import { Ballot } from "@ronin/contracts/libraries/Ballot.sol"; import { Transfer as LibTransfer } from "@ronin/contracts/libraries/Transfer.sol"; import { GlobalCoreGovernance } from "@ronin/contracts/extensions/sequential-governance/GlobalCoreGovernance.sol"; +import { IBridgeManager } from "@ronin/contracts/interfaces/bridge/IBridgeManager.sol"; import { IHasContracts } from "@ronin/contracts/interfaces/collections/IHasContracts.sol"; import { IBridgeManagerCallbackRegister } from "@ronin/contracts/interfaces/bridge/IBridgeManagerCallbackRegister.sol"; import { ContractType } from "@ronin/contracts/utils/ContractType.sol"; @@ -298,6 +299,7 @@ contract BaseIntegration_Test is Base_Test { _param.roninBridgeManager.callbackRegisters = wrapAddress(address(_bridgeSlash)); _param.roninBridgeManager.targetOptions = options; _param.roninBridgeManager.targets = targets; + _param.roninBridgeManager.minRequiredGovernor = 3; ISharedArgument.BridgeManagerParam memory param = _param.roninBridgeManager; uint256 length = param.governors.length; @@ -355,6 +357,23 @@ contract BaseIntegration_Test is Base_Test { vm.prank(_param.roninBridgeManager.governors[0]); _roninBridgeManager.proposeGlobalProposalStructAndCastVotes(globalProposal, supports_, signatures); } + + { + // set min governors + GlobalProposal.GlobalProposalDetail memory globalProposal = _roninProposalUtils.createGlobalProposal({ + expiryTimestamp: block.timestamp + 10, + targetOption: GlobalProposal.TargetOption.BridgeManager, + value: 0, + calldata_: abi.encodeCall(IBridgeManager.setMinRequiredGovernor, (_param.roninBridgeManager.minRequiredGovernor)), + gasAmount: 500_000, + nonce: _roninBridgeManager.round(0) + 1 + }); + + SignatureConsumer.Signature[] memory signatures = _roninProposalUtils.generateSignaturesGlobal(globalProposal, _param.test.governorPKs); + + vm.prank(_param.roninBridgeManager.governors[0]); + _roninBridgeManager.proposeGlobalProposalStructAndCastVotes(globalProposal, supports_, signatures); + } } function _constructForMainchainBridgeManager() internal { @@ -369,6 +388,7 @@ contract BaseIntegration_Test is Base_Test { _param.mainchainBridgeManager.callbackRegisters = getEmptyAddressArray(); _param.mainchainBridgeManager.targetOptions = options; _param.mainchainBridgeManager.targets = targets; + _param.mainchainBridgeManager.minRequiredGovernor = 3; ISharedArgument.BridgeManagerParam memory param = _param.mainchainBridgeManager; uint256 length = param.governors.length; @@ -426,6 +446,23 @@ contract BaseIntegration_Test is Base_Test { vm.prank(_param.roninBridgeManager.governors[0]); _mainchainBridgeManager.relayGlobalProposal(globalProposal, supports_, signatures); } + + { + // set min governors + GlobalProposal.GlobalProposalDetail memory globalProposal = _roninProposalUtils.createGlobalProposal({ + expiryTimestamp: block.timestamp + 10, + targetOption: GlobalProposal.TargetOption.BridgeManager, + value: 0, + calldata_: abi.encodeCall(IBridgeManager.setMinRequiredGovernor, (_param.roninBridgeManager.minRequiredGovernor)), + gasAmount: 500_000, + nonce: _mainchainBridgeManager.round(0) + 1 + }); + + SignatureConsumer.Signature[] memory signatures = _mainchainProposalUtils.generateSignaturesGlobal(globalProposal, _param.test.governorPKs); + + vm.prank(_param.roninBridgeManager.governors[0]); + _mainchainBridgeManager.relayGlobalProposal(globalProposal, supports_, signatures); + } } function _mainchainGatewayV3Initialize() internal { @@ -576,7 +613,7 @@ contract BaseIntegration_Test is Base_Test { LibTransfer.Receipt memory receipt, uint256[] memory signerPKs, bytes32 domainSeparator - ) internal pure returns (SignatureConsumer.Signature[] memory sigs) { + ) internal pure returns (SignatureConsumer.Signature[] memory sigs) { sigs = new SignatureConsumer.Signature[](signerPKs.length); for (uint256 i; i < signerPKs.length; i++) { diff --git a/test/bridge/unit/concrete/bridge-manager/BridgeManager.t.sol b/test/bridge/unit/concrete/bridge-manager/BridgeManager.t.sol index 78ed19fe..25498fb2 100644 --- a/test/bridge/unit/concrete/bridge-manager/BridgeManager.t.sol +++ b/test/bridge/unit/concrete/bridge-manager/BridgeManager.t.sol @@ -35,20 +35,26 @@ contract BridgeManager_Unit_Concrete_Test is Base_Test { } function setUp() public virtual { - address[] memory bridgeOperators = new address[](3); + address[] memory bridgeOperators = new address[](5); bridgeOperators[0] = address(0x10000); bridgeOperators[1] = address(0x10001); bridgeOperators[2] = address(0x10002); + bridgeOperators[3] = address(0x10003); + bridgeOperators[4] = address(0x10004); - address[] memory governors = new address[](3); + address[] memory governors = new address[](5); governors[0] = address(0x20000); governors[1] = address(0x20001); governors[2] = address(0x20002); + governors[3] = address(0x20003); + governors[4] = address(0x20004); - uint96[] memory voteWeights = new uint96[](3); + uint96[] memory voteWeights = new uint96[](5); voteWeights[0] = 100; voteWeights[1] = 100; voteWeights[2] = 100; + voteWeights[3] = 100; + voteWeights[4] = 100; for (uint i; i < bridgeOperators.length; i++) { _bridgeOperators.push(bridgeOperators[i]); @@ -56,18 +62,18 @@ contract BridgeManager_Unit_Concrete_Test is Base_Test { _voteWeights.push(voteWeights[i]); } - _totalWeight = 300; - _totalOperator = 3; + _totalWeight = 500; + _totalOperator = 5; _bridgeManager = new MockBridgeManager(bridgeOperators, governors, voteWeights); } function _generateNewOperators() internal pure returns (address[] memory operators, address[] memory governors, uint96[] memory weights) { operators = new address[](1); - operators[0] = address(0x10003); + operators[0] = address(0x10099); governors = new address[](1); - governors[0] = address(0x20003); + governors[0] = address(0x20099); weights = new uint96[](1); weights[0] = 100; diff --git a/test/bridge/unit/concrete/bridge-manager/constructor.t.sol b/test/bridge/unit/concrete/bridge-manager/constructor.t.sol index 06e48220..16dd066f 100644 --- a/test/bridge/unit/concrete/bridge-manager/constructor.t.sol +++ b/test/bridge/unit/concrete/bridge-manager/constructor.t.sol @@ -33,7 +33,7 @@ contract Constructor_BridgeManager_Unit_Concrete_Test is BridgeManager_Unit_Conc comparingWeights: _voteWeights, expectingWeights: voteWeights }); - assertEq(_bridgeManager.totalBridgeOperator(), 3); + assertEq(_bridgeManager.totalBridgeOperator(), 5); assertEq(_bridgeManager.getTotalWeight(), _totalWeight); } diff --git a/test/bridge/unit/concrete/bridge-manager/remove/remove.t.sol b/test/bridge/unit/concrete/bridge-manager/remove/remove.t.sol index 96318586..a37ea153 100644 --- a/test/bridge/unit/concrete/bridge-manager/remove/remove.t.sol +++ b/test/bridge/unit/concrete/bridge-manager/remove/remove.t.sol @@ -20,7 +20,7 @@ contract Remove_Unit_Concrete_Test is BridgeManager_Unit_Concrete_Test { function test_RevertWhen_NotSelfCall() external { // Prepare data - (address[] memory removingOperators, , , , , ) = _generateRemovingOperators(1); + (address[] memory removingOperators,,,,,) = _generateRemovingOperators(1); // Make the caller not self-call. changePrank({ msgSender: _bridgeOperators[0] }); @@ -62,9 +62,8 @@ contract Remove_Unit_Concrete_Test is BridgeManager_Unit_Concrete_Test { _bridgeManager.getGovernorOf(removingOperators[0]); // Compare after and before state - (address[] memory afterBridgeOperators, address[] memory afterGovernors, uint96[] memory afterVoteWeights) = _getBridgeMembersByGovernors( - remainingGovernors - ); + (address[] memory afterBridgeOperators, address[] memory afterGovernors, uint96[] memory afterVoteWeights) = + _getBridgeMembersByGovernors(remainingGovernors); _assertBridgeMembers({ comparingOperators: afterBridgeOperators, @@ -77,7 +76,7 @@ contract Remove_Unit_Concrete_Test is BridgeManager_Unit_Concrete_Test { } function test_RevertWhen_TwoAddress_Duplicated() external { - (address[] memory removingOperators, address[] memory removingGovernors, uint96[] memory removingWeights, , , ) = _generateRemovingOperators(2); + (address[] memory removingOperators, address[] memory removingGovernors, uint96[] memory removingWeights,,,) = _generateRemovingOperators(2); removingOperators[1] = removingOperators[0]; removingGovernors[1] = removingGovernors[0]; @@ -122,9 +121,8 @@ contract Remove_Unit_Concrete_Test is BridgeManager_Unit_Concrete_Test { } // Compare after and before state - (address[] memory afterBridgeOperators, address[] memory afterGovernors, uint96[] memory afterVoteWeights) = _getBridgeMembersByGovernors( - remainingGovernors - ); + (address[] memory afterBridgeOperators, address[] memory afterGovernors, uint96[] memory afterVoteWeights) = + _getBridgeMembersByGovernors(remainingGovernors); _assertBridgeMembers({ comparingOperators: afterBridgeOperators, @@ -135,4 +133,14 @@ contract Remove_Unit_Concrete_Test is BridgeManager_Unit_Concrete_Test { expectingWeights: remainingWeights }); } + + function test_RevertWhen_ThreeAddress_BelowMinRequiredGovernor() external { + uint TO_REMOVE_NUM = 3; + + (address[] memory removingOperators,,,,,) = _generateRemovingOperators(TO_REMOVE_NUM); + + // Run the test. + vm.expectRevert(abi.encodeWithSelector(IBridgeManager.ErrBelowMinRequiredGovernors.selector)); + _bridgeManager.removeBridgeOperators(removingOperators); + } } diff --git a/test/bridge/unit/fuzz/bridge-manager/BridgeManagerCRUD.t.sol b/test/bridge/unit/fuzz/bridge-manager/BridgeManagerCRUD.t.sol index 74a35d36..18380c80 100644 --- a/test/bridge/unit/fuzz/bridge-manager/BridgeManagerCRUD.t.sol +++ b/test/bridge/unit/fuzz/bridge-manager/BridgeManagerCRUD.t.sol @@ -4,7 +4,9 @@ pragma solidity ^0.8.0; import { console } from "forge-std/console.sol"; import { IBridgeManager, BridgeManagerUtils } from "../utils/BridgeManagerUtils.t.sol"; import { RoninGatewayV3 } from "@ronin/contracts/ronin/gateway/RoninGatewayV3.sol"; -import { RoleAccess, ContractType, AddressArrayUtils, MockBridgeManager } from "@ronin/contracts/mocks/ronin/MockBridgeManager.sol"; +import { RoleAccess, ContractType, MockBridgeManager } from "@ronin/contracts/mocks/ronin/MockBridgeManager.sol"; +import "@ronin/contracts/libraries/Uint96ArrayUtils.sol"; +import "@ronin/contracts/libraries/AddressArrayUtils.sol"; import { ErrBridgeOperatorUpdateFailed, ErrBridgeOperatorAlreadyExisted, @@ -16,6 +18,7 @@ import { contract BridgeManagerCRUDTest is BridgeManagerUtils { using AddressArrayUtils for address[]; + using Uint96ArrayUtils for uint96[]; enum InputIndex { VoteWeights, @@ -30,6 +33,10 @@ contract BridgeManagerCRUDTest is BridgeManagerUtils { _label(); } + address[] private _initOperators; + address[] private _initGovernors; + uint96[] private _initWeights; + function testFail_MaliciousUpdateBridgeOperator() external { (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = getValidInputs(DEFAULT_R1, DEFAULT_R2, DEFAULT_R3, DEFAULT_NUM_BRIDGE_OPERATORS); @@ -60,7 +67,8 @@ contract BridgeManagerCRUDTest is BridgeManagerUtils { ) external virtual { vm.assume(caller != _bridgeManager); - (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = getValidInputs(r1, r2, r3, numBridgeOperators); + (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = + getValidAndNonExistingInputs(_bridgeManager, r1, r2, r3, numBridgeOperators); vm.expectRevert(abi.encodeWithSelector(ErrUnexpectedInternalCall.selector, IBridgeManager.addBridgeOperators.selector, ContractType.BRIDGE, caller)); @@ -71,11 +79,13 @@ contract BridgeManagerCRUDTest is BridgeManagerUtils { * @notice Checks whether bridge contract can add bridge operators. */ function test_AddBridgeOperators_CallerIsBridgeAdminOperator(uint256 r1, uint256 r2, uint256 r3, uint256 numBridgeOperators) external virtual { - (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = getValidInputs(r1, r2, r3, numBridgeOperators); + (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = + getValidAndNonExistingInputs(_bridgeManager, r1, r2, r3, numBridgeOperators); IBridgeManager bridgeManager = _addBridgeOperators(_bridgeManager, _bridgeManager, voteWeights, governors, bridgeOperators); - _invariantTest(bridgeManager, voteWeights, governors, bridgeOperators); + _invariantTest(bridgeManager, _initWeights.extend(voteWeights), _initGovernors.extend(governors), _initOperators.extend(bridgeOperators)); + // _invariantTest(bridgeManager, voteWeights, governors, bridgeOperators); } /** @@ -111,10 +121,16 @@ contract BridgeManagerCRUDTest is BridgeManagerUtils { * @notice Checks whether bridge contract can remove bridge operators. */ function test_RemoveBridgeOperators_CallerIsBridgeContract(uint256 r1, uint256 r2, uint256 r3, uint16 numBridgeOperators) external virtual { - (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = getValidInputs(r1, r2, r3, numBridgeOperators); + (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = + getValidAndNonExistingInputs(_bridgeManager, r1, r2, r3, numBridgeOperators); IBridgeManager bridgeManager = _addBridgeOperators(_bridgeManager, _bridgeManager, voteWeights, governors, bridgeOperators); - uint256 removeAmount = _randomize(voteWeights.length, 1, voteWeights.length); + + bridgeOperators = _initOperators.extend(bridgeOperators); + governors = _initGovernors.extend(governors); + voteWeights = _initWeights.extend(voteWeights); + + uint256 removeAmount = _randomize(voteWeights.length, 1, voteWeights.length - 3); // Keep at least 3 governors uint256 tailIdx = voteWeights.length - 1; uint256 r = _randomize(_triShuffle(r1, r2, r3), 0, tailIdx); @@ -158,7 +174,8 @@ contract BridgeManagerCRUDTest is BridgeManagerUtils { */ function testFuzz_UpdateBridgeOperator_CallerIsGovernor(uint256 r1, uint256 r2, uint256 r3, uint16 numBridgeOperators) external virtual { vm.skip(true); - (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = getValidInputs(r1, r2, r3, numBridgeOperators); + (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = + getValidAndNonExistingInputs(_bridgeManager, r1, r2, r3, numBridgeOperators); IBridgeManager bridgeManager = _addBridgeOperators(_bridgeManager, _bridgeManager, voteWeights, governors, bridgeOperators); uint256 randomSeed = _randomize(_triShuffle(r1, r2, r3), 0, voteWeights.length - 1); @@ -186,7 +203,8 @@ contract BridgeManagerCRUDTest is BridgeManagerUtils { */ function test_UpdateBridgeOperator_CallerIsNotGovernor(uint256 r1, uint256 r2, uint256 r3, uint16 numBridgeOperators) external virtual { vm.skip(true); - (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = getValidInputs(r1, r2, r3, numBridgeOperators); + (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = + getValidAndNonExistingInputs(_bridgeManager, r1, r2, r3, numBridgeOperators); IBridgeManager bridgeManager = _addBridgeOperators(_bridgeManager, _bridgeManager, voteWeights, governors, bridgeOperators); address unauthorizedCaller = makeAddr("UNAUTHORIZED_CALLER"); @@ -210,8 +228,11 @@ contract BridgeManagerCRUDTest is BridgeManagerUtils { _bridgeManager = address(new MockBridgeManager(bridgeOperators, governors, voteWeights)); // empty storage for testing - vm.prank(_bridgeManager); - IBridgeManager(_bridgeManager).removeBridgeOperators(bridgeOperators); + // vm.prank(_bridgeManager); + // IBridgeManager(_bridgeManager).removeBridgeOperators(bridgeOperators); + _initOperators = bridgeOperators; + _initGovernors = governors; + _initWeights = voteWeights; } function _label() internal virtual { diff --git a/test/bridge/unit/fuzz/bridge-manager/BridgeSlash.t.sol b/test/bridge/unit/fuzz/bridge-manager/BridgeSlash.t.sol index b166af7b..3f40d9fc 100644 --- a/test/bridge/unit/fuzz/bridge-manager/BridgeSlash.t.sol +++ b/test/bridge/unit/fuzz/bridge-manager/BridgeSlash.t.sol @@ -110,9 +110,6 @@ contract BridgeSlashTest is IBridgeSlashEvents, BridgeManagerUtils { uint256 numBridgeOperators, uint256 period ) external { - address[] memory currentOperators = IBridgeManager(_bridgeManagerContract).getBridgeOperators(); - vm.prank(_bridgeManagerContract, _bridgeManagerContract); - IBridgeManager(_bridgeManagerContract).removeBridgeOperators(currentOperators); // Assume the input values are not equal to the default values vm.assume(r1 != DEFAULT_R1 && r2 != DEFAULT_R2 && r3 != DEFAULT_R3); // Bound the period between 1 and the maximum value of uint64 @@ -128,7 +125,8 @@ contract BridgeSlashTest is IBridgeSlashEvents, BridgeManagerUtils { MockBridgeManager(payable(_bridgeManagerContract)).registerCallbacks(registers); // Generate valid inputs for bridge operators - (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = getValidInputs( + (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = getValidAndNonExistingInputs( + _bridgeManagerContract, r1, r2, r3, @@ -288,7 +286,7 @@ contract BridgeSlashTest is IBridgeSlashEvents, BridgeManagerUtils { } function _setUp() internal virtual { - _admin = vm.addr(1); + _admin = makeAddr("central-admin"); _validatorContract = address(new MockValidatorSet_ForFoundryTest()); (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) = getValidInputs( DEFAULT_R1, diff --git a/test/bridge/unit/fuzz/utils/BridgeManagerUtils.t.sol b/test/bridge/unit/fuzz/utils/BridgeManagerUtils.t.sol index 6fae02cd..fb0c5de2 100644 --- a/test/bridge/unit/fuzz/utils/BridgeManagerUtils.t.sol +++ b/test/bridge/unit/fuzz/utils/BridgeManagerUtils.t.sol @@ -40,6 +40,17 @@ abstract contract BridgeManagerUtils is Randomizer { bridgeManager.addBridgeOperators(voteWeights, governors, bridgeOperators); } + function getValidAndNonExistingInputs( + address bridgeManager, + uint256 r1, + uint256 r2, + uint256 r3, + uint256 numBridgeOperators + ) public virtual returns (address[] memory bridgeOperators, address[] memory governors, uint96[] memory voteWeights) { + (bridgeOperators, governors, voteWeights) = getValidInputs(r1, r2, r3, numBridgeOperators); + _ensureNonExistingInputs(bridgeManager, bridgeOperators.extend(governors)); + } + function getValidInputs( uint256 r1, uint256 r2, @@ -104,20 +115,14 @@ abstract contract BridgeManagerUtils is Randomizer { modifiedInputIdx == 0 ? uintVoteWeights : modifiedInputIdx == 1 ? uintGovernors : uintBridgeOperators ) ); - (outputs, ) = abi.decode(returnData, (uint256[], uint256[])); + (outputs,) = abi.decode(returnData, (uint256[], uint256[])); } // point outputs to modified inputs assembly { - if iszero(modifiedInputIdx) { - voteWeights := outputs - } - if eq(modifiedInputIdx, 1) { - governors := outputs - } - if eq(modifiedInputIdx, 2) { - bridgeOperators := outputs - } + if iszero(modifiedInputIdx) { voteWeights := outputs } + if eq(modifiedInputIdx, 1) { governors := outputs } + if eq(modifiedInputIdx, 2) { bridgeOperators := outputs } } } @@ -137,7 +142,7 @@ abstract contract BridgeManagerUtils is Randomizer { // bound index to range [0, inputLength - 1] inputLength--; - for (uint256 i; i < duplicateAmount; ) { + for (uint256 i; i < duplicateAmount;) { r1 = _randomize(seed, 0, inputLength); r2 = _randomize(r1, 0, inputLength); vm.assume(r1 != r2); @@ -171,7 +176,7 @@ abstract contract BridgeManagerUtils is Randomizer { uint256 r; nullifyIndices = new uint256[](nullAmount); - for (uint256 i; i < nullAmount; ) { + for (uint256 i; i < nullAmount;) { r = _randomize(seed, 0, inputLength); delete inputs[r]; nullifyIndices[i] = r; @@ -211,12 +216,13 @@ abstract contract BridgeManagerUtils is Randomizer { _ensureNonZero(uintBridgeOperators); _ensureNonDuplicated(governors.extend(bridgeOperators)); + // _ensureNotExisted(governors.extend(bridgeOperators)); } function _ensureNonZero(uint256[] memory arr) internal pure { uint256 length = arr.length; - for (uint256 i; i < length; ) { + for (uint256 i; i < length;) { vm.assume(arr[i] != 0); unchecked { ++i; @@ -232,6 +238,13 @@ abstract contract BridgeManagerUtils is Randomizer { vm.assume(!addrs.hasDuplicate()); } + function _ensureNonExistingInputs(address bridgeManager, address[] memory addrs) internal view { + for (uint i; i < addrs.length; i++) { + vm.assume(!IBridgeManager(bridgeManager).isBridgeOperator(addrs[i])); + vm.assume(IBridgeManager(bridgeManager).getGovernorWeight(addrs[i]) == 0); + } + } + function _invariantTest( IBridgeManager bridgeManager, uint96[] memory voteWeights, @@ -244,7 +257,7 @@ abstract contract BridgeManagerUtils is Randomizer { assertEq(bridgeOperators.length, bridgeManager.totalBridgeOperator()); uint256 totalWeight; - for (uint256 i; i < voteWeights.length; ) { + for (uint256 i; i < voteWeights.length;) { totalWeight += voteWeights[i]; unchecked { ++i; diff --git a/test/mocks/MockBridgeManager.sol b/test/mocks/MockBridgeManager.sol index 8ff151d7..3fa77340 100644 --- a/test/mocks/MockBridgeManager.sol +++ b/test/mocks/MockBridgeManager.sol @@ -4,47 +4,45 @@ pragma solidity >=0.8.17 <0.9.0; import { IBridgeManager } from "@ronin/contracts/interfaces/bridge/IBridgeManager.sol"; contract MockBridgeManager is IBridgeManager { - function DOMAIN_SEPARATOR() external view returns (bytes32) {} + function DOMAIN_SEPARATOR() external view returns (bytes32) { } function addBridgeOperators( uint96[] calldata voteWeights, address[] calldata governors, address[] calldata bridgeOperators - ) external returns (bool[] memory addeds) {} + ) external returns (bool[] memory addeds) { } - function getBridgeOperatorOf(address[] calldata gorvernors) external view returns (address[] memory bridgeOperators_) {} + function setMinRequiredGovernor(uint min) external { } - function getOperatorOf(address governor) external view returns (address operator) {} + function getBridgeOperatorOf(address[] calldata gorvernors) external view returns (address[] memory bridgeOperators_) { } - function getBridgeOperatorWeight(address bridgeOperator) external view returns (uint96 weight) {} + function getOperatorOf(address governor) external view returns (address operator) { } - function getBridgeOperators() external view returns (address[] memory) {} + function getBridgeOperatorWeight(address bridgeOperator) external view returns (uint96 weight) { } - function getFullBridgeOperatorInfos() - external - view - returns (address[] memory governors, address[] memory bridgeOperators, uint96[] memory weights) - {} + function getBridgeOperators() external view returns (address[] memory) { } - function getGovernorWeight(address governor) external view returns (uint96) {} + function getFullBridgeOperatorInfos() external view returns (address[] memory governors, address[] memory bridgeOperators, uint96[] memory weights) { } - function getGovernorWeights(address[] calldata governors) external view returns (uint96[] memory weights) {} + function getGovernorWeight(address governor) external view returns (uint96) { } - function getGovernors() external view returns (address[] memory) {} + function getGovernorWeights(address[] calldata governors) external view returns (uint96[] memory weights) { } - function getGovernorsOf(address[] calldata bridgeOperators) external view returns (address[] memory governors) {} + function getGovernors() external view returns (address[] memory) { } - function getGovernorOf(address operator) external view returns (address governor) {} + function getGovernorsOf(address[] calldata bridgeOperators) external view returns (address[] memory governors) { } - function getTotalWeight() external view returns (uint256) {} + function getGovernorOf(address operator) external view returns (address governor) { } - function isBridgeOperator(address addr) external view returns (bool) {} + function getTotalWeight() external view returns (uint256) { } - function removeBridgeOperators(address[] calldata bridgeOperators) external returns (bool[] memory removeds) {} + function isBridgeOperator(address addr) external view returns (bool) { } - function sumGovernorsWeight(address[] calldata governors) external view returns (uint256 sum) {} + function removeBridgeOperators(address[] calldata bridgeOperators) external returns (bool[] memory removeds) { } - function totalBridgeOperator() external view returns (uint256) {} + function sumGovernorsWeight(address[] calldata governors) external view returns (uint256 sum) { } - function updateBridgeOperator(address currOperator, address newOperator) external {} + function totalBridgeOperator() external view returns (uint256) { } + + function updateBridgeOperator(address currOperator, address newOperator) external { } }