MultisigBase.sol : Unsafe `onlySigners` modifier

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/auth/MultisigBase.sol#L44-L77 https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/governance/Multisig.sol#L30-L36 https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/governance/AxelarServiceGovernance.sol#L52

Vulnerability details

Impact

The multi sig based implementation will not serve its purpose. Proposals can be executed even by a single signer.

Proof of Concept

Contract has the MultisigBase implementation which will be used as a custom multisignature wallet where transactions must be confirmed by a threshold of signers. Where the signers and threshold may be updated every epoch

when we look at the onlySigners implementation, it return when there are not enough signed threshold met. But the functions where this modifier is used will continued executing.

    modifier onlySigners() {
        if (!signers.isSigner[msg.sender]) revert NotSigner();

        bytes32 topic = keccak256(msg.data);
        Voting storage voting = votingPerTopic[signerEpoch][topic];

        // Check that signer has not voted, then record that they have voted.
        if (voting.hasVoted[msg.sender]) revert AlreadyVoted();

        voting.hasVoted[msg.sender] = true;

        // Determine the new vote count.
        uint256 voteCount = voting.voteCount + 1;

        // Do not proceed with operation execution if insufficient votes.
        if (voteCount < signers.threshold) { ----------------------------------->> threshold not met.
            // Save updated vote count.
            voting.voteCount = voteCount;
            return; ------------------------>> returned. this would allow the continuation of function execution
        }

        // Clear vote count and voted booleans.
        voting.voteCount = 0;

        uint256 count = signers.accounts.length;

        for (uint256 i; i < count; ++i) {
            voting.hasVoted[signers.accounts[i]] = false;
        }

        emit MultisigOperationExecuted(topic);

        _;
    }

Tools Used

Manual review.

Recommended Mitigation Steps

Add, separate function to vote by the valid signers. Once the enough signer threshold met for a particular proposal, then only allow function execution ( proposal execution)

Assessed type

Error

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #441

[c4-judge]

berndartmueller marked the issue as unsatisfactory: Overinflated severity

[aktech297]

@berndartmueller

Thanks for your work on this.

we kindly request you to check this issue once again.

we are looking at this issue as a flaw in the multi sig implementation, where, even though the required number of threshold is not met, the functionality is allowed to be continued.

For example, the natspec comments in the modifier says // Do not proceed with operation execution if insufficient votes.

But when we noticed that wherever this modifier used, the functions allow further operation without revert.

Thanks!

[berndartmueller]

@berndartmueller

Thanks for your work on this.

we kindly request you to check this issue once again.

we are looking at this issue as a flaw in the multi sig implementation, where, even though the required number of threshold is not met, the functionality is allowed to be continued.

For example, the natspec comments in the modifier says // Do not proceed with operation execution if insufficient votes.

But when we noticed that wherever this modifier used, the functions allow further operation without revert.

Thanks!

Hey @aktech297!

your assumption is not correct. The function body will be inlined in the onlySigner modifier at the place where "_;" is found (see https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/auth/MultisigBase.sol#L76). Thus, if the modifier simply returns, the function is not executed.