Merge pull request #2210 from hyunsooda/kaia-bridge

[KAIABridge] Added zero address check and prevented guardianless

contracts/contracts/system_contracts/kaiabridge/Bridge.sol

@@ -34,7 +34,13 @@ contract KAIABridge is Initializable, ReentrancyGuardUpgradeable, UUPSUpgradeabl
     /// @param initOperator operator address
     /// @param initGuardian guardian address
     /// @param initJudge Judge contract address
-    function initialize(address initOperator, address initGuardian, address initJudge, uint256 newMaxTryTransfer) public initializer {
+    function initialize(address initOperator, address initGuardian, address initJudge, uint256 newMaxTryTransfer)
+        public
+        initializer
+        notNull(initOperator)
+        notNull(initGuardian)
+        notNull(initJudge)
+    {
         require(IERC165(initOperator).supportsInterface(type(IOperator).interfaceId), "KAIA::Bridger: Operator contract address does not implement IOperator");
         __ReentrancyGuard_init();
         bridgeServiceStarted = block.timestamp;
@@ -292,19 +298,19 @@ contract KAIABridge is Initializable, ReentrancyGuardUpgradeable, UUPSUpgradeabl
     }
 
     /// @dev See {IBridge-changeOperator}
-    function changeOperator(address newOperator) public override onlyGuardian {
+    function changeOperator(address newOperator) public override onlyGuardian notNull(newOperator) {
         emit ChangeOperator(operator, newOperator);
         operator = newOperator;
     }
 
     /// @dev See {IBridge-changeGuardian}
-    function changeGuardian(address newGuardian) public override onlyGuardian {
+    function changeGuardian(address newGuardian) public override onlyGuardian notNull(newGuardian) {
         emit ChangeGuardian(guardian, newGuardian);
         guardian = newGuardian;
     }
 
     /// @dev See {IBridge-changeJudge}
-    function changeJudge(address newJudge) public override onlyGuardian {
+    function changeJudge(address newJudge) public override onlyGuardian notNull(newJudge) {
         emit ChangeJudge(judge, newJudge);
         judge = newJudge;
     }
@@ -465,7 +471,7 @@ contract KAIABridge is Initializable, ReentrancyGuardUpgradeable, UUPSUpgradeabl
         emit KAIACharged(msg.sender, msg.value);
     }
 
-    function burnBridgeBalance() public override onlyGuardian inPause {
+    function burnBridgeBalance() public override onlyGuardian inPause nonReentrant {
         require(block.timestamp > bridgeServicePeriod, "KAIA::Bridge: Service period is not expired yet");
         uint256 bridgeBalance = address(this).balance;
         (bool sent, ) = BURN_TARGET.call{value: bridgeBalance}("");

contracts/contracts/system_contracts/kaiabridge/Guardian.sol

@@ -74,6 +74,7 @@ contract Guardian is Initializable, ReentrancyGuardUpgradeable, UUPSUpgradeable,
         onlyGuardian
         guardianExists(guardian)
     {
+        require(guardians.length > 1, "KAIA::Guardian: Guardian size must be greater than one to remove a guardian");
         isGuardian[guardian] = false;
         for (uint256 i=0; i<guardians.length - 1; i++)
             if (guardians[i] == guardian) {
@@ -94,6 +95,7 @@ contract Guardian is Initializable, ReentrancyGuardUpgradeable, UUPSUpgradeable,
         onlyGuardian
         guardianExists(guardian)
         guardianDoesNotExist(newGuardian)
+        notNull(newGuardian)
     {
         for (uint256 i=0; i<guardians.length; i++) {
             if (guardians[i] == guardian) {
@@ -210,7 +212,7 @@ contract Guardian is Initializable, ReentrancyGuardUpgradeable, UUPSUpgradeable,
         emit Submission(txID);
 
         if (uniqUserTxIndex != 0) {
-            require(userIdx2TxID[uniqUserTxIndex] == 0, "KAIA::Operator: Submission to txID exists");
+            require(userIdx2TxID[uniqUserTxIndex] == 0, "KAIA::Guardian: Submission to txID exists");
             userIdx2TxID[uniqUserTxIndex] = txID;
         }
         return txID;

contracts/contracts/system_contracts/kaiabridge/IBridge.sol

@@ -53,6 +53,11 @@ abstract contract IBridge {
     }
 
     //////////////////// Modifier ////////////////////
+    modifier notNull(address addr) {
+        require(addr != address(0), "KAIA::Guardian: A zero address is not allowed");
+        _;
+    }
+
     modifier onlyOperator {
         require(operator == msg.sender, "KAIA::Bridge: Not an operator");
         _;

contracts/contracts/system_contracts/kaiabridge/Judge.sol

@@ -98,6 +98,7 @@ contract Judge is Initializable, ReentrancyGuardUpgradeable, UUPSUpgradeable, IE
         onlyGuardian
         judgeExists(judge)
         judgeDoesNotExist(newJudge)
+        notNull(newJudge)
     {
         for (uint256 i=0; i<judges.length; i++) {
             if (judges[i] == judge) {
@@ -116,6 +117,7 @@ contract Judge is Initializable, ReentrancyGuardUpgradeable, UUPSUpgradeable, IE
         public
         override
         onlyGuardian
+        notNull(newGuardian)
     {
         emit ChangeGuardian(guardian, newGuardian);
         guardian = newGuardian;
@@ -224,7 +226,7 @@ contract Judge is Initializable, ReentrancyGuardUpgradeable, UUPSUpgradeable, IE
         emit Submission(txID);
 
         if (uniqUserTxIndex != 0) {
-            require(userIdx2TxID[uniqUserTxIndex] == 0, "KAIA::Operator: Submission to txID exists");
+            require(userIdx2TxID[uniqUserTxIndex] == 0, "KAIA::Judge: Submission to txID exists");
             userIdx2TxID[uniqUserTxIndex] = txID;
         }
         return txID;

contracts/contracts/system_contracts/kaiabridge/Operator.sol

@@ -99,6 +99,7 @@ contract Operator is Initializable, ReentrancyGuardUpgradeable, UUPSUpgradeable,
         onlyGuardian
         operatorExists(operator)
         operatorDoesNotExist(newOperator)
+        notNull(newOperator)
     {
         for (uint64 i=0; i<operators.length; i++) {
             if (operators[i] == operator) {
@@ -117,6 +118,7 @@ contract Operator is Initializable, ReentrancyGuardUpgradeable, UUPSUpgradeable,
         public
         override
         onlyGuardian
+        notNull(newGuardian)
     {
         emit ChangeGuardian(guardian, newGuardian);
         guardian = newGuardian;
@@ -127,6 +129,7 @@ contract Operator is Initializable, ReentrancyGuardUpgradeable, UUPSUpgradeable,
         public
         override
         onlyGuardian
+        notNull(newBridge)
     {
         emit ChangeBridge(bridge, newBridge);
         bridge = newBridge;

contracts/contracts/system_contracts/kaiabridge/ReentrancyGuardUpgradeable.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: MIT
 // OpenZeppelin Contracts (last updated v5.0.0) (utils/ReentrancyGuard.sol)
 
-pragma solidity ^0.8.20;
+pragma solidity 0.8.24;
 import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 
 /**

contracts/test/KAIABridge/multisig.ts

@@ -247,6 +247,26 @@ describe("[Multisig Test]", function () {
     expect((await guardian.getGuardians()).length).to.equal(initGuardianLen - 1);
   });
 
+  it("#guardian removal is not available if not guardian size is not greater than one", async function () {
+    let rawTxData = (await guardian.populateTransaction.removeGuardian(guardian4.address)).data;
+    await guardian.connect(guardian1).submitTransaction(guardian.address, rawTxData, 0);
+    await guardian.connect(guardian2).confirmTransaction(guardianTxID);
+    await guardian.connect(guardian3).confirmTransaction(guardianTxID);
+
+    rawTxData = (await guardian.populateTransaction.removeGuardian(guardian3.address)).data;
+    await guardian.connect(guardian1).submitTransaction(guardian.address, rawTxData, 0);
+    await guardian.connect(guardian2).confirmTransaction(guardianTxID + 1);
+    await guardian.connect(guardian3).confirmTransaction(guardianTxID + 1);
+
+    rawTxData = (await guardian.populateTransaction.removeGuardian(guardian2.address)).data;
+    await guardian.connect(guardian1).submitTransaction(guardian.address, rawTxData, 0);
+    await guardian.connect(guardian2).confirmTransaction(guardianTxID + 2);
+
+    rawTxData = (await guardian.populateTransaction.removeGuardian(guardian1.address)).data;
+    await expect(guardian.connect(guardian1).submitTransaction(guardian.address, rawTxData, 0))
+      .to.be.revertedWith("KAIA::Guardian: Guardian size must be greater than one to remove a guardian");
+  });
+
   it("#guardian removal may change the number of required confirmation", async function () {
     let rawTxData = (await guardian.populateTransaction.changeRequirement(4)).data;
     await guardian.connect(guardian1).submitTransaction(guardian.address, rawTxData, 0);