-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 #114
Comments
1 comment(s) were left on this issue during the judging contest. WangAudit commented:
|
Escalate // 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 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. |
@cryptotechmaker Might want to take a look to not miss a finding to be addressed |
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:
This is why the fix proposed does not mitigate this issue, because |
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. |
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 |
Result: |
Escalations have been resolved successfully! Escalation status:
|
We can ACK the following. It's stated in our docs that SC have this risk and it's not recommended to use them |
@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 |
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:
|
Again this is all assuming this risk is not previously known and added after the contest period. |
I agree with Lead Judge's comment. Sherlock's rules are these:
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. |
As per the hierarchy you mentioned, wouldn't the issue be valid given that |
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. |
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:As the code snippet shows, tokens will be transferred in
_internalTransferWithAllowance()
fromremoteTransferMsg_.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.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.
The text was updated successfully, but these errors were encountered: