code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/its/token-manager/TokenManager.sol#L77-L100 https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/its/interchain-token-service/InterchainTokenService.sol#L493-L523 https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/AxelarGateway.sol#L455-L467

Vulnerability details

Impact

Introduction

According to the docs, the Axelar network supports cross-chain bridging of external ERC20 tokens as well as their own StandardizedToken (using lock/unlock, mint/burn or liquidity pools).

However, there exist legitimate ERC20 tokens like USDT and USDC that have 6 decimals on Ethereum (see USDT on Etherscan and USDC on Etherscan but 18 decimals on Binance Smart Chain (see USDT on BSCScan and USDC on BSCScan), just to name an example. So there are tokens which have different decimals on different chains.
Furthermore, the Axelar network allows to deploy and register StandardizedToken with the same TokenId but different decimals on different chains, see PoC.

Bug description

A cross-chain bridging can be performed using the TokenManager.sendToken(...) method which correctly collects the source tokens from the sender and subsequently calls the InterchainTokenService.transmitSendToken(...) method that generates the payload for the remote ContractCall and also emits the TokenSent event. However, this payload as well as the susequently emitted ContractCall and TokenSent events, see InterchainTokenService:L512-L514 contain the unscaled source amount with respect to the source token's decimals.
Next, this exact payload (actually its keccak256 hash) gets relayed on the remote chain as it is via AxelarGateway.approveContractCall(...) and the ContractCall is now approved to be executed with the source amount irrespective of the remote token's decimals.
Therefore, the bridged amounts are off by a factor of 10^abs(sourceDecimals - remoteDecimals).

Note that there are also other ways/entry-points to reproduce this issue with the current codebase.

Consequences

This leads to loss of funds for user/protocol/pool when source token decimals are lower/higher than remote token decimals, because the token amount is just passed through instead of being scaled accordingly.

Proof of Concept

The first part of the PoC demonstrates the following:

The Axelar network allows to deploy and register StandardizedToken with the same TokenId but different decimals on different chains. In this example, 18 decimals on source chain and 16 decimals on remote chains.
The sendToken method creates the aforementioned payload (to be relayed) and the respective ContractCall/TokenSent events with the unscaled source amount.

Just apply the diff below and run the test with npm run test test/its/tokenServiceFullFlow.js:

diff --git a/test/its/tokenServiceFullFlow.js b/test/its/tokenServiceFullFlow.js
index c1d72a2..3eb873b 100644
--- a/test/its/tokenServiceFullFlow.js
+++ b/test/its/tokenServiceFullFlow.js
@@ -31,6 +31,7 @@ describe('Interchain Token Service', () => {
     const name = 'tokenName';
     const symbol = 'tokenSymbol';
     const decimals = 18;
+    const otherDecimals = 16;

     before(async () => {
         const wallets = await ethers.getSigners();
@@ -151,13 +152,13 @@ describe('Interchain Token Service', () => {
         });
     });

-    describe('Full standardized token registration, remote deployment and token send', async () => {
+    describe.only('Full standardized token registration, remote deployment and token send', async () => {
         let token;
         let tokenId;
         const salt = getRandomBytes32();
         const otherChains = ['chain 1', 'chain 2'];
         const gasValues = [1234, 5678];
-        const tokenCap = BigInt(1e18);
+        const tokenCap = BigInt(10000e18);

         before(async () => {
             tokenId = await service.getCustomTokenId(wallet.address, salt);
@@ -184,7 +185,7 @@ describe('Interchain Token Service', () => {
                     salt,
                     name,
                     symbol,
-                    decimals,
+                    otherDecimals, // use other decimals on remote chains
                     '0x',
                     wallet.address,
                     otherChains[i],
@@ -197,19 +198,19 @@ describe('Interchain Token Service', () => {
             const params = defaultAbiCoder.encode(['bytes', 'address'], [wallet.address, token.address]);
             const payload = defaultAbiCoder.encode(
                 ['uint256', 'bytes32', 'string', 'string', 'uint8', 'bytes', 'bytes'],
-                [SELECTOR_DEPLOY_AND_REGISTER_STANDARDIZED_TOKEN, tokenId, name, symbol, decimals, '0x', wallet.address],
+                [SELECTOR_DEPLOY_AND_REGISTER_STANDARDIZED_TOKEN, tokenId, name, symbol, otherDecimals, '0x', wallet.address],
             );
             await expect(service.multicall(data, { value }))
                 .to.emit(service, 'TokenManagerDeployed')
                 .withArgs(tokenId, LOCK_UNLOCK, params)
                 .and.to.emit(service, 'RemoteStandardizedTokenAndManagerDeploymentInitialized')
-                .withArgs(tokenId, name, symbol, decimals, '0x', wallet.address, otherChains[0], gasValues[0])
+                .withArgs(tokenId, name, symbol, otherDecimals, '0x', wallet.address, otherChains[0], gasValues[0])
                 .and.to.emit(gasService, 'NativeGasPaidForContractCall')
                 .withArgs(service.address, otherChains[0], service.address.toLowerCase(), keccak256(payload), gasValues[0], wallet.address)
                 .and.to.emit(gateway, 'ContractCall')
                 .withArgs(service.address, otherChains[0], service.address.toLowerCase(), keccak256(payload), payload)
                 .and.to.emit(service, 'RemoteStandardizedTokenAndManagerDeploymentInitialized')
-                .withArgs(tokenId, name, symbol, decimals, '0x', wallet.address, otherChains[1], gasValues[1])
+                .withArgs(tokenId, name, symbol, otherDecimals, '0x', wallet.address, otherChains[1], gasValues[1])
                 .and.to.emit(gasService, 'NativeGasPaidForContractCall')
                 .withArgs(service.address, otherChains[1], service.address.toLowerCase(), keccak256(payload), gasValues[1], wallet.address)
                 .and.to.emit(gateway, 'ContractCall')
@@ -217,30 +218,32 @@ describe('Interchain Token Service', () => {
         });

         it('Should send some token to another chain', async () => {
-            const amount = 1234;
+            const amountSrc = BigInt(1234e18); // same amount on source and destination chain
+            const amountDst = BigInt(1234e16); // just scaled according to decimals
             const destAddress = '0x1234';
             const destChain = otherChains[0];
             const gasValue = 6789;

             const payload = defaultAbiCoder.encode(
                 ['uint256', 'bytes32', 'bytes', 'uint256'],
-                [SELECTOR_SEND_TOKEN, tokenId, destAddress, amount],
+                [SELECTOR_SEND_TOKEN, tokenId, destAddress, amountDst], // expect scaled amount according to remote decimals
             );
             const payloadHash = keccak256(payload);

-            await expect(token.approve(tokenManager.address, amount))
+            await expect(token.approve(tokenManager.address, amountSrc))
                 .to.emit(token, 'Approval')
-                .withArgs(wallet.address, tokenManager.address, amount);
-
-            await expect(tokenManager.sendToken(destChain, destAddress, amount, '0x', { value: gasValue }))
+                .withArgs(wallet.address, tokenManager.address, amountSrc);
+            
+            // call succeeds but doesn't take into account remote decimals
+            await expect(tokenManager.sendToken(destChain, destAddress, amountSrc, '0x', { value: gasValue }))
                 .and.to.emit(token, 'Transfer')
-                .withArgs(wallet.address, tokenManager.address, amount)
+                .withArgs(wallet.address, tokenManager.address, amountSrc)
                 .and.to.emit(gateway, 'ContractCall')
-                .withArgs(service.address, destChain, service.address.toLowerCase(), payloadHash, payload)
+                .withArgs(service.address, destChain, service.address.toLowerCase(), payloadHash, payload) // should fail
                 .and.to.emit(gasService, 'NativeGasPaidForContractCall')
-                .withArgs(service.address, destChain, service.address.toLowerCase(), payloadHash, gasValue, wallet.address)
+                .withArgs(service.address, destChain, service.address.toLowerCase(), payloadHash, gasValue, wallet.address) // should fail
                 .to.emit(service, 'TokenSent')
-                .withArgs(tokenId, destChain, destAddress, amount);
+                .withArgs(tokenId, destChain, destAddress, amountDst); // should fail
         });

         // For this test the token must be a standardized token (or a distributable token in general)

The second part of the PoC demonstrates that the aforementioned payload is relayed/approved as it is to the remote chain and the source token amount is received on the remote chain irrespective of the remote token's decimals.
Just apply the diff below and run the test with npm run test test/its/tokenService.js:

diff --git a/test/its/tokenService.js b/test/its/tokenService.js
index f9843c1..161ac8a 100644
--- a/test/its/tokenService.js
+++ b/test/its/tokenService.js
@@ -797,10 +797,10 @@ describe('Interchain Token Service', () => {
         }
     });

-    describe('Receive Remote Tokens', () => {
+    describe.only('Receive Remote Tokens', () => {
         const sourceChain = 'source chain';
         let sourceAddress;
-        const amount = 1234;
+        const amount = 1234; // this unscaled source amount gets processed with respect to remote decimals
         let destAddress;
         before(async () => {
             sourceAddress = service.address.toLowerCase();
@@ -813,7 +813,7 @@ describe('Interchain Token Service', () => {

             const payload = defaultAbiCoder.encode(
                 ['uint256', 'bytes32', 'bytes', 'uint256'],
-                [SELECTOR_SEND_TOKEN, tokenId, destAddress, amount],
+                [SELECTOR_SEND_TOKEN, tokenId, destAddress, amount], // amount should have been scaled according to remote decimals before relaying
             );
             const commandId = await approveContractCall(gateway, sourceChain, sourceAddress, service.address, payload);

@@ -825,11 +825,11 @@ describe('Interchain Token Service', () => {
         });

         it('Should be able to receive mint/burn token', async () => {
-            const [token, , tokenId] = await deployFunctions.mintBurn(`Test Token Mint Burn`, 'TT', 12, 0);
+            const [token, , tokenId] = await deployFunctions.mintBurn(`Test Token Mint Burn`, 'TT', 14, 0);

             const payload = defaultAbiCoder.encode(
                 ['uint256', 'bytes32', 'bytes', 'uint256'],
-                [SELECTOR_SEND_TOKEN, tokenId, destAddress, amount],
+                [SELECTOR_SEND_TOKEN, tokenId, destAddress, amount], // amount should have been scaled according to remote decimals before relaying
             );
             const commandId = await approveContractCall(gateway, sourceChain, sourceAddress, service.address, payload);

@@ -841,11 +841,11 @@ describe('Interchain Token Service', () => {
         });

         it('Should be able to receive liquidity pool token', async () => {
-            const [token, , tokenId] = await deployFunctions.liquidityPool(`Test Token Liquidity Pool`, 'TTLP', 12, amount);
+            const [token, , tokenId] = await deployFunctions.liquidityPool(`Test Token Liquidity Pool`, 'TTLP', 16, amount);
             (await await token.transfer(liquidityPool.address, amount)).wait();
             const payload = defaultAbiCoder.encode(
                 ['uint256', 'bytes32', 'bytes', 'uint256'],
-                [SELECTOR_SEND_TOKEN, tokenId, destAddress, amount],
+                [SELECTOR_SEND_TOKEN, tokenId, destAddress, amount], // amount should have been scaled according to remote decimals before relaying
             );
             const commandId = await approveContractCall(gateway, sourceChain, sourceAddress, service.address, payload);

Tools Used

VS Code, Hardhat

Recommended Mitigation Steps

This issue cannot be solved easily since the remote chain doesn't know about the token decimals on the source chain and vice versa. I suggest the following options:

Explicitly exclude tokens with different decimals on different chains and emphasize this in the documentation. However, this would be a loss of opportunity and against the mantra "Axelar is a decentralized interoperability network connecting all blockchains, assets and apps through a universal set of protocols and APIs."
Normalize all bridged token amounts to e.g. 18 decimals (beware of loss of precision) before creating the aforementioned payload and the respective ContractCall/TokenSent events at all instances. De-normalize token amounts on the remote chain accordingly.

Assessed type

Decimal

0xSorryNotSorry commented 1 year ago

The issue is well demonstrated, properly formatted, contains a coded POC. Marking as HQ.

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as high quality report

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as primary issue

c4-sponsor commented 1 year ago

deanamiel marked the issue as disagree with severity

deanamiel commented 1 year ago

Corrected Severity: QA We might account for this if a better approach is found than rounding off decimals. For pre-existing tokens with different decimals a wrapper can be made by the deployer that does the decimal handling, which is the current intended use for such tokens.

berndartmueller commented 1 year ago

This is a very well-written submission!

However, I agree with the sponsor that wrapper tokens are supposed to be used in case of different decimals across chains. For instance, see the wrapper ERC-20 token for USDC on BNB chain, Axelar Wrapped USDC (axlUSDC), https://bscscan.com/address/0x4268B8F0B87b6Eae5d897996E6b845ddbD99Adf3#readContract.

Thus, I'm downgrading to QA (Low).

c4-judge commented 1 year ago

berndartmueller changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

berndartmueller marked the issue as grade-c

MarioPoneder commented 1 year ago

@berndartmueller Please be aware that the sponor's comment and severity suggestion is only about the first part of the issue (pre-existing tokens) and therefore not conclusive for the whole report.

Pre-existing tokens:
Even if the mentioned wrapper tokens were in scope of this audit (please correct me if I am mistaken and they are), they are not sufficiently mitigating the issue, since the codebase does not explicitly enforce their usage nor does it require the source and destination decimals to be equal (should revert with error). Furthermore the docs explicitly mention the use of external ERC20 tokens but do not mention the wrappers, therefore the issue is unmitigated as of now.
As a result, pre-existing tokens as well as potentially wrong wrappers with different decimals on different chains can be used and subsequently lead to loss of funds for the user or token pool.
StandardizedToken (not mentioned by the sponsor):
The Axelar network allows to deploy and register StandardizedToken with the same TokenId but different decimals on different chains, see first part of PoC about (remote) deployment and InterchainTokenService.getCustomTokenId(...). This should not be possible in the first place as long as the transfer amount does not get scaled according to decimals automatically, i.e. it's a bug which is even more severe than the above issue also leading to loss of funds.
Both, the InterchainTokenService.deployAndRegisterStandardizedToken(...) method as well as the InterchainTokenService.deployAndRegisterRemoteStandardizedToken(...) method are lacking underlying checks to ensure that there is no other token with the same TokenId but different decimals already registered on another chain.

I am looking forward to more fact-based opinions about this and fair judgement considering the proven impacts, see also second part of PoC that demonstrates loss of funds (wrong transfer of funds by factor 100).

Have a nice day, everyone!

berndartmueller commented 1 year ago

Hey @MarioPoneder!

According to the Interchain token service docs, there are two types of bridges:

Canonical bridges
Custom bridges

Canonical bridges are used to enable bridging pre-existing ERC-20 tokens across chains. Such bridges are deployed via InterchainTokenService.registerCanonicalToken on the source chain and InterchainTokenService.deployRemoteCanonicalToken for the remote (destination) chains. Here, the trust assumption is that anyone can create canonical bridges - the resulting StandardizedToken on the remote chains will have matching decimals with the pre-existing ERC-20 token (see InterchainTokenService.sol#L334. Thus, there is no such issue with non-matching decimals for canonical bridges.

In regards to custom bridges, the trust assumption is different (docs):

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.

In the submission's PoC, the described issue is demonstrated by deploying a StandardizedToken with 18 decimals on the source chain and deploying corresponding StandardizedToken tokens with 16 decimals on remote chains (via deployAndRegisterRemoteStandardizedToken. In this case, it truly shows a mismatch of token decimals between the source and remote chain, leading to a loss of bridged funds.

Even though the docs mention that the deployer of such a custom bridge has to be trusted, implementing the warden's second mitigation recommendation (normalizing token transfer amounts to, e.g., 18 decimals) certainly helps mitigate this issue and reduces the surface for potential errors (malicious or non-malicious) when deploying custom bridges. For example, the Wormhole bridge does this as well by normalizing the token transfer amounts to 8 decimals.

Ultimately, this leads me to acknowledge the outlined issue and medium severity chosen by the warden.

Kindly invite the sponsor's feedback before changing the severity back to medium. @deanamiel

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by berndartmueller

c4-judge commented 1 year ago

berndartmueller marked the issue as satisfactory

c4-judge commented 1 year ago

berndartmueller marked the issue as selected for report

deanamiel commented 1 year ago

The standardized token scenario would still not lead to loss of funds. One could still bridge tokens back and get the same amount, the amounts seen would be mismatched (when divided by 10^ decimals) but this is only a cosmetic issue. We still believe this is a QA level issue.

berndartmueller commented 1 year ago

The standardized token scenario would still not lead to loss of funds. One could still bridge tokens back and get the same amount, the amounts seen would be mismatched (when divided by 10^ decimals) but this is only a cosmetic issue. We still believe this is a QA level issue.

Loss of funds may not be permanent, indeed. Still, it presents an issue for users bridging StandardizedToken tokens and the token pools ( itself. Such tokens with mismatching decimals on the source- and destination chains can also be purposefully bridged to steal funds.

milapsheth commented 1 year ago

We consider this QA or Low severity. As mentioned before, there is no actual loss of funds since you can always bridge the same amount back. Furthermore, Standardized tokens still require trusting the deployer, unlike Canonical tokens. Tokens deployed and whitelisted via the UI will make sure the same decimals are used for deployments. Users are not recommended to interact with arbitrary Standardized tokens given that the deployer can still be malicious in various ways.

The ERC20 spec does not require the presence of name, symbol, decimals. While commonly supported, these are still optional fields, so we left it flexible in the main protocol.

You can also build deployer contracts on top of this that do enforce various checks such as using the on-chain name, symbol, decimals before deploying.

code-423n4 / 2023-07-axelar-findings

Insufficient support for tokens with different decimals on different chains lead to loss of funds on cross-chain bridging #52

Lines of code

Vulnerability details

Impact

Introduction

Bug description

Consequences

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type