0xadrii - Remote transfers can be used to drain contract accounts due to wrongly assuming that the owner of the contract account address in the source chain also controls that address in the destination chain

[sherlock-admin3]

0xadrii

high

Remote transfers can be used to drain contract accounts due to wrongly assuming that the owner of the contract account address in the source chain also controls that address in the destination chain

Summary

Wrong assumption about ownership of accounts between different EVM chains can lead to draining smart contract wallets.

Vulnerability Detail

Tapioca’s remote transfers allow compose calls to burn tokens in a source chain and mint them in a destination chain. When these type of compose call is triggered, the _remoteTransferReceiver() will be called, and the amount to be burnt will be transferred via the _internalTransferWithAllowance() function:

// TapiocaOmnichainReceiver.sol

function _remoteTransferReceiver(address _srcChainSender, bytes memory _data) internal virtual {
        RemoteTransferMsg memory remoteTransferMsg_ = TapiocaOmnichainEngineCodec.decodeRemoteTransferMsg(_data);

        /// @dev xChain owner needs to have approved dst srcChain `sendPacket()` msg.sender in a previous composedMsg. Or be the same address.
        _internalTransferWithAllowance(
            remoteTransferMsg_.owner, _srcChainSender, remoteTransferMsg_.lzSendParam.sendParam.amountLD
        );   

        ...
    }

  function _internalTransferWithAllowance(address _owner, address srcChainSender, uint256 _amount) internal {

        if (_owner != srcChainSender) {  
            _spendAllowance(_owner, srcChainSender, _amount);
        }

        _transfer(_owner, address(this), _amount);
    } 

As the code snippet shows, tokens will be transferred in _internalTransferWithAllowance() from remoteTransferMsg_.owner to the USDO contract so that they can be burnt later. The problem with this transfer is that USDO wrongly assumes that if _owner == srcChainSender, then no allowance check must be performed because it wrongly believes that the controller of that address in chain A is the same as the controller of that address in the destination chain.

This assumption is correct for EOA addresses across EVM chains as they rely in private keys, but might not be true for smart contract accounts.

Let’s consider a scenario where a victim has control of a smart contract account with address 0xaaaa… in chain B, and such address holds bUSDO tokens.

Such address is not necessarily controlled by the victim in chain A, so an attacker can create a contract account and gain control of address 0xaaaa… in chain A, and trigger a remote transfer compose call to steal all funds from 0xaaaa… in chain B.

Because _internalTransferWithAllowance() assumes such addresses are controlled by the same person, the allowance check will not be performed if the mentioned attacker’s remote transfer compose call takes place. This will make bUSDO tokens be effectively burnt from the victim’s 0xaaaa… smart contract account in chain B, and aUSDO minted to the attacker’s 0xaaaa… smart contract account in chain A.

Note: It is recommended to check this article from Rekt where a similar attack is described. It is also recommended to have a strong understanding about deterministic addresses. The bug is simillar to the one found in C4's Maia contest

Impact

High. Smart contract accounts interacting with Tapioca might be drained if the situation detailed in the report arises.

Code Snippet

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/gitmodule/tapioca-periph/contracts/tapiocaOmnichainEngine/TapiocaOmnichainReceiver.sol#L287-L289

Tool used

Manual Review

Recommendation

It is recommended to verify if accounts performing the compose messages are smart contract accounts, and add some logic that allows to verify if the actual account initiating the transaction is the same in the source chain and in the destination chain. One example would be to perform the approval checks even if the account is the same, so that it can be ensured that the account is actually trying to execute the remote transfer.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

WangAudit commented:

as I understand; high issue from Morpheus CodeHawks contest can also be used as reference (regarding account abstraction)

[0xadrii]

Escalate I believe this issue has also been wrongly set as duplicate of #111 . The root cause of the vulnerability described in this report is wrongly assuming that a smart contract account will be controlled by the same person in chain A and in chain B, which is a conceptual issue rather than a wrong code implementation. The allowance check inside _internalTransferWithAllowance() will NOT be performed if _owner == srcChainSender:

// TapiocaOmnichainReceiver.sol
...

if (_owner != srcChainSender) {  
            _spendAllowance(_owner, srcChainSender, _amount);
        }

On the other hand, the root cause for issue #111 is passing a wrong parameter when calling the _internalRemoteTransferSendPacket() function, which is completely different from the issue described here. The fix proposed by the Tapioca team mitigates #111 , but the issue described in this report will persist.

Even with the fix proposed, an attacker can still leverage remote transfers to steal all the balance from a smart account with address 0xaaa... in chain B, given that it is wrongly assumed that the owner of such address in chain A will be the same. As shown in the example scenario in my report, the attacker can gain access of account 0xaaa... in chain A and perform the attack described above.

[sherlock-admin2]

Escalate I believe this issue has also been wrongly set as duplicate of #111 . The root cause of the vulnerability described in this report is wrongly assuming that a smart contract account will be controlled by the same person in chain A and in chain B, which is a conceptual issue rather than a wrong code implementation. The allowance check inside _internalTransferWithAllowance() will NOT be performed if _owner == srcChainSender:

// TapiocaOmnichainReceiver.sol
...

if (_owner != srcChainSender) {  
            _spendAllowance(_owner, srcChainSender, _amount);
        }

On the other hand, the root cause for issue #111 is passing a wrong parameter when calling the _internalRemoteTransferSendPacket() function, which is completely different from the issue described here. The fix proposed by the Tapioca team mitigates #111 , but the issue described in this report will persist.

Even with the fix proposed, an attacker can still leverage remote transfers to steal all the balance from a smart account with address 0xaaa... in chain B, given that it is wrongly assumed that the owner of such address in chain A will be the same. As shown in the example scenario in my report, the attacker can gain access of account 0xaaa... in chain A and perform the attack described above.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

@cryptotechmaker Might want to take a look to not miss a finding to be addressed @0xadrii Would it be possible to prove the bug still exist after fix in #111?

[0xadrii]

The thing is that the issue is not related in any way to #111 . At a conceptual level, the problem here is related to the fact that the same address might be controlled by a different entity in different chains if the address is a smart contract account.

As I mentioned in the comment, this has nothing to do with which parameter is passed to the function. It is rather a conceptual issue, where it is wrongly assumed that the same address is always controlled by the same party across chains.

I really recommend checking this article where the same attack vector is clearly explained with a real-world scenario.

The step-by-step in Tapioca's case would look like this:

• Prerequisites: A victim has a smart contract account with address 0xaaa... in chain B. It interacts with Tapioca and holds USDO tokens. With this information, an attacker creates a smart contract account with address 0xaaa... in chain A. The actual attack:
  1. The attacker creates a compose message to be sent from chain A (a chain where he has control of address 0xaaa...) to chain B (the compose message is actually a remote transfer that will later be triggered from chain B to chain A). It is important to note that the compose message will have the source chain sender set to address 0xaaa... from chain A (which is actually controlled by the attacker)
  2. When the message is received on chain B, the remote transfer logic handling will be triggered, and the _remoteTransferReceiver() function will be called. As shown in the code, the tapComposeMsg_ is completely arbitrary and can be crafted by the attacker.
  3. The _remoteTransferReceiver() will decode the tapComposeMsg_ into a remoteTransferMsg_ variable, which among other data contains a completely aribitrary owner parameter. Again, this parameter is extracted from arbitrary data, so the attacker can set the owner to be the 0xaaa... address.
  4. Finally, the _internalTransferWithAllowance() function will be called. As shown in the code, the allowance will only be spent if _owner != srcChainSender (which in this case is not true, because srcChainSender is address 0xaaa... from chain A, and _owner is the arbitrary parameter set by the attacker, which is also address 0xaaa...), so the USDO tokens from address 0xaaa... in chain B (the victim's smart account address) will be transferred to Tapioca's contract.
  5. After executing _internalTransferWithAllowance(), and having the victim's tokens already transferred to the contract, a final message from chain B to chain A will be triggered, which will transfer the stolen USDO tokens back from chain B to chain A.

This is why the fix proposed does not mitigate this issue, because remoteTransferMsg_.owner and _srcChainSender are actually the same address, so it doesn't matter which one of those two is passed as parameter when calling _internalRemoteTransferSendPacket(). The problem here is that conceptually it is wrong to assume that addresses will be controlled by the same entity in all chains.

[nevillehuang]

I think such issues are generally not considered given it is the responsibility of users to verify their contract addresses. Will loop back after further review.

[0xadrii]

Although it is true that usually several cross-chain protocols face this type of issue, it is important to be aware of the fact that the same issue might be leveraged to cause different types of attacks/vulnerabilities with a wide range of impacts and severities, depending on the functionalities implemented by such protocols. Some of these attacks will actually be responsibility of (or could be easily mitigated by) the user, and some others won't.

In this particular situation, the problem isn't as simple as the victim attempting to bridge funds from an origin chain to a destination chain and never receiving the funds because they don't own the smart account on the destination chain. If that were the case, asking users to verify the accounts would make sense given that conceptually it would be the user's responsibility to avoid bridging assets to another chain if they actually don't own the destination chain's smart account.

On the other hand, the actual problem with the issue found in Tapioca is that attackers can leverage the fact that users usually won’t have control of the same multisig across several chains in order to steal all USDO held in the victim's smart accounts. As shown in the steps described in this comment, the user is actually never responsible of the attack. The attacker will simply initiate the hack and will effectively steal all funds from the victim's smart account without the victim having anything to do and not being responsible for it.

The only possible victim responsibility here would be to enforce smart account users to create the same multisig across all the supported chains in Tapioca, which is not a feasible request and does not actually make sense given that usually, smart account users won't create the same multisig across all chains. To make it more clear, if Tapioca supported n chains, it would be compulsory for account users to have control of that same account over the whole n chains if they wanted to interact with Tapioca in any way. If for example a user only had control of the account in n-1 chains (which is already a wide range of chains and a huge demand for the user), the attack would then be possible because the missing chain enables the whole attack to take place. From my point of view, it is clear and straightforward that enforcing these kind of requirements for smart account users simply doesn’t make sense.

Another good point to consider is the fact that users could simply not want to use the cross-chain functionalities from Tapioca. A smart account/multisig user could decide to only mint some USDO on Ethereum and not perform any cross-chain call at all. In this case, the user would still be vulnerable to the attack mentioned in my report, and as stated before, it would simply not make sense to enforce the user to have the same account across chains if they only want to mint USDO on Ethereum.

Finally, as mentioned in my report, the mitigation for the issue is as simple as enforcing the allowance check inside the _internalTransferWithAllowance() if the addresses interacting with Tapioca are smart contracts (even if _owner == srcChainSender). This easily mitigates the issue and prevents this kind of attack from taking place, also preventing users from being required of having the same account across chains, and making the interaction with Tapioca be way more satisfactory for smart account/multisig users.

[cvetanovv]

I agree with the escalation and especially @0xadrii last comment, so I decided to accept the escalation and separate #111 and #114.

[Evert0x]

Result: Unique
High

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xadrii: accepted

[cryptotechmaker]

We can ACK the following. It's stated in our docs that SC have this risk and it's not recommended to use them

https://docs.tapioca.xyz/tapioca/information/frequently-asked-questions#can-i-use-a-legacy-gnosis-safe-or-non-multichain-contract-to-interact-with-the-tapioca-protocol

[nevillehuang]

@cryptotechmaker Was this information present during the time of the audit or is this the “fix” implemented? If yes it could be a known issue and hence invalid, if not this issue should remain valid

CC: @cvetanovv

[0xadrii]

Honestly I did not see that section in the docs and don't know if it the comment was added after the audit or not. However, even if the issue is briefly mentioned in the doc's FAQ's, there's several reasons why I believe the issue should still be accepted:

1. The vulnerability was not listed in the known issues/acceptable risks in the contest's details. Checking the known issues section we can see that only zero address checks should be considered as invalid, and hence not result in a valid finding. As I described in my report, the impact of this issue is high and must not be overlooked, and if it was known it should have been mentioned in its respective section in the contest details.
2. As mentioned in my previous comments and report, the fix for this issue is trivial. From my point of view, adding a section in the FAQ's inside the docs and leaving the vulnerability in the code when the fix simply implies enforcing a check is not a valid solution (it breaks the industry standard where it is always recommended that one must not trust, but verify). If we extrapolate this to a typical issue such as a reentrancy, simply mentioning in the docs that the code is vulnerable to reentrancy and that users should be careful and protect themselves is not a proper way to handle it. Also, at the moment of writing this there's more than 8.1M safe wallets deployed across chains, so the probability of several Safe users directly interacting with Tapioca without even reading Tapioca's docs is really high, which will definitely lead to a loss of funds.
3. The section in the documentation says that users should "not utilize any non-multichain contract (e.g. legacy Gnosis Safe) to interact with the Tapioca protocol". However, it is important to be aware of the fact that the vulnerability will still take place even if a multisig does not directly interact with a Tapioca contract, and it indirectly receives USDO. Simply holding USDO will make multisigs and smart accounts vulnerable to this attack. As I mentioned in the previous point, there is a huge amount of Safe accounts deployed, so encouraging multisig users to not interact with Tapioca will affect to USDO's decentralization.

[nevillehuang]

@0xadrii

1. The documentation is part of the contest details, so I believe information present should be considered, unless I am misunderstanding sherlock's hierarchy of truth

2. Agree, but I don't think sherlock makes exceptions to previously acknowledged risks. Since this risk is already highlighted (assuming it is not subsequently added after audit period ends), any user action would be considered user error.

3. Agree, although this may be suboptimal design decision, but I don't think it is discouraging users to interact with Tapioca. There are many compatible wallets that they can freely use to interact with the protocol.

Again this is all assuming this risk is not previously known and added after the contest period.

CC: @cryptotechmaker @maarcweiss @cvetanovv @Evert0x

[cvetanovv]

@0xadrii

1. The documentation is part of the contest details, so I believe information present should be considered, unless I am misunderstanding sherlock's hierarchy of truth
2. Agree, but I don't think sherlock makes exceptions to previously acknowledged risks. Since this risk is already highlighted (assuming it is not subsequently added after audit period ends), any user action would be considered user error.
3. Agree, although this may be suboptimal design decision, but I don't think it is discouraging users to interact with Tapioca. There are many compatible wallets that they can freely use to interact with the protocol.

Again this is all assuming this risk is not previously known and added after the contest period.

CC: @cryptotechmaker @maarcweiss @cvetanovv @Evert0x

I agree with Lead Judge's comment. Sherlock's rules are these:

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation

Not having it in the Readme means we have to see it in other places by weight. The Tapioca documentation has it as a "known issue". Despite the correct report, there are rules, so I suggest @Evert0x to change the severity to Invalid.

[0xadrii]

As per the hierarchy you mentioned, wouldn't the issue be valid given that Sherlock rules for valid issues > protocol documentation, and as per Sherlock's rules this is a valid issue? Don't know if I'm missing something here.

[cvetanovv]

As per the hierarchy you mentioned, wouldn't the issue be valid given that Sherlock rules for valid issues > protocol documentation, and as per Sherlock's rules this is a valid issue? Don't know if I'm missing something here.

This means that if there is a conflict between "Sherlock rules for valid issues" and "protocol documentation" then "Sherlock rules for valid issues" will be taken into account. In this case there is no conflict between the rules.