Skip to content
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

TOFT triggerSendFrom can be used to steal all the balance #1290

Open
code423n4 opened this issue Aug 4, 2023 · 3 comments
Open

TOFT triggerSendFrom can be used to steal all the balance #1290

code423n4 opened this issue Aug 4, 2023 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working edited-by-warden H-13 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

code423n4 commented Aug 4, 2023

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
}
  1. 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);
}
  1. 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.
  2. 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

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Aug 4, 2023
code423n4 added a commit that referenced this issue Aug 4, 2023
@c4-pre-sort
Copy link

minhquanym marked the issue as primary issue

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Aug 9, 2023
@c4-sponsor
Copy link

0xRektora (sponsor) confirmed

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Sep 1, 2023
@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Sep 29, 2023
@c4-judge
Copy link

dmvt marked the issue as selected for report

@C4-Staff C4-Staff added the H-13 label Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working edited-by-warden H-13 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

5 participants