search

code-423n4 / 2024-01-decent-findings

3 stars 3 forks source link

Attacker can block LayerZero channel due to variable gas cost of saving payload #697

Closed c4-bot-5 closed 7 months ago

c4-bot-5 commented 7 months ago

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L197 https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L103-L109 https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L161

Vulnerability details

Impact

This issue is internally the same as described in https://github.com/code-423n4/2023-07-tapioca-findings/issues/1220. To avoid duplication of efforts and unnecessary copy-pasting, only the differences are explained in this issue.

Proof of Concept

The entry point here is the bridgeWithPayload() function in the DecentEthRouter contract, which allows the caller to pass a variable-length additionalPayload variable. This variable is then encoded as part of the payload variable returned by _getCallParams() and passed as parameter to DcntEth.sendAndCall(). This is a function that DcntEth inherits from OFTV2, which inherits it from BaseOFTV2 . This function calls the internal _sendAndCall() function inherited from OFTCoreV2, which finally calls _lzSend(). This ties into the description of the vulnerability in the aforementioned issue.

On the other end, the message will be processed in the onOFTReceived() function of the same contract, which is invoked in the OFTCoreV2.callOnOFTReceived() function when receiving a message. Our payload will be extracted as the callPayload variable and passed on to the executor, where the msgType is hardcoded as MT_ETH_TRANSFER_WITH_PAYLOAD in bridgeWithPayload(). Finally, in DecentBridgeExecutor, we always get a call to our target address with the payload we defined (of which only its size matters), in which we can drain as much gas as needed for the attack. This completes the exploit as all intermediary steps are explained in great detail in the issue referenced above.

Tools Used

Manual review

Recommended Mitigation Steps

The recommended mitigation is similar to the one in the reference issue. Alternatively or in addition to it, a maximum size for any variable-length parameters that can be passed in a LayerZero message to an arbitrary address could be implemented.

Assessed type

Other

c4-pre-sort commented 7 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 7 months ago

raymondfam marked the issue as duplicate of #693

c4-judge commented 7 months ago

alex-ppg marked the issue as selected for report

alex-ppg commented 7 months ago

The Warden has demonstrated that even though the V2 OFT token implements a non-blocking LayerZero application, the insecure DecentEthRouter::onOFTReceived function will perform an arbitrary call that can consume a significant portion of gas allocated to the call, preventing the failed message from being stored and thus blocking the LayerZero channel.

Such an action is irreversible, and I consider a severity of High to be appropriate.

As feedback for the Warden, this submission would not have been selected the best if any of its duplicates properly explained the issue. I strongly advise the Warden to not cite other findings to avoid "effort" as this finding will be part of a publicly available C4 report that should properly describe the issue without having the readers go through other resources.

c4-judge commented 7 months ago

alex-ppg marked the issue as satisfactory

NicolaMirchev commented 7 months ago

Hey, @alex-ppg

I believe the following issue is not valid, because LZ implement a check for the payload size inisde lzSend function: This is the code that they are using: https://github.com/LayerZero-Labs/solidity-examples/blob/2d68d812260d1f96d0a7a46abd6ade4e8962aae1/contracts/lzApp/LzApp.sol#L70-L73

https://github.com/LayerZero-Labs/solidity-examples/blob/2d68d812260d1f96d0a7a46abd6ade4e8962aae1/contracts/lzApp/LzApp.sol#L95-L102

    function _lzSend(
        uint16 _dstChainId,
        bytes memory _payload,
        address payable _refundAddress,
        address _zroPaymentAddress,
        bytes memory _adapterParams,
        uint _nativeFee
    ) internal virtual {
        bytes memory trustedRemote = trustedRemoteLookup[_dstChainId];
        require(trustedRemote.length != 0, "LzApp: destination chain is not a trusted source");
        _checkPayloadSize(_dstChainId, _payload.length);
        lzEndpoint.send{value: _nativeFee}(_dstChainId, trustedRemote, _payload, _refundAddress, _zroPaymentAddress, _adapterParams);
    }
    function _checkPayloadSize(uint16 _dstChainId, uint _payloadSize) internal view virtual {
        uint payloadSizeLimit = payloadSizeLimitLookup[_dstChainId];
        if (payloadSizeLimit == 0) {
            // use default if not set
            payloadSizeLimit = DEFAULT_PAYLOAD_SIZE_LIMIT;
        }
        require(_payloadSize <= payloadSizeLimit, "LzApp: payload size is too large");
    }
stalinMacias commented 7 months ago

Hey @alex-ppg , what @NicolaMirchev has just mentioned is true, the LZ contracts have an internal mechanism to prevent exactly what is been reported here. Additionally, if we take a look at the report that this report is referencing, there is a unique precondition for the "issue" to even occur, such a precondition (I'm quoting from the referenced report) is: "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."

DadeKuma commented 7 months ago

I agree with @NicolaMirchev and @stalinMacias, this issue is invalid.

The issue cited: https://github.com/code-423n4/2023-07-tapioca-findings/issues/1220 is valid only because the attacker can pass some arbitrary length data in the form of an ICommonData.IApproval[] calldata approvals array to get near the max gas limit; then the payload decoding will not have enough gas if it's large enough, and under the payloadSizeLimit limit.

~But in this case, the attacker cannot pass arbitrary length data, this function doesn't accept arrays: https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L197~

~If the attacker tries to send a large payload, the TX will fail here without any consequences:~

require(_payloadSize <= payloadSizeLimit, "LzApp: payload size is too large");

EDIT: Nevermind, I misread the issue, additionalPayload is part of the payload, it has a variable length, and there are no checks about its validity or length, so it can be anything.

0xEVom commented 7 months ago

Hi @alex-ppg, many thanks for the detailed response on every submission and the feedback on this one, that is incredibly helpful. I will keep that in mind for future submissions.

@NicolaMirchev the referenced finding does take into account that the maximum payload size is 10k bytes, which is the DEFAULT_PAYLOAD_SIZE_LIMIT.

@stalinMacias the excerpt you're mentioning is not a precondition, but simply mentioned as an aside in the referenced issue to inform the finding's remediation. The full quote is as follows:

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.

@DadeKuma the bytes type is a dynamically-sized byte array.

stalinMacias commented 7 months ago

@0xEVom For one second I almost believed that your report was valid, but guess what, it is totally invalid, here is why.

So, you are saying that the DecentEthRouter::onOFTReceived() will call a malicious contract that will drain all the gas that is used for the execution, and as a result of that, the payload that was sent would get stored in the failedMessages by using the _storeFailedMessage() which is called by the _blockingLzReceive() from the lzReceive().

Or in other words, you are saying that the DcntEth.lzReceive() is able to store payloads when the executions fail.

So, you are saying that when the DecentEthRouter::onOFTReceived() forwards the execution to the DecentBridgeExecutor to call your malicious contract that will force the tx to fail and as a result of that, the huge payload will be stored. But there is something you missed, executions on the BrdigeExecutors don't revert _executeWeth() & _executeEth(). Any of those tx can't revert, if the external call (the one to your contract) fails, the DecentBridgeExecutor will attempt to refund the funds. And, surprise surprise, if your malicious contract consumes all the gas that is used for the execution, in the DecentBridgeExecutor the whole tx will blow up as an OOG exception, thus, nothing will be stored on the failedMessages, the channel won't be blocked because the state won't be updated, it will be as if the transaction would've never been executed.

You missed an important thing in the report you mentioned, in those contracts, if the external call to the final target contract fails, they indeed explicitly cause a revert, but in this protocol, if the external call fails, they refund the funds back and they never revert the tx.

@alex-ppg Please read carefully this comment to understand why this report is totally invalid. The report of the other contest is valid in the context of the other contest, but in the execution flow of this contest is impossible to deliberately force the same issue to occur on these contracts.

windhustler commented 7 months ago

My report was referenced here so I believe I can provide some additional context.

So there is no return bomb and the onOFTReceived doesn't drain any base gas.

The Warden has demonstrated that even though the V2 OFT token implements a non-blocking LayerZero application, the insecure DecentEthRouter::onOFTReceived function will perform an arbitrary call that can consume a significant portion of gas allocated to the call, preventing the failed message from being stored and thus blocking the LayerZero channel.

By draining this gas the attacker would only drain what he has additionally paid on the sending side. The whole point of the OFT::sendAndCall interface is to allow arbitrary calls while having the rest of the call succeed regardless.

So the basic assumptions of this report are not accurate.

The only real issue is the size of the payload, which in cases when it's too big causes a huge overhead when it's being saved inside the failedMessages. As the base gas is hardcoded to 100k, this is an issue.

I have explained these dynamics in the: https://github.com/code-423n4/2024-01-decent-findings/issues/670.

alex-ppg commented 7 months ago

Thanks to everyone for contributing to the PJQA process! I will remind you that all Wardens should adhere to civilized conduct and that snarky remarks are unwelcome.

We have already established that the gas limit allocation is very small to properly execute across-chain transactions and that may lead to a failure in the cross-chain message relay.

This is a valid concern and can be rectified by updating how gas is configured and so on.

This particular submission relates to how even with a configurable payload it may be possible to still hijack the transaction, causing blockage of the LayerZero channel.

@NicolaMirchev the default payload size is indeed accounted for in the submission as it is quite high at 10_000 bytes.

@DadeKuma as you have correctly re-evaluated an arbitrary payload can indeed be passed in the cross-chain transaction.

@stalinMacias your initial reply treats the adjustment of the default payload size as a precondition but it is not. The original submission clearly uses the 10_000 default limitation and as such no privileged action needs to occur.

Your second reply seems to misunderstand what would occur in this scenario. If the called contract would result in an OOG error, the bridge executors would have to perform refunds with the remaining 1/64 gas which is not going to be sufficient. This will result in the call also running into an OOG error until it is "bubbled up" to the instance the Warden has submitted. To note, the caller of the contract can also be forced to fail via return-bombing which is applicable for the bridge executors.

What we are concerned with is whether the final 1/64 remaining at that point in time is sufficient for storing the failed payload. As the Tapioca submission describes, the code can fail even with the default payload size limitation (it uses a simulated gas limit of 1_000_000, far greater than the LayerZero recommendation, and a payload size of 10_000, which is the default limitation).

I will re-evaluate the issue as it may potentially be a superset of the out-of-gas error described in #525, but will retain it as separate for now.

stalinMacias commented 7 months ago

@alex-ppg

First of all, apologies for the snarky remarks, will pay more attention to the wording on my comments.

So, if we track the execution flow on the lzContracts, we have:

LzApp.lzReceive() => NonBlockingLzApp._blockingLzReceive() => NonBlockingLzApp.nonblockingLzReceive() => NonBlockingLzApp._nonblockingLzReceive() => OFTCoreV2._sendAndCallAck() => OFTCoreV2.callOnOFTReceived() => DcntEthRouter.onOFTReceived() => DecentBridgeExecutor.execute() => targetContract()

When retying a failed payload:

NonBlockingLzApp.retryMessage() => NonBlockingLzApp._nonblockingLzReceive() => OFTCoreV2._sendAndCallAck() => OFTCoreV2.callOnOFTReceived() => DcntEthRouter.onOFTReceived() => DecentBridgeExecutor.execute() => targetContract()

By taking a deeper look at the lz contracts involved in the DcntEth contract, I realized that when an execution to retry a payload is sent, it is possible to send more gas for this execution, see here.

Because of the logic of the contracts in this protocol and the fail-safe mechanism to refund funds in the DecentBridgeExecutor if the external call fails rather than reverting the whole execution, the layer zero messaging can't be blocked permanently (With that said and after I've reviewed #525, I believe this report should be a dupe of #525 instead of being assessed as a unique finding

0xEVom commented 7 months ago

@alex-ppg really appreciate how much thought and consideration you're putting into judging.

I reached out to @windhustler yesterday and while I would gladly take the solo high, I've come around to agreeing with him that the situation here is not quite the same as in the Tapioca case and this finding is pretty much just a needlessly complicated duplicate of #525.

The main difference is that in the referenced finding, the gas was available for the entire call trace and causing a revert required the target contract to consume so much gas that the storing of the payload hit an OOG with the remaining 1/64 of the gas.

However, in this case, DcntEth will only pass as much gas to onOFTReceived() as the user specified as _dstGasForCall in DecentEthRouter.bridgeWithPayload(). This amount is passed as argument to sendAndCall() and encoded in the lzPayload passed to _lzSend(), which is decoded again as gasForCall in _sendAndCallAck(). This is the gas that is forwarded to the onOFTReceived() function and not any more.

At the same time, the endpoint passes a different amount of gas to lzReceive(), which DcntEth inherits from LzApp and ends calling _sendAndCallAck() via blockingLzReceive() -> nonblockingLzReceive() -> _nonblockingLzReceive(). This amount of gas is presumably extracted by the off-chain relayer from the adapterParams, in which it is encoded as GAS_FOR_RELAY + _dstGasForCall.

So we have:

Since _dstGasForCall is user-controlled, it is trivial to cause onOFTReceived() to fail and there is no need to consume as much gas as to cause an OOG exception—we can just pass 0 or a small amount. The real issue is the fact that GAS_FOR_RELAY is not large enough to handle the storing of arbitrarily large payloads of failed messages. If it was appropriately sized, causing onOFTReceived() to revert due to OOG in any way would still not consume the base GAS_FOR_RELAY and only cause the message to be stored without blocking the channel.

alex-ppg commented 7 months ago

Hey @stalinMacias and @0xEVom, thank you for providing additional information to this submission.

@stalinMacias I believe you misunderstand how LayerZero works and how an uncaught revert on the OFT would be handled. Specifically, a cross-chain transaction cannot be retried on-chain if it fails in an uncaught error; it can only be retried if it was properly stored in the failed payloads. Otherwise, it is repeated as a normal transaction until it executes (or is gracefully handled) and the channel is blocked until that occurs.

@0xEVom I appreciate the re-evaluation of your own submission, but you have to keep in mind that point 2 is relatively indeterminate. The gas cost is theoretically unbound for the input argument (only limited by the type(uint256).max limitation as well as block-level gas limits). This means that even if GAS_FOR_RELAY is set to the block's gas limit, we could still incur an out-of-gas exception by utilizing an arbitrarily long payload due to the 63/64 rule.

Based on the above, the submission is somewhat related but not directly linked to what value the GAS_FOR_RELAY has. The variable _dstGasForCall is unrelated as well given that it is user-controlled and can be considered 0 as a user who wishes to block the cross-chain relay channel would act maliciously.

I would normally keep the submissions separate based on the above, however, given that the wardens all agree to their merging (including the main submission's original author), I will de-dupe this submission under the gas-related LayerZero flaw. This discussion is considered final as PJQA has concluded the contest; thanks to everyone who contributed their knowledge to this ruling!

c4-judge commented 7 months ago

alex-ppg marked the issue as duplicate of #525

c4-judge commented 7 months ago

alex-ppg marked the issue as not selected for report