A malicious signer can freeze usage of multi-sig governance functions

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/auth/MultisigBase.sol#L142 https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/auth/MultisigBase.sol#L149 https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/governance/AxelarServiceGovernance.sol#L48 https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/governance/Multisig.sol#L30

Vulnerability details

Impact

Since signers are trusted entities, a malicious signer has access to call the rotateSigners() function. This means the attacker can set a random smart contract address or his own EOA account to be a signer while erasing all the previous signers. This prevents anyone from calling the rotateSigners() function in the MultiSigBase.sol contract. Additionally, the executeMultisigProposal() function in AxelarServiceGovernance.sol contract and the execute() function in the MultiSig.sol contract will be rendered useless as no one will be able to call them. The impact of a missing check is huge here as one malicious signer can renounce on behalf of other signers as well.

Proof of Concept

https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/auth/MultisigBase.sol#L142

A malicious signer calls this function and passes the onlySigners modifier check.

File: contracts/cgp/auth/MultisigBase.sol
142: function rotateSigners(address[] memory newAccounts, uint256 newThreshold) external virtual onlySigners {
143:         _rotateSigners(newAccounts, newThreshold);
144:     }

https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/auth/MultisigBase.sol#L44

In order for the malicious signer to pass the onlySigners modifier and execute the rotateSigners() function, they need to meet two conditions:

1. Be a signer who has not voted
2. Be a signer who votes when vote count is one less than threshold required (since if vote count is not met, we return early without further execution of rotateSigners(). But if we meet the threshold, we get to enter the rotateSigners() function)

   File: contracts/cgp/auth/MultisigBase.sol
   45: if (!signers.isSigner[msg.sender]) revert NotSigner(); //@audit check for signer
   51: if (voting.hasVoted[msg.sender]) revert AlreadyVoted(); //@audit check to see if signer already voted
   59: if (voteCount < signers.threshold) { 
   60:             // Save updated vote count.
   61:             voting.voteCount = voteCount;
   62:             return; //@audit we return early if threshold not met yet, this is why the malicious signer needs to vote one vote before threshold is met
   63: }

https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/auth/MultisigBase.sol#L119

The malicious signer monitors the vote count through the getSignerVotesCount() function to perfectly call the rotateSigners() function when the vote count is one less than the threshold. This allows the signer to enter the rotateSigners() function since threshold is met.

File: contracts/cgp/auth/MultisigBase.sol
119: function getSignerVotesCount(bytes32 topic) external view override returns (uint256) {
120:         uint256 length = signers.accounts.length;
121:         uint256 voteCount;
122:         for (uint256 i; i < length; ++i) {
123:             if (votingPerTopic[signerEpoch][topic].hasVoted[signers.accounts[i]]) {
124:                 voteCount++;
125:             }
126:         }
127: 
128:         return voteCount;
129:    }

https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/auth/MultisigBase.sol#L149

The rotateSigners() function in turn calls this _rotateSigners() internal function. In this function:

1. Line 153-155 cleans up the old signers - meaning a malicious signer can remove all signers
2. Line 168-176 allows the malicious signer to set a random contract address or his own EOA to be a signer

File: contracts/cgp/auth/MultisigBase.sol
149: function _rotateSigners(address[] memory newAccounts, uint256 newThreshold) internal {
150:         uint256 length = signers.accounts.length;
151: 
152:         // Clean up old signers.
153:         for (uint256 i; i < length; ++i) {
154:             signers.isSigner[signers.accounts[i]] = false;
155:         }
156: 
157:         length = newAccounts.length;
158: 
159:         if (newThreshold > length) revert InvalidSigners();
160: 
161:         if (newThreshold == 0) revert InvalidSignerThreshold();
162: 
163:         ++signerEpoch;
164: 
165:         signers.accounts = newAccounts;
166:         signers.threshold = newThreshold;
167: 
168:         for (uint256 i; i < length; ++i) {
169:             address account = newAccounts[i];
170: 
171:             // Check that the account wasn't already set as a signer for this epoch.
172:             if (signers.isSigner[account]) revert DuplicateSigner(account);
173:             if (account == address(0)) revert InvalidSigners();
174: 
175:             signers.isSigner[account] = true;
176:         }
177: 
178:         emit SignersRotated(newAccounts, newThreshold);
179:     }

This way a malicious signer can freeze functions relying on the onlySigners() modifier.

Tools Used

Manual Review

Recommended Mitigation Steps

One thing we can notice in this issue is that everytime a signer votes (i.e. basically calling the rotateSigners() function to increase vote count in onlySigners() modifier), the modifier does not check whether all the signers are voting for the same parameter values (address[] memory newAccounts and uint256 newThreshold). This is what allows the malicious signer to vote differently than the rest of the signers, thus allowing the signer to set some random contract address or his own EOA as the sole signer.

The solution to this problem would be to:

1. keccak256 hash the parameter newAccounts

   keccak256(abi.encodePacked(newAccounts));

2. Store the hash in a state variable proposalHash (add an if condition to check that this hash is only set if the state variable proposalHash has the default value, which represents that the previous proposalHash was deleted since the threshold was met - check step 4 for reference)

   
   bytes public proposalHash;
   error proposalOngoing();

function setProposalHash(address[] memory newAccounts) external onlySigners { if(proposalHash != 0x) revert proposalOngoing(); proposalHash = keccak256(abi.encodePacked(newAccounts)); //emit event here }

Note that here the first signer (i.e. first caller to function setProposal() above) gets to decide the newAccounts to vote on. If the first signer turns out to be malicious, the rest of the signers can just disagree and not vote for the newAccounts provided. In this case, if the rest of the signers want to change the proposalHash, there are two solutions:
a. An admin role or bypasser role should be added to change the proposalHash in "break-glass" type emergency situations like these.
OR 
b. Add an expiry to the proposalHash. This way if block.timestamp >= expiryTime, we delete the proposalHash set by the malicious signer in the onlySigners() modifier.

3. In onlySigners() modifier, check if all signers are voting for the same new account addresses (i.e. the same hash).
```solidity
modifier onlySigners(address[] memory newAccounts) {
    if(keccak256(abi.encodePacked(newAccounts)) != proposalHash) revert InvalidAccounts();

4. Once threshold is met, delete the proposalHash for future use of the state variable

   delete proposalHash;

Assessed type

Governance

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #315

[c4-judge]

berndartmueller marked the issue as unsatisfactory: Out of scope

[mcgrathcoutinho]

Hi @berndartmueller , thank you for taking the time to read this.

I believe this issue is a valid Medium-severity issue due to the following reasons:

1. The MultiSigBase contract is supposed to work like a multi-sig where the power to execute a proposal or rotateSigners is dependent on the signers who vote "on a specific set of values" to meet the threshold. But the current implementation does not do that and only takes into consideration the power of the signer voting one less than the threshold.

For example, let's say the threshold is 10 signers. The first 9 signers vote "honestly" with parameter value A but the last signer (malicious) votes with some parameter value B to take over the multisig (as depicted in my POC above). This is the same as giving the power to one centralized role and completely defeats the purpose of the multi-sig.

This is even more dangerous in our case since the signer has access to the rotateSigners() function which updates the signers.

I believe the flaw is in the logic of how the MultiSig is implemented. It should be checking the hashed id of the values the signers vote for to ensure each signer is voting for the same values without any malicious input to take over the multi-sig.

(I believe this resonates with the comments provided by @pcarranzav on #333 as well)

Thank you once again for re-evaluating this.

[c4-judge]

berndartmueller marked the issue as not a duplicate

[berndartmueller]

Hey @mcgrathcoutinho,

firstly, I'm marking this submission as not a duplicate of #315.

Secondly, your assumption

The MultiSigBase contract is supposed to work like a multi-sig where the power to execute a proposal or rotateSigners is dependent on the signers who vote "on a specific set of values" to meet the threshold. But the current implementation does not do that and only takes into consideration the power of the signer voting one less than the threshold.

is unfortunately not correct.

The topic which signers vote on is based on the calldata (msg.data), see https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/auth/MultisigBase.sol#L47

Your described scenario is not possible as any signer who calls the rotateSigners with different arguments would vote for a different topic. Thus, the submission is invalid.

[mcgrathcoutinho]

Thank you for clarifying this, a very silly error from my side.