addGasFee function in CCMPSendMessageFacet contract allows users to add gas fee even if a gateway is paused, which would go against the business logic of the protocol.

As a consequence, users could lose their recently deposited tokens if a gateway has been paused because of an unexpected scenario: cyberattacks, market adverse conditions, security issues with the adaptors, etc.

Code Location

gateway/facets/CCMPSendMessageFacet.sol

function addGasFee(
  GasFeePaymentArgs memory _args,
  bytes32 _messageHash,
  address _sender
) public payable {
  uint256 feeAmount = _args.feeAmount;
  address relayer = _args.relayer;
  address tokenAddress = _args.feeTokenAddress;

  LibDiamond.CCMPDiamondStorage storage ds = LibDiamond._diamondStorage();

  if (feeAmount > 0) {
    ds.gasFeePaidByToken[_messageHash][tokenAddress][
      relayer
    ] += feeAmount;

    if (tokenAddress == NATIVE_ADDRESS) {
      if (msg.value != feeAmount) {
        revert NativeAmountMismatch();
      }
      (bool success, bytes memory returndata) = relayer.call{
        value: msg.value
      }("");
      if (!success) {
        revert NativeTransferFailed(relayer, returndata);
      }
    } else {
      if (msg.value != 0) {
        revert NativeAmountMismatch();
      }
      IERC20(tokenAddress).safeTransferFrom(
        _sender,
        relayer,
        feeAmount
      );
    }

    emit FeePaid(tokenAddress, feeAmount, relayer);
  }
}

transferOwnership and setCCMPExecutor functions do not validate the new values of the contract owner and CCMPExecutor respectively before making the changes. For instance, if the ownership of a gateway is mistakenly transferred to CCMPExecutor, any user will be able to call restricted functions on CCMPConfigurationFacet contract.

As a consequence, a malicious user could modify gateways, router adaptors and pausers in order to make a takeover of the CCMP contracts. It is worth noting that the likelihood of this issue being triggered is very limited because the functions mentioned above can be called only by the contract owner.

Code Location

gateway/facets/CCMPConfigurationFacet.sol

function transferOwnership(
  address _newOwner
) external override(IERC173, ICCMPConfiguration) {
  LibDiamond._enforceIsContractOwner();
  LibDiamond._setContractOwner(_newOwner);
}

gateway/facets/CCMPConfigurationFacet.sol

function setCCMPExecutor(ICCMPExecutor _ccmpExecutor) external {
  LibDiamond._enforceIsContractOwner();
  LibDiamond._diamondStorage().ccmpExecutor = _ccmpExecutor;
  emit CCMPExecutorUpdated(_ccmpExecutor);
}

An incorrect use of the transferOwnership function in CCMPConfigurationFacet contract can set owner to an invalid address and inadvertently lose control of a gateway, which cannot be undone in any way. Currently, the owner of the contract can change owner address using the aforementioned function in a single transaction and without confirmation from the new address.

Code Location

Ownership transfer for gateways in a single step:

gateway/facets/CCMPConfigurationFacet.sol

function transferOwnership(
  address _newOwner
) external override(IERC173, ICCMPConfiguration) {
  LibDiamond._enforceIsContractOwner();
  LibDiamond._setContractOwner(_newOwner);
}

libraries/LibDiamond.sol

function _setContractOwner(address _newOwner) internal {
  CCMPDiamondStorage storage ds = _diamondStorage();
  address previousOwner = ds.contractOwner;
  ds.contractOwner = _newOwner;
  emit OwnershipTransferred(previousOwner, _newOwner);
}

If a relayer mistakenly uses the getGasFeePaymentDetails function in CCMPSendMessageFacet contract with a repeated token in _tokens argument, the function will not throw an error message.

This situation could create a potential issue for the relayer if it directly accrues the values of all the tokens by just querying this function because the real value will be different from the calculated one with the function mentioned above.

Code Location

gateway/facets/CCMPSendMessageFacet.sol

function getGasFeePaymentDetails(
 bytes32 _messageHash,
 address[] calldata _tokens,
 address _relayer
) external view returns (uint256[] memory balances) {
 LibDiamond.CCMPDiamondStorage storage ds = LibDiamond._diamondStorage();
 balances = new uint256[](_tokens.length);
 unchecked {
  for (uint256 i = 0; i < _tokens.length; ++i) {
   balances[i] = ds.gasFeePaidByToken[_messageHash][_tokens[i]][
    _relayer
   ];
  }
 }
}

There is no validation that the _owner address in the constructor of CCMPAdaptorBase contract - which is then inherited by Axelar, Hyperlane and Wormhole adaptors - is different from zero address.

It is worth noting that the call to _transferOwnership does not validate the address mentioned, either.

Code Location

contracts/adaptors/base/CCMPAdaptorBase.sol

constructor(
  address _ccmpGateway,
  address _owner,
  address _pauser
) PausableBase(_pauser) {
  ccmpGateway = ICCMPGateway(_ccmpGateway);
  _transferOwnership(_owner);
}

setPauser and pauser functions in CCMPConfigurationFacet contract implement their logic and do not reuse some internal functions that do the same tasks. Although it is not a security issue, it is worth noting that this recommendation is more focused on code hygiene and to avoid potential issues in the future if some part of this code is refactored.

Code Location

setPauser function could call _setContractPauser to set the new pauser:

gateway/facets/CCMPConfigurationFacet.sol

function setPauser(address _pauser) external {
  LibDiamond._enforceIsContractOwner();
  LibDiamond._diamondStorage().pauser = _pauser;
  emit PauserUpdated(_pauser);
}

libraries/LibDiamond.sol

function _setContractPauser(address _newPauser) internal {
  _diamondStorage().pauser = _newPauser;
}

\color{black}\color{white}pauser function could call _contractPauser to query the pauser address:

gateway/facets/CCMPConfigurationFacet.sol

function pauser() external view returns (address pauser_) {
  pauser_ = LibDiamond._diamondStorage().pauser;
}

libraries/LibDiamond.sol

function _contractPauser() internal view returns (address pauser_) {
  pauser_ = _diamondStorage().pauser;
}