fix(governance): add check for strict Multisig threshold validation (#…

…177) * feat: add check to incorporate strict signatures length validation * test: add test for strict signatures length validation * refactor: update error name for exessive signature provided * refactor: add comment stating operator is optional param * refactor: resolve PR comments * refactor: update comment for redundant signatures check Co-authored-by: Milap Sheth <[email protected]> * refactor: check for redundant signatures Co-authored-by: Milap Sheth <[email protected]> * refactor: comment for proof validation condition Co-authored-by: Milap Sheth <[email protected]> * refactor: update comment for optional parameter operator Co-authored-by: Milap Sheth <[email protected]> * test: add test to deploy gateway without operator * fix: failing operator test * Update contracts/gateway/AxelarAmplifierGateway.sol * refactor: remove address check from transfer operatorship * test: add test to transfer operatorship to address zero * refactor: add comment for transferring operator role to address 0 * refactor: remove unused error --------- Co-authored-by: Milap Sheth <[email protected]>
axelarnetwork · Jul 3, 2024 · 1db893b · 1db893b
1 parent ddff2f9
commit 1db893b
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 21 deletions.
diff --git a/contracts/gateway/AxelarAmplifierGateway.sol b/contracts/gateway/AxelarAmplifierGateway.sol
@@ -53,6 +53,8 @@ contract AxelarAmplifierGateway is BaseAmplifierGateway, BaseWeightedMultisig, U
     function _setup(bytes calldata data) internal override {
         (address operator_, WeightedSigners[] memory signers) = abi.decode(data, (address, WeightedSigners[]));
 
+        // operator is an optional parameter. The gateway owner can set it later via `transferOperatorship` if needed.
+        // This also simplifies setup for upgrades so if the current operator doesn't need to be changed, then it can be skipped, instead of having to specify the current operator again.
         if (operator_ != address(0)) {
             _transferOperatorship(operator_);
         }
@@ -124,11 +126,10 @@ contract AxelarAmplifierGateway is BaseAmplifierGateway, BaseWeightedMultisig, U
 
     /**
      * @notice Transfer the operatorship to a new address.
+     * @dev The owner or current operator can set the operator to address 0.
      * @param newOperator The address of the new operator.
      */
     function transferOperatorship(address newOperator) external onlyOperatorOrOwner {
-        if (newOperator == address(0)) revert InvalidOperator();
-
         _transferOperatorship(newOperator);
     }
 

diff --git a/contracts/governance/BaseWeightedMultisig.sol b/contracts/governance/BaseWeightedMultisig.sol
@@ -107,6 +107,7 @@ abstract contract BaseWeightedMultisig is IBaseWeightedMultisig {
      * @param proof The multisig proof data
      * @return isLatestSigners True if the proof is from the latest signer set
      * @dev The proof data should have signers, weights, threshold and signatures encoded
+     *      The proof is only valid if the signers weight crosses the threshold and there are no redundant signatures
      *      The signers and signatures should be sorted by signer address in ascending order
      *      Example: abi.encode([0x11..., 0x22..., 0x33...], [1, 1, 1], 2, [signature1, signature3])
      */
@@ -216,8 +217,13 @@ abstract contract BaseWeightedMultisig is IBaseWeightedMultisig {
             // accumulating signatures weight
             totalWeight = totalWeight + signers[signerIndex].weight;
 
-            // weight needs to reach or surpass threshold
-            if (totalWeight >= weightedSigners.threshold) return;
+            // weight needs to reach threshold
+            if (totalWeight >= weightedSigners.threshold) {
+                // validate the proof if there are no redundant signatures
+                if (i + 1 == signaturesLength) return;
+
+                revert RedundantSignaturesProvided(i + 1, signaturesLength);
+            }
 
             // increasing signers index if match was found
             ++signerIndex;

diff --git a/contracts/interfaces/IAxelarAmplifierGateway.sol b/contracts/interfaces/IAxelarAmplifierGateway.sol
@@ -16,7 +16,6 @@ import { Message } from '../types/AmplifierGatewayTypes.sol';
 interface IAxelarAmplifierGateway is IBaseAmplifierGateway, IBaseWeightedMultisig, IUpgradable {
     error NotLatestSigners();
     error InvalidSender(address sender);
-    error InvalidOperator();
 
     event OperatorshipTransferred(address newOperator);
 

diff --git a/contracts/interfaces/IBaseWeightedMultisig.sol b/contracts/interfaces/IBaseWeightedMultisig.sol
@@ -9,6 +9,7 @@ interface IBaseWeightedMultisig {
     error LowSignaturesWeight();
     error InvalidWeights();
     error DuplicateSigners(bytes32 signersHash);
+    error RedundantSignaturesProvided(uint256 required, uint256 provided);
     error InsufficientRotationDelay(uint256 minimumRotationDelay, uint256 lastRotationTimestamp, uint256 timeElapsed);
 
     event SignersRotated(uint256 indexed epoch, bytes32 indexed signersHash, bytes signers);

diff --git a/test/gateway/AxelarAmplifierGateway.js b/test/gateway/AxelarAmplifierGateway.js
@@ -212,6 +212,14 @@ describe('AxelarAmplifierGateway', () => {
             expect(await gateway.operator()).to.equal(operator.address);
         });
 
+        it('should allow transferring operatorship to the zero address', async () => {
+            await expect(gateway.connect(owner).transferOperatorship(AddressZero))
+                .to.emit(gateway, 'OperatorshipTransferred')
+                .withArgs(AddressZero);
+
+            expect(await gateway.operator()).to.equal(AddressZero);
+        });
+
         it('reject transferring ownership by non-owner', async () => {
             await expectRevert(
                 (gasOptions) => gateway.connect(operator).transferOwnership(operator.address, gasOptions),
@@ -228,14 +236,6 @@ describe('AxelarAmplifierGateway', () => {
                 [user.address],
             );
         });
-
-        it('reject transferring operatorship to the zero address', async () => {
-            await expectRevert(
-                (gasOptions) => gateway.connect(operator).transferOperatorship(AddressZero, gasOptions),
-                gateway,
-                'InvalidOperator',
-            );
-        });
     });
 
     describe('validate message', async () => {

diff --git a/test/governance/BaseWeightedMultisig.js b/test/governance/BaseWeightedMultisig.js
@@ -374,14 +374,6 @@ describe('BaseWeightedMultisig', () => {
                 await multisig.validateProofCall(dataHash, proof).then((tx) => tx.wait());
             });
 
-            it('validate the proof from the current signers with extra signatures', async () => {
-                // sign with all signers, i.e more than threshold
-                const proof = await getWeightedSignersProof(data, domainSeparator, weightedSigners, signers);
-
-                const isCurrentSigners = await multisig.validateProof(dataHash, proof);
-                expect(isCurrentSigners).to.be.true;
-            });
-
             it('validate the proof with last threshold signers', async () => {
                 const proof = await getWeightedSignersProof(
                     data,
@@ -481,6 +473,18 @@ describe('BaseWeightedMultisig', () => {
                     'LowSignaturesWeight',
                 );
             });
+
+            it('reject the proof if there are redundant signatures', async () => {
+                // sign with all signers, i.e more than threshold
+                const proof = await getWeightedSignersProof(data, domainSeparator, weightedSigners, signers);
+
+                await expectRevert(
+                    async (gasOptions) => multisig.validateProof(dataHash, proof),
+                    multisig,
+                    'RedundantSignaturesProvided',
+                    [signers.slice(0, threshold), proof.signatures.length],
+                );
+            });
         });
     });