From 2760da1ded71159d8b681b75a3d2a797586e81d4 Mon Sep 17 00:00:00 2001 From: Kunal Arora <55632507+aroralanuk@users.noreply.github.com> Date: Tue, 22 Oct 2024 12:40:09 +0530 Subject: [PATCH] fix: ensure no duplicate signatures verified for `AbstractWeightedMultisigIsm.verify()` (#4468) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Description - There's a bug in the verify function of `AbstractWeightedMultisigIsm` that an attacker might use to bypass the verification of multiple signatures. The code tried to check the duplication of the signers if not found, but the code does not increment validatorIndex when the recovered signer matches to the stored signer. For instance: - validatorsAndThresholdWeight returns [A, B, C, D] - an attacker uses signatures as [sig from A, sig from A, sig from A, sig from A, ...] or [sig from B, sig from B, …] Fix is to add a ++validatorIndex at the end of the for loop implies we don't allow the next signer to be the same as the signer we just verified. ### Drive-by changes None ### Related issues From Chainlight's audit findings ### Backward compatibility We haven't deployed these contracts yet on testnet/mainnet ### Testing Fuzz testing --- .../multisig/AbstractWeightedMultisigIsm.sol | 12 ++- solidity/test/isms/MultisigIsm.t.sol | 26 +++++++ solidity/test/isms/WeightedMultisigIsm.t.sol | 75 ++++++++++++++++++- 3 files changed, 107 insertions(+), 6 deletions(-) diff --git a/solidity/contracts/isms/multisig/AbstractWeightedMultisigIsm.sol b/solidity/contracts/isms/multisig/AbstractWeightedMultisigIsm.sol index 2264b192c1..8b7fee0497 100644 --- a/solidity/contracts/isms/multisig/AbstractWeightedMultisigIsm.sol +++ b/solidity/contracts/isms/multisig/AbstractWeightedMultisigIsm.sol @@ -73,11 +73,14 @@ abstract contract AbstractStaticWeightedMultisigIsm is // assumes that signatures are ordered by validator for ( - uint256 i = 0; - _totalWeight < _thresholdWeight && i < _validatorCount; - ++i + uint256 signatureIndex = 0; + _totalWeight < _thresholdWeight && signatureIndex < _validatorCount; + ++signatureIndex ) { - address _signer = ECDSA.recover(_digest, signatureAt(_metadata, i)); + address _signer = ECDSA.recover( + _digest, + signatureAt(_metadata, signatureIndex) + ); // loop through remaining validators until we find a match while ( _validatorIndex < _validatorCount && @@ -90,6 +93,7 @@ abstract contract AbstractStaticWeightedMultisigIsm is // add the weight of the current validator _totalWeight += _validators[_validatorIndex].weight; + ++_validatorIndex; } require( _totalWeight >= _thresholdWeight, diff --git a/solidity/test/isms/MultisigIsm.t.sol b/solidity/test/isms/MultisigIsm.t.sol index 8c0b61d380..29098475fa 100644 --- a/solidity/test/isms/MultisigIsm.t.sol +++ b/solidity/test/isms/MultisigIsm.t.sol @@ -186,6 +186,32 @@ abstract contract AbstractMultisigIsmTest is Test { metadata[index] = ~metadata[index]; assertFalse(ism.verify(metadata, message)); } + + function test_verify_revertWhen_duplicateSignatures( + uint32 destination, + bytes32 recipient, + bytes calldata body, + uint8 m, + uint8 n, + bytes32 seed + ) public virtual { + vm.assume(1 < m && m <= n && n < 10); + bytes memory message = getMessage(destination, recipient, body); + bytes memory metadata = getMetadata(m, n, seed, message); + + bytes memory duplicateMetadata = new bytes(metadata.length); + for (uint256 i = 0; i < metadata.length - 65; i++) { + duplicateMetadata[i] = metadata[i]; + } + for (uint256 i = 0; i < 65; i++) { + duplicateMetadata[metadata.length - 65 + i] = metadata[ + metadata.length - 130 + i + ]; + } + + vm.expectRevert("!threshold"); + ism.verify(duplicateMetadata, message); + } } contract MerkleRootMultisigIsmTest is AbstractMultisigIsmTest { diff --git a/solidity/test/isms/WeightedMultisigIsm.t.sol b/solidity/test/isms/WeightedMultisigIsm.t.sol index df2a3d0ea4..0c5fd7ee4b 100644 --- a/solidity/test/isms/WeightedMultisigIsm.t.sol +++ b/solidity/test/isms/WeightedMultisigIsm.t.sol @@ -65,7 +65,6 @@ abstract contract AbstractStaticWeightedMultisigIsmTest is } } - // ism = IInterchainSecurityModule(deployedIsm); ism = IInterchainSecurityModule( weightedFactory.deploy(validators, threshold) ); @@ -136,7 +135,7 @@ abstract contract AbstractStaticWeightedMultisigIsmTest is return metadata; } - function testVerify_revertInsufficientWeight( + function test_verify_revertInsufficientWeight( uint32 destination, bytes32 recipient, bytes calldata body, @@ -161,6 +160,34 @@ abstract contract AbstractStaticWeightedMultisigIsmTest is vm.expectRevert("Insufficient validator weight"); ism.verify(insufficientMetadata, message); } + + function test_verify_revertWhen_duplicateSignatures( + uint32 destination, + bytes32 recipient, + bytes calldata body, + uint8 m, + uint8 n, + bytes32 seed + ) public virtual override { + vm.assume(1 < m && m <= n && n < 10); + bytes memory message = getMessage(destination, recipient, body); + bytes memory metadata = getMetadata(m, n, seed, message); + + bytes memory duplicateMetadata = new bytes(metadata.length); + for (uint256 i = 0; i < metadata.length - 65; i++) { + duplicateMetadata[i] = metadata[i]; + } + for (uint256 i = 0; i < 65; i++) { + duplicateMetadata[metadata.length - 65 + i] = metadata[ + metadata.length - 130 + i + ]; + } + + if (weightedIsm.signatureCount(metadata) >= 2) { + vm.expectRevert("Invalid signer"); + ism.verify(duplicateMetadata, message); + } + } } contract StaticMerkleRootWeightedMultisigIsmTest is @@ -194,6 +221,28 @@ contract StaticMerkleRootWeightedMultisigIsmTest is message ); } + + function test_verify_revertWhen_duplicateSignatures( + uint32 destination, + bytes32 recipient, + bytes calldata body, + uint8 m, + uint8 n, + bytes32 seed + ) + public + override(AbstractMultisigIsmTest, AbstractStaticWeightedMultisigIsmTest) + { + AbstractStaticWeightedMultisigIsmTest + .test_verify_revertWhen_duplicateSignatures( + destination, + recipient, + body, + m, + n, + seed + ); + } } contract StaticMessageIdWeightedMultisigIsmTest is @@ -227,4 +276,26 @@ contract StaticMessageIdWeightedMultisigIsmTest is message ); } + + function test_verify_revertWhen_duplicateSignatures( + uint32 destination, + bytes32 recipient, + bytes calldata body, + uint8 m, + uint8 n, + bytes32 seed + ) + public + override(AbstractMultisigIsmTest, AbstractStaticWeightedMultisigIsmTest) + { + AbstractStaticWeightedMultisigIsmTest + .test_verify_revertWhen_duplicateSignatures( + destination, + recipient, + body, + m, + n, + seed + ); + } }