code-423n4 / 2024-02-tapioca-findings

1 stars 1 forks source link

Funds can be stolen through remote transfer functionality #76

Open c4-bot-9 opened 4 months ago

c4-bot-9 commented 4 months ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/tapiocaOmnichainEngine/TapiocaOmnichainReceiver.sol#L224

Vulnerability details

Proof of Concept

User can send LZ message through any Oft token using TapiocaOmnichainSender.sendPacket function. User provides params that should be used and also provides composed message if he needs to send it. What is important for composed message is during crafting message, msg.sender is stored as srcChainSender_. In this way we know who have triggered composed call.

function encode(
        bytes32 _sendTo,
        uint64 _amountShared,
        bytes memory _composeMsg
    ) internal view returns (bytes memory _msg, bool hasCompose) {
        hasCompose = _composeMsg.length > 0;
        // @dev Remote chains will want to know the composed function caller ie. msg.sender on the src.
        _msg = hasCompose
            ? abi.encodePacked(_sendTo, _amountShared, addressToBytes32(msg.sender), _composeMsg)
            : abi.encodePacked(_sendTo, _amountShared);
    }

Then amount that should be sent to other chain is burnt(if any) and LZ call is sent.

On another chain the call will be handled by TapiocaOmnichainReceiver._lzReceive function. This function will mint tokens to recipient and then in case if composed message was included, then it will sent it to endpoint, so it can be triggered later.

When composed message is triggered, then lzCompose function handles it. As you can see function retrieves srcChainSender_ to know who was initiator of compose call on source chain. Then _lzCompose function continue processing of message.

Using msgType user can provide operation he wants to execute on target chain. One of operations is MSG_REMOTE_TRANSFER that allows to remotely send tokens to another chain. The flow is next: on chain A user initiates compose call to chain B, that will send his tokens on chain B to chain A, or will use allowance to send tokens of other user on chain B to chain A. Let's check how it works.

First, the function should transfer tokens from owner to address(this). This function receives owner of funds and _srcChainSender as inputs to check allowance. As you can see in case if _srcChainSender is owner then we don't need any approve.

After transfer is done to address(this) then contract can send them back to chain A. So function burns tokens and crafts message to another chain and it can have composed call again, which means that it will include _srcChainSender, so contract on chain A knows who initiated the call.

The problem is that _srcChainSender that will be included is owner of funds on chain B, which is incorrect.

So now i am ready to describe attack flow.

  1. Victim has funds on chain A, that attacker is going to steal to chain B.
  2. Attacker on chain A initiates compose call with victim as owner of funds and provides amount 0 as amount to transfer of chain B.
  3. Compose call succeed on chain B as it is possible to transfer 0 tokens and then another compose message was included, which transfers all tokens from victim to attacker on chain B.
  4. Because _srcChainSender was set to victim on first compose call, then next compose call on chain A will think that victim is initiator of remote transfer, which means that no allowance will be checked.
  5. Funds are stolen to attacker address on chain B.

Impact

Possible to steal funds.

Tools Used

VsCode

Recommended Mitigation Steps

Provide _srcChainSender as initiator of compose call.

_internalRemoteTransferSendPacket(
            _srcChainSender, remoteTransferMsg_.lzSendParam, remoteTransferMsg_.composeMsg
        );

Assessed type

Error

cryptotechmaker commented 4 months ago

I remember I found a similar issue. Might be a duplicate, but I cannot find it anymore

c4-judge commented 4 months ago

dmvt marked the issue as duplicate of #173

c4-judge commented 3 months ago

dmvt changed the severity to 2 (Med Risk)

c4-judge commented 3 months ago

dmvt marked the issue as satisfactory

rvierdiiev commented 3 months ago

pls, reread this report, it's not duplicate of #106

in this report i don't talk about any other owners of same address on another chain. here i describe the issue, when protocol incorrectly set owner of funds as initiator of compose call.

pls, go through the report and all described steps.

c4-judge commented 3 months ago

dmvt marked the issue as unsatisfactory: Out of scope

c4-judge commented 3 months ago

dmvt marked the issue as not a duplicate

c4-judge commented 3 months ago

dmvt changed the severity to 3 (High Risk)

dmvt commented 3 months ago

Reinstituted this as a valid high risk pending further comment from the sponsor. It does appear to be a unique vector that is possible.

c4-judge commented 3 months ago

dmvt marked the issue as selected for report

0xRektora commented 3 months ago

It's a valid one. Worth mentioning that it was also found in https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/111

thebrittfactor commented 2 months ago

For transparency, the sponsor (0xWeiss) confirmed this finding outside of Github. C4 staff have added the applicable label on Tapioca's behalf.