Axelar Network v2 contest - berndartmueller's results

QA Report

Table of Contents

• Low Risk
  • [L-01] Ensure weight of operator is not 0
  • [L-02] Function NatSpec comment mentions wrong msg.sender context
  • [L-03] Native tokens sent by DepositReceiver to msg.sender on selfdestruct are locked

Low Risk

[L-01] Ensure weight of operator is not 0

Description

Operators are assigned a weight when transferring operatorship. Validating signatures requires reaching a certain weight threshold of operators who signed a message to consider the message to be valid.

It is assumed that each honest operator has a weight != 0. However, the current implementation lacks such a check, thus allowing operator weights to have a value of 0 (as long as the total weights of all operators equal or surpass newThreshold).

Findings

AxelarAuthWeighted.sol#L70

function _transferOperatorship(bytes memory params) internal {
    (address[] memory newOperators, uint256[] memory newWeights, uint256 newThreshold) = abi.decode(
        params,
        (address[], uint256[], uint256)
    );
    uint256 operatorsLength = newOperators.length;
    uint256 weightsLength = newWeights.length;

    // operators must be sorted binary or alphabetically in lower case
    if (operatorsLength == 0 || !_isSortedAscAndContainsNoDuplicate(newOperators)) revert InvalidOperators();

    if (weightsLength != operatorsLength) revert InvalidWeights();

    uint256 totalWeight = 0;
    for (uint256 i = 0; i < weightsLength; ++i) {
        totalWeight += newWeights[i]; // @audit-info Weight of an operator can potentially be 0
    }
    if (newThreshold == 0 || totalWeight < newThreshold) revert InvalidThreshold();

    bytes32 newOperatorsHash = keccak256(params);

    if (epochForHash[newOperatorsHash] > 0) revert SameOperators();

    uint256 epoch = currentEpoch + 1;
    currentEpoch = epoch;
    hashForEpoch[epoch] = newOperatorsHash;
    epochForHash[newOperatorsHash] = epoch;

    emit OperatorshipTransferred(newOperators, newWeights, newThreshold);
}

Recommended mitigation steps

Consider adding a check to _transferOperatorship to ensure operator weights are != 0 (see @audit-info annotation):

function _transferOperatorship(bytes memory params) internal {
    (address[] memory newOperators, uint256[] memory newWeights, uint256 newThreshold) = abi.decode(
        params,
        (address[], uint256[], uint256)
    );
    uint256 operatorsLength = newOperators.length;
    uint256 weightsLength = newWeights.length;

    // operators must be sorted binary or alphabetically in lower case
    if (operatorsLength == 0 || !_isSortedAscAndContainsNoDuplicate(newOperators)) revert InvalidOperators();

    if (weightsLength != operatorsLength) revert InvalidWeights();

    uint256 totalWeight = 0;
    for (uint256 i = 0; i < weightsLength; ++i) {
        require(newWeights[i] != 0, 'Invalid operator weight'); // @audit-info Check if operator weight is not 0
        totalWeight += newWeights[i];
    }
    if (newThreshold == 0 || totalWeight < newThreshold) revert InvalidThreshold();

    bytes32 newOperatorsHash = keccak256(params);

    if (epochForHash[newOperatorsHash] > 0) revert SameOperators();

    uint256 epoch = currentEpoch + 1;
    currentEpoch = epoch;
    hashForEpoch[epoch] = newOperatorsHash;
    epochForHash[newOperatorsHash] = epoch;

    emit OperatorshipTransferred(newOperators, newWeights, newThreshold);
}

[L-02] Function NatSpec comment mentions wrong msg.sender context

Description

The ReceiverImplementation.receiveAndUnwrapNative mentions the wrong (or not completely correct) msg.sender of the function. Contrary to the other functions within the ReceiverImplementation contract, this function specifies that the msg.sender is DepositBase, but in fact it is AxelarDepositService.

Findings

ReceiverImplementation.sol#L69

// @dev This function is used for delegate by DepositReceiver deployed above
// Context: msg.sender == DepositBase, this == DepositReceiver @audit-info Incorrect msg.sender
function receiveAndUnwrapNative(address payable refundAddress, address payable recipient) external {
  ...
}

Recommended mitigation steps

Consider changing the comment to mention the correct msg.sender:

// @dev This function is used for delegate by DepositReceiver deployed above
// Context: msg.sender == AxelarDepositService, this == DepositReceiver
function receiveAndUnwrapNative(address payable refundAddress, address payable recipient) external {
  ...
}

[L-03] Native tokens sent by DepositReceiver to msg.sender on selfdestruct are locked

Description

The DepositReceiver contract calls selfdestruct immediately after the delegatecall executed successfully. In the event that any left-over native tokens are in the deployed contract, those native tokens would be sent to msg.sender which is the AxelarDepositService contract.

However, the AxelarDepositService has no way of transferring out those native tokens, hence they are locked.

Findings

deposit-service/DepositReceiver.sol#L25

constructor(bytes memory delegateData) {
    // Reading the implementation of the AxelarDepositService
    // and delegating the call back to it
    // solhint-disable-next-line avoid-low-level-calls
    (bool success, ) = IAxelarDepositService(msg.sender).receiverImplementation().delegatecall(delegateData);

    // if not success revert with the original revert data
    if (!success) {
        // solhint-disable-next-line no-inline-assembly
        assembly {
            let ptr := mload(0x40)
            let size := returndatasize()
            returndatacopy(ptr, 0, size)
            revert(ptr, size)
        }
    }

    selfdestruct(payable(msg.sender));
}

Recommended mitigation steps

Consider adding an owner-callable function to the AxelarDepositService contract to transfer native tokens out.