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

Attacker can block LayerZero channel due to missing check of minimum gas passed #1207

Open
code423n4 opened this issue Aug 4, 2023 · 2 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working edited-by-warden H-17 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report

Comments

@code423n4
Copy link
Contributor

code423n4 commented Aug 4, 2023

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:

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

@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 duplicate of #841

@c4-judge c4-judge reopened this Sep 29, 2023
@c4-judge c4-judge added primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report labels Sep 29, 2023
@c4-judge
Copy link

dmvt marked the issue as selected for report

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-17 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report
Projects
None yet
Development

No branches or pull requests

4 participants