Axelar Network v2 contest - xiaoming90's results

Lines of code

https://github.com/code-423n4/2022-07-axelar/blob/3729dd4aeff8dc2b8b9c3670a1c792c81fc60e7c/contracts/AxelarGateway.sol#L262 https://github.com/code-423n4/2022-07-axelar/blob/3729dd4aeff8dc2b8b9c3670a1c792c81fc60e7c/contracts/auth/AxelarAuthWeighted.sol#L86 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/auth/AxelarAuthWeighted.sol#L36

Vulnerability details

The administrator will call AxelarAuthWeighted.transferOperatorship function to transfer the operatorship to a new set of {Operators/Weights/Threshold}.

However, it was observed that after transferring the operatorship to a new set of {Operators/Weights/Threshold}, the previous sets of {Operators/Weights/Threshold} are still able to generate a valid proof, and subsequently execute the command.

The following piece of code shows that as long as valid proof is submitted, the commands will be executed by the system.

https://github.com/code-423n4/2022-07-axelar/blob/3729dd4aeff8dc2b8b9c3670a1c792c81fc60e7c/contracts/AxelarGateway.sol#L262

function execute(bytes calldata input) external override {
    (bytes memory data, bytes memory proof) = abi.decode(input, (bytes, bytes));

    bytes32 messageHash = ECDSA.toEthSignedMessageHash(keccak256(data));

    // TEST auth and getaway separately
    bool currentOperators = IAxelarAuth(AUTH_MODULE).validateProof(messageHash, proof);
	..SNIP..
}

The following piece of code shows that the past 16 sets of {Operators/Weights/Threshold} are considered valid and can be used within the AxelarAuthWeighted._validateSignatures function. Thus, the past 16 sets of {Operators/Weights/Threshold} are able to sign and submit a valid proof, and the proof will be accepted by the AxelarAuthWeighted.validateProof that allows them to execute the commands.

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/auth/AxelarAuthWeighted.sol#L36

uint8 internal constant OLD_KEY_RETENTION = 16;

function validateProof(bytes32 messageHash, bytes calldata proof) external view returns (bool currentOperators) {
	(address[] memory operators, uint256[] memory weights, uint256 threshold, bytes[] memory signatures) = abi.decode(
		proof,
		(address[], uint256[], uint256, bytes[])
	);

	bytes32 operatorsHash = keccak256(abi.encode(operators, weights, threshold));
	uint256 operatorsEpoch = epochForHash[operatorsHash];
	uint256 epoch = currentEpoch;

	if (operatorsEpoch == 0 || epoch - operatorsEpoch >= OLD_KEY_RETENTION) revert InvalidOperators();

	_validateSignatures(messageHash, operators, weights, threshold, signatures);

	currentOperators = operatorsEpoch == epoch;
}

Understood from the team that the reason for allowing past 16 sets of {Operators/Weights/Threshold} is that after a transfer of operatorship, the commands that were signed recently but have not been executed yet will not become invalid. Further understood from the team the operatorship transfer is performed when there is a significant change in stake distribution on the Axelar network.

It makes sense for commands that were already signed recently by past operators before the operatorship transfer to be executable. However, based on the current design, it is also possible for the past 16 sets of {Operators/Weights/Threshold} to submit a new valid proof/signature for new commands to be executed after the operatorship transfer, and the AxelarAuthWeighted._validateSignatures function will happily accept the proof/signature, which should not be allowed.

It was understood that the operatorship transfer is performed when there is a significant change in stake distribution on the Axelar network, therefore, it does not make sense for all the past 16 sets of {Operators/Weights/Threshold} to be still able to sign and execute new commands after the operatorship transfer, because they follow the old stake distribution that is no longer considered as valid.

Only the current set of operators and its stake distribution should be used to verify any new command signed and issued after the operatorship transfer.

Proof-of-Concept

Assuming that there are 3 validators (Alice, Bob and Charles)

Operation at Time 1 (T1)

At T1, the following is the state:

currentEpoch = 1

hashForEpoch[Epoch 1] = {operators: [Alice, Bob, Charles], weights: [0.5, 0.25, 0.25], threshold: 0.5} convert to hash

At T1, Alice could submit the following input to AxelarGateway.execute(bytes calldata input) function to execute the commands:

input = {

​ bytes memory data = commands to be executed

​ bytes memory proof = {operators: [Alice, Bob, Charles], weights: [0.5, 0.25, 0.25], threshold: 0.5, signatures: [Alice's signature]}

}

Since Alice's signature weight is 0.5, having Alice's signature alone is sufficient to meet the threshold of 0.5 and the commands will be executed.

Operation at Time 2 (T2)

At T2, the Axelar administrator decided to change the stake distribution. The admin called the AxelarAuthWeighted.transferOperatorship and change the state to as follows:

currentEpoch = 2

hashForEpoch[Epoch 2] = {operators: [Alice, Bob, Charles], weights: [0.25, 0.4, 0.4], threshold: 0.5} convert to hash <== newly added

hashForEpoch[Epoch 1] = {operators: [Alice, Bob, Charles], weights: [0.5, 0.25, 0.25], threshold: 0.5} convert to hash

At T2, Alice's weight has reduced from 0.5 to 0.25. As per the current stake distribution, Alice's signature alone is not sufficient to meet the threshold of 0.5. Thus, she is not able to execute any new command without an additional signature from Bob or Charles.

However, note that the past 16 sets of {operators/weights/threshold} are considered valid by the system, so in another word, all the past 16 stake distributions are considered valid too.

Thus, Alice simply needs to re-use back to the previous set of {operators/weights/threshold} in Epoch 1 and she can continue to execute new commands without the signature of Bob or Charles, thus bypassing the current stake distribution.

At T2, Alice could still submit the following input to AxelarGateway.execute(bytes calldata input) function with only Alice's signature to execute the command:

input = {

​ bytes memory data = commands to be executed

​ bytes memory proof = {operators: [Alice, Bob, Charles], weights: [0.5, 0.25, 0.25], threshold: 0.5, signatures: [Alice's signature]}

}

No additional signature from Bob or Charles is needed.

Following is from Epoch 1

{operators: [Alice, Bob, Charles], weights: [0.5, 0.25, 0.25], threshold: 0.5

Operator Address Changed After Operatorship Is Being Transferred

Noted from the discord channel the following clarification from the team.

Based on couple of questions I have received, I'd like to clarify one assumption we are making for the contracts (which is enforced at the axelar proof of stake network): Operators correspond to validators on the Axelar network. However, the operator address for a given epoch is derived from the validator key along with a nonce that is unique for each operator epoch. i.e Whenever operatorship is being transferred, an honest validator will always generate a new operator address (and not reuse their old one) due to a nonce.

With this control in place, even if the validator has generated a new operator address after the operatorship has been transferred, it is still possible for the validator to re-use back the old operator address and sign the command as the validator is aware of the private key needed to sign on behalf of the old operator address. Thus, the above issue still exists.

Additionally, there is always a risk of a "dishonest" validator not generating a new operator address after operatorship is being transferred if the new stake distribution does not benefit them. In the above example, Alice who has its weightage reduced from 0.5 to 0.25 do not see the benefit of the new stake distribution can decide not to generate a new operator address and continue to use the old operator address that allowed her to sign and execute any command without an additional signature from Bob or Charles.

Impact

Current stake distribution can be bypassed.

Recommendation

Consider updating the system to ensure that the following requirements are followed:

• Command signed by the past 16 sets of {operators/weights/threshold} AFTER the operatorship transfer should not be executable and should be rejected. Only commands signed by the current set of {operators/weights/threshold} AFTER the operatorship transfer should be accepted and executable.
• Commands signed by the past 16 sets of {operators/weights/threshold} BEFORE the operatorship transfer should be accepted and executable.

Did Not Check If The operators and weights Array Length Is The Same

The AxelarAuthWeighted._validateSignatures function did not validate that the operators and weights array length are the same.

https://github.com/code-423n4/2022-07-axelar/blob/3729dd4aeff8dc2b8b9c3670a1c792c81fc60e7c/contracts/auth/AxelarAuthWeighted.sol#L86

function _validateSignatures(
    bytes32 messageHash,
    address[] memory operators,
    uint256[] memory weights,
    uint256 threshold,
    bytes[] memory signatures
) internal pure {
    uint256 operatorsLength = operators.length;
    uint256 operatorIndex = 0;
    uint256 weight = 0;
    // looking for signers within operators
    // assuming that both operators and signatures are sorted
    for (uint256 i = 0; i < signatures.length; ++i) {
        address signer = ECDSA.recover(messageHash, signatures[i]);
        // looping through remaining operators to find a match
        for (; operatorIndex < operatorsLength && signer != operators[operatorIndex]; ++operatorIndex) {}
        // checking if we are out of operators
        if (operatorIndex == operatorsLength) revert MalformedSigners();
        // return if weight sum above threshold
        weight += weights[operatorIndex];
        // weight needs to reach or surpass threshold
        if (weight >= threshold) return;
        // increasing operators index if match was found
        ++operatorIndex;
    }
    // if weight sum below threshold
    revert MalformedSigners();
}

Recommendation

Implement the following check to ensure that the operators and weights array length are the same.

require(operators.length == weights.length, "operators and weights length not the same")

            if (operatorIndex == operatorsLength) revert MalformedSigners();