Axelar Network - pcarranzav's results

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/9f642fe854eb11ad9f18fe028e5a8353c258e926/contracts/its/interchain-token/InterchainToken.sol#L79-L105

Vulnerability details

Impact

In interchainTransferFrom, the msg.sender specifies a sender address from where tokens will be pulled. The tokens are then sent to a different chain using the TokenManager's tokenManager.transmitInterchainTransfer function, which will specify the token sender as the refund address for the Axelar GMP gas. But in this scenario, the fee (in native token msg.value) is provided by the msg.sender, not the token sender, so the refund will be sent to the wrong address. Moreover, the token sender may be a contract that can only handle the transferred token and has no way to transfer native tokens from the source chain, so the refunded value may be lost forever.

Proof of Concept

Consider the following scenario: On Ethereum mainnet, we have a simple token storage contract that only allows handling SOME_TOKEN (that is an InterchainToken), but not ETH:

contract TokenStorage {
  address public constant SOME_TOKEN_ADDRESS = <SOME_TOKEN's address goes here>;
  address public owner;
  constructor() {
    owner = msg.sender;
  }
  function approve(address beneficiary, uint256 amount) public {
    require(msg.sender == owner, "Only owner can call");
    IERC20(SOME_TOKEN_ADDRESS).approve(beneficiary, amount);
  }
}

• This token storage is sent some SOME_TOKEN.
• An intermediary EOA is approved by the owner of the TokenStorage for these SOME_TOKEN.
• The intermediary EOA performs an interchain transfer directly, calling interchainTransferFrom() and specifying the TokenStorage as sender
• The token calls tokenManager.transmitInterchainTransfer specifying the TokenStorage address as sender (https://github.com/code-423n4/2023-07-axelar/blob/9f642fe854eb11ad9f18fe028e5a8353c258e926/contracts/its/interchain-token/InterchainToken.sol#L104)
• The token manager calls transmitSendToken on InterchainTokenService (https://github.com/code-423n4/2023-07-axelar/blob/9f642fe854eb11ad9f18fe028e5a8353c258e926/contracts/its/token-manager/TokenManager.sol#L145-L152) specifying the TokenStorage address as sourceAddress.
• The InterchainTokenService performs the interchain call using GMP, but specifying the TokenStorage address (sourceAddress) as refund address (https://github.com/code-423n4/2023-07-axelar/blob/9f642fe854eb11ad9f18fe028e5a8353c258e926/contracts/its/interchain-token-service/InterchainTokenService.sol#L510-L522)
• If the intermediary EOA used a high msg.value to ensure the transfer is executed, all of the refund goes to the TokenStorage contract, where it can't be recovered. (In this case the contract can't even receive ETH, so it's unclear how Axelar network would even handle the refund. If the TokenStorage contract had a receive() function, the ETH would be received by TokenStorage and then stuck there).

Tools Used

Visual inspection.

Recommended Mitigation Steps

Consider setting the refund address to be the msg.sender. This might require some modifications to TokenManager.transmitInterchainTransfer and InterchainTokenService.transmitSendToken so that they accept a refund address parameter, so if adding this is problematic, consider documenting this behavior for interchainTransferFrom in a way that is clear to its users.

Assessed type

ETH-Transfer

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

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/aeabaa7086eb35e8614e58b42f0d50728e023881/contracts/cgp/auth/MultisigBase.sol#L44-L77 https://github.com/code-423n4/2023-07-axelar/blob/9f642fe854eb11ad9f18fe028e5a8353c258e926/contracts/cgp/governance/Multisig.sol#L30-L36

Vulnerability details

Impact

In MultisigBase and its use in Multisig, the onlySigners modifier will reset the vote count and execute the external call when the vote threshold is met. But this means that if many signers send their transactions during the same block, the votes that are executed after the call execution will start a new tally, potentially re-executing the same external call if the votes are enough to meet the threshold again. This is probably low-likelihood for multisigs where the threshold is high relative to the number of signers, but could be quite likely if the threshold is relatively low.

In the general case, users of Multisig may not be aware of this behavior and have no good way of avoiding this other than off-chain coordination. An accidental double-execution could easily lead to loss of funds.

This doesn't affect AxelarServiceGovernance because of the additional requirement of an interchain message, but it might still leave behind unwanted votes, which reduces the overall security of the governance mechanism.

Proof of Concept

Arguably this unit test is a PoC in itself: https://github.com/code-423n4/2023-07-axelar/blob/aeabaa7086eb35e8614e58b42f0d50728e023881/test/cgp/Multisig.js#L152-L176

But the following example might be a better illustration. The following test passes (when based on the explanation above, it shouldn't), and is a modification of the above test but using threshold 1:

    it('executes the same call twice, accidentally', async () => {
        const targetInterface = new Interface(['function callTarget() external']);
        const calldata = targetInterface.encodeFunctionData('callTarget');
        const nativeValue = 1000;

        // Set the threshold to 1
        await multisig.connect(signer1).rotateSigners(accounts, 1)
        await multisig.connect(signer2).rotateSigners(accounts, 1)

        // Say these two signers send their vote at the same time:
        await expect(multisig.connect(signer1).execute(targetContract.address, calldata, nativeValue, { value: nativeValue })).to.emit(
            targetContract,
            'TargetCalled',
        );
        await expect(multisig.connect(signer2).execute(targetContract.address, calldata, nativeValue, { value: nativeValue })).to.emit(
            targetContract,
            'TargetCalled',
        );
        // The call was executed twice!
    });

Tools Used

Visual inspection.

Recommended Mitigation Steps

Consider adding an incrementing nonce to each topic, so that repeating the call requires using a new nonce. If the intent is to allow arbitrary-order execution, then using a random or unique topic ID in addition to the topic hash could be used instead (like you did with the commandId in AxelarGateway).

Assessed type

Access Control

Summary of the contracts in scope

The audited contracts are part of Axelar Network’s interchain general message passing, governance and token transfer services. The assets in scope implement a few new features:

• In AxelarGateway, a new upgrade mechanism (that uses interchain decentralized governance) is introduced. Additionally, a mint limiter role is added, setting limits to the amounts that can be minted for any token that supports interchain minting.
• Related to the above, a general interchain governance mechanism is introduced, that allows a combination of interchain messages and multisignature wallet transactions to execute arbitrary governance proposals across chains.
• A general interchain token service. This service allows users to permissionlessly deploy new token managers that act as bridges for arbitrary tokens, using a variety of patterns, e.g. a typical mint/burn bridge or a lock/unlock bridge.

The codebase also includes helpers that allow deploying contracts using the create3 pattern, that produces a deployment address that is solely dependent on the sender address and a chosen salt, not the bytecode.

Methodology and time spent

I spent about 11 hours diving deep into the codebase, and then a couple more cleaning up the reports, thinking through the proofs of concept, and preparing the submissions (including this report). My approach was straightforward: visual inspection of the codebase, cross-checking with unit tests and documentation, and thinking through potential edge cases related to timing, reentrancy, ordering of transactions, etc.

I believe I did a thorough review of the governance-related changes, but a slightly less in-depth review of the interchain token service. The following summarizes my findings and thoughts from what I could gather in this time.

I focused on High/Medium severity issues (though I only found Medium ones) and this Analysis, so it’s likely that I won’t be submitting a QA or gas report. The audit bots seem to have caught some of the usual QA issues, so I will highlight some things that are not covered in bot output, and affect the code in a broader way, in “Other recommendations” below.

New/unexpected things

• I wasn’t familiar with the CREATE3 pattern, and it looks very useful. It does introduce some risks, as it makes it easier to deploy contracts with the same address in other chains, which could have unintended consequences (this relates to one of the Medium severity issues I submitted).
• Even though it’s out of scope, the EternalStorage pattern is an interesting approach that I wasn’t familiar with either. I can see how it adds flexibility to handle storage during upgrades. However, it also increases the complexity as it requires specific getters for each variable, and it’s unclear to me if the benefits outweigh the increased attack surface and potential for errors. I’d be curious to hear how this has worked out for this team so far.
• The express token transfer mechanism is impressive; the idea that someone can send the tokens to the destination ahead of time and then be paid back when the message is received is a great way to do fast bridging in a mostly trustless way.

Architecture feedback

Generally the codebase looks clean and the architecture seems sound. Interchain governance and generalized token bridging are massive problems to tackle and the developers have approached them in a straightforward, simple way.

I’ve raised a couple Medium issues that relate to timing and ordering of messages and transactions, on the multisig and the interchain governance contracts. This would prompt me to suggest taking a closer look at potential timing conditions with interchain messages, and the impact of messages getting delayed due to gas. This is one of the trickiest things to get right when doing cross-chain messaging, so I wouldn’t be surprised if there are a few other edge cases where this could cause issues.

A general suggestion I have for the architecture is on the amount of proxy contracts that are defined in the codebase. A lot of different proxies are available, and in some cases there is very little difference between each type. This leads to some duplication, e.g. proxy fallback functions re-implemented in several places, which increases the attack surface and risk of errors. I would suggest, on a future iteration, to attempt to combine some of these into fewer contracts to improve maintainability. For instance, proxies that differ only in the contractId() could have the ID be defined as an immutable and set at proxy deployment time.

Centralization risks

Some of the contracts in scope present upgrade mechanisms but use decentralized governance with timelocks. The cases where multisigs are used are a form of centralization risk (especially if the number of signers is low). AxelarServiceGovernance requiring both an interchain approval and a multisig execution provides some additional security against compromised signers.

In general, token projects using the Axelar gateway token transfer service are trusting the Axelar network and its governance mechanism with minting power for their tokens - this governance is decentralized and this is part of the core trust assumption when using Axelar, but it’s worth noting that the mint limiter is a separate, more centralized role (according to the docs, to be handled through a multisig).

For the interchain token service, an important centralization risk is noted in DESIGN.md:

Users using Custom Bridges need to trust the deployers as they could easily confiscate the funds of users if they wanted to, same as any ERC20 distributor could confiscate the funds of users.

This is an important consideration and ideally it should be mentioned in user documentation or user interfaces. Luckily, not all bridges suffer this risk, just like not all ERC20s allow confiscation. In particular canonical tokens, and bridges using standardized tokens where the distributor is the token manager, are less susceptible to the deployer doing arbitrary confiscations or minting. This is something that could be surfaced in documentation or UI as well, so that users can know what trust assumptions they are working with when they bridge with each particular token and bridge combination.

In the interchain token service, Axelar governance is able to add trusted addresses that can perform arbitrary token transfers on the InterchainTokenService, so this should be noted as an important centralization risk that affects both canonical and custom bridges. (I have also raised a Medium severity issue related to this, where the deployer address retains this ability even after the ownership is transferred to a governance account).

Systemic risks

As mentioned above, timing and gas are some things that are tricky to handle in any protocol that uses cross-chain messaging, and this additional complexity introduces some risk from the possibility of stalling/deadlocks and also simply from the fact that it’s a bigger attack sufrace.

In general, users of these services that are, for instance, extending a DAO or token with interchain governance or bridging, would be plugging their tokens or DAOs into a multiple-chain, multiple-consensus, multiple-VM super-system, so this should always be done with care. Such a system, going beyond a single chain’s consensus, can suddenly be exposed to unexpected issues like reorgs or consensus problems (e.g. forking) in a separate chain causing an impact in token supply in the token’s “native” chain.

Luckily, it seems the team has considered these risks and at first sight Axelar Network has mechanisms to mitigate them (as mentioned in section 5.2 of the Axelar whitepaper), but I’d need to do a deeper dive into the rest of the codebase to assert this with more confidence that all the potential risks stemming from this are covered.

Other recommendations

• Comments in a lot of the contracts are sparse; spending some time adding NatSpec to all functions, storage variables and modifiers could really help with maintainability. Even just having a @notice on all external/public functions would be a big improvement. This is especially important for things like this: https://github.com/code-423n4/2023-07-axelar/blob/9f642fe854eb11ad9f18fe028e5a8353c258e926/contracts/cgp/AxelarGateway.sol#L291-L292 - this audit note is important enough that including it in the NatSpec for the function would be useful. But in general, the meaning of all parameters and return values is valuable information. Similarly, it is not clear in TokenManagerDeployer and StandardizedTokenDeployer that these are meant to be used through delegatecalls - without some documentation, it looks like someone could try to deploy tokens or managers by calling these directly.
• I’ve noticed the project is not using any external dependencies (e.g. OpenZeppelin libraries) that could greatly simplify the codebase; the project could reuse token contracts or even rely on a third-party multisig implementation. I assume this is a conscious choice, and would love to hear more about the rationale for this, but it’s worth pointing out as a tradeoff where it’s unclear if the risk of bugs in third-party code outweighs the risks from lots of additional custom code.
• The use of commandId := calldataload(4) in some command processing functions suggests that it would be cleaner to pass the commandId to the internal _execute functions explicitly to avoid the need to use assembly. This assignment is a sort of “out-of-band” parameter-passing mechanism and could be prone to errors, as it obscures the fact that the params for _execute are not everything that the function needs from the caller. If there is a concern for backwards compatibility, a separate _executeWithCommandId or similar could be added to AxelarExecutable.
• Interchain calls add complexity to any codebase, and are hard to reason about, so I strongly suggest adding automated end-to-end tests if you don’t have them already. Ideally these tests would run on a testnet or local but real nodes for at least two different chains, and test running governance actions or token transfers between the two. I appreciate this is very time consuming but it helps mitigate a lot of the risks.

Time spent:

13 hours