Submitted by Bauchibred, also found by mxuse, trachev, grearlake, ayden, ZanyBonzy, gjaldon, and jasonxiale
First, it would be key to note that per the scope of the audit we should count rebasing tokens (whose balance change outside of transfers) in scope. See whats been stated in the readMe.
There are different token manager types that take care of the different types of tokens that get integrated to the system, i.e., here.
Now whereas this logic includes a type for fee-on-transfer tokens, there is no logic for supporting rebasing tokens.
Now from the readMe, we can see that protocol plans to integrate rebasing tokens. Since these tokens do not charge fees during transfers, the method as to which their integration would be done, would be via LOCK_UNLOCK.
However, the problem is that after the initial lock at the token manager, there could be a positive/negative rebase of the token before the unlock which would then mean that the amount of tokens transferred in via takeToken() would have changed by the time giveToken() is to be called to send out these tokens:
https://github.com/code-423n4/2024-08-axelar-network/blob/69c4f2c3fcefb1b8eb2129af9c3685a44ae5b6fe/interchain-token-service/contracts/TokenHandler.sol#L91-L123
https://github.com/code-423n4/2024-08-axelar-network/blob/69c4f2c3fcefb1b8eb2129af9c3685a44ae5b6fe/interchain-token-service/contracts/TokenHandler.sol#L177-L180
https://github.com/code-423n4/2024-08-axelar-network/blob/69c4f2c3fcefb1b8eb2129af9c3685a44ae5b6fe/interchain-token-service/contracts/TokenHandler.sol#L45-L80
Now this would be problematic when we consider the integration in the InterchainTokenService, when processing interchain transfers, since the payload attached to the execution would include the right amount of tokens to be sent:
https://github.com/code-423n4/2024-08-axelar-network/blob/69c4f2c3fcefb1b8eb2129af9c3685a44ae5b6fe/interchain-token-service/contracts/InterchainTokenService.sol#L733-L739
But during the time frame where the transfer gets initiated to the time it gets processed, the amount specified on that asset might have changed; which would then cause for either the wrong amount of tokens to be given out or for the attempt at transfer to revert when being attempted to be given out here.
When executing a message, there is a need to apply the balance tracking logic, see here.
While applying the balance logic, this is documented pattern for interchainTransfers.
We can confirm this in the code snippets:
https://github.com/code-423n4/2024-08-axelar-network/blob/69c4f2c3fcefb1b8eb2129af9c3685a44ae5b6fe/axelar-amplifier/interchain-token-service/src/contract/execute.rs#L101-L132
Evidently the update_token_balance() is being queried and in our case would be to the source chain and wed have our is_deposited bool to be false since we are withdrawing, going to the implementation of update_token_balance() we can see here.
Evidently, there is a query of the last stored tracked balance, and an attempt to withdraw more than was last stored, would fail, due to the revert that occurs in rusts checked_sub() implementation:
However, this would be wrong for rebasing tokens, cause throughout the duration the token is existing, the tracked balance != the real balance this is because these tokens update their balances outside transfers and in the case where theyve been multiple positive rebases. Then, we effectively have funds locked in the source chain, because attempting to withdraw what has been tracked, would always revert here, so simplistic POC would be:
Sub-section 1:
As hinted under Proof of Concept, in the case where a rebasing token is integrated then the amount specified in the payload is not necessarily the amount that needs to end up being sent to the user by the time the interchain attempt gets processed; which is because within the time frame the token might have rebased, positively or negatively:
Which could even open up a sneaky window for tech savvy users to steal value from protocol. This is because, for example, the LIDO stETH token normally rebases around 12pm, so an attacker can always watch the mempool on the rebase from LIDO around the time and in the case where the rebase is going to skim off or heavily increase the balances they can position themselves to make the most out of the transaction by frontrunning the rebase update with requesting an interchain. By the time this is executed, the balance would have already changed for the asset, effectively tricking the system and stealing value from other users.
Sub-section 2:
Asides the hinted cases above, if a tokens balance is tracked, then there is also no active method to ensure the right tracking is being done for this token, considering positive and negative rebases would occur. This would then mean that during interchain transfers we can have reverts when we should not, essentially meaning a chunk of the assets are going to be stuck/untransferrable from the source chain.
Sub-section 1:
Do not support these type of tokens, or integrate a new type and then have a way to check the amount of rebase that has happened since the users specified their transfer request.
Sub-section 2:
Do not track the balances for rebasing tokens and instead have them naturally revert here if not enough balance is available.
ERC20
milapsheth (Axelar) disputed and commented via duplicate Issue #42:
0xsomeone (judge) decreased severity to Medium and commented:
milapsheth (Axelar) commented:
0xsomeone (judge) commented:
