Deployer wallet retains ability to spoof validated senders after ownership transfer

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/9f642fe854eb11ad9f18fe028e5a8353c258e926/contracts/its/remote-address-validator/RemoteAddressValidator.sol#L72-L74 https://github.com/code-423n4/2023-07-axelar/blob/9f642fe854eb11ad9f18fe028e5a8353c258e926/contracts/its/remote-address-validator/RemoteAddressValidator.sol#L83

Vulnerability details

Impact

The InterchainTokenService is deployed using create3: https://github.com/code-423n4/2023-07-axelar/blob/9f642fe854eb11ad9f18fe028e5a8353c258e926/scripts/deploy.js#L60-L64 - this means that its address depends on the address of the deployer wallet, the address of the Create3Deployer contract, and the salt that is extracted from a deployment key.

It should be feasible, then, for the same deployer wallet to deploy a Create3Deployer with the same address on a new chain, and use the same deployment key to deploy a contract with the same address as the InterchainTokenService but arbitrary implementation. A team member has confirmed on Discord that "the address will be the interchainTokenServiceAddress which is constant across EVM chains.". However, at deployment time, only a subset of all the possible EVM chains will be supported, and more may be added in the future. When that happens, it is possible that the original deployer wallet is compromised or no longer available.

On the other hand, the addTrustedAddress function validates that the sender is the owner of the RemoteAddressValidator contract. This owner is originally the deployer wallet, but ownership may be transferred (and it would be a good practice to transfer it to a more secure governance multisig immediately after deployment). After ownership transfer, the previous owner should not be allowed to add trusted addresses.

However, the validateSender function will treat any address that is the same as the current chain's InterchainTokenService as valid. A malicious contract deployed by the deployer wallet after the ownership transfer could be treated as valid and would have the ability to deploy token managers and standardized tokens, or perform arbitrary token transfers. This is a form of centralization risk but is also a serious privilege escalation, as it should be possible to strip the deployer of the ability to perform these actions, and this gives them virtually unlimited power even after an ownership transfer.

Proof of Concept

• A deployer account is used to deploy all contracts on chain A.
• Ownership of the RemoteAddressValidator and all other contracts is transferred to governance multisigs. After this point, the deployer should have no ability to add trusted addresses.
• The deployer account is compromised (which is easier to do than compromising a governance multisig), or a team member with access to the deployer account becomes compromised or malicious.
• The deployer repeats the same steps used for the deployment, starting from the same wallet nonce, on chain B, but replaces the InterchainTokenService contract with a contract that allows arbitrary messages, e.g. by adding this function to a regular InterchainTokenService:

    function callContract(
        string calldata destinationChain,
        bytes memory payload,
        uint256 gasValue,
        address refundTo
    ) external onlyOwner {
        _callContract(destinationChain, payload, gasValue, refundTo);
    }

• The deployer then uses this contract to send a payload with SELECTOR_SEND_TOKEN and using the deployer address as destination to the InterchainTokenService on chain A.
• When running validateSender on the RemoteAddressValidator on chain A, this check will pass and the address will be treated as valid:

        if (sourceAddressHash == interchainTokenServiceAddressHash) {
            return true;
        }

• Therefore, the tokens will be transferred to the deployer address on chain A.

Tools Used

Visual inspection.

Recommended Mitigation Steps

The assumption that the InterchainTokenService address is valid in any chain is dangerous, because of how the contract is created and the possibility that new EVM chains may exist in the future. A deployer EOA should not have this amount of permission for an indefinite time. I would recommend breaking that assumption and requiring that all addresses are added as trusted addresses explicitly.

A check can therefore be added to only treat the interchain token service's address as valid if the source chain is also the same chain where the RemoteAddressValidator is deployed. The following diff shows a way to do this:

diff --git a/contracts/its/remote-address-validator/RemoteAddressValidator.sol b/contracts/its/remote-address-validator/RemoteAddressValidator.sol
index bb101e5..c2e5382 100644
--- a/contracts/its/remote-address-validator/RemoteAddressValidator.sol
+++ b/contracts/its/remote-address-validator/RemoteAddressValidator.sol
@@ -4,6 +4,7 @@ pragma solidity ^0.8.0;
 import { IRemoteAddressValidator } from '../interfaces/IRemoteAddressValidator.sol';
 import { AddressToString } from '../../gmp-sdk/util/AddressString.sol';
 import { Upgradable } from '../../gmp-sdk/upgradable/Upgradable.sol';
+import { StringToBytes32 } from '../../gmp-sdk/util/Bytes32String.sol';

 /**
  * @title RemoteAddressValidator
@@ -11,23 +12,25 @@ import { Upgradable } from '../../gmp-sdk/upgradable/Upgradable.sol';
  */
 contract RemoteAddressValidator is IRemoteAddressValidator, Upgradable {
     using AddressToString for address;
+    using StringToBytes32 for string;

     mapping(string => bytes32) public remoteAddressHashes;
     mapping(string => string) public remoteAddresses;
     address public immutable interchainTokenServiceAddress;
     bytes32 public immutable interchainTokenServiceAddressHash;
     mapping(string => bool) public supportedByGateway;
-
+    bytes32 public immutable currentChainName;
     bytes32 private constant CONTRACT_ID = keccak256('remote-address-validator');

     /**
      * @dev Constructs the RemoteAddressValidator contract, both array parameters must be equal in length
      * @param _interchainTokenServiceAddress Address of the interchain token service
      */
-    constructor(address _interchainTokenServiceAddress) {
+    constructor(address _interchainTokenServiceAddress, string memory _chainName) {
         if (_interchainTokenServiceAddress == address(0)) revert ZeroAddress();
         interchainTokenServiceAddress = _interchainTokenServiceAddress;
         interchainTokenServiceAddressHash = keccak256(bytes(_lowerCase(interchainTokenServiceAddress.toString())));
+        currentChainName = _chainName.toBytes32();
     }

     /**
@@ -69,7 +72,7 @@ contract RemoteAddressValidator is IRemoteAddressValidator, Upgradable {
     function validateSender(string calldata sourceChain, string calldata sourceAddress) external view returns (bool) {
         string memory sourceAddressLC = _lowerCase(sourceAddress);
         bytes32 sourceAddressHash = keccak256(bytes(sourceAddressLC));
-        if (sourceAddressHash == interchainTokenServiceAddressHash) {
+        if (sourceAddressHash == interchainTokenServiceAddressHash && sourceChain.toBytes32() == currentChainName) {
             return true;
         }
         return sourceAddressHash == remoteAddressHashes[sourceChain];

Assessed type

Access Control

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #348

[c4-judge]

berndartmueller marked the issue as unsatisfactory: Invalid

[pcarranzav]

Hi again @berndartmueller - I'd also like to point out this one, which was marked as duplicate of https://github.com/code-423n4/2023-07-axelar-findings/issues/348 which is marked as invalid. The argumentation in my submission is different as it is framed as a privilege escalation of the deployer wallet, which persists after ownership transfer and in the hypothetical of a new EVM chain being added to the Axelar network.

This modifies the trust assumptions of the protocol as users can reasonably expect to trust the multisig Owner, but not a deployer EOA, for an indefinite time. In the closed issue, @deanamiel commented that "It would be impossible to deploy a different contract at this address because the address will depend on the deployer address and salt." However, there is a different level of "impossible" when comparing an EOA to a multisig and users now need to trust that nobody with access to that EOA is compromised or that it is disposed of properly.

The probability of this being exploited is low (requires deployer EOA compromise and new chain added to the network), but the impact would be huge which is why I honestly believe it warrants a Medium severity and is a valid finding to surface in the report so that users are aware.

[c4-judge]

berndartmueller marked the issue as not a duplicate

[c4-judge]

berndartmueller marked the issue as primary issue

[berndartmueller]

Hi again @berndartmueller - I'd also like to point out this one, which was marked as duplicate of #348 which is marked as invalid. The argumentation in my submission is different as it is framed as a privilege escalation of the deployer wallet, which persists after ownership transfer and in the hypothetical of a new EVM chain being added to the Axelar network.

This modifies the trust assumptions of the protocol as users can reasonably expect to trust the multisig Owner, but not a deployer EOA, for an indefinite time. In the closed issue, @deanamiel commented that "It would be impossible to deploy a different contract at this address because the address will depend on the deployer address and salt." However, there is a different level of "impossible" when comparing an EOA to a multisig and users now need to trust that nobody with access to that EOA is compromised or that it is disposed of properly.

The probability of this being exploited is low (requires deployer EOA compromise and new chain added to the network), but the impact would be huge which is why I honestly believe it warrants a Medium severity and is a valid finding to surface in the report so that users are aware.

Hey @pcarranzav!

thanks for pointing this out!

I've marked your submission as the primary report due to pointing out the deployer privilege escalation. But I also consider #348 a duplicate, as the recommended fix would also fix the underlying issue.

Overall, I agree with the outlined privilege escalation. Even though the likelihood of this to happen is low, the impact would be critical. Thus, I consider medium severity to be appropriate.

[c4-judge]

berndartmueller marked the issue as satisfactory

[c4-judge]

berndartmueller marked the issue as selected for report

[c4-sponsor]

deanamiel (sponsor) acknowledged

[deanamiel]

We acknowledge the severity, and while we’ve considered using a deployer multisig contract which reduces this risk, our operations team is planning on whitelisting explicitly, to minimize the impact of the deployer or a non-whitelisted chain being compromised. Note that RemoteAddressValidator is deployed on the destination chain, so the recommendation to compare the source chain to the chain in remote address validator won’t work.